Re: [PATCH] rebase -i: redo tasks that die during cherry-pick
- From
Fabian Ruch <bafain@gmail.com>
- Date
- Apr 30, 2015, 09:54 UTC
- Message-ID
- <5541FBEA.6080804@gmail.com>
- In-Reply-To
- <1430261720-9051-1-git-send-email-hordp@cisco.com>
Hi,
Phil Hord writes:
Show 25 quoted lines
> 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.)
Show 42 quoted lines
> 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
Show 57 quoted lines
> 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