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

Phillip Wood <phillip.wood123@xxxxxxxxx> · Thu, 19 Aug 2021 11:09:26 +0100

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

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@xxxxxxxxx>
Signed-off-by: Phillip Wood <phillip.wood@xxxxxxxxxxxxx>
---
  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








