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

From: Patrick Steinhardt <ps@pks.im>
Date: Sep 2, 2025, 08:50 UTC
Message-ID: <aLav7vdfxPGTVDQd@pks.im>
In-Reply-To: <aK5hpwcCgjkgQB1N@nand.local>

On Tue, Aug 26, 2025 at 09:38:47PM -0400, Taylor Blau wrote:

Show 21 quoted lines

> 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()`.

Show 25 quoted lines

> 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