Re: [PATCH 2/2] builtin/repack: don't regenerate MIDX unless needed

[Patrick Steinhardt <ps@pks.im>]

On Tue, Dec 09, 2025 at 09:48:47PM -0500, Taylor Blau wrote:
> On Mon, Dec 08, 2025 at 07:27:15PM +0100, Patrick Steinhardt wrote:
> > Address this issue by introducing a new function that determines whether
> > a rewrite of the MIDX would cause any user-visible changes. This covers
> > the following cases:
> >
> >   - No multi-pack index exists at all.
> >
> >   - The user asked us to write a bitmap, and we don't have any.
> >
> >   - The request preferred pack is different than the one that we have.
> >
> >   - The packfiles covered by the MIDX are changing.
> 
> I can't think of any cases beyond the ones you listed here that would
> require us to regenerate the MIDX. One kind-of-exception here would be:
> 
>     $ git repack [...] --write-midx --write-bitmap-index
>     $ git repack [...] --write-midx
> 
> where the second repack would generate an identical MIDX, but does not
> want to retain a bitmap. That case is already handled in the MIDX
> writing code if you search for "want_bitmap".
> 
> That makes me wonder whether the repack layer is the most appropriate
> one to handle this logic. It seems like write_midx_internal() would
> reasonably be able to detect whether or not the MIDX we have already is
> up-to-date with respect to the given input.

One upside of having it in git-repack(1) is that we need to care about
less situations in general as we are operating on a higher level. And
because of that we can make more assumptions.

That being said, putting it into `write_midx_internal()` has the benefit
that we're of course covering more potential cases where we can avoid a
needless rewrite of the MIDX, and that we have better information to
decide whether it would be needed or not.

> I think that makes some things about your patch easier and other things
> a little harder ;-).
> 
>  - On the "easier" front: while both the MIDX code and the portion of
>    the repack code that drives it receive the same set of packs to
>    include, the MIDX code already has the packs it would compare
>    in a standard format. That would avoid you having to handle ends_with
>    ends_with(include_name, ".idx") and ends_with(existing_name, ".pack")
>    as special cases, which would be nice.
> 
>  - On the "harder" front: when driving MIDX generation with the
>    '--stdin-packs' option, we *don't* load an existing MIDX ever since
>    0c5a62f14bc (midx-write.c: do not read existing MIDX with
>    `packs_to_include`, 2024-06-11).
> 
> I don't think that "harder" one is a show-stopper, though. Commit
> t0c5a62f14bc has enough gory details around how we generate pack IDs and
> various subtle assumptions about how and when we load packs that I am
> very hesitant to recommend changing it given its fragility (though we
> should examine and harden any fragilities within midx-write.c, maybe
> just separately ;-)).
> 
> So I don't think that we should make that change ahead of this patch.
> While you can't rely on being able to read 'ctx.m', I think you could
> load the MIDX belonging to "source" ad-hoc after we have computed the
> packs to fill from the MIDX's perspective, which is right around where
> that want_bitmap code lives.

Yeah, we're already loading the MIDX on-demand because in git-repack(1)
as we have closed the object database at the point in time where we're
about to write the MIDX. So overall the change is rather easy to make.

Also, now that I see that we already have some short-circuiting
conditions in git-multi-pack-index(1) I guess it makes sense to extend
those checks. They explicitly don't cover `--stdin-packs` rewrites right
now, so adding that check is an obvious improvement.

One thing I'm a bit torn on is whether or not to handle preferred packs
in the adjusted logic. We don't do so right now either, so I think I'll
drop this for now. I can see an argument that us not handling a changed
preferred pack is a bug though, in which case I'm happy to iterate.

Thanks for your feedback!

Patrick