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

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