threads / patch / 56277

patch, 4 partsrebase -r: some merge related fixes

Subject: [PATCH 0/4] rebase -r: some merge related fixes

## tl;dr

15 messages between Aug 13, 2021 and Aug 24, 2021. Diffs are folded; open one to read it.

replies: 14people: 3as markdown or json

Phillip Wood via GitGitGadget· Aug 13, 2021, 13:07 UTC · lore
This is a collection of merge related fixes for rebase -r
 * Make merge -c behave like reword.
 * When fast-forwarding a merge don't leave .git/MERGE_MSG around (reported
   by Gábor)
 * Make merge -c work when with --strategy
Phillip Wood (4):
  rebase -r: make 'merge -c' behave like reword
  rebase -i: Add another reword test
  rebase -r: don't write .git/MERGE_MSG when fast-forwarding
  rebase -r: fix merge -c with a merge strategy
 sequencer.c                   | 106 ++++++++++++++++++----------------
 t/lib-rebase.sh               |  56 ++++++++++++++++++
 t/t3404-rebase-interactive.sh |  13 +++++
 t/t3430-rebase-merges.sh      |  38 +++++++++---
 4 files changed, 155 insertions(+), 58 deletions(-)
base-commit: 66262451ec94d30ac4b80eb3123549cf7a788afd
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1015%2Fphillipwood%2Fwip%2Fsequencer-merge-c-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1015/phillipwood/wip/sequencer-merge-c-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1015
-- 
gitgitgadget
Phillip Wood via GitGitGadget· Aug 13, 2021, 13:07 UTC · re: Phillip Wood via GitGitGadget · lore

[PATCH 1/4] rebase -r: make 'merge -c' behave like reword

From: Phillip Wood <phillip.wood@dunelm.org.uk>

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 <phillip.wood@dunelm.org.uk>
---
 sequencer.c              | 24 +++++++++++--------
 t/lib-rebase.sh          | 50 ++++++++++++++++++++++++++++++++++++++++
 t/t3430-rebase-merges.sh | 20 ++++++++--------
 3 files changed, 75 insertions(+), 19 deletions(-)
Show changes to 3 files +75 −19

sequencer.c, t/lib-rebase.sh, t/t3430-rebase-merges.sh

diff --git a/sequencer.c b/sequencer.c
index 7f07cd00f3f..cc8a361cceb 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 dc75b834518..99d9e7efd2d 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 6748070df52..183c3a23838 100755
--- a/t/t3430-rebase-merges.sh
+++ b/t/t3430-rebase-merges.sh
@@ -172,17 +172,19 @@ test_expect_success 'failed `merge <branch>` 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' '
-- 
gitgitgadget
Phillip Wood via GitGitGadget· Aug 13, 2021, 13:07 UTC · re: Phillip Wood via GitGitGadget · lore

[PATCH 2/4] rebase -i: Add another reword test

From: Phillip Wood <phillip.wood@dunelm.org.uk>

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 <phillip.wood@dunelm.org.uk>
---
 t/t3404-rebase-interactive.sh | 13 +++++++++++++
 1 file changed, 13 insertions(+)
Show changes to t/t3404-rebase-interactive.sh +13 −0
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 66bcbbf9528..d877872e8f4 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/*" &&
-- 
gitgitgadget
Phillip Wood via GitGitGadget· Aug 13, 2021, 13:07 UTC · re: Phillip Wood via GitGitGadget · lore

[PATCH 3/4] rebase -r: don't write .git/MERGE_MSG when fast-forwarding

From: Phillip Wood <phillip.wood@dunelm.org.uk>

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 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 <szeder.dev@gmail.com>
Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 sequencer.c     | 81 +++++++++++++++++++++++++------------------------
 t/lib-rebase.sh | 10 ++++--
 2 files changed, 49 insertions(+), 42 deletions(-)
Show changes to 2 files +49 −42

sequencer.c, t/lib-rebase.sh

diff --git a/sequencer.c b/sequencer.c
index cc8a361cceb..c2cba5ed4b1 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 99d9e7efd2d..ec6b9b107da 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 &&
-- 
gitgitgadget
SZEDER Gábor· Aug 17, 2021, 17:26 UTC · re: Phillip Wood via GitGitGadget · lore

Re: [PATCH 3/4] rebase -r: don't write .git/MERGE_MSG when fast-forwarding

On Fri, Aug 13, 2021 at 01:07:32PM +0000, Phillip Wood via GitGitGadget wrote:
Show 21 quoted lines
> From: Phillip Wood <phillip.wood@dunelm.org.uk>
> 
> 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 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.
Thanks you for fixing this bug.

FWIW (not that much, I'm afraid), I think your reasoning about the harmlessness of the behavior change concerning the author script makes sense.

My only nit is that the movement of a ~40 lines block of code makes out the bulk of the patch; perhaps it would be worth mentioning it explicitly in the commit message, so future readers of this commit won't look for changes in those hunks.

Show 139 quoted lines
> Reported-by: SZEDER Gábor <szeder.dev@gmail.com>
> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> ---
>  sequencer.c     | 81 +++++++++++++++++++++++++------------------------
>  t/lib-rebase.sh | 10 ++++--
>  2 files changed, 49 insertions(+), 42 deletions(-)
> 
> diff --git a/sequencer.c b/sequencer.c
> index cc8a361cceb..c2cba5ed4b1 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 99d9e7efd2d..ec6b9b107da 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 &&
> -- 
> gitgitgadget
> 
Phillip Wood· Aug 19, 2021, 10:09 UTC · re: SZEDER Gábor · lore

Re: [PATCH 3/4] rebase -r: don't write .git/MERGE_MSG when fast-forwarding

On 17/08/2021 18:26, SZEDER Gábor wrote:
Show 33 quoted lines
> On Fri, Aug 13, 2021 at 01:07:32PM +0000, Phillip Wood via GitGitGadget wrote:
>> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>>
>> 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 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.
> 
> Thanks you for fixing this bug.
> 
> FWIW (not that much, I'm afraid), I think your reasoning about the
> harmlessness of the behavior change concerning the author script makes
> sense.
> 
> My only nit is that the movement of a ~40 lines block of code makes
> out the bulk of the patch; perhaps it would be worth mentioning it
> explicitly in the commit message, so future readers of this commit
> won't look for changes in those hunks.

Thanks for reading through the patch, I take your point about the code movement and will add a comment (I have --color-moved turned on by default and tend to forget others may not)

Best Wishes
Phillip
Show 139 quoted lines
>> Reported-by: SZEDER Gábor <szeder.dev@gmail.com>
>> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
>> ---
>>   sequencer.c     | 81 +++++++++++++++++++++++++------------------------
>>   t/lib-rebase.sh | 10 ++++--
>>   2 files changed, 49 insertions(+), 42 deletions(-)
>>
>> diff --git a/sequencer.c b/sequencer.c
>> index cc8a361cceb..c2cba5ed4b1 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 99d9e7efd2d..ec6b9b107da 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 &&
>> -- 
>> gitgitgadget
>>
Phillip Wood via GitGitGadget· Aug 13, 2021, 13:07 UTC · re: Phillip Wood via GitGitGadget · lore

[PATCH 4/4] rebase -r: fix merge -c with a merge strategy

From: Phillip Wood <phillip.wood@dunelm.org.uk>

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 <phillip.wood@dunelm.org.uk>
---
 sequencer.c              |  5 ++++-
 t/t3430-rebase-merges.sh | 18 ++++++++++++++++++
 2 files changed, 22 insertions(+), 1 deletion(-)
Show changes to 2 files +22 −1

sequencer.c, t/t3430-rebase-merges.sh

diff --git a/sequencer.c b/sequencer.c
index c2cba5ed4b1..a19980f62d9 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 183c3a23838..43c82d9a33b 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)" &&
-- 
gitgitgadget
Johannes Schindelin· Aug 14, 2021, 22:43 UTC · re: Phillip Wood via GitGitGadget · lore

Re: [PATCH 0/4] rebase -r: some merge related fixes

Hi Phillip,
On Fri, 13 Aug 2021, Phillip Wood via GitGitGadget wrote:
Show 12 quoted lines
> This is a collection of merge related fixes for rebase -r
>
>  * Make merge -c behave like reword.
>  * When fast-forwarding a merge don't leave .git/MERGE_MSG around (reported
>    by Gábor)
>  * Make merge -c work when with --strategy
>
> Phillip Wood (4):
>   rebase -r: make 'merge -c' behave like reword
>   rebase -i: Add another reword test
>   rebase -r: don't write .git/MERGE_MSG when fast-forwarding
>   rebase -r: fix merge -c with a merge strategy

I reviewed all four patches (the first one took the most time, obviously) and it was quite the pleasant read. I am in favor of integrating them as-are.

Thank you, Dscho

Show 15 quoted lines
>
>  sequencer.c                   | 106 ++++++++++++++++++----------------
>  t/lib-rebase.sh               |  56 ++++++++++++++++++
>  t/t3404-rebase-interactive.sh |  13 +++++
>  t/t3430-rebase-merges.sh      |  38 +++++++++---
>  4 files changed, 155 insertions(+), 58 deletions(-)
>
>
> base-commit: 66262451ec94d30ac4b80eb3123549cf7a788afd
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1015%2Fphillipwood%2Fwip%2Fsequencer-merge-c-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1015/phillipwood/wip/sequencer-merge-c-v1
> Pull-Request: https://github.com/gitgitgadget/git/pull/1015
> --
> gitgitgadget
>
Phillip Wood· Aug 19, 2021, 10:07 UTC · re: Johannes Schindelin · lore

Re: [PATCH 0/4] rebase -r: some merge related fixes

Hi Dscho
On 14/08/2021 23:43, Johannes Schindelin wrote:
Show 20 quoted lines
> Hi Phillip,
> 
> On Fri, 13 Aug 2021, Phillip Wood via GitGitGadget wrote:
> 
>> This is a collection of merge related fixes for rebase -r
>>
>>   * Make merge -c behave like reword.
>>   * When fast-forwarding a merge don't leave .git/MERGE_MSG around (reported
>>     by Gábor)
>>   * Make merge -c work when with --strategy
>>
>> Phillip Wood (4):
>>    rebase -r: make 'merge -c' behave like reword
>>    rebase -i: Add another reword test
>>    rebase -r: don't write .git/MERGE_MSG when fast-forwarding
>>    rebase -r: fix merge -c with a merge strategy
> 
> I reviewed all four patches (the first one took the most time, obviously)
> and it was quite the pleasant read. I am in favor of integrating them
> as-are.
Thanks for reviewing them
Best Wishes
Phillip
Show 18 quoted lines
> Thank you,
> Dscho
> 
>>
>>   sequencer.c                   | 106 ++++++++++++++++++----------------
>>   t/lib-rebase.sh               |  56 ++++++++++++++++++
>>   t/t3404-rebase-interactive.sh |  13 +++++
>>   t/t3430-rebase-merges.sh      |  38 +++++++++---
>>   4 files changed, 155 insertions(+), 58 deletions(-)
>>
>>
>> base-commit: 66262451ec94d30ac4b80eb3123549cf7a788afd
>> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1015%2Fphillipwood%2Fwip%2Fsequencer-merge-c-v1
>> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1015/phillipwood/wip/sequencer-merge-c-v1
>> Pull-Request: https://github.com/gitgitgadget/git/pull/1015
>> --
>> gitgitgadget
>>
Phillip Wood via GitGitGadget· Aug 20, 2021, 15:40 UTC · re: Phillip Wood via GitGitGadget · lore

[PATCH v2 0/4] rebase -r: some merge related fixes

Thanks for the feedback on V1. I have added a sentence to the commit message of the third patch to make it clear that the fast-forward code is just moved, not changed

Cover letter from V1: This is a collection of merge related fixes for rebase -r

 * Make merge -c behave like reword.
 * When fast-forwarding a merge don't leave .git/MERGE_MSG around (reported
   by Gábor)
 * Make merge -c work when with --strategy
Phillip Wood (4):
  rebase -r: make 'merge -c' behave like reword
  rebase -i: Add another reword test
  rebase -r: don't write .git/MERGE_MSG when fast-forwarding
  rebase -r: fix merge -c with a merge strategy
 sequencer.c                   | 106 ++++++++++++++++++----------------
 t/lib-rebase.sh               |  56 ++++++++++++++++++
 t/t3404-rebase-interactive.sh |  13 +++++
 t/t3430-rebase-merges.sh      |  38 +++++++++---
 4 files changed, 155 insertions(+), 58 deletions(-)
base-commit: 66262451ec94d30ac4b80eb3123549cf7a788afd
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1015%2Fphillipwood%2Fwip%2Fsequencer-merge-c-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1015/phillipwood/wip/sequencer-merge-c-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1015
Range-diff vs v1:
 1:  b514dbdf928 = 1:  b514dbdf928 rebase -r: make 'merge -c' behave like reword
 2:  511cf9204ad = 2:  511cf9204ad rebase -i: Add another reword test
 3:  01d5ed4cba0 ! 3:  080e580e11c rebase -r: don't write .git/MERGE_MSG when fast-forwarding
     @@ Commit message
          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.
     +    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
 4:  f2a2e3531a1 = 4:  b6981ea5439 rebase -r: fix merge -c with a merge strategy
-- 
gitgitgadget
Phillip Wood via GitGitGadget· Aug 20, 2021, 15:40 UTC · re: Phillip Wood via GitGitGadget · lore

[PATCH v2 1/4] rebase -r: make 'merge -c' behave like reword

From: Phillip Wood <phillip.wood@dunelm.org.uk>

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 <phillip.wood@dunelm.org.uk>
---
 sequencer.c              | 24 +++++++++++--------
 t/lib-rebase.sh          | 50 ++++++++++++++++++++++++++++++++++++++++
 t/t3430-rebase-merges.sh | 20 ++++++++--------
 3 files changed, 75 insertions(+), 19 deletions(-)
Show changes to 3 files +75 −19

sequencer.c, t/lib-rebase.sh, t/t3430-rebase-merges.sh

diff --git a/sequencer.c b/sequencer.c
index 7f07cd00f3f..cc8a361cceb 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 dc75b834518..99d9e7efd2d 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 6748070df52..183c3a23838 100755
--- a/t/t3430-rebase-merges.sh
+++ b/t/t3430-rebase-merges.sh
@@ -172,17 +172,19 @@ test_expect_success 'failed `merge <branch>` 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' '
-- 
gitgitgadget
Phillip Wood via GitGitGadget· Aug 20, 2021, 15:40 UTC · re: Phillip Wood via GitGitGadget · lore

[PATCH v2 2/4] rebase -i: Add another reword test

From: Phillip Wood <phillip.wood@dunelm.org.uk>

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 <phillip.wood@dunelm.org.uk>
---
 t/t3404-rebase-interactive.sh | 13 +++++++++++++
 1 file changed, 13 insertions(+)
Show changes to t/t3404-rebase-interactive.sh +13 −0
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 66bcbbf9528..d877872e8f4 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/*" &&
-- 
gitgitgadget
Phillip Wood via GitGitGadget· Aug 20, 2021, 15:40 UTC · re: Phillip Wood via GitGitGadget · lore

[PATCH v2 3/4] rebase -r: don't write .git/MERGE_MSG when fast-forwarding

From: Phillip Wood <phillip.wood@dunelm.org.uk>

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 <szeder.dev@gmail.com>
Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 sequencer.c     | 81 +++++++++++++++++++++++++------------------------
 t/lib-rebase.sh | 10 ++++--
 2 files changed, 49 insertions(+), 42 deletions(-)
Show changes to 2 files +49 −42

sequencer.c, t/lib-rebase.sh

diff --git a/sequencer.c b/sequencer.c
index cc8a361cceb..c2cba5ed4b1 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 99d9e7efd2d..ec6b9b107da 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 &&
-- 
gitgitgadget
Phillip Wood via GitGitGadget· Aug 20, 2021, 15:40 UTC · re: Phillip Wood via GitGitGadget · lore

[PATCH v2 4/4] rebase -r: fix merge -c with a merge strategy

From: Phillip Wood <phillip.wood@dunelm.org.uk>

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 <phillip.wood@dunelm.org.uk>
---
 sequencer.c              |  5 ++++-
 t/t3430-rebase-merges.sh | 18 ++++++++++++++++++
 2 files changed, 22 insertions(+), 1 deletion(-)
Show changes to 2 files +22 −1

sequencer.c, t/t3430-rebase-merges.sh

diff --git a/sequencer.c b/sequencer.c
index c2cba5ed4b1..a19980f62d9 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 183c3a23838..43c82d9a33b 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)" &&
-- 
gitgitgadget
Johannes Schindelin· Aug 24, 2021, 13:33 UTC · re: Phillip Wood via GitGitGadget · lore

Re: [PATCH v2 0/4] rebase -r: some merge related fixes

Hi Phillip,
On Fri, 20 Aug 2021, Phillip Wood via GitGitGadget wrote:
Show 42 quoted lines
> Thanks for the feedback on V1. I have added a sentence to the commit message
> of the third patch to make it clear that the fast-forward code is just
> moved, not changed
>
> Cover letter from V1: This is a collection of merge related fixes for rebase
> -r
>
>  * Make merge -c behave like reword.
>  * When fast-forwarding a merge don't leave .git/MERGE_MSG around (reported
>    by Gábor)
>  * Make merge -c work when with --strategy
>
> Phillip Wood (4):
>   rebase -r: make 'merge -c' behave like reword
>   rebase -i: Add another reword test
>   rebase -r: don't write .git/MERGE_MSG when fast-forwarding
>   rebase -r: fix merge -c with a merge strategy
>
>  sequencer.c                   | 106 ++++++++++++++++++----------------
>  t/lib-rebase.sh               |  56 ++++++++++++++++++
>  t/t3404-rebase-interactive.sh |  13 +++++
>  t/t3430-rebase-merges.sh      |  38 +++++++++---
>  4 files changed, 155 insertions(+), 58 deletions(-)
>
>
> base-commit: 66262451ec94d30ac4b80eb3123549cf7a788afd
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1015%2Fphillipwood%2Fwip%2Fsequencer-merge-c-v2
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1015/phillipwood/wip/sequencer-merge-c-v2
> Pull-Request: https://github.com/gitgitgadget/git/pull/1015
>
> Range-diff vs v1:
>
>  1:  b514dbdf928 = 1:  b514dbdf928 rebase -r: make 'merge -c' behave like reword
>  2:  511cf9204ad = 2:  511cf9204ad rebase -i: Add another reword test
>  3:  01d5ed4cba0 ! 3:  080e580e11c rebase -r: don't write .git/MERGE_MSG when fast-forwarding
>      @@ Commit message
>           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.
>      +    checks. Note that there are no changes to the fast-forward code, it is
>      +    simply moved.
The result still looks fine to me.

Thank you, Dscho

Show 8 quoted lines
>
>           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
>  4:  f2a2e3531a1 = 4:  b6981ea5439 rebase -r: fix merge -c with a merge strategy
>
> --
> gitgitgadget
>

← back to recent threads