Re: [PATCH v2 11/12] t5524: test --log=1 limits shortlog length

From: Thomas Gummerer <t.gummerer@gmail.com>
Date: May 8, 2015, 10:59 UTC
Message-ID: <87r3qrwd1j.fsf@hank.tgummerer.com>
In-Reply-To: <8e34bc9ea27d147f86de0cf60141687a@www.dscho.org>

Johannes Schindelin <johannes.schindelin@gmx.de> writes:

Show 43 quoted lines

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

t/README states:

   On the other hand, don't use test_must_fail for running regular
   platform commands; just use '! cmd'.  We are not in the business
   of verifying that the world given to us sanely works.

Except for a few cases that is also respected in the test scripts.

$ git grep "! grep" | wc -l 203 $ git grep "test_must_fail grep" | wc -l 19

So I think using ! grep is the right way to go.

Show 6 quoted lines

> Thank you,
> Johannes
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html