Re: [PATCH v3 18/25] t5326: test multi-pack bitmap behavior

[Jeff King <peff@peff.net>]

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.

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

> +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).

> +	git checkout -b other2 HEAD~8 &&
> +	for i in $(test_seq 1 8)
> +	do
> +		test_commit "side-$i" &&
> +		git repack -d
> +	done &&

Ditto here.

> +test_expect_success 'create multi-pack midx with bitmaps' '
> +	git multi-pack-index write --bitmap &&
> +
> +	ls $objdir/pack/pack-*.pack >packs &&
> +	test_line_count = 25 packs &&
> +
> +	test_path_is_file $midx &&
> +	test_path_is_file $midx-$(midx_checksum $objdir).bitmap
> +'

Possible spot for checking the .rev file again (though really, it is
belt-and-suspenders at this point).

> +basic_bitmap_tests

I love how the earlier refactoring made it easy to test the single- and
multi-pack cases thoroughly.

> +test_expect_success '--no-bitmap is respected when bitmaps exist' '
> +	git multi-pack-index write --bitmap &&
> +
> +	test_commit respect--no-bitmap &&
> +	GIT_TEST_MULTI_PACK_INDEX=0 git repack -d &&

Do we need to set this env variable? We've already set it to 0 at the
top of the script.

> +	test_path_is_file $midx &&
> +	test_path_is_file $midx-$(midx_checksum $objdir).bitmap &&
> +
> +	git multi-pack-index write --no-bitmap &&
> +
> +	test_path_is_file $midx &&
> +	test_path_is_missing $midx-$(midx_checksum $objdir).bitmap
> +'

OK, so we expect "--no-bitmap" to drop the bitmap (just like it does for
a regular pack bitmap). Makes sense. We probably should check:

  test_path_is_missing $midx-$(midx_checksum $objdir).rev

here, too (unlike the other spots, it isn't redundant; we could leave a
stale file around and likely nobody would notice).

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

> +test_expect_success 'removing a MIDX clears stale bitmaps' '
> +	rm -fr repo &&
> +	git init repo &&
> +	test_when_finished "rm -fr repo" &&
> +	(
> +		cd repo &&
> +		test_commit base &&
> +		git repack &&
> +		git multi-pack-index write --bitmap &&
> +
> +		# Write a MIDX and bitmap; remove the MIDX but leave the bitmap.
> +		stale_bitmap=$midx-$(midx_checksum $objdir).bitmap &&
> +		rm $midx &&
> +
> +		# Then write a new MIDX.
> +		test_commit new &&
> +		git repack &&
> +		git multi-pack-index write --bitmap &&
> +
> +		test_path_is_file $midx &&
> +		test_path_is_file $midx-$(midx_checksum $objdir).bitmap &&
> +		test_path_is_missing $stale_bitmap
> +	)

Another spot where we might want to check that the stale .rev file has
gone away (and optionally that the new one was written; I haven't noted
all of those, though).

> +test_expect_success 'pack.preferBitmapTips' '
> +	git init repo &&
> +	test_when_finished "rm -fr repo" &&
> +	(
> +		cd repo &&
> +
> +		test_commit_bulk --message="%s" 103 &&
> +
> +		git log --format="%H" >commits.raw &&
> +		sort <commits.raw >commits &&
> +
> +		git log --format="create refs/tags/%s %H" HEAD >refs &&
> +		git update-ref --stdin <refs &&
> +
> +		git multi-pack-index write --bitmap &&
> +		test_path_is_file $midx &&
> +		test_path_is_file $midx-$(midx_checksum $objdir).bitmap &&
> +
> +		test-tool bitmap list-commits | sort >bitmaps &&
> +		comm -13 bitmaps commits >before &&
> +		test_line_count = 1 before &&
> +
> +		perl -ne "printf(\"create refs/tags/include/%d \", $.); print" \
> +			<before | git update-ref --stdin &&
> +
> +		rm -fr $midx-$(midx_checksum $objdir).bitmap &&
> +		rm -fr $midx-$(midx_checksum $objdir).rev &&
> +		rm -fr $midx &&
> +
> +		git -c pack.preferBitmapTips=refs/tags/include \
> +			multi-pack-index write --bitmap &&
> +		test-tool bitmap list-commits | sort >bitmaps &&
> +		comm -13 bitmaps commits >after &&
> +
> +		! test_cmp before after
> +	)
> +'

OK, so we are not depending on any _specific_ commits to get bitmapped,
but just confirming that we have some impact. That may be the best we
can do given that we are subject to the bitmap code's heuristics (and
anyway, this is exactly what the pack version does).

Any other parts of the patch that I didn't quote looked very good to me.
I'm happy to have such a thorough set of tests.

-Peff