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

From: Taylor Blau <me@ttaylorr.com>
Date: Oct 8, 2025, 21:15 UTC
Message-ID: <aObUivWiMdtprQSu@nand.local>
In-Reply-To: <20251007-pks-packfiles-convert-get-all-v1-5-428227657a89@pks.im>

On Tue, Oct 07, 2025 at 02:41:11PM +0200, Patrick Steinhardt wrote:

Show 6 quoted lines

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

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

Show 7 quoted lines

>  	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