Re: [PATCH v2] rev-list: teach --oid-only to enable piping

[Jeff King <peff@peff.net>]

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