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

[Junio C Hamano <gitster@pobox.com>]

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

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