From 2be6b6f411136c6773a90d2cfdacf3cccb79d3e6 Mon Sep 17 00:00:00 2001 From: Phillip Wood Date: Fri, 20 Aug 2021 15:40:35 +0000 Subject: [PATCH 1/4] rebase -r: make 'merge -c' behave like reword 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]. For this reason the reword command ensures that there are no uncommitted changes when rewording. The reword command also allows the user to edit the todo list while the rebase is paused. As 'merge -c' also rewords commits make it behave like reword and add a test. [1] https://lore.kernel.org/git/xmqqlfvu4be3.fsf@gitster-ct.c.googlers.com/T/#m133009cb91cf0917bcf667300f061178be56680a Signed-off-by: Phillip Wood Signed-off-by: Junio C Hamano --- sequencer.c | 24 +++++++++++-------- t/lib-rebase.sh | 50 ++++++++++++++++++++++++++++++++++++++++ t/t3430-rebase-merges.sh | 20 ++++++++-------- 3 files changed, 75 insertions(+), 19 deletions(-) diff --git a/sequencer.c b/sequencer.c index 7f07cd00f3..cc8a361cce 100644 --- a/sequencer.c +++ b/sequencer.c @@ -3739,10 +3739,9 @@ static struct commit *lookup_label(const char *label, int len, static int do_merge(struct repository *r, struct commit *commit, const char *arg, int arg_len, - int flags, struct replay_opts *opts) + int flags, int *check_todo, struct replay_opts *opts) { - int run_commit_flags = (flags & TODO_EDIT_MERGE_MSG) ? - EDIT_MSG | VERIFY_MSG : 0; + int run_commit_flags = 0; struct strbuf ref_name = STRBUF_INIT; struct commit *head_commit, *merge_commit, *i; struct commit_list *bases, *j, *reversed = NULL; @@ -3898,10 +3897,9 @@ static int do_merge(struct repository *r, rollback_lock_file(&lock); ret = fast_forward_to(r, &commit->object.oid, &head_commit->object.oid, 0, opts); - if (flags & TODO_EDIT_MERGE_MSG) { - run_commit_flags |= AMEND_MSG; + if (flags & TODO_EDIT_MERGE_MSG) goto fast_forward_edit; - } + goto leave_merge; } @@ -4035,10 +4033,17 @@ static int do_merge(struct repository *r, * value (a negative one would indicate that the `merge` * command needs to be rescheduled). */ - fast_forward_edit: ret = !!run_git_commit(git_path_merge_msg(r), opts, run_commit_flags); + if (!ret && flags & TODO_EDIT_MERGE_MSG) { + fast_forward_edit: + *check_todo = 1; + run_commit_flags |= AMEND_MSG | EDIT_MSG | VERIFY_MSG; + ret = !!run_git_commit(NULL, opts, run_commit_flags); + } + + leave_merge: strbuf_release(&ref_name); rollback_lock_file(&lock); @@ -4405,9 +4410,8 @@ static int pick_commits(struct repository *r, if ((res = do_reset(r, arg, item->arg_len, opts))) reschedule = 1; } else if (item->command == TODO_MERGE) { - if ((res = do_merge(r, item->commit, - arg, item->arg_len, - item->flags, opts)) < 0) + if ((res = do_merge(r, item->commit, arg, item->arg_len, + item->flags, &check_todo, opts)) < 0) reschedule = 1; else if (item->commit) record_in_rewritten(&item->commit->object.oid, diff --git a/t/lib-rebase.sh b/t/lib-rebase.sh index dc75b83451..99d9e7efd2 100644 --- a/t/lib-rebase.sh +++ b/t/lib-rebase.sh @@ -151,3 +151,53 @@ test_editor_unchanged () { EOF test_cmp expect actual } + +# Set up an editor for testing reword commands +# Checks that there are no uncommitted changes when rewording and that the +# todo-list is reread after each +set_reword_editor () { + >reword-actual && + >reword-oid && + + # Check rewording keeps the original authorship + GIT_AUTHOR_NAME="Reword Author" + GIT_AUTHOR_EMAIL="reword.author@example.com" + GIT_AUTHOR_DATE=@123456 + + write_script reword-sequence-editor.sh <<-\EOF && + todo="$(cat "$1")" && + echo "exec git log -1 --pretty=format:'%an <%ae> %at%n%B%n' \ + >>reword-actual" >"$1" && + printf "%s\n" "$todo" >>"$1" + EOF + + write_script reword-editor.sh <<-EOF && + # Save the oid of the first reworded commit so we can check rebase + # fast-forwards to it + if ! test -s reword-oid + then + git rev-parse HEAD >reword-oid + fi && + # There should be no uncommited changes + git diff --exit-code HEAD && + # The todo-list should be re-read after a reword + GIT_SEQUENCE_EDITOR="\"$PWD/reword-sequence-editor.sh\"" \ + git rebase --edit-todo && + echo edited >>"\$1" + EOF + + test_set_editor "$PWD/reword-editor.sh" +} + +# Check the results of a rebase after calling set_reword_editor +# Pass the commits that were reworded in the order that they were picked +# Expects the first pick to be a fast-forward +check_reworded_commits () { + test_cmp_rev "$(cat reword-oid)" "$1^{commit}" && + git log --format="%an <%ae> %at%n%B%nedited%n" --no-walk=unsorted "$@" \ + >reword-expected && + test_cmp reword-expected reword-actual && + git log --format="%an <%ae> %at%n%B" -n $# --first-parent --reverse \ + >reword-log && + test_cmp reword-expected reword-log +} diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh index 6748070df5..183c3a2383 100755 --- a/t/t3430-rebase-merges.sh +++ b/t/t3430-rebase-merges.sh @@ -172,17 +172,19 @@ test_expect_success 'failed `merge ` does not crash' ' grep "^Merge branch ${SQ}G${SQ}$" .git/rebase-merge/message ' -test_expect_success 'fast-forward merge -c still rewords' ' - git checkout -b fast-forward-merge-c H && +test_expect_success 'merge -c commits before rewording and reloads todo-list' ' + cat >script-from-scratch <<-\EOF && + merge -c E B + merge -c H G + EOF + + git checkout -b merge-c H && ( - set_fake_editor && - FAKE_COMMIT_MESSAGE=edited \ - GIT_SEQUENCE_EDITOR="echo merge -c H G >" \ - git rebase -ir @^ + set_reword_editor && + GIT_SEQUENCE_EDITOR="\"$PWD/replace-editor.sh\"" \ + git rebase -i -r D ) && - echo edited >expected && - git log --pretty=format:%B -1 >actual && - test_cmp expected actual + check_reworded_commits E H ' test_expect_success 'with a branch tip that was cherry-picked already' ' From 0c164ae7a65f811ef3fb3188c63cd157335463bb Mon Sep 17 00:00:00 2001 From: Phillip Wood Date: Fri, 20 Aug 2021 15:40:36 +0000 Subject: [PATCH 2/4] rebase -i: add another reword test None of the existing reword tests check that there are no uncommitted changes when the editor is opened. Reuse the editor script from the last commit to fix this omission. Signed-off-by: Phillip Wood Signed-off-by: Junio C Hamano --- t/t3404-rebase-interactive.sh | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh index 66bcbbf952..d877872e8f 100755 --- a/t/t3404-rebase-interactive.sh +++ b/t/t3404-rebase-interactive.sh @@ -839,6 +839,19 @@ test_expect_success 'reword' ' git show HEAD~2 | grep "C changed" ' +test_expect_success 'no uncommited changes when rewording the todo list is reloaded' ' + git checkout E && + test_when_finished "git checkout @{-1}" && + ( + set_fake_editor && + GIT_SEQUENCE_EDITOR="\"$PWD/fake-editor.sh\"" && + export GIT_SEQUENCE_EDITOR && + set_reword_editor && + FAKE_LINES="reword 1 reword 2" git rebase -i C + ) && + check_reworded_commits D E +' + test_expect_success 'rebase -i can copy notes' ' git config notes.rewrite.rebase true && git config notes.rewriteRef "refs/notes/*" && From baf8ec8d3a0327307f79efd31d5717a0c91c4d8c Mon Sep 17 00:00:00 2001 From: Phillip Wood Date: Fri, 20 Aug 2021 15:40:37 +0000 Subject: [PATCH 3/4] rebase -r: don't write .git/MERGE_MSG when fast-forwarding MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When fast-forwarding we do not create a new commit so .git/MERGE_MSG is not removed and can end up seeding the message of a commit made after the rebase has finished. Avoid writing .git/MERGE_MSG when we are fast-forwarding by writing the file after the fast-forward checks. Note that there are no changes to the fast-forward code, it is simply moved. Note that the way this change is implemented means we no longer write the author script when fast-forwarding either. I believe this is safe for the reasons below but it is a departure from what we do when fast-forwarding a non-merge commit. If we reword the merge then 'git commit --amend' will keep the authorship of the commit we're rewording as it ignores GIT_AUTHOR_* unless --reset-author is passed. It will also export the correct GIT_AUTHOR_* variables to any hooks and we already test the authorship of the reworded commit. If we are not rewording then we no longer call spilt_ident() which means we are no longer checking the commit author header looks sane. However this is what we already do when fast-forwarding non-merge commits in skip_unnecessary_picks() so I don't think we're breaking any promises by not checking the author here. Reported-by: SZEDER Gábor Signed-off-by: Phillip Wood Signed-off-by: Junio C Hamano --- sequencer.c | 81 +++++++++++++++++++++++++------------------------ t/lib-rebase.sh | 10 ++++-- 2 files changed, 49 insertions(+), 42 deletions(-) diff --git a/sequencer.c b/sequencer.c index cc8a361cce..c2cba5ed4b 100644 --- a/sequencer.c +++ b/sequencer.c @@ -983,7 +983,8 @@ static int run_git_commit(const char *defmsg, cmd.git_cmd = 1; - if (is_rebase_i(opts) && read_env_script(&cmd.env_array)) { + if (is_rebase_i(opts) && !(!defmsg && (flags & AMEND_MSG)) && + read_env_script(&cmd.env_array)) { const char *gpg_opt = gpg_sign_opt_quoted(opts); return error(_(staged_changes_advice), @@ -3815,6 +3816,45 @@ static int do_merge(struct repository *r, goto leave_merge; } + /* + * If HEAD is not identical to the first parent of the original merge + * commit, we cannot fast-forward. + */ + can_fast_forward = opts->allow_ff && commit && commit->parents && + oideq(&commit->parents->item->object.oid, + &head_commit->object.oid); + + /* + * If any merge head is different from the original one, we cannot + * fast-forward. + */ + if (can_fast_forward) { + struct commit_list *p = commit->parents->next; + + for (j = to_merge; j && p; j = j->next, p = p->next) + if (!oideq(&j->item->object.oid, + &p->item->object.oid)) { + can_fast_forward = 0; + break; + } + /* + * If the number of merge heads differs from the original merge + * commit, we cannot fast-forward. + */ + if (j || p) + can_fast_forward = 0; + } + + if (can_fast_forward) { + rollback_lock_file(&lock); + ret = fast_forward_to(r, &commit->object.oid, + &head_commit->object.oid, 0, opts); + if (flags & TODO_EDIT_MERGE_MSG) + goto fast_forward_edit; + + goto leave_merge; + } + if (commit) { const char *encoding = get_commit_output_encoding(); const char *message = logmsg_reencode(commit, NULL, encoding); @@ -3864,45 +3904,6 @@ static int do_merge(struct repository *r, } } - /* - * If HEAD is not identical to the first parent of the original merge - * commit, we cannot fast-forward. - */ - can_fast_forward = opts->allow_ff && commit && commit->parents && - oideq(&commit->parents->item->object.oid, - &head_commit->object.oid); - - /* - * If any merge head is different from the original one, we cannot - * fast-forward. - */ - if (can_fast_forward) { - struct commit_list *p = commit->parents->next; - - for (j = to_merge; j && p; j = j->next, p = p->next) - if (!oideq(&j->item->object.oid, - &p->item->object.oid)) { - can_fast_forward = 0; - break; - } - /* - * If the number of merge heads differs from the original merge - * commit, we cannot fast-forward. - */ - if (j || p) - can_fast_forward = 0; - } - - if (can_fast_forward) { - rollback_lock_file(&lock); - ret = fast_forward_to(r, &commit->object.oid, - &head_commit->object.oid, 0, opts); - if (flags & TODO_EDIT_MERGE_MSG) - goto fast_forward_edit; - - goto leave_merge; - } - if (strategy || to_merge->next) { /* Octopus merge */ struct child_process cmd = CHILD_PROCESS_INIT; diff --git a/t/lib-rebase.sh b/t/lib-rebase.sh index 99d9e7efd2..ec6b9b107d 100644 --- a/t/lib-rebase.sh +++ b/t/lib-rebase.sh @@ -173,10 +173,16 @@ set_reword_editor () { write_script reword-editor.sh <<-EOF && # Save the oid of the first reworded commit so we can check rebase - # fast-forwards to it + # fast-forwards to it. Also check that we do not write .git/MERGE_MSG + # when fast-forwarding if ! test -s reword-oid then - git rev-parse HEAD >reword-oid + git rev-parse HEAD >reword-oid && + if test -f .git/MERGE_MSG + then + echo 1>&2 "error: .git/MERGE_MSG exists" + exit 1 + fi fi && # There should be no uncommited changes git diff --exit-code HEAD && From f2563c9ef3c028b5d4165df02fdfc0fcd613102d Mon Sep 17 00:00:00 2001 From: Phillip Wood Date: Fri, 20 Aug 2021 15:40:38 +0000 Subject: [PATCH 4/4] rebase -r: fix merge -c with a merge strategy If a rebase is started with a --strategy option other than "ort" or "recursive" then "merge -c" does not allow the user to reword the commit message. Signed-off-by: Phillip Wood Signed-off-by: Junio C Hamano --- sequencer.c | 5 ++++- t/t3430-rebase-merges.sh | 18 ++++++++++++++++++ 2 files changed, 22 insertions(+), 1 deletion(-) diff --git a/sequencer.c b/sequencer.c index c2cba5ed4b..a19980f62d 100644 --- a/sequencer.c +++ b/sequencer.c @@ -3934,7 +3934,10 @@ static int do_merge(struct repository *r, strvec_pushf(&cmd.args, "-X%s", opts->xopts[k]); } - strvec_push(&cmd.args, "--no-edit"); + if (!(flags & TODO_EDIT_MERGE_MSG)) + strvec_push(&cmd.args, "--no-edit"); + else + strvec_push(&cmd.args, "--edit"); strvec_push(&cmd.args, "--no-ff"); strvec_push(&cmd.args, "--no-log"); strvec_push(&cmd.args, "--no-stat"); diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh index 183c3a2383..43c82d9a33 100755 --- a/t/t3430-rebase-merges.sh +++ b/t/t3430-rebase-merges.sh @@ -187,6 +187,24 @@ test_expect_success 'merge -c commits before rewording and reloads todo-list' ' check_reworded_commits E H ' +test_expect_success 'merge -c rewords when a strategy is given' ' + git checkout -b merge-c-with-strategy H && + write_script git-merge-override <<-\EOF && + echo overridden$1 >G.t + git add G.t + EOF + + PATH="$PWD:$PATH" \ + GIT_SEQUENCE_EDITOR="echo merge -c H G >" \ + GIT_EDITOR="echo edited >>" \ + git rebase --no-ff -ir -s override -Xxopt E && + test_write_lines overridden--xopt >expect && + test_cmp expect G.t && + test_write_lines H "" edited "" >expect && + git log --format=%B -1 >actual && + test_cmp expect actual + +' test_expect_success 'with a branch tip that was cherry-picked already' ' git checkout -b already-upstream main && base="$(git rev-parse --verify HEAD)" &&