Re: [PATCH v3 06/14] packfile: introduce function to iterate through objects

[Patrick Steinhardt <ps@pks.im>]

On Thu, Jan 22, 2026 at 07:06:09PM -0500, Taylor Blau wrote:
> 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/ ?

Hm, isn't "to" correct in this case? The remainder of the sentence reads
weird though.

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

I'll also rephrase this a bit.

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

Yeah, I agree that the current interfaces we have around reading objects
is weird because of the mixed in/out behaviour of `struct object_info`.
I didn't really feel like changing it in this series though because it
would lead to a lot changes all over the place.

Maybe this is something we can do as a follow-up?

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

There is, yeah. The nice thing about combining iteration with the object
info request is that we have more information available when reading the
object info:

  - For packfiles we already have the info where exactly the object
    sits, so there is no need to do another search for the object.

  - For loose objects we already have the path available, even though
    this probably doesn't matter too much as the path is trivial to
    compute.

For other backends I very much expect that we'll be able to make use of
similar optimizations. For a remote database for example you could craft
the query in such a way that we yield all objects with the exact info
required instead of having to perform a separate query for the object
info.

Patrick