On Thu, Aug 12, 2021 at 05:02:26PM -0400, Jeff King wrote:
> On Tue, Jul 27, 2021 at 05:20:10PM -0400, Taylor Blau wrote:
>
> > diff --git a/t/t5326-multi-pack-bitmaps.sh b/t/t5326-multi-pack-bitmaps.sh
> > new file mode 100755
> > index 0000000000..c1b7d633e2
> > --- /dev/null
> > +++ b/t/t5326-multi-pack-bitmaps.sh
> > @@ -0,0 +1,277 @@
> > +#!/bin/sh
> > +
> > +test_description='exercise basic multi-pack bitmap functionality'
> > +. ./test-lib.sh
> > +. "${TEST_DIRECTORY}/lib-bitmap.sh"
> > +
> > +# We'll be writing our own midx and bitmaps, so avoid getting confused by the
> > +# automatic ones.
> > +GIT_TEST_MULTI_PACK_INDEX=0
> > +GIT_TEST_MULTI_PACK_INDEX_WRITE_BITMAP=0
>
> This latter variable doesn't do anything at this point in the series.
> Probably not a big deal (it is simply a noop until then), but if it's
> not hard, it may make sense to bump the "respect ... WRITE_BITMAP" patch
> earlier in the series.
If my memory serves me correctly, I think the very first version of this
patch didn't have a GIT_TEST_MULTI_PACK_INDEX{,_WRITE_BITMAP}=0 at the
top, and so individual invocations needed to set it in their own
environment. Presumably at some point I added this, but forgot to clean
up the redundant ones. I removed the ones you mentioned in your
response, and a few others.
> > +test_expect_success 'create single-pack midx with bitmaps' '
> > + git repack -ad &&
> > + git multi-pack-index write --bitmap &&
> > + test_path_is_file $midx &&
> > + test_path_is_file $midx-$(midx_checksum $objdir).bitmap
> > +'
> > +
> > +basic_bitmap_tests
>
> We can't use a midx bitmap without a .rev file. The basic_bitmap_tests
> function covers that, but I wonder if we should also check:
>
> test_path_is_file $midx-$(midx_checksum $objdir).rev
>
> in that first test.
Good idea. These tests probably preceded the invention of .rev files, so
a lot of them needed updating. I made sure to add them where
appropriate.
> > +test_expect_success 'create new additional packs' '
> > + for i in $(test_seq 1 16)
> > + do
> > + test_commit "$i" &&
> > + git repack -d
> > + done &&
>
> This loop needs an "|| return 1" inside to catch &&-chain problems (not
> that we expect "repack -d" to fail, but just on principle).
Nice catch, thanks.
> I love how the earlier refactoring made it easy to test the single- and
> multi-pack cases thoroughly.
Likewise :-).
> > +test_expect_success 'setup midx with base from later pack' '
> > + # Write a and b so that "a" is a delta on top of base "b", since Git
> > + # prefers to delete contents out of a base rather than add to a shorter
> > + # object.
> > + test_seq 1 128 >a &&
> > + test_seq 1 130 >b &&
> > +
> > + git add a b &&
> > + git commit -m "initial commit" &&
> > +
> > + a=$(git rev-parse HEAD:a) &&
> > + b=$(git rev-parse HEAD:b) &&
> > +
> > + # In the first pack, "a" is stored as a delta to "b".
> > + p1=$(git pack-objects .git/objects/pack/pack <<-EOF
> > + $a
> > + $b
> > + EOF
> > + ) &&
>
> This is brittle with respect to Git's delta heuristics, of course, but I
> don't think there's a better way to do it with pack-objects. And this is
> not the first test to make similar assumptions. I think you can
> construct a known set of deltas using lib-pack.sh. It may get a bit
> complicated. As an alternative, maybe it makes sense to confirm that the
> deltas are set up as expected? You can do it with cat-file
> --batch-check.
Yeah, I definitely agree that this test is brittle. But it would fail if
our assumptions about what gets delta'd with what changes, because we do
check that 'a' is a delta on top of 'b' (see the call to have_delta
towards the end of this test). That have_delta helper does use
`--batch-check=%(deltabase)`, which is (I think) the cat-file invocation
you're mentioning.
Thanks,
Taylor
