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

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

[Phillip Wood via GitGitGadget]

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

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

[Phillip Wood via GitGitGadget]

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(-)

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

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

[Phillip Wood via GitGitGadget]

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(+)

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

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

[Phillip Wood via GitGitGadget]

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(-)

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

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

[Phillip Wood via GitGitGadget]

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(-)

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

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

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 1400 bytes --]

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.

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
>

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

[SZEDER Gábor]

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.

> 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
> 

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

[Phillip Wood]

Hi Dscho

On 14/08/2021 23:43, Johannes Schindelin wrote:
> 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

> 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
>>

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

[Phillip Wood]

On 17/08/2021 18:26, SZEDER Gábor wrote:
> 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

>> 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
>>

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

[Phillip Wood via GitGitGadget]

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

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

[Phillip Wood via GitGitGadget]

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(-)

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

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

[Phillip Wood via GitGitGadget]

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(+)

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

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

[Phillip Wood via GitGitGadget]

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(-)

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

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

[Phillip Wood via GitGitGadget]

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(-)

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

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

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 2417 bytes --]

Hi Phillip,

On Fri, 20 Aug 2021, Phillip Wood via GitGitGadget wrote:

> 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

>
>           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
>