Re: [PATCH v3 5/8] repack: add `--filter=<filter-spec>` option

[Taylor Blau <me@ttaylorr.com>]

On Tue, Aug 08, 2023 at 10:34:25AM +0200, Christian Couder wrote:
> > > +     git -C bare.git -c repack.writebitmaps=false repack -a -d --filter=blob:none &&
> > > +     test_stdout_line_count = 2 ls bare.git/objects/pack/*.pack &&
> > > +     commit_pack=$(test-tool -C bare.git find-pack HEAD) &&
> > > +     test -n "$commit_pack" &&
> >
> > I wonder if the test-tool itself should exit with a non-zero code if it
> > can't find the given object in any pack. It would at least allow us to
> > drop the "test -n $foo" after every invocation of the test-helper in
> > this test.
> >
> > Arguably callers may want to ensure that an object doesn't exist in any
> > pack, and this would be inconvenient for them, since they'd have to
> > write something like:
> >
> >     test_must_fail test-tool find-pack $obj
> >
> > but I think a more direct test like
> >
> >     test_must_fail git cat-file -t $obj
> >
> > would do just as well.
>
> Thanks for these suggestions, but I prefered to add the `--check-count
> <n>` option to `test-tool find-pack` in version 4.
>
> This way `--check-count 0` or `-c 0` for short can be used to check
> that an object is in no packfile, though it could be for example in a
> promisor remote or a loose object file. It's also nice to be able to
> check that an object is in exactly 2 packfiles in some cases.

"--check-count 0" is a nice approach, thanks!

> > This all looks good, but I think there are a couple of more things that
> > we'd want to test for here:
> >
> >   - That the list of all objects appears the same before and after all
> >     of the repacking. I think that this is tested implicitly already in
> >     your test, but having it written down explicitly would harden this
> >     against regressions that cause us to inadvertently delete an object
> >     we shouldn't have.
>
> I don't think we need to test this. `git pack-objects
> --filter=<filter-spec>` already existed before this series and is
> tested elsewhere. We can trust that command and its tests, and just
> check that we used it correctly by checking that only a few objects
> are in the right packfiles.

Yeah, I don't think we should be worried about whether or not
pack-objects is doing the right thing here: I agree that we have
sufficient coverage for that elsewhere throughout the test suite. I was
more concerned at catching bugs or regressions at the 'repack' layer.

But you're more familiar with these changes than I am, so I trust your
judgement.

> > > +test_expect_success '--filter fails with --write-bitmap-index' '
> > > +     test_must_fail git -C bare.git repack -a -d --write-bitmap-index \
> > > +             --filter=blob:none &&
> >
> > Do we want to ensure that we get the exit code corresponding with
> > showing the usage text? I could go either way, but I do think that we
> > should grep through the output on stderr to ensure that we get the
> > appropriate error message.
>
> I am not sure that testing the exit code and the stderr output is
> always needed. Here I think that this test is more for documentation
> purposes than really enforcing something important. In fact if the
> behavior would change and `--write-bitmap-index` would understand that
> it should write an MIDX instead of a regular index, that behavior
> change could be considered in some ways as an improvement and we would
> only need to remove 'test_must_fail' here.

I don't feel that strongly about it, TBH, I think I was more commenting
on that we seem to have many of these tests that go

    test_must_fail git <some arguments that don't go together> 2>err &&
    grep "appropriate error message" err

throughout the suite. I don't feel strongly enough to suggest that we
add more for this specific purpose.

Thanks,
Taylor