On 2014-11-22 20.19, Jeff King wrote: > 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. > 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 -- 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