On Mon, Jun 23, 2025 at 02:47:29PM -0400, Taylor Blau wrote:
> > 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.
Ah, right, that makes perfect sense.
> 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.
-Peff
