From: Patrick Steinhardt <ps@pks.im>
Date: Tue, 02 Sep 2025 08:51:13 GMT
Subject: Re: [PATCH v2 15/16] packfile: refactor `get_all_packs()` to work on packfile store
Message-ID: <aLawAalFhp9j0qVz@pks.im>
In-Reply-To: <aK5jK2Q3McLnJ9Uw@nand.local>

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.

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.

> > diff --git a/builtin/cat-file.c b/builtin/cat-file.c
> > index fce0b06451c..7124c43fb14 100644
> > --- a/builtin/cat-file.c
> > +++ b/builtin/cat-file.c
> > @@ -854,7 +854,7 @@ static void batch_each_object(struct batch_options *opt,
> >  						 batch_one_object_bitmapped, &payload)) {
> >  		struct packed_git *pack;
> >
> > -		for (pack = get_all_packs(the_repository); pack; pack = pack->next) {
> > +		for (pack = packfile_store_get_packs(the_repository->objects->packfiles); pack; pack = pack->next) {
> 
> If we do go this route, it might be nice to introduce the pattern of
> having a stack variable to hold the packfile_store pointer, since the
> line above here is getting a little long at >100 characters just to
> enumerate packs.

Okay, will do for now. In a subsequent patch series I'm going to
introduce a helper `packfile_store_for_each_pack()` that'll make this a
bit less verbose, too.

Patrick