Re: [RFC 2/3] rebase -i: Reschedule tasks that failed before the index was touched

2014-05-27 Thread Phil Hord
Hi Fabian,

Thanks for looking into this.

On 05/27/2014 07:56 AM, Michael Haggerty wrote:
>> +reschedule_last_action () {
>> +tail -n 1 "$done" | cat - "$todo" >"$todo".new
>> +sed -e \$d <"$done" >"$done".new
>> +mv -f "$todo".new "$todo"
>> +mv -f "$done".new "$done"
>> +}
>> +
>>  append_todo_help () {
>>  git stripspace --comment-lines >>"$todo" <<\EOF
>>  
>> @@ -470,11 +480,15 @@ do_pick () {
>> --no-post-rewrite -n -q -C $1 &&
>>  pick_one -n $1 &&
>>  git commit --allow-empty --allow-empty-message \
>> -   --amend --no-post-rewrite -n -q -C $1 ||
>> -die_with_patch $1 "Could not apply $1... $2"
>> +   --amend --no-post-rewrite -n -q -C $1
> "git cherry-pick" indicates its error status specifically as 1 or some
> other value.  But here it could be that pick_one succeeds but "git
> commit" fails; in that case ret is set to the return code of "git
> commit".  So, if "git commit" fails with a retcode different than 1,
> then reschedule_last_action will be called a few lines later.  This
> seems incorrect to me.

I agree.  I was thinking that pick_one should get this new behavior
instead of do_pick, but some callers may not appreciate the new behavior
(i.e. squash/fixup), though I expect those callers have similar problems
as this commit is trying to fix.

I think adding a pick_one_with_reschedule function (to be called in both
places from do_pick) could solve the issue Michael mentioned without
breaking other pick_one callers.

I have not tested doing so, but I think it would look like this:

===

diff --git i/git-rebase--interactive.sh w/git-rebase--interactive.sh
index fe813ba..ae5603a 100644
--- i/git-rebase--interactive.sh
+++ w/git-rebase--interactive.sh
@@ -235,6 +235,17 @@ git_sequence_editor () {
 eval "$GIT_SEQUENCE_EDITOR" '"$@"'
 }
 
+pick_one_with_reschedule () {
+pick_one $1
+ret=$?
+case "$ret" in
+0) ;;
+1) ;;
+*) reschedule_last_action ;;
+esac
+return $ret
+}
+
 pick_one () {
 ff=--ff
 
@@ -474,13 +485,13 @@ do_pick () {
 # rebase --continue.
 git commit --allow-empty --allow-empty-message --amend \
--no-post-rewrite -n -q -C $1 &&
-pick_one -n $1 &&
+pick_one_with_reschedule -n $1 &&
 git commit --allow-empty --allow-empty-message \
--amend --no-post-rewrite -n -q -C $1 \
${gpg_sign_opt:+"$gpg_sign_opt"} ||
 die_with_patch $1 "Could not apply $1... $2"
 else
-pick_one $1 ||
+pick_one_with_reschedule $1 ||
 die_with_patch $1 "Could not apply $1... $2"
 fi
 }

===

I don't much like the name 'pick_one_with_reschedule', but I didn't like
my other choices, either.

I'll try to look at this and your patches more closely when I have a bit
more time.

Phil
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [RFC 2/3] rebase -i: Reschedule tasks that failed before the index was touched

2014-05-27 Thread Michael Haggerty
Hi,

Overall, this approach seems reasonable.

Please see the inline comments below.

On 05/27/2014 12:19 AM, Fabian Ruch wrote:
> When `rebase--interactive` processes a task, it removes the item from
> the todo list and appends it to another list of executed tasks. If a
> `pick` (this includes `squash` and `fixup`) fails before the index has
> recorded the changes, take the corresponding item and put it on the todo
> list again. Otherwise, the changes introduced by the scheduled commit
> would be lost.
> 
> That kind of decision is possible since the `cherry-pick` command
> signals why it failed to apply the changes of the given commit. Either
> the changes are recorded in the index using a conflict (return value 1)
> and `rebase` does not continue until they are resolved or the changes
> are not recorded in the index (return value neither 0 nor 1) and
> `rebase` has to try again with the same task.
> 
> Reported-by: Phil Hord 
> Signed-off-by: Fabian Ruch 
> ---
>  git-rebase--interactive.sh | 27 ++-
>  1 file changed, 22 insertions(+), 5 deletions(-)
> 
> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> index 9e1dd1e..bba4f3a 100644
> --- a/git-rebase--interactive.sh
> +++ b/git-rebase--interactive.sh
> @@ -132,6 +132,16 @@ mark_action_done () {
>   fi
>  }
>  
> +# Put the last action marked done at the beginning of the todo list
> +# again. If there has not been an action marked done yet, the list of
> +# items on the todo list is left unchanged.

The comment would read better if the second sentence were also in active
voice, like the first sentence:

If there has not been an action marked done yet, leave the list of
items on the todo list unchanged.

> +reschedule_last_action () {
> + tail -n 1 "$done" | cat - "$todo" >"$todo".new
> + sed -e \$d <"$done" >"$done".new
> + mv -f "$todo".new "$todo"
> + mv -f "$done".new "$done"
> +}
> +
>  append_todo_help () {
>   git stripspace --comment-lines >>"$todo" <<\EOF
>  
> @@ -470,11 +480,15 @@ do_pick () {
>  --no-post-rewrite -n -q -C $1 &&
>   pick_one -n $1 &&
>   git commit --allow-empty --allow-empty-message \
> ---amend --no-post-rewrite -n -q -C $1 ||
> - die_with_patch $1 "Could not apply $1... $2"
> +--amend --no-post-rewrite -n -q -C $1

"git cherry-pick" indicates its error status specifically as 1 or some
other value.  But here it could be that pick_one succeeds but "git
commit" fails; in that case ret is set to the return code of "git
commit".  So, if "git commit" fails with a retcode different than 1,
then reschedule_last_action will be called a few lines later.  This
seems incorrect to me.

>   else
> - pick_one $1 ||
> - die_with_patch $1 "Could not apply $1... $2"
> + pick_one $1
> + fi
> + ret=$?
> + if test $ret -ne 0
> + then
> + test $ret -ne 1 && reschedule_last_action
> + die_with_patch $1 "Could not apply $1... $2"
>   fi
>  }
>  
> @@ -533,8 +547,11 @@ do_next () {
>   author_script_content=$(get_author_ident_from_commit HEAD)
>   echo "$author_script_content" > "$author_script"
>   eval "$author_script_content"
> - if ! pick_one -n $sha1
> + pick_one -n $sha1
> + ret=$?
> + if test $ret -ne 0
>   then
> + test $ret -ne 1 && reschedule_last_action
>   git rev-parse --verify HEAD >"$amend"
>   die_failed_squash $sha1 "$rest"
>   fi
> 

I suggest that you add a comment for pick_one explaining that if it
fails, its failure code is like that of cherry-pick, namely ...etc...
This will warn future developers to preserve the error code semantics.

It is preferable to squash the next commit, containing the tests,
together with this commit.

Thanks,
Michael

-- 
Michael Haggerty
mhag...@alum.mit.edu
http://softwareswirl.blogspot.com/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[RFC 2/3] rebase -i: Reschedule tasks that failed before the index was touched

2014-05-26 Thread Fabian Ruch
When `rebase--interactive` processes a task, it removes the item from
the todo list and appends it to another list of executed tasks. If a
`pick` (this includes `squash` and `fixup`) fails before the index has
recorded the changes, take the corresponding item and put it on the todo
list again. Otherwise, the changes introduced by the scheduled commit
would be lost.

That kind of decision is possible since the `cherry-pick` command
signals why it failed to apply the changes of the given commit. Either
the changes are recorded in the index using a conflict (return value 1)
and `rebase` does not continue until they are resolved or the changes
are not recorded in the index (return value neither 0 nor 1) and
`rebase` has to try again with the same task.

Reported-by: Phil Hord 
Signed-off-by: Fabian Ruch 
---
 git-rebase--interactive.sh | 27 ++-
 1 file changed, 22 insertions(+), 5 deletions(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 9e1dd1e..bba4f3a 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -132,6 +132,16 @@ mark_action_done () {
fi
 }
 
+# Put the last action marked done at the beginning of the todo list
+# again. If there has not been an action marked done yet, the list of
+# items on the todo list is left unchanged.
+reschedule_last_action () {
+   tail -n 1 "$done" | cat - "$todo" >"$todo".new
+   sed -e \$d <"$done" >"$done".new
+   mv -f "$todo".new "$todo"
+   mv -f "$done".new "$done"
+}
+
 append_todo_help () {
git stripspace --comment-lines >>"$todo" <<\EOF
 
@@ -470,11 +480,15 @@ do_pick () {
   --no-post-rewrite -n -q -C $1 &&
pick_one -n $1 &&
git commit --allow-empty --allow-empty-message \
-  --amend --no-post-rewrite -n -q -C $1 ||
-   die_with_patch $1 "Could not apply $1... $2"
+  --amend --no-post-rewrite -n -q -C $1
else
-   pick_one $1 ||
-   die_with_patch $1 "Could not apply $1... $2"
+   pick_one $1
+   fi
+   ret=$?
+   if test $ret -ne 0
+   then
+   test $ret -ne 1 && reschedule_last_action
+   die_with_patch $1 "Could not apply $1... $2"
fi
 }
 
@@ -533,8 +547,11 @@ do_next () {
author_script_content=$(get_author_ident_from_commit HEAD)
echo "$author_script_content" > "$author_script"
eval "$author_script_content"
-   if ! pick_one -n $sha1
+   pick_one -n $sha1
+   ret=$?
+   if test $ret -ne 0
then
+   test $ret -ne 1 && reschedule_last_action
git rev-parse --verify HEAD >"$amend"
die_failed_squash $sha1 "$rest"
fi
-- 
1.9.3

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html