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

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