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

From: Patrick Steinhardt <ps@pks.im>
Date: Aug 21, 2025, 06:40 UTC
Message-ID: <aKa_cg0Q8QMSBKGp@pks.im>
In-Reply-To: <CAOLa=ZR7eXDAsrRifyvxzTt9RwSR1TcQFs4KLaV7n6byxQM0zA@mail.gmail.com>

On Wed, Aug 20, 2025 at 06:50:05AM -0700, Karthik Nayak wrote:

Show 13 quoted lines

> Patrick Steinhardt <ps@pks.im> writes:
> 
> > We have two different functions to retrieve packfiles for a packfile
> > store:
> >
> >   - `get_packed_git()` returns the list of packfiles directly.
> >
> >   - `get_all_packs()` does more work and also prepares packfiles that
> >     are being indexed by a multi-pack-index.
> >
> 
> Question, under what situation would a packfile not returned by
> `get_packed_git()` but be indexed by a multi-pack-index.

Okay, this whole commit message isn't all that enlightening. The only difference between these two functions is that `get_packed_git()` only calls `packfile_store_prepare()` and then returns the list of packs, whereas `get_all_packs()` does the same and then also prepares the pack so that it's properly marked as being indexed by an MIDX.

Now that means that `get_packed_git()` does _less_ work. But both functions already load the MIDX via `packfile_store_prepare()` anyway, so the only difference is that we now also call `prepare_midx_pack()` for each indexed pack. And that function does not do a lot: we have already loaded the pack itself, so `packfile_store_load_pack()` returns the already-in-memory pack. So all we end up doing is to figure out whether the pack is indexed in the MIDX and then store this info in `p->multi_pack_index` as well as `m->packs[pack_int_id]`.

So the amount of extra work shouldn't really matter. I'll rephrase the commit message.

Show 17 quoted lines

> > The distinction is not immediately obvious. Furthermore, to make the
> > situation even worse, `get_packed_git()` would return the same result as
> > `get_all_packs()` once the latter has been called once as they both
> > refer to the same list.
> >
> > As it turns out, the distinction isn't necessary. We only have a couple
> > of callers of `get_packed_git()`, and all of those callers are prepared
> > to call `get_all_packs()` instead:
> >
> >   - "builtin/gc.c": We explicitly check how many packfiles aren't
> >     contained in the multi-pack-index, so loading extra packfiles that
> >     are indexed by it won't change the result.
> >
> >   - "builtin/grep.c": We only care `get_packed_git()` to prepare eagerly
> >     load packfiles. In the preceding commit we have started to expose
> 
> Nit: the first sentence reads a bit weird.

Ah, that was supposed to read "call", not "care".

Patrick