From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Fri, 08 May 2015 10:07:29 GMT
Subject: Re: [PATCH v2 11/12] t5524: test --log=1 limits shortlog length
Message-ID: <8e34bc9ea27d147f86de0cf60141687a@www.dscho.org>
In-Reply-To: <554BB93A.3090000@kdbg.org>

Hi Hannes,

On 2015-05-07 21:12, Johannes Sixt wrote:
> Am 07.05.2015 um 19:06 schrieb Paul Tan:
>
>> On Fri, May 8, 2015 at 12:28 AM, Johannes Schindelin
>> <johannes.schindelin@gmx.de> wrote:
>>
>>> On 2015-05-07 10:44, Paul Tan wrote:
>>>> @@ -32,4 +35,18 @@ test_expect_success pull '
>>>>   )
>>>>   '
>>>>
>>>> +test_expect_failure '--log=1 limits shortlog length' '
>>>> +(
>>>> +     cd cloned &&
>>>> +     git reset --hard HEAD^ &&
>>>> +     test `cat afile` = original &&
>>>> +     test `cat bfile` = added &&
>>>> +     git pull --log &&
>>>> +     git log -3 &&
>>>> +     git cat-file commit HEAD >result &&
>>>> +     grep Dollar result &&
>>>> +     ! grep "second commit" result
>>>> +)
>>>
>>> I think it might be better to use `test_must_fail` here, just for
>>> consistency (the `!` operator would also pass if `grep` itself could not
>>> be executed correctly, quite academic, I know, given that `grep` is
>>> exercised plenty of times by the test suite, but still...)
>>>
>>> What do you think?
>>
>> Yep, it's definitely better. Sometimes I forget about the existence of
>> some test utility functions :-/.
> 
> Nope, it's not better. test_must_fail is explicitly only for git
> invocations. We do not expect 'grep' to segfault or something.
> 
> Cf. eg.
> http://thread.gmane.org/gmane.comp.version-control.git/258725/focus=258752

That link leads to a patch that changes `! grep` to a `test_must_fail grep` and is not contested, at least not in the thread visible on GMane. Would you have a link with a more convincing argument for me?

Thank you,
Johannes