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

On Tue, Aug 08, 2023 at 10:34:25AM +0200, Christian Couder wrote:

quoted

quoted

+     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!

quoted

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.

quoted

quoted

+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