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

From: Taylor Blau <me@ttaylorr.com>
Date: Jun 23, 2025, 18:47 UTC
Message-ID: <aFmhQZZnYyvxDelO@nand.local>
In-Reply-To: <20250621043551.GA3002138@coredump.intra.peff.net>

On Sat, Jun 21, 2025 at 12:35:51AM -0400, Jeff King wrote:

Show 36 quoted lines

> On Thu, Jun 19, 2025 at 07:30:33PM -0400, Taylor Blau wrote:
>
> > +test_expect_success 'repack --write-midx excludes cruft where possible' '
> > +	setup_cruft_exclude_tests exclude-cruft-when-possible &&
> > +	(
> > +		cd exclude-cruft-when-possible &&
> > +
> > +		GIT_TEST_MULTI_PACK_INDEX=0 \
> > +		git repack -d --geometric=2 --write-midx --write-bitmap-index &&
> > +
> > +		test-tool read-midx --show-objects $objdir >midx &&
> > +		cruft="$(ls $packdir/*.mtimes)" &&
> > +		test_grep ! "$(basename "$cruft" .mtimes).idx" midx &&
> > +
> > +		git rev-list --all --objects --no-object-names >reachable.raw &&
> > +		sort reachable.raw >reachable.objects &&
> > +		awk "/\.pack$/ { print \$1 }" <midx | sort >midx.objects &&
> > +
> > +		test_cmp reachable.objects midx.objects
> > +	)
> > +'
>
> This test (but none of the others) fails when run with:
>
>   GIT_TEST_MULTI_PACK_INDEX=1 \
>   GIT_TEST_MULTI_PACK_INDEX_WRITE_INCREMENTAL=1 \
>   ./t7704-repack-cruft.sh
>
> The culprit is the incremental flag, but you need the first one for the
> second to do anything. The issue is that the cruft pack unexpectedly
> appears in the midx:
>
> [...]
>
> I'm not sure if it's just a funky interaction with the hacky GIT_TEST_*
> variables, or if it's a real bug.

Thanks for spotting. This is definitely a real bug. The root cause here is that our loop to gather the set of packs we know are in the MIDX does not account for multi-layered / incremental MIDXs.

In our example, if there's a cruft pack in any other layer of a MIDX besides the tip, the proposed implementation here won't realize it, and thus (incorrectly) conclude that the cruft pack is not in the MIDX already, so can thusly be omitted.

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

(Note that this assumes that 'tb/prepare-midx-pack-cleanup' has not
landed, this could be a little bit simplified with the
nth_midx_pack_names() function. I kept these two separate so they could
proceed independently.)

Things work as expected. I'll send out a new round with this fix
Incorporated as well as a style issue that Junio noted earlier in this
thread.

Thanks,
Taylor