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

[Taylor Blau <me@ttaylorr.com>]

On Wed, Jan 21, 2026 at 01:50:26PM +0100, Patrick Steinhardt wrote:
> 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.

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