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

[Taylor Blau <me@ttaylorr.com>]

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.

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 get that that may be counterproductive to your goal here of getting
the tree prepared to handle multiple object sources. Another approach
might be to keep the proposed semantics, but call it "for_each_pack()"
instead of "packfile_store_for_each_pack()".

I know that the latter is our preferred convention, but I am not sure
that the rule makes total sense in this case. It is obvious from
"for_each_pack()" alone that we are iterating over packs, and the only
way to obtain a collection of packs is from the a packfile_store. So
here I think that the "packfile_store_" prefix is doing more harm than
good by making the code overly verbose.

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

Thanks,
Taylor