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

From: Phil Hord <hordp@cisco.com>
Date: May 27, 2014, 18:26 UTC
Message-ID: <5384D8DA.40005@cisco.com>
In-Reply-To: <53847D7D.5050000@alum.mit.edu>

Hi Fabian,

Thanks for looking into this.

On 05/27/2014 07:56 AM, Michael Haggerty wrote:

Show 23 quoted lines

>> +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