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

[Taylor Blau <me@ttaylorr.com>]

On Tue, Jun 24, 2025 at 06:54:47AM -0400, Jeff King wrote:
> > 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