Re: [PATCH] sequencer: honor signoff opt in run_git_commit

> "David Bimmler via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
>> From: David Bimmler <david.bimmler@isovalent.com>
>>
>> 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.

>> 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.
> 
>> 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.
> 
>> diff --git a/sequencer.c b/sequencer.c
>> index fa838f264f5..16595e26a17 100644
>> --- a/sequencer.c
>> +++ b/sequencer.c
>> @@ -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.
> 
>> diff --git a/t/t3428-rebase-signoff.sh b/t/t3428-rebase-signoff.sh
>> index e1b1e947647..fcecdf41978 100755
>> --- a/t/t3428-rebase-signoff.sh
>> +++ b/t/t3428-rebase-signoff.sh
>> @@ -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
> 
>>   # 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 &&
>>   	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.
> 
>> +	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.
> 
>> +	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?
> 
>> +'
>> +
>>   test_done
>> diff --git a/t/t3437/expected-squash-message b/t/t3437/expected-squash-message
>> index ab2434f90ed..d74af0bcf6b 100644
>> --- a/t/t3437/expected-squash-message
>> +++ b/t/t3437/expected-squash-message
>> @@ -48,4 +48,6 @@ edited 1
>>   edited 2
>>   
>>   edited 3
>> +
>> +Signed-off-by: Rebase Committer <rebase.committer@example.com>
>>   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.
>