Re: [PATCH v2 14/16] packfile: remove `get_packed_git()`

[Patrick Steinhardt <ps@pks.im>]

On Tue, Aug 26, 2025 at 09:38:47PM -0400, Taylor Blau wrote:
> On Thu, Aug 21, 2025 at 09:39:12AM +0200, Patrick Steinhardt wrote:
> > We have two different functions to retrieve packfiles for a packfile
> > store:
> >
> >   - `get_packed_git()` returns the list of packfiles after having called
> >     `prepare_packed_git()`.
> >
> >   - `get_all_packs()` calls `prepare_packed_git()`, as well, but also
> >     calls `prepare_midx_pack()` for each pack.
> 
> Yeah, having two of these functions that are named so similarly as to
> suggest they do the same thing (even though they don't) is unfortunate,
> and I am glad that we are looking at it here.
> 
> > This means that the latter function also properly loads the info of
> > whether or not a packfile is part of a multi-pack index. Preparing this
> > extra information also shouldn't be significantly more expensive:
> 
> Right; get_packed_git() only loads the non-MIDX'd packs, and
> get_all_packs() loads everything (regardless whether or not a pack is
> part of the MIDX or not).

I initially understood the distinction of these functions to be exactly
this. But after looking further I don't think this is the actual
distinction: both functions end up loading all packfiles in the repo,
with the only distinction being that `get_all_packs()` also prepares the
MIDX for each MIDX'd packfile.

The important thing to note is that an MIDX may only ever refer to
packfiles in the same object directory. And as we have already loaded
all packfiles in that object directory via `prepare_packed_git_one()`
there isn't really any difference in the returned list of packfiles
whatsoever. Both functions end up loading all packfiles, regardless of
whether or not they have an MIDX.

Please double check my understanding though, I have been staring at this
code for quite a while to figure out what the actual differences are,
and whether or not `get_all_packs()` may actually end up loading more
packs than `get_packed_git()`.

> Are all of the get_packed_git() callers prepared to handle packs that
> are in the MIDX? Looking through them:
> 
>  - builtin/gc.c::incremental_repack_auto_condition() skips over
>   'p->multi_pack_index', so this one is fine to convert.
> 
>  - builtin/grep.c::cmd_grep() calls get_packed_git() but doesn't
>    actually use the result, so this should be fine to convert, though I
>    think there is some subtlty here.
> 
>  - builtin/pack-objects.c::want_object_in_pack_mtime() takes a separate
>    pass over the MIDX'd packs before calling get_packed_git_mru() (which
>    itself calls prepare_packed_git()). I think in practice this is OK,
>    since we will have already handled the MIDX'd packs, but this
>    function is now iterating over packs in the MIDX twice, so it may be
>    worth adding a "if (p->multi_pack_index) continue;" in there.
> 
>  - object-name.c::find_short_packed_object() handles MIDX'd packs
>    separately, and unique_in_pack() is a noop for MIDX'd packs, so this
>    one is fine.
> 
>  - object-name.c::find_abbrev_len_packed() is OK for the same reasons.
> 
> So I think that want_object_in_pack_mtime() may need a small tweak, and
> I am not 100% certain that cmd_grep() is OK to convert.

I initially misunderstood the distinction between these two functions
the same as you did, and had a similar list to the above in the initial
commit message. But with the adjusted understanding of the actual
difference between these functions I think it shouldn't be necessary
anymore to go through each caller one by one.

I'll adapt the commit message a bit.

Patrick