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

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:

quoted

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.

quoted

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

quoted

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

quoted

+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