From: Taylor Blau <me@ttaylorr.com>
Date: Fri, 23 Jan 2026 00:06:09 GMT
Subject: Re: [PATCH v3 06/14] packfile: introduce function to iterate through objects
Message-ID: <aXK7cSJW2syew89a@nand.local>
In-Reply-To: <20260121-pks-odb-for-each-object-v3-6-12c4dfd24227@pks.im>

On Wed, Jan 21, 2026 at 01:50:22PM +0100, Patrick Steinhardt wrote:
> Introduce a new function `packfile_store_for_each_object()`. This
> function is the equivalent to `odb_source_loose_for_each_object()` in

s/to/of/ ?

> that it:
>
>   - Works on a single packfile store and thus per object source.

s/thus per/thus/ ?

> diff --git a/packfile.c b/packfile.c
> index d15a2ce12b..cd45c6f21c 100644
> --- a/packfile.c
> +++ b/packfile.c
> @@ -2360,6 +2360,54 @@ int for_each_packed_object(struct repository *repo, each_packed_object_fn cb,
>  	return ret ? ret : pack_errors;
>  }
>
> +struct packfile_store_for_each_object_wrapper_data {
> +	struct packfile_store *store;
> +	struct object_info *oi;
> +	odb_for_each_object_cb cb;
> +	void *cb_data;
> +};
> +
> +static int packfile_store_for_each_object_wrapper(const struct object_id *oid,
> +						  struct packed_git *pack,
> +						  uint32_t index_pos,
> +						  void *cb_data)
> +{
> +	struct packfile_store_for_each_object_wrapper_data *data = cb_data;
> +
> +	if (data->oi) {

Interesting. Is it the case that if the caller provides a non-NULL
pointer to an object_info struct, that we will reuse the request portion
for all iterated objects, updating the response portion as we go along?

If so, I am a little uneasy about the potential for us to mix portions
of the response from an earlier object with a later one. Skimming
packed_object_info(), I don't think that we are in any immediate danger
since it overwrites all fields in the response section. But that feels
somewhat fragile to me, say, if packed_object_info() were to at some
point conditionally assign a field.

I wonder if we should split the request/response sections of object_info
into their own object_info_req and object_info_resp structs. If we did
that, then we could invert the pattern for providing the response,
filling it out ourselves and then passing a pointer to it back to the
caller via the callback function.

TBH, I wonder whether we should push this onto the caller entirely. If
they need to make an object_info request for each object, is there any
cost to having them do that explicitly themselves?

Thanks,
Taylor