rebase -i: always update HEAD before rewording

If the user runs git log while rewording a commit it is confusing if
sometimes we're amending the commit that's being reworded and at other
times we're creating a new commit depending on whether we could
fast-forward or not[1]. Fix this inconsistency by always committing the
picked commit and then running 'git commit --amend' to do the reword.

The first commit is performed by the sequencer without forking git
commit and does not impact on the speed of rebase. In a test rewording
100 commits with

    GIT_EDITOR=true GIT_SEQUENCE_EDITOR='sed -i s/pick/reword/' \
	../bin-wrappers/git rebase -i --root

and taking the best of three runs the current master took
957ms and with this patch it took 961ms.

This change fixes rewording the new root commit when rearranging commits
with --root.

Note that the new code no longer updates CHERRY_PICK_HEAD after creating
a root commit - I'm not sure why the old code was that creating that ref
after a successful commit, everywhere else it is removed after a
successful commit.

[1] https://public-inbox.org/git/xmqqlfvu4be3.fsf@gitster-ct.c.googlers.com/T/#m133009cb91cf0917bcf667300f061178be56680a

Reported-by: SZEDER Gábor <szeder.dev@gmail.com>
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 1d206fd22456cce12f7496321df8a47fcb94db9e..3ef0d5e04f3076435111ae03ea98718323de6475 100644 (file)
--- a/sequencer.c
+++ b/sequencer.c
@@ -986,12 +986,10 @@ static int run_git_commit(struct repository *r,
 
                strbuf_release(&msg);
                strbuf_release(&script);
-               if (!res) {
-                       update_ref(NULL, "CHERRY_PICK_HEAD", &root_commit, NULL,
-                                  REF_NO_DEREF, UPDATE_REFS_MSG_ON_ERR);
+               if (!res)
                        res = update_ref(NULL, "HEAD", &root_commit, NULL, 0,
                                         UPDATE_REFS_MSG_ON_ERR);
-               }
+
                return res < 0 ? error(_("writing root commit")) : 0;
        }
 
@@ -1785,7 +1783,7 @@ static int do_pick_commit(struct repository *r,
        char *author = NULL;
        struct commit_message msg = { NULL, NULL, NULL, NULL };
        struct strbuf msgbuf = STRBUF_INIT;
-       int res, unborn = 0, allow;
+       int res, unborn = 0, reword = 0, allow;
 
        if (opts->no_commit) {
                /*
@@ -1855,7 +1853,7 @@ static int do_pick_commit(struct repository *r,
                        opts);
                if (res || command != TODO_REWORD)
                        goto leave;
-               flags |= EDIT_MSG | AMEND_MSG | VERIFY_MSG;
+               reword = 1;
                msg_file = NULL;
                goto fast_forward_edit;
        }
@@ -1913,7 +1911,7 @@ static int do_pick_commit(struct repository *r,
        }
 
        if (command == TODO_REWORD)
-               flags |= EDIT_MSG | VERIFY_MSG;
+               reword = 1;
        else if (is_fixup(command)) {
                if (update_squash_messages(r, command, commit, opts))
                        return -1;
@@ -1997,13 +1995,18 @@ static int do_pick_commit(struct repository *r,
        } else if (allow)
                flags |= ALLOW_EMPTY;
        if (!opts->no_commit) {
-fast_forward_edit:
                if (author || command == TODO_REVERT || (flags & AMEND_MSG))
                        res = do_commit(r, msg_file, author, opts, flags);
                else
                        res = error(_("unable to parse commit author"));
+               if (!res && reword)
+fast_forward_edit:
+                       res = run_git_commit(r, NULL, opts, EDIT_MSG |
+                                            VERIFY_MSG | AMEND_MSG |
+                                            (flags & ALLOW_EMPTY));
        }
 
+
        if (!res && final_fixup) {
                unlink(rebase_path_fixup_msg());
                unlink(rebase_path_squash_msg());

diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 461dd539ffd4803c62d54e22e8921fbefa2c0786..d2f1d5bd235fd529007e4c1f17231289af357bd6 100755 (executable)
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -1016,9 +1016,9 @@ test_expect_success 'rebase -i --root fixup root commit' '
        test 0 = $(git cat-file commit HEAD | grep -c ^parent\ )
 '
 
-test_expect_success 'rebase -i --root reword root commit' '
+test_expect_success 'rebase -i --root reword original root commit' '
        test_when_finished "test_might_fail git rebase --abort" &&
-       git checkout -b reword-root-branch master &&
+       git checkout -b reword-original-root-branch master &&
        set_fake_editor &&
        FAKE_LINES="reword 1 2" FAKE_COMMIT_MESSAGE="A changed" \
        git rebase -i --root &&
@@ -1026,6 +1026,16 @@ test_expect_success 'rebase -i --root reword root commit' '
        test -z "$(git show -s --format=%p HEAD^)"
 '
 
+test_expect_success 'rebase -i --root reword new root commit' '
+       test_when_finished "test_might_fail git rebase --abort" &&
+       git checkout -b reword-now-root-branch master &&
+       set_fake_editor &&
+       FAKE_LINES="reword 3 1" FAKE_COMMIT_MESSAGE="C changed" \
+       git rebase -i --root &&
+       git show HEAD^ | grep "C changed" &&
+       test -z "$(git show -s --format=%p HEAD^)"
+'
+
 test_expect_success 'rebase -i --root when root has untracked file conflict' '
        test_when_finished "reset_rebase" &&
        git checkout -b failing-root-pick A &&
@@ -1054,7 +1064,7 @@ test_expect_success 'rebase -i --root reword root when root has untracked file c
 '
 
 test_expect_success C_LOCALE_OUTPUT 'rebase --edit-todo does not work on non-interactive rebase' '
-       git checkout reword-root-branch &&
+       git checkout reword-original-root-branch &&
        git reset --hard &&
        git checkout conflict-branch &&
        set_fake_editor &&

diff --git a/t/t7505-prepare-commit-msg-hook.sh b/t/t7505-prepare-commit-msg-hook.sh
index ba8bd1b51497df1aedf2280210a98b389f12b9e7..94f85cdf831790a27ac2878bf483a6a0c0f1c441 100755 (executable)
--- a/t/t7505-prepare-commit-msg-hook.sh
+++ b/t/t7505-prepare-commit-msg-hook.sh
@@ -241,13 +241,7 @@ test_rebase () {
                        git add b &&
                        git rebase --continue
                ) &&
-               if test "$mode" = -p # reword amended after pick
-               then
-                       n=18
-               else
-                       n=17
-               fi &&
-               git log --pretty=%s -g -n$n HEAD@{1} >actual &&
+               git log --pretty=%s -g -n18 HEAD@{1} >actual &&
                test_cmp "$TEST_DIRECTORY/t7505/expected-rebase${mode:--i}" actual
        '
 }

diff --git a/t/t7505/expected-rebase-i b/t/t7505/expected-rebase-i
index c514bdbb9422c0f699a3e1c1514b41e0796214a5..93bada596e25f7148fdf0b955211cedfc0fbdba3 100644 (file)
--- a/t/t7505/expected-rebase-i
+++ b/t/t7505/expected-rebase-i
@@ -7,7 +7,8 @@ message (no editor) [edit rebase-10]
 message [fixup rebase-9]
 message (no editor) [fixup rebase-8]
 message (no editor) [squash rebase-7]
-message [reword rebase-6]
+HEAD [reword rebase-6]
+message (no editor) [reword rebase-6]
 message [squash rebase-5]
 message (no editor) [fixup rebase-4]
 message (no editor) [pick rebase-3]