rebase -m: fix --signoff with conflicts

When rebasing with "--signoff" the commit created by "rebase --continue"
after resolving conflicts or editing a commit fails to add the
"Signed-off-by:" trailer. This happens because it reuses the message
from the original commit instead of the message that would have used if
the sequencer had not stopped for the user interaction. The correct
message is stored in ctx->message and so (with a couple of exceptions)
we write this to rebase_path_message() when stopping for user
interaction instead. The exceptions are (i) "fixup" and "squash"
commands where the file is written by error_failed_squash() and (ii)
"edit" commands that are fast-forwarded where we continue to reuse the
original message. The latter is safe because "--signoff" will never
fast-forward.

Note this introduces a change in behavior as the message file now
contains conflict comments. This is safe because commit_staged_changes()
passes an explicit cleanup flag when not editing the message and when
the message is being edited it will be cleaned up automatically. This
means user now sees the same message in editor with "rebase --continue"
as they would if they ran "git commit" themselves before continuing and
matches the behavior of "git cherry-pick", "git merge" etc. which all
show the list of files with conflicts.

The tests are extended to check that all commits made after continuing a
rebase have a "Signed-off-by:" trailer.

Reported-by: David Bimmler <[email protected]>
Signed-off-by: Phillip Wood <[email protected]>

sequencer.c

@@ -3663,13 +3663,24 @@ static int error_with_patch(struct repository *r,
 			    struct replay_opts *opts,
 			    int exit_code, int to_amend)
 {
-	if (commit) {
-		if (make_patch(r, commit, opts))
+	struct replay_ctx *ctx = opts->ctx;
+
+	/*
+	 * Write the commit message to be used by "git rebase
+	 * --continue". If a "fixup" or "squash" command has conflicts
+	 * then we will have already written rebase_path_message() in
+	 * error_failed_squash(). If an "edit" command was
+	 * fast-forwarded then we don't have a message in ctx->message
+	 * and rely on make_patch() to write rebase_path_message()
+	 * instead.
+	 */
+	if (ctx->have_message && !file_exists(rebase_path_message()) &&
+	    write_message(ctx->message.buf, ctx->message.len,
+			  rebase_path_message(), 0))
+		return error(_("could not write commit message file"));
+
+	if (commit && make_patch(r, commit, opts))
 			return -1;
-	} else if (copy_file(rebase_path_message(),
-			     git_path_merge_msg(r), 0666))
-		return error(_("unable to copy '%s' to '%s'"),
-			     git_path_merge_msg(r), rebase_path_message());
 
 	if (to_amend) {
 		if (intend_to_amend())

t/t3428-rebase-signoff.sh

@@ -7,10 +7,16 @@ This test runs git rebase --signoff and make sure that it works.
 
 TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
+. "$TEST_DIRECTORY"/lib-rebase.sh
 
 test_expect_success 'setup' '
 	git commit --allow-empty -m "Initial empty commit" &&
 	test_commit first file a &&
+	test_commit second file &&
+	git checkout -b conflict-branch first &&
+	test_commit file-2 file-2 &&
+	test_commit conflict file &&
+	test_commit third file &&
 
 	ident="$GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>" &&
 
@@ -28,6 +34,22 @@ test_expect_success 'setup' '
 	Signed-off-by: $ident
 	EOF
 
+	# Expected commit message after conflict resolution for rebase --signoff
+	cat >expected-signed-conflict <<-EOF &&
+	third
+
+	Signed-off-by: $ident
+
+	conflict
+
+	Signed-off-by: $ident
+
+	file-2
+
+	Signed-off-by: $ident
+
+	EOF
+
 	# Expected commit message after rebase without --signoff (or with --no-signoff)
 	cat >expected-unsigned <<-EOF &&
 	first
@@ -39,8 +61,12 @@ test_expect_success 'setup' '
 # We configure an alias to do the rebase --signoff so that
 # on the next subtest we can show that --no-signoff overrides the alias
 test_expect_success 'rebase --apply --signoff adds a sign-off line' '
-	git rbs --apply HEAD^ &&
-	test_commit_message HEAD expected-signed
+	test_must_fail git rbs --apply second third &&
+	git checkout --theirs file &&
+	git add file &&
+	git rebase --continue &&
+	git log --format=%B -n3 >actual &&
+	test_cmp expected-signed-conflict actual
 '
 
 test_expect_success 'rebase --no-signoff does not add a sign-off line' '
@@ -51,28 +77,50 @@ test_expect_success 'rebase --no-signoff does not add a sign-off line' '
 
 test_expect_success 'rebase --exec --signoff adds a sign-off line' '
 	test_when_finished "rm exec" &&
-	git commit --amend -m "first" &&
-	git rebase --exec "touch exec" --signoff HEAD^ &&
+	git rebase --exec "touch exec" --signoff first^ first &&
 	test_path_is_file exec &&
 	test_commit_message HEAD expected-signed
 '
 
 test_expect_success 'rebase --root --signoff adds a sign-off line' '
-	git commit --amend -m "first" &&
+	git checkout first &&
 	git rebase --root --keep-empty --signoff &&
 	test_commit_message HEAD^ expected-initial-signed &&
 	test_commit_message HEAD expected-signed
 '
 
-test_expect_success 'rebase -i --signoff fails' '
-	git commit --amend -m "first" &&
-	git rebase -i --signoff HEAD^ &&
-	test_commit_message HEAD expected-signed
+test_expect_success 'rebase -m --signoff adds a sign-off line' '
+	test_must_fail git rebase -m --signoff second third &&
+	git checkout --theirs file &&
+	git add file &&
+	GIT_EDITOR="sed -n /Conflicts:/,/^\\\$/p >actual" \
+		git rebase --continue &&
+	cat >expect <<-\EOF &&
+	# Conflicts:
+	#	file
+
+	EOF
+	test_cmp expect actual &&
+	git log --format=%B -n3 >actual &&
+	test_cmp expected-signed-conflict actual
 '
 
-test_expect_success 'rebase -m --signoff fails' '
-	git commit --amend -m "first" &&
-	git rebase -m --signoff HEAD^ &&
-	test_commit_message HEAD expected-signed
+test_expect_success 'rebase -i --signoff adds a sign-off line when editing commit' '
+	(
+		set_fake_editor &&
+		FAKE_LINES="edit 1 edit 2 edit 3" \
+			git rebase -i --signoff second third
+	) &&
+	echo a >a &&
+	git add a &&
+	test_must_fail git rebase --continue &&
+	git checkout --theirs file &&
+	git add file &&
+	git rebase --continue &&
+	echo b >a &&
+	git add a &&
+	git log --format=%B -n3 >actual &&
+	test_cmp expected-signed-conflict actual
 '
+
 test_done

t/t3434-rebase-i18n.sh

@@ -71,7 +71,7 @@ test_rebase_continue_update_encode () {
 		git config i18n.commitencoding $new &&
 		test_must_fail git rebase -m main &&
 		test -f .git/rebase-merge/message &&
-		git stripspace <.git/rebase-merge/message >two.t &&
+		git stripspace -s <.git/rebase-merge/message >two.t &&
 		git add two.t &&
 		git rebase --continue &&
 		compare_msg $msgfile $old $new &&