Re: [PATCH v5 9/9] repack: exclude cruft pack(s) from the MIDX where possible

On Tue, Jun 24, 2025 at 06:54:47AM -0400, Jeff King wrote:

quoted

If we do this on top:

--- 8< ---
diff --git a/builtin/repack.c b/builtin/repack.c
index 346d44fbcd..8d1540a0fd 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c

@@ -1614,13 +1614,16 @@ int cmd_repack(int argc,
 	string_list_sort(&names);

 	if (get_local_multi_pack_index(the_repository)) {
-		uint32_t i;
 		struct multi_pack_index *m =
 			get_local_multi_pack_index(the_repository);

-		ALLOC_ARRAY(midx_pack_names, m->num_packs);
-		for (i = 0; i < m->num_packs; i++)
-			midx_pack_names[midx_pack_names_nr++] = xstrdup(m->pack_names[i]);
+		ALLOC_ARRAY(midx_pack_names,
+			    m->num_packs + m->num_packs_in_base);
+
+		for (; m; m = m->base_midx)
+			for (uint32_t i = 0; i < m->num_packs; i++)
+				midx_pack_names[midx_pack_names_nr++] =
+					xstrdup(m->pack_names[i]);
 	}

 	close_object_store(the_repository->objects);
--- >8 ---

And this fix looks reasonable to me. It is a bit unfortunate that the
incremental midx concept bleeds all the way out to callers like this,
because it means we might have the same problem in other spots. But that
is nothing new, and I'm not sure of a good solution. If the
public-facing API pretended as if "struct multi_pack_midx" contained the
packs for all of the sub-midx entries of the chain, that would solve it.
But then all of the internal parts of the code that look at the
incremental entries would need a separate representation. And I suspect
there's a lot more code in that latter group than the former (most
callers won't be this intimate with the midx, and just want to convert
an oid to a pack/offset pair).

Would we want a test to cover this case? We do catch it in the
linux-TEST-vars build, but it might be nice to have coverage in normal
test runs. I'm not sure how much of a pain that would be.

I thought quite a bit about this and decided against it. The extra test
would really just be this on top:

--- 8< ---
diff --git a/t/t7704-repack-cruft.sh b/t/t7704-repack-cruft.sh
index aa2e2e6ad8..9b71387325 100755
--- a/t/t7704-repack-cruft.sh
+++ b/t/t7704-repack-cruft.sh

@@ -842,7 +842,9 @@ test_expect_success 'repack --write-midx includes cruft when already geometric'
 		# actually write a new object and subsequently a new
 		# pack to contain it.
 		git merge --no-ff $C &&
-		git repack -d &&
+		GIT_TEST_MULTI_PACK_INDEX=1 \
+		GIT_TEST_MULTI_PACK_INDEX_WRITE_INCREMENTAL=1 \
+			git repack -d &&

 		ls $packdir/pack-*.idx | sort >packs.all &&
 		cruft="$(ls $packdir/pack-*.mtimes)" &&
--- >8 ---

, to force us to put the cruft pack in an earlier MIDX layer. But that
felt like making this test too-specific to incremental MIDXs when the
original test has very little to do with incremental- vs non-incremental
MIDXs.

I tried to write a smaller test case that demonstrates the problem but
couldn't find a straightforward way to minimize the reproduction. As an
alternative, we could duplicate and/or parameterize the test entirely,
but that felt like overkill.

Thanks,
Taylor