Re: [PATCH v3 10/14] treewide: drop uses of `for_each_{loose,packed}_object()`

From: Taylor Blau <me@ttaylorr.com>
Date: Jan 23, 2026, 00:46 UTC
Message-ID: <aXLEzAnNRTf5A6bt@nand.local>
In-Reply-To: <20260121-pks-odb-for-each-object-v3-10-12c4dfd24227@pks.im>

On Wed, Jan 21, 2026 at 01:50:26PM +0100, Patrick Steinhardt wrote:

Show 38 quoted lines

> We're using `for_each_loose_object()` and `for_each_packed_object()` at
> a couple of callsites to enumerate all loose and packed objects,
> respectively. These functions will be removed in a subsequent commit in
> favor of the newly introduced `odb_source_loose_for_each_object()` and
> `packfile_store_for_each_object()` replacements.
>
> Prepare for this by refactoring the sites accordingly.
>
> Note that ideally, we'd convert all callsites to use the generic
> `odb_for_each_object()` function already. But for some callers this is
> not possible (yet), and it would require some significant refactorings
> to make this work. Converting these site will thus be deferred to a
> later patch series.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  builtin/cat-file.c | 28 ++++++++++++++++++++++------
>  commit-graph.c     | 44 +++++++++++++++++++++++++++++++-------------
>  2 files changed, 53 insertions(+), 19 deletions(-)
>
> diff --git a/builtin/cat-file.c b/builtin/cat-file.c
> index 6964a5a52c..7d16fbc1b8 100644
> --- a/builtin/cat-file.c
> +++ b/builtin/cat-file.c
> @@ -806,11 +806,14 @@ struct for_each_object_payload {
>  	void *payload;
>  };
>
> -static int batch_one_object_loose(const struct object_id *oid,
> -				  const char *path UNUSED,
> -				  void *_payload)
> +static int batch_one_object_oi(const struct object_id *oid,
> +			       struct object_info *oi,
> +			       void *_payload)
>  {
>  	struct for_each_object_payload *payload = _payload;
> +	if (oi && oi->whence == OI_PACKED)
> +		return payload->callback(oid, oi->u.packed.pack, oi->u.packed.offset,

Ah, here's a good argument for having the API provide the caller with the object_info response it requested. Obviously the packfile_store knows which packfile it's looking at, so asking the caller to re-discover the same information is wasteful.

That said, I'm still a little leery of the way we're passing that information around for the same reasons as I shared earlier in the thread, but I definitely can see the motivation.

Show 14 quoted lines

> @@ -846,8 +849,15 @@ static void batch_each_object(struct batch_options *opt,
>  		.payload = _payload,
>  	};
>  	struct bitmap_index *bitmap = prepare_bitmap_git(the_repository);
> +	struct odb_source *source;
>
> -	for_each_loose_object(the_repository->objects, batch_one_object_loose, &payload, 0);
> +	odb_prepare_alternates(the_repository->objects);
> +	for (source = the_repository->objects->sources; source; source = source->next) {
> +		int ret = odb_source_loose_for_each_object(source, NULL, batch_one_object_oi,
> +							   &payload, flags);
> +		if (ret)
> +			break;
> +	}

OK, I'm guessing that this is one such case where we can't yet use odb_for_each_object() function directly because of the refactoring which you alluded to in the commit message. That seems reasonable, though I wonder if it's worth adding a /* TODO */ comment here to that effect.

Just out of curiosity, what does that refactoring entail? I'm curious because I wonder whether the caller is just written in such a way that it makes it hard to immediately plug into the new API, or whether there are more fundamental issues at play that make the refactoring less than straightforward. If the latter, those could potentially help inform the direction here.

(To be clear, I figure that this is likely work that you have already done, I'm just curious to see if the details would yield any benefit to the immediate patch series under discussion.)

> diff --git a/commit-graph.c b/commit-graph.c
> index 7f1145a082..a3087d7883 100644
> --- a/commit-graph.c
> +++ b/commit-graph.c

The conversion here all looks great to me.

Thanks, Taylor