Re: [PATCHv2] add: ignore only ignored files

From: Torsten Bögershausen <tboegi@web.de>
Date: Nov 22, 2014, 21:20 UTC
Message-ID: <5470FE0A.1030802@web.de>
In-Reply-To: <20141122191932.GA13340@peff.net>

On 2014-11-22 20.19, Jeff King wrote:

Show 29 quoted lines

> On Sat, Nov 22, 2014 at 03:59:12PM +0100, Torsten Bögershausen wrote:
> 
>>>> +test_expect_success 'error out when attempting to add ignored ones but add others' '
>>>> +	touch a.if &&
>>>> +	test_must_fail git add a.?? &&
>>>> +	! (git ls-files | grep "\\.ig") &&
>>>> +	(git ls-files | grep a.if)
>>>> +'
>> [...]
>>
>> 2 small comments:
>> Why the escaped "\\.ig" and the unescaped "a.if"  ?
> 
> I agree that is inconsistent, and I don't see any reason for it.
> 
>> The other question, this is a more general one, strikes me every time I see
>> ! grep
>>
>> Should we avoid it by writing "test_must_fail" instead of "!" ?
> 
> No. The point of test_must_fail over "!" is to check that not only does
> the command fail, but it fails with a clean exit rather than a signal
> death.  The general philosophy is that this is useful for git (which we
> are testing), and not for third-party tools that we are using to check
> our outputs. In other words, we do not expect grep to segfault, and do
> not need to bother checking it.
> 
> I do not think there is a real _downside_ to using test_must_fail for
> grep, except that it is a bit more verbose.

We may burn CPU cycles

> 
> And that describes the goal, of course; actual implementation has been
> less consistent. Possibly because I do not know that those instructions
> are written down anywhere.

There is a hint in test-lib-functions.sh, but a short notice in CodingGuidelines could be useful, once there is an agreement about grep, which I think we have.

Show 21 quoted lines

> We usually catch such things in review these
> days, but there are many inconsistent spots in the existing suite.
> 
>> The following came into my mind when working on another grepy thing,
>> and it may be unnecessary clumsy:
>>
>> test_expect_success 'error out when attempting to add ignored ones but add others' '
>> 	touch a.if &&
>> 	test_must_fail git add a.?? &&
>> 	git ls-files >files.txt &&
>> 	test_must_fail grep a.ig files.txt >/dev/null &&
>> 	grep a.if files.txt >/dev/null &&
>> 	rm files.txt
> 
> Right, my "allergic to pipes" was basically advocating using a tempfile.
> But as noted above, it should remain "! grep" here. And there is no need
> to redirect the output of grep, as the test suite does it already (in
> fact, it is preferable not to, because somebody debugging the test with
> "-v" will get more output).
> 
> -Peff

I counted 19 "test_must_fail grep" under t/*sh, and 201 "! grep".

As a general rule for further review of shell scripts can we say ? ! git # incorrect, we don't capture e.g. segfaults of signal test_must_fail grep # correct, but not preferred for new code ! grep # preferred for new code test_must_fail git # correct