Subject: Re: [PATCH 06/22] mailmap tests: modernize syntax & test idioms

From: Junio C Hamano <[email-masked]>
Date: Thu, 14 Jan 2021 12:00:32 -0800

Denton Liu <[email-masked]> writes:

> On Tue, Jan 12, 2021 at 09:17:50PM +0100, Ævar Arnfjörð Bjarmason wrote:
>> @@ -480,7 +545,7 @@ test_expect_success 'log.mailmap=false disables mailmap' '
>>  	Author: nick1 <[email-masked]>
>>  	Author: $GIT_AUTHOR_NAME <$GIT_AUTHOR_EMAIL>
>>  	EOF
>> -	git -c log.mailmap=False log | grep Author >actual &&
>> +	git -c log.mailmap=false log | grep Author >actual &&
>
> While you're doing test cleanup, here's another suggestion: we should
> break all these pipes where git is in the upstream of a pipe. The return
> code of a pipe comes from the last thing run which means if git outputs
> correctly but then somehow fails after, we won't detect the failure.
>
> In general, I've stopped my crusade against these because it seems like
> it's more noise than it's worth in most cases but in this case, since
> we're exercising mailmap codepaths that aren't tested in other test
> cases, this pipe could plausibly hide a failure that isn't seen
> anywhere else.

Yeah, I agree with your assessment that it does make sense to make
sure this "log" does not crash silently.

I find it unlikely for "log" to crash while giving an expected
Author line to its output stream, though.

Can you make your suggestion into a follow-up patch to be applied on
top of the series?

Thanks.