Re: [PATCH v2 15/16] packfile: refactor `get_all_packs()` to work on packfile store

[Taylor Blau <me@ttaylorr.com>]

On Tue, Sep 02, 2025 at 10:51:13AM +0200, Patrick Steinhardt wrote:
> On Tue, Aug 26, 2025 at 09:45:15PM -0400, Taylor Blau wrote:
> > On Thu, Aug 21, 2025 at 09:39:13AM +0200, Patrick Steinhardt wrote:
> > > The `get_all_packs()` function prepares the packfile store and then
> > > returns its packfiles. Refactor it to accept a packfile store instead of
> > > a repository to clarify its scope.
> >
> > I think that clarifying the scope here is a good idea. But I am a little
> > sad to see this patch proposing that we drop get_all_packs(), which IMHO
> > is a useful convenience function.
> >
> > In effect this is pushing out the implementation details of the
> > packfile_store out to every caller that wants to use get_all_packs(),
> > which I am not sure is a win. Should those callers care where the array
> > of packs is found, or have to write
> >
> >     packfile_store_get_packs(the_repository->objects->packfiles)
> >
> > each time they want to get the set of packs in a repository?
> >
> > I could see an argument in the future where we have object stores that
> > aren't packfile-based and thus calling "get_all_packs()" is not
> > meaningful. But I don't think we are there yet, so I think that this
> > patch is pushing the burden of that future hypothetical on all existing
> > callers of get_all_packs().
>
> We aren't there yet indeed, but the entire goal of this patch series is
> to prepare for that future. So we have to do some steps into that
> direction that might not yet be entirely sensible by themselves, but
> that are necessary prerequisites.

I understand what you're saying, but I think this is the wrong place to
be applying that trade-off.

Yes, we will eventually want to have callers handle object lookups
differently depending on what ODB is in use. When that happens, yes, we
should be pushing callers to go through the ODB abstraction layer. But
we are not there yet, and I do not see a compelling reason to force this
burden on *all* callers of get_all_packs() today.

> In the end there ideally shouldn't be that many callers that want to
> access packfiles directly, but it should be the case that most of them
> go via the ODB. But many of the callers that we're adapting in this
> patch are callers that are deeply tied to the actual ODB on-disk layout,
> and we'll have to tear down the abstraction layer between ODB and the
> actual backend used to store objects. It's unfortunate, but we cannot
> really avoid that in a bunch of situations.

Maybe at some point, but that is definitely not the case today. I count
nearly 50 mentions of either "get_all_packs()" or "get_packed_git()".

I am fine with evolving those callers one at a time, and think that the
"the_repository" conversion has a good model for us to follow here.
Replacing, say, get_oid() with repo_get_oid() over time instead for
forcing all callers to do so at once was a good idea for a number of
reasons, and I think we should follow the same model here.

Thanks,
Taylor