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

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 4, 2024, 16:45 UTC
Message-ID: <xmqqr0fl13gh.fsf@gitster.g>
In-Reply-To: <xmqqzfu914jc.fsf@gitster.g>

Junio C Hamano <gitster@pobox.com> writes:

Show 13 quoted lines

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

The answer is No. It is plausible that somebody else in a future may think that a better fix is to always prepare the final commit message inside sequencer.c and call append_signoff(), and use it in both the "in-process commit" codepath in try_to_commit() and in the code that was fixed in the patch we are discussing. If such a future update is done carelessly, we might end up adding duplicate sign off. A "at least one instance must be there" test_grep would not be a good tool to catch such a breakage.

	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.