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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html