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@xxxxxxxxx>
> Signed-off-by: Phil Hord <hordp@xxxxxxxxx>
> ---
>
> 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@xxxxxxxxxxxx>
>
> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html