Re: [PATCH] rebase -i: redo tasks that die during cherry-pick
From: Fabian Ruch <hidden>
Date: 2016-06-15 23:04:35
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 <redacted>
Signed-off-by: Phil Hord <redacted>
---
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.)
quoted hunk ↗ jump to hunk
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 [off-list ref] 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
quoted hunk ↗ jump to hunk
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