sequencer: honor signoff opt in run_git_commit

From: David Bimmler <[email protected]>

When rebasing interactively, --signoff would not take effect for commits which conflict. That is, commits applying cleanly would be signed off, but commits requiring intervention would miss the sign off trailer.

The reason is that run_git_commit did not check for the signoff replay opt, and hence even though the option was picked up and passed correctly, the actual committing dropped the ball.

The patch adds a test specifically for this case, as well as amending a squash test which codified the broken behaviour.

t/t3428-rebase-signoff.sh +0 -0

index e1b1e947647..fcecdf41978 100755

t/t3428-rebase-signoff.sh +0 -0

 # Expected commit message after rebase without --signoff (or with --no-signoff)
 cat >expected-unsigned <<EOF
 first

@@ -82,4 +89,30 @@ test_expect_success 'rebase -m --signoff fails' '
 	git cat-file commit HEAD | sed -e "1,/^\$/d" >actual &&

t/t3428-rebase-signoff.sh +2 -0

+'
+
 test_done

t/t3437/expected-squash-message +0 -0

index ab2434f90ed..d74af0bcf6b 100644

t/t3428-rebase-signoff.sh test_expect_success 'rebase -m --signoff fails' ' · line 115

Quoted 2 lines

t/t3428-rebase-signoff.sh +2 -0

+	git cat-file commit HEAD | sed -e "1,/^\$/d" >actual &&
+	test_cmp expected-signed-after-conflict actual

Running any git command on the left hand side of a pipe is frowned upon, as it will hide exit status from it when it fails.

In this case, the primary thing we care about is that we have added the sign off that did not exist in the original, so I wonder

	git cat-file commit HEAD >actual &&
	test_grep "Signed-off-by: " actual

would be sufficient?

	git show -s --format=%B HEAD >actual &&
	test_cmp expect actual

may be a good replacement.

But having said that, and then after having looked at the existing tests in the file, I see that it is littered with the same "do not run git on the upstream of the pipe" violation. So let's not worry about this one. The whole t3428 script needs to be cleaned up after the dust settles.

Thanks.

Quoted 5 lines · David Bimmler via GitGitGadget

From: David Bimmler <[email protected]>

When rebasing interactively, --signoff would not take effect for commits which conflict. That is, commits applying cleanly would be signed off, but commits requiring intervention would miss the sign off trailer.

Good finding.

Quoted 3 lines

The reason is that run_git_commit did not check for the signoff replay opt, and hence even though the option was picked up and passed correctly, the actual committing dropped the ball.

We record and read back the initial "--signoff" given from the command line (and options.signoff from the "options sheet" when not REPLAY_INTERACTIVE_REBASE) in opts->signoff, but that piece of information is used only to decide if we call append_signoff(). When do_commit(), after try_to_commit() punts, calls "git commit", the invocation lacks "--signoff" on the command line.

Side note: the "let's avoid spawning 'git commit' and duplicate it in-process" codepath in try_to_commit() does not seem to pay attention to opts->signoff at all, and it does not do the append_signoff() to add the trailer. Is this something this patch needs to fix as well, or am I misreading the control flow (e.g., perhaps opts->signoff is set we somehow refrain from doing the "let's do it in-process" logic)?

If we forgot to tell "git commit" that we want the result signed-off, of course it would not add the trailer. Perhaps

-correctly, the actual committing dropped the ball" +correctly to run_git_commit(), it failed to pass it to +underlying 'git commit'

is less misleading.

Quoted 2 lines

The patch adds a test specifically for this case, as well as amending a squash test which codified the broken behaviour.

"The patch adds" -> "Add"

I love this kind of test update that corrects a mistaken expectation after a careful analysis of how the commands should behave.

Very good job. Nicely done.

sequencer.c static int run_git_commit(const char *defmsg, · line 1128

Quoted 13 lines

sequencer.c +2 -0

index fa838f264f5..16595e26a17 100644
@@ -1121,6 +1121,8 @@ static int run_git_commit(const char *defmsg,
  		strvec_pushf(&cmd.args, "-S%s", opts->gpg_sign);
  	else
  		strvec_push(&cmd.args, "--no-gpg-sign");
+	if (opts->signoff)
+		strvec_push(&cmd.args, "--signoff");
  	if (defmsg)
  		strvec_pushl(&cmd.args, "-F", defmsg, NULL);
  	else if (!(flags & EDIT_MSG))

This is straight-forward and obviously correct.

Quoted 5 lines

t/t3428-rebase-signoff.sh +0 -0

index e1b1e947647..fcecdf41978 100755

t/t3428-rebase-signoff.sh first · line 36

Quoted 10 lines

t/t3428-rebase-signoff.sh +7 -0

@@ -27,6 +27,13 @@ first
  Signed-off-by: $(git var GIT_COMMITTER_IDENT | sed -e "s/>.*/>/")
  EOF
 
+# Expected signed off message after resolving the conflict
+cat >expected-signed-after-conflict <<EOF
+update file on side
+
+Signed-off-by: $(git var GIT_COMMITTER_IDENT | sed -e "s/>.*/>/")
+EOF
+

The t3428 script uses ancient style to prepare these expectations outside the test_expect_success blocks, and following the pattern is fine (until we clean-up the style of the whole script). Cleaning it up is better done as a separate series, and when the dust settles after this fix lands. #leftoverbits

Quoted 5 lines

t/t3428-rebase-signoff.sh +0 -0

  # Expected commit message after rebase without --signoff (or with --no-signoff)
  cat >expected-unsigned <<EOF
  first

@@ -82,4 +89,30 @@ test_expect_success 'rebase -m --signoff fails' '
  	git cat-file commit HEAD | sed -e "1,/^\$/d" >actual &&

t/t3428-rebase-signoff.sh test_expect_success 'rebase -m --signoff fails' ' · line 99

Quoted 10 lines

t/t3428-rebase-signoff.sh +8 -0

  	test_cmp expected-signed actual
  '
+
+test_expect_success 'rebase -i signs commits even if a conflict occurs' '
+	git branch -M main &&
+
+	git branch side &&
+	echo "b" >file &&
+	git add file &&
+	git commit -m"update file" &&

Have a SP between "-m" and its value, imitating existing tests.

t/t3428-rebase-signoff.sh test_expect_success 'rebase -m --signoff fails' ' · line 100

Quoted 1 line

t/t3428-rebase-signoff.sh +1 -0

+	test_tick &&

Why? Does the rest of the test depend on commits to have different timestamps?

If you want to use test_tick, the pattern to follow is to call it _before_ creating a commit, not after. Up to this point in the original script nobody calls it (or test_commit that by default calls it), so "test_tick && git_commit" would be OK.

But if there is no dependency on exact sequence of timestamps, letting the commit share the same initial timestamp (hardcoded in test-lib.sh for better reproducibility) is more preferable, as use of test_tick sends a wrong message that the test results may be affected by the exact timestamps.

t/t3428-rebase-signoff.sh test_expect_success 'rebase -m --signoff fails' ' · line 115

Quoted 8 lines

t/t3428-rebase-signoff.sh +8 -0

+	test_must_fail git rebase -i --signoff main &&
+
+	echo "merged" >file &&
+	git add file &&
+	git rebase --continue &&
+
+	git cat-file commit HEAD | sed -e "1,/^\$/d" >actual &&
+	test_cmp expected-signed-after-conflict actual

Running any git command on the left hand side of a pipe is frowned upon, as it will hide exit status from it when it fails.

In this case, the primary thing we care about is that we have added the sign off that did not exist in the original, so I wonder

	git cat-file commit HEAD >actual &&
	test_grep "Signed-off-by: " actual

would be sufficient?

Quoted 8 lines

t/t3428-rebase-signoff.sh +2 -0

+'
+
  test_done

t/t3437/expected-squash-message +0 -0

index ab2434f90ed..d74af0bcf6b 100644

t/t3437/expected-squash-message edited 1 · line 53

Quoted 6 lines

t/t3437/expected-squash-message +2 -0

@@ -48,4 +48,6 @@ edited 1
  edited 2
 
  edited 3
+
+Signed-off-by: Rebase Committer <[email protected]>
  squashed

This defines the expected outcome from

     git rebase -ik --signoff A

in t3437-rebase-fixup-options.sh; I am not sure if the location the sign-off is inserted is sensible, though.

Thanks.

I think it is more complicated than that. We do not use the "--signoff" option of "git commit" because we do not want to append the "Signed-off-by:" trailer when processing "fixup" and "squash" commands. The trailer is appended to the commit message by the sequencer in do_pick_commits(). The problem is that when we commit a conflict resolution we end up using the original commit message rather than the file containing the commit message that we would have used if we had not stopped for conflicts. I've got some patches which need their commit messages cleaning up at https://github.com/phillipwood/git/commits/wip/fix-rebase-signoff-with-conflicts/ which use the correct file when committing conflict resolutions. I'll try and clean them up next week.