Re: [PATCHv2 5/6] t7510: test verify-commit

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

Michael J Gruber <git@drmicha.warpmail.net> writes:

> Jeff King venit, vidit, dixit 13.06.2014 13:51:
>> On Fri, Jun 13, 2014 at 12:42:47PM +0200, Michael J Gruber wrote:
>> 
>>>  test_expect_success GPG 'detect fudged signature' '
>>>  	git cat-file commit master >raw &&
>>>  
>>>  	sed -e "s/seventh/7th forged/" raw >forged1 &&
>>>  	git hash-object -w -t commit forged1 >forged1.commit &&
>>> +	! git verify-commit $(cat forged1.commit) &&
>> 
>> Please use test_must_fail here (and further down), which will catch
>> things like signal death.
>
> Again, that is an issue of keeping the style of the surrounding code
> (which is relatively new) vs. doing it differently. I don't mind
> changing t7510 to a different style, though.

We do not have any "! git anything" in that script, don't we?  We do
not expect "grep" supplied by the platform to lie, and that is why
we should never use test_must_fail on them, but we do want to notice
when git starts segfaulting, and that is what test_must_fail is for.

We tell novices and cluelesses who do not know better to try to
follow and match the surrounding style.  This is because it would at
least not make things worse than if we let them run loose on our
codebase.  It is not about "if we have a bad style, it is better to
match that existing bad style to spread the badness than making it
partly better".  But you have been here long enough and know what is
preferred and and what is to be avoided---more importantly, Peff has
been here long enough to know and has given such an advice, so...

In any case, I do not offhand see anything wrong style-wise in 7510,
so please do not change anything in it for the sake of styles.