Re: [PATCH 5/6] packfile: introduce macro to iterate through packs

From: Patrick Steinhardt <ps@pks.im>
Date: Oct 9, 2025, 06:36 UTC
Message-ID: <aOdX3ZmITr0FVq4b@pks.im>
In-Reply-To: <aObUivWiMdtprQSu@nand.local>

On Wed, Oct 08, 2025 at 05:15:54PM -0400, Taylor Blau wrote:

Show 21 quoted lines

> On Tue, Oct 07, 2025 at 02:41:11PM +0200, Patrick Steinhardt wrote:
> > We have a bunch of different sites that want to iterate through all
> > packs of a given `struct packfile_store`. This pattern is somewhat
> > verbose and repetitive, which makes it somewhat cumbersome.
> >
> > Introduce a new macro `packfile_store_for_each_pack()` that removes some
> > of the boilerplate.
> 
> Makes sense.
> 
> Since we're talking about adding a convenience macro, I wonder if it
> might make sense to name/treat this one slightly differently. I would
> imagine something like:
> 
>     repo_for_each_pack(the_repository, p) { ... }
> 
> I understand that packfile_store_for_each_pack() is trying to operate on
> a single packfile_source. In practice, would someone ever want to
> iterate over the packfiles in some source but not another one? It's hard
> to know the answer to that question right now without any other
> implementations.

I don't think so, so I'm fine with this direction. In fact, it might even make my life easier. See further down.

> Perhaps in the meantime you could imagine doing something similar to
> your macro, but instead of reading from the store directly, it could
> read from the repository's store _and_ BUG() if there is more than one
> store to choose from.

I don't think we should BUG() when there's multiple different packfile stores, as it is mostly likely the intent of the caller anyway to iterate through all of them.

But that being said, there is a different reason why we may want to BUG() eventually: if we get pluggable ODBs, then one of the object sources may not be a source that uses packs in the first place. And in _that_ case we definitely should alert the developer that something is wrong and BUG().

So I'll adapt this series to use your proposed `repo_for_each_pack()` solution.

Show 26 quoted lines

> > diff --git a/builtin/fsck.c b/builtin/fsck.c
> > index 8ee95e0d67..5462c442dc 100644
> > --- a/builtin/fsck.c
> > +++ b/builtin/fsck.c
> > @@ -869,18 +869,19 @@ static int check_pack_rev_indexes(struct repository *r, int show_progress)
> >  {
> >  	struct packfile_store *packs = r->objects->packfiles;
> >  	struct progress *progress = NULL;
> > +	struct packed_git *p;
> 
> I wondered if this needed to be in scope for the whole function, and
> looking just at the first portion of this hunk it seems like it does
> not. But continuing to read further down shows other spots where it is
> beneficial to have "p" defined at the scope of the whole function.
> 
> >  	uint32_t pack_count = 0;
> >  	int res = 0;
> >
> >  	if (show_progress) {
> > -		for (struct packed_git *p = packfile_store_get_all_packs(packs); p; p = p->next)
> > +		packfile_store_for_each_pack(packs, p)
> >  			pack_count++;
> 
> This conversion looks obviously correct. Though I wonder, as an aside,
> do you think it makes sense to have a packfile_store_num_packs()
> function or similar that does this for us?

We probably could, but I don't think we have enough callsites to warrant it now.

Thanks!

Patrick