rebase -i: fix adding failed command to the todo list

When rebasing commands are moved from the todo list in "git-rebase-todo"
to the "done" file (which is used by "git status" to show the recently
executed commands) just before they are executed. This means that if a
command fails because it would overwrite an untracked file it has to be
added back into the todo list before the rebase stops for the user to
fix the problem.

Unfortunately when a failed command is added back into the todo list the
command preceding it is erroneously appended to the "done" file.  This
means that when rebase stops after "pick B" fails the "done" file
contains

	pick A
	pick B
	pick A

instead of

	pick A
	pick B

This happens because save_todo() updates the "done" file with the
previous command whenever "git-rebase-todo" is updated. When we add the
failed pick back into "git-rebase-todo" we do not want to update
"done". Fix this by adding a "reschedule" parameter to save_todo() which
prevents the "done" file from being updated when adding a failed command
back into the "git-rebase-todo" file. A couple of the existing tests are
modified to improve their coverage as none of them trigger this bug or
check the "done" file.

Reported-by: Stefan Haller <lists@haller-berlin.de>
Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/sequencer.c b/sequencer.c
index f9ef301d632980d3890aeabbba6e412dee765161..f014c60cc8a8914a4ae8a42b217278b09cbbeffd 100644 (file)
--- a/sequencer.c
+++ b/sequencer.c
@@ -3380,7 +3380,8 @@ give_advice:
        return -1;
 }
 
-static int save_todo(struct todo_list *todo_list, struct replay_opts *opts)
+static int save_todo(struct todo_list *todo_list, struct replay_opts *opts,
+                    int reschedule)
 {
        struct lock_file todo_lock = LOCK_INIT;
        const char *todo_path = get_todo_path(opts);
@@ -3390,7 +3391,7 @@ static int save_todo(struct todo_list *todo_list, struct replay_opts *opts)
         * rebase -i writes "git-rebase-todo" without the currently executing
         * command, appending it to "done" instead.
         */
-       if (is_rebase_i(opts))
+       if (is_rebase_i(opts) && !reschedule)
                next++;
 
        fd = hold_lock_file_for_update(&todo_lock, todo_path, 0);
@@ -3403,7 +3404,7 @@ static int save_todo(struct todo_list *todo_list, struct replay_opts *opts)
        if (commit_lock_file(&todo_lock) < 0)
                return error(_("failed to finalize '%s'"), todo_path);
 
-       if (is_rebase_i(opts) && next > 0) {
+       if (is_rebase_i(opts) && !reschedule && next > 0) {
                const char *done = rebase_path_done();
                int fd = open(done, O_CREAT | O_WRONLY | O_APPEND, 0666);
                int ret = 0;
@@ -4716,7 +4717,7 @@ static int pick_commits(struct repository *r,
                const char *arg = todo_item_get_arg(todo_list, item);
                int check_todo = 0;
 
-               if (save_todo(todo_list, opts))
+               if (save_todo(todo_list, opts, reschedule))
                        return -1;
                if (is_rebase_i(opts)) {
                        if (item->command != TODO_COMMENT) {
@@ -4797,8 +4798,7 @@ static int pick_commits(struct repository *r,
                               get_item_line_length(todo_list,
                                                    todo_list->current),
                               get_item_line(todo_list, todo_list->current));
-                       todo_list->current--;
-                       if (save_todo(todo_list, opts))
+                       if (save_todo(todo_list, opts, reschedule))
                                return -1;
                        if (item->commit)
                                write_rebase_head(&item->commit->object.oid);

diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index a8ad398956ab47c2de223094270fd8d6fb0887c7..71da9c465a1fb2095621c5047b5e7a1b77a42dc7 100755 (executable)
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -1277,19 +1277,24 @@ test_expect_success 'todo count' '
 '
 
 test_expect_success 'rebase -i commits that overwrite untracked files (pick)' '
-       git checkout --force branch2 &&
+       git checkout --force A &&
        git clean -f &&
+       cat >todo <<-EOF &&
+       exec >file2
+       pick $(git rev-parse B) B
+       pick $(git rev-parse C) C
+       pick $(git rev-parse D) D
+       exec cat .git/rebase-merge/done >actual
+       EOF
        (
-               set_fake_editor &&
-               FAKE_LINES="edit 1 2" git rebase -i A
+               set_replace_editor todo &&
+               test_must_fail 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 &&
-       test_cmp_rev REBASE_HEAD I &&
-       rm file6 &&
+       test_cmp_rev HEAD B &&
+       test_cmp_rev REBASE_HEAD C &&
+       head -n3 todo >expect &&
+       test_cmp expect .git/rebase-merge/done &&
+       rm file2 &&
        test_path_is_missing .git/rebase-merge/patch &&
        echo changed >file1 &&
        git add file1 &&
@@ -1297,7 +1302,9 @@ test_expect_success 'rebase -i commits that overwrite untracked files (pick)' '
        grep "error: you have staged changes in your working tree" err &&
        git reset --hard HEAD &&
        git rebase --continue &&
-       test_cmp_rev HEAD I
+       test_cmp_rev HEAD D &&
+       tail -n3 todo >>expect &&
+       test_cmp expect actual
 '
 
 test_expect_success 'rebase -i commits that overwrite untracked files (squash)' '

diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh
index bf75080281e334a48559b1ee022adb409ae21f86..2668be6a4b0f158f5d02d133aae59d6899da8c64 100755 (executable)
--- a/t/t3430-rebase-merges.sh
+++ b/t/t3430-rebase-merges.sh
@@ -128,14 +128,24 @@ test_expect_success 'generate correct todo list' '
 '
 
 test_expect_success '`reset` refuses to overwrite untracked files' '
-       git checkout -b refuse-to-reset &&
+       git checkout B &&
        test_commit dont-overwrite-untracked &&
-       git checkout @{-1} &&
-       : >dont-overwrite-untracked.t &&
-       echo "reset refs/tags/dont-overwrite-untracked" >script-from-scratch &&
+       cat >script-from-scratch <<-EOF &&
+       exec >dont-overwrite-untracked.t
+       pick $(git rev-parse B) B
+       reset refs/tags/dont-overwrite-untracked
+       pick $(git rev-parse C) C
+       exec cat .git/rebase-merge/done >actual
+       EOF
        test_config sequence.editor \""$PWD"/replace-editor.sh\" &&
-       test_must_fail git rebase -ir HEAD &&
-       git rebase --abort
+       test_must_fail git rebase -ir A &&
+       test_cmp_rev HEAD B &&
+       head -n3 script-from-scratch >expect &&
+       test_cmp expect .git/rebase-merge/done &&
+       rm dont-overwrite-untracked.t &&
+       git rebase --continue &&
+       tail -n3 script-from-scratch >>expect &&
+       test_cmp expect actual
 '
 
 test_expect_success '`reset` rejects trees' '