Re: [PATCHv2] add: ignore only ignored files

[Jeff King <peff@peff.net>]

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.

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