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
