Re: [PATCH] [GSOC] cat-file: fix --batch report changed-type bug

From: Jeff King <peff@peff.net>
Date: Jun 1, 2021, 01:49 UTC
Message-ID: <YLWSRBJHiph+Bejo@coredump.intra.peff.net>
In-Reply-To: <60b509be97423_24d2820856@natae.notmuch>

On Mon, May 31, 2021 at 11:07:26AM -0500, Felipe Contreras wrote:

Show 14 quoted lines

> Jeff King wrote:
> > The simplest test is just:
> > 
> >   git -C all-two cat-file --batch-all-objects --batch="%(objectname)" >/dev/null
> > 
> > which will currently fail. It would be nice to verify that its output is
> > sensible, but I'm not sure how to easily do that (it will spew a bunch
> > of binary tree data, and it cannot even be parsed reliably since we
> > haven't output the sizes).
> 
> I use ruby to parse binary data from git all the time:
> 
>         git log --format='%b%x00' |
>                 ruby -e 'ARGF.each("\0", chomp: true) { |chunk| p chunk }'

I doubt we'd want to add a ruby dependency to our test suite, but sure, we could do the same thing with perl.

The trickier part is that without the sizes, the output is ambiguous (the command above will dump trees that contain arbitrary bytes including NULs, so there's no solid delimiter). We could probably devise a hacky perl snippet that checks for an expected sequence of output that is unlikely to appear otherwise.

Or we could just generate the entire expected output and check it with "cmp". The most robust way is probably to loop over the objects, running "git cat-file" for each, but that's slow. Maybe:

  git cat-file --batch-all-objects --batch-check='%(objectname)' >objects &&
  git cat-file --batch='%(objectname)' <objects >expect &&
  git cat-file --batch-all-objects --batch='%(objectname)' >actual &&
  # not test_cmp, as it is not binary clean!
  cmp expect actual

That feels a bit circular in that it's mostly just exercising most of the same code in the "expect" and "actual" paths. The interesting part is the combination of the two options, which is why I think that just making sure we don't hit an error might be enough.

-Peff