Re: [PATCH 1/7] odb/source-packed: improve lookup when enumerating objects

[Patrick Steinhardt <ps@pks.im>]

On Thu, Jul 09, 2026 at 02:54:17PM -0500, Justin Tobler wrote:
> On 26/07/09 10:35AM, Patrick Steinhardt wrote:
> > When iterating through packed objects via `odb_for_each_object()` we
> > do so via two different mechanisms:
> > 
> >   - When a multi-pack index is available we use that one to efficiently
> >     loop through all objects.
> > 
> >   - We then loop through all packfiles that aren't covered by a
> >     multi-pack index.
> 
> To be specific, we are talking only about the for_each_object callback
> for the packed source `odb_source_packed_for_each_object()` correct?
> Also, this appears to only matter when we are enumerating OIDs with a
> specific prefix.

Yeah, true. I'll clarify this a bit.

> > Regardless of which mechanism we use, we then iterate through all the
> > objects indexed by the respective data structure. Curiously though,
> > while we use the indices for enumerating the objects, we completely
> > ignore it for the actual object lookup. Instead, we call into the
> > generic `odb_source_read_object_info()` function, which will itself
> > consult the indices to figure out where the object in question even
> > lives.
> > 
> > This has two consequences:
> > 
> >   - It's inefficient, as we basically have to figure out the position of
> >     the object a second time.
> 
> Since we already have the position from the index, there is no need to
> start over. Makes sense.
> 
> >   - It's subtly wrong, as it may now happen that a specific object will
> >     be looked up via a different pack in case it exists multiple times.
> 
> Naive question: Is there any real harm in reading the same object, but
> from a different packfile here?

The answer is probably "no". At least I cannot think of any case where
it'd really matter, but semantically it's the wrong thing to do anyway.

> > diff --git a/odb/source-packed.c b/odb/source-packed.c
> > index 0edea5356d..9cfa02b7a2 100644
> > --- a/odb/source-packed.c
> > +++ b/odb/source-packed.c
> > @@ -177,9 +178,8 @@ static int for_each_prefixed_object_in_midx(
> >  			if (!match_hash(len, opts->prefix->hash, current->hash))
> >  				break;
> >  
> > -			if (opts->flags) {
> > +			if (opts->flags || data->request) {
> 
> I'm not sure I follow why the above condition needed to change.

This needs to change because we now require access to the pack so that
we can call `packed_object_info()`. Otherwise the pack would be not be
populated if we're invoked without any flags.

Patrick