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

[Patrick Steinhardt <ps@pks.im>]

On Thu, Sep 11, 2025 at 07:25:59PM -0400, Taylor Blau wrote:
> On Tue, Sep 02, 2025 at 10:50:54AM +0200, Patrick Steinhardt wrote:
> > 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().
> 
> > > 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.

Yeah, I'll drop that part from this patch series for now and will handle
this in a subsequent series.

Patrick