On Fri, Jun 14, 2019 at 04:29:46PM -0700, Emily Shaffer wrote: > On Fri, Jun 14, 2019 at 12:07:28PM -0400, Jeff King wrote: > > On Thu, Jun 13, 2019 at 02:51:03PM -0700, Emily Shaffer wrote: > > > > +test_expect_success 'rev-list --objects --oid-only is usable by > > > cat-file' ' > > > + git rev-list --objects --oid-only --all >list-output && > > > + git cat-file --batch-check <list-output >cat-output && > > > + ! grep missing cat-output > > > +' > > > > Usually we prefer to look for the expected output, rather than making > > sure we did not find the unexpected. But I'm not sure if that might be > > burdensome in this case (i.e., if there's a bunch of cruft coming out of > > "rev-list" that would be annoying to match, and might even change as > > people add more tests). So I'm OK with it either way. > > My (newbie) opinion is that in this case, we specifically want to know > that cat-file didn't choke on objects which we know exist (since they > came from rev-list). I have the feeling that checking for the exact > objects returned instead (or a sample of them) would be more brittle and > would also make the wording of the test less direct. > > So if there's no complaint either way, I'd prefer to leave it the way it > is.
Yeah, that's fine with me if it seems more clear to use grep here (and I was on the fence). > By the way, rev-list-misc.sh has a number of other existing "! grep ..." > lines. It never fails that when I complain about a style issue, the surrounding code is full of the same thing. ;) I'd have to look at each one to determine if they're sensible or not, and it's probably not worth anybody's time to do that cleanup at this point in time. -Peff
