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

From: Taylor Blau <me@ttaylorr.com>
Date: Sep 11, 2025, 23:25 UTC
Message-ID: <aMNahx/RBoRljSZd@nand.local>
In-Reply-To: <aLav7vdfxPGTVDQd@pks.im>

On Tue, Sep 02, 2025 at 10:50:54AM +0200, Patrick Steinhardt wrote:

Show 28 quoted lines

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

Calling both get_packed_git() and get_all_packs() both result in:

 - prepare_packed_git(), which calls
 - prepare_packed_git_one(), which calls
 - for_each_file_in_pack_dir(), which calls (via callback)
 - prepare_pack

prepare_pack() only calls add_packed_git() and install_packed_git() if the file it's look at ends in "*.idx", *and* there is either no MIDX, or (if there is one) the packfile is not listed in the MIDX.

get_all_packs(), on the other hand, *does* call prepare_midx_pack() on all MIDXs across all ODB sources, after having called prepare_packed_git(). And prepare_midx_pack() will add MIDX'd packs to the global list of packs, which is what differentiates these two calls.

I think this gets a little funky in practice if you have already called prepare_midx_pack() on one or more MIDX'd packfiles before calling get_packed_git(), because of the side effect of prepare_midx_pack() installing packs into the global list.

So the above behavior isn't guaranteed, but there definitely is a difference between those two functions depending on whether or not some other site has called prepare_midx_pack().

Show 8 quoted lines

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

From the above, I am not sure that this is true.

Thanks, Taylor