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