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
