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

From: Patrick Steinhardt <ps@pks.im>
Date: Jan 23, 2026, 09:42 UTC
Message-ID: <aXNCjT6Al-4YLah5@pks.im>
In-Reply-To: <aXK7cSJW2syew89a@nand.local>

On Thu, Jan 22, 2026 at 07:06:09PM -0500, Taylor Blau wrote:

Show 5 quoted lines

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

Show 5 quoted lines

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

I'll also rephrase this a bit.

Show 40 quoted lines

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