[PATCH] rebase -i: redo tasks that die during cherry-pick

[PATCH] rebase -i: redo tasks that die during cherry-pick

[Phil Hord]

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.

Add a test cases for regression testing to the "rebase-interactive"
test suite.

Signed-off-by: Fabian Ruch <bafain@gmail.com>
Signed-off-by: Phil Hord <hordp@cisco.com>
---

Notes:
    Last year in ${gmane}/250126 Fabian Ruch helpfully provided a patch
    to fix a rebase bug I complained about. I have simplified it a bit
    and merged in the tests which had been in a separate commit.
    
    It has bitten me twice since the original discussion and has also
    been reported by others, though I haven't found those emails to
    add them to the CC list yet.
    
    CC: Michael Haggerty <mhagger@alum.mit.edu>

 git-rebase--interactive.sh    | 16 +++++++++++++++
 t/t3404-rebase-interactive.sh | 47 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 63 insertions(+)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 08e5d86..bab0dcc 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, 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
 
@@ -252,6 +262,12 @@ pick_one () {
 	output eval git cherry-pick \
 			${gpg_sign_opt:+$(git rev-parse --sq-quote "$gpg_sign_opt")} \
 			"$strategy_args" $empty_args $ff "$@"
+
+	# If cherry-pick dies it leaves the to-be-picked commit unrecorded. Reschedule
+	# previous task so this commit is not lost.
+	ret=$?
+	case "$ret" in [01]) ;; *) reschedule_last_action ;; esac
+	return $ret
 }
 
 pick_one_preserving_merges () {
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index eed76cc..ac429a0 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -1055,4 +1055,51 @@ test_expect_success 'todo count' '
 	grep "^# Rebase ..* onto ..* ([0-9]" actual
 '
 
+test_expect_success 'rebase -i commits that overwrite untracked files (pick)' '
+	git checkout --force branch2 &&
+	git clean -f &&
+	set_fake_editor &&
+	FAKE_LINES="edit 1 2" git rebase -i A &&
+	test_cmp_rev HEAD F &&
+	test_path_is_missing file6 &&
+	>file6 &&
+	test_must_fail git rebase --continue &&
+	test_cmp_rev HEAD F &&
+	rm file6 &&
+	git rebase --continue &&
+	test_cmp_rev HEAD I
+'
+
+test_expect_success 'rebase -i commits that overwrite untracked files (squash)' '
+	git checkout --force branch2 &&
+	git clean -f &&
+	git tag original-branch2 &&
+	set_fake_editor &&
+	FAKE_LINES="edit 1 squash 2" git rebase -i A &&
+	test_cmp_rev HEAD F &&
+	test_path_is_missing file6 &&
+	>file6 &&
+	test_must_fail git rebase --continue &&
+	test_cmp_rev HEAD F &&
+	rm file6 &&
+	git rebase --continue &&
+	test $(git cat-file commit HEAD | sed -ne \$p) = I &&
+	git reset --hard original-branch2
+'
+
+test_expect_success 'rebase -i commits that overwrite untracked files (no ff)' '
+	git checkout --force branch2 &&
+	git clean -f &&
+	set_fake_editor &&
+	FAKE_LINES="edit 1 2" git rebase -i --no-ff A &&
+	test $(git cat-file commit HEAD | sed -ne \$p) = F &&
+	test_path_is_missing file6 &&
+	>file6 &&
+	test_must_fail git rebase --continue &&
+	test $(git cat-file commit HEAD | sed -ne \$p) = F &&
+	rm file6 &&
+	git rebase --continue &&
+	test $(git cat-file commit HEAD | sed -ne \$p) = I
+'
+
 test_done
-- 
2.4.0.rc3.329.gd1f7d3b

Re: [PATCH] rebase -i: redo tasks that die during cherry-pick

[Johannes Schindelin]

Hi,

On 2015-04-29 00:55, Phil Hord 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.
> 
> Add a test cases for regression testing to the "rebase-interactive"
> test suite.
> 
> Signed-off-by: Fabian Ruch <bafain@gmail.com>
> Signed-off-by: Phil Hord <hordp@cisco.com>
> ---

ACK.

It would be even nicer to avoid removing the task from the `todo` list until it has been performed correctly, of course, but I believe that would require a much more invasive patch. So this here patch is fine with me.

Ciao,
Dscho

Re: [PATCH] rebase -i: redo tasks that die during cherry-pick

[Junio C Hamano]

Johannes Schindelin <johannes.schindelin@gmx.de> writes:

> Hi,
>
> On 2015-04-29 00:55, Phil Hord 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.
>> 
>> Add a test cases for regression testing to the "rebase-interactive"
>> test suite.
>> 
>> Signed-off-by: Fabian Ruch <bafain@gmail.com>
>> Signed-off-by: Phil Hord <hordp@cisco.com>
>> ---
>
> ACK.
>
> It would be even nicer to avoid removing the task from the `todo` list
> until it has been performed correctly, of course, but I believe that
> would require a much more invasive patch. So this here patch is fine
> with me.

Thanks, will queue.

Aside from the "much more invasive" possibility, the patch makes me
wonder if it would have been a better design to have a static "todo"
with a "current" pointer as two state files.  Then reschedule would
have been just the matter of decrementing the number in "current",
instead of "grab the last line of one file and prepend to the other
file, and then lose the last line".

Re: [PATCH] rebase -i: redo tasks that die during cherry-pick

[Phil Hord]

On Wed, Apr 29, 2015 at 1:15 PM, Junio C Hamano <gitster@pobox.com> wrote:
>
> Thanks, will queue.
>
> Aside from the "much more invasive" possibility, the patch makes me
> wonder if it would have been a better design to have a static "todo"
> with a "current" pointer as two state files.  Then reschedule would
> have been just the matter of decrementing the number in "current",
> instead of "grab the last line of one file and prepend to the other
> file, and then lose the last line".

That's an interesting idea.  Changing it now would impact anyone who
now depends on the current todo/done behavior, and I imagine there
are many.

Re: [PATCH] rebase -i: redo tasks that die during cherry-pick

[Junio C Hamano]

Phil Hord <phil.hord@gmail.com> writes:

> On Wed, Apr 29, 2015 at 1:15 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>
>> Thanks, will queue.
>>
>> Aside from the "much more invasive" possibility, the patch makes me
>> wonder if it would have been a better design to have a static "todo"
>> with a "current" pointer as two state files.  Then reschedule would
>> have been just the matter of decrementing the number in "current",
>> instead of "grab the last line of one file and prepend to the other
>> file, and then lose the last line".
>
> That's an interesting idea.  Changing it now would impact anyone who
> now depends on the current todo/done behavior, and I imagine there
> are many.

Yeah, in case it wasn't clear, I was merely lamenting over water
under the bridge, not seriously suggesting to break users to
simplify our logic.

rebase -i's todo/done list, was Re: [PATCH] rebase -i: redo tasks that die during cherry-pick

[Johannes Schindelin]

Hi Junio,

On 2015-04-29 19:15, Junio C Hamano wrote:

> [I] wonder if it would have been a better design to have a static "todo" with a "current" pointer as two state files.

I decided against this in my original design because I frequently edited the todo list during the rebase run, already back when I had still called it `edit-patch-series.sh` (you will remember that "Saturday project" I reported on IRC).

One example for such an editing happens to me quite frequently: I reorder patches into a more logical order, except that I make mistakes and try to pull a patch too early, i.e. before the code it touches was added. In such a case, I obviously get a merge conflict, and after analysing the problem I have a better idea where to put that patch, which I do by adding a "pick deadbeef" into the `git-rebase-todo` list (these days, I use `git rebase --edit-todo`, of course).

If the list and the "current" pointer would be two different things, it would be way too easy to incur inconsistencies. Even for the inventor of rebase -i.

> Then reschedule would have been just the matter of decrementing the number in "current", instead of "grab the last line of one file and prepend to the other file, and then lose the last line".

I think that is only an implementation detail: whether we decrement a pointer or put back a line we moved to another file is essentially the same operation: it is a roll-back. The "more invasive" change I implied was to *avoid* the need for a roll-back, i.e. only move that line (or in your example, increment the pointer) after the operation completed successfully.

My implied change would mean that all the `mark_action_done` calls would have to be moved just before the `record_in_rewritten` calls -- but not all actions have that call. The tricky bit is that there are side effects: `peek_next_command` relies on the current behavior and would have to be adjusted. And that is only the side effect that I can think of off the top of my head.

Ciao,
Dscho

Re: [PATCH] rebase -i: redo tasks that die during cherry-pick

[Fabian Ruch]

Hi,

Phil Hord writes:
> 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.
> 
> Add a test cases for regression testing to the "rebase-interactive"
> test suite.
> 
> Signed-off-by: Fabian Ruch <bafain@gmail.com>
> Signed-off-by: Phil Hord <hordp@cisco.com>
> ---
> 
> Notes:
>     Last year in ${gmane}/250126 Fabian Ruch helpfully provided a patch
>     to fix a rebase bug I complained about. I have simplified it a bit
>     and merged in the tests which had been in a separate commit.

Thanks for picking this up again.

It seems like the bug still shows when git-rebase is asked to
recreate merges (option -p):

   set_fake_editor &&
   # 1 (A) adds file4, 2 (I) adds file6
   FAKE_LINES="edit 1 2" git rebase -i -p A &&
   git commit --amend &&
   >file6 &&
   # merge fails because file6 would be overwritten
   test_must_fail git rebase --continue &&
   # succeeds because 2 was popped and is not tried again
   git rebase --continue &&
   # ERROR: top commit is A because I was skipped
   test $(git cat-file commit HEAD | sed -ne \$p) = I

(The git-commit command in the third line forces
pick_one_preserving_merges to recreate rather than fast-forward to
the commit that adds file6. Normally, one would instruct git-rebase
to do so by specifying the --no-ff option. However, another bug
(thread 252946) causes the --no-ff to be ignored in conjunction with
-p; this example case then succeeds for the wrong reasons.)

>     It has bitten me twice since the original discussion and has also
>     been reported by others, though I haven't found those emails to
>     add them to the CC list yet.
>     
>     CC: Michael Haggerty <mhagger@alum.mit.edu>
> 
>  git-rebase--interactive.sh    | 16 +++++++++++++++
>  t/t3404-rebase-interactive.sh | 47 +++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 63 insertions(+)
> 
> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> index 08e5d86..bab0dcc 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, 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
>  
> @@ -252,6 +262,12 @@ pick_one () {
>  	output eval git cherry-pick \
>  			${gpg_sign_opt:+$(git rev-parse --sq-quote "$gpg_sign_opt")} \
>  			"$strategy_args" $empty_args $ff "$@"
> +
> +	# If cherry-pick dies it leaves the to-be-picked commit unrecorded. Reschedule
> +	# previous task so this commit is not lost.
> +	ret=$?
> +	case "$ret" in [01]) ;; *) reschedule_last_action ;; esac
> +	return $ret
>  }

If -p is specified, pick_one doesn't reach the git-cherry-pick
command line shown here. Instead, it enters
pick_one_preserving_merges one line above and returns from pick_one
immediately afterwards.

   Fabian

>  pick_one_preserving_merges () {
> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> index eed76cc..ac429a0 100755
> --- a/t/t3404-rebase-interactive.sh
> +++ b/t/t3404-rebase-interactive.sh
> @@ -1055,4 +1055,51 @@ test_expect_success 'todo count' '
>  	grep "^# Rebase ..* onto ..* ([0-9]" actual
>  '
>  
> +test_expect_success 'rebase -i commits that overwrite untracked files (pick)' '
> +	git checkout --force branch2 &&
> +	git clean -f &&
> +	set_fake_editor &&
> +	FAKE_LINES="edit 1 2" git rebase -i A &&
> +	test_cmp_rev HEAD F &&
> +	test_path_is_missing file6 &&
> +	>file6 &&
> +	test_must_fail git rebase --continue &&
> +	test_cmp_rev HEAD F &&
> +	rm file6 &&
> +	git rebase --continue &&
> +	test_cmp_rev HEAD I
> +'
> +
> +test_expect_success 'rebase -i commits that overwrite untracked files (squash)' '
> +	git checkout --force branch2 &&
> +	git clean -f &&
> +	git tag original-branch2 &&
> +	set_fake_editor &&
> +	FAKE_LINES="edit 1 squash 2" git rebase -i A &&
> +	test_cmp_rev HEAD F &&
> +	test_path_is_missing file6 &&
> +	>file6 &&
> +	test_must_fail git rebase --continue &&
> +	test_cmp_rev HEAD F &&
> +	rm file6 &&
> +	git rebase --continue &&
> +	test $(git cat-file commit HEAD | sed -ne \$p) = I &&
> +	git reset --hard original-branch2
> +'
> +
> +test_expect_success 'rebase -i commits that overwrite untracked files (no ff)' '
> +	git checkout --force branch2 &&
> +	git clean -f &&
> +	set_fake_editor &&
> +	FAKE_LINES="edit 1 2" git rebase -i --no-ff A &&
> +	test $(git cat-file commit HEAD | sed -ne \$p) = F &&
> +	test_path_is_missing file6 &&
> +	>file6 &&
> +	test_must_fail git rebase --continue &&
> +	test $(git cat-file commit HEAD | sed -ne \$p) = F &&
> +	rm file6 &&
> +	git rebase --continue &&
> +	test $(git cat-file commit HEAD | sed -ne \$p) = I
> +'
> +
>  test_done