Re: [PATCH 2/3] builtin/pack-objects.c: simplify add_objects_in_unpacked_packs()

[Taylor Blau <me@ttaylorr.com>]

On Thu, Sep 02, 2021 at 07:04:36PM -0400, Jeff King wrote:
> On Sun, Aug 29, 2021 at 10:48:54PM -0400, Taylor Blau wrote:
>
> > +static int add_object_in_unpacked_pack(const struct object_id *oid,
> > +				       struct packed_git *pack,
> > +				       uint32_t pos,
> > +				       void *_data)
> >  {
> > [...]
> > +	struct object *obj = lookup_unknown_object(the_repository, oid);
> > +	if (obj->flags & OBJECT_ADDED)
> > +		return 0;
> > +	add_object_entry(oid, obj->type, "", 0);
> > +	obj->flags |= OBJECT_ADDED;
> > +	return 0;
> >  }
>
> This is not new in your patch series, but while merging this with
> another topic I had, I noticed another opportunity for optimization
> here. We already know the pack/pos in which we found the object. But
> when we call add_object_entry(), it will do another lookup, via
> want_object_in_pack().
>
> [...]
>
> It would also probably involve some slight refactoring of
> add_object_entry() to avoid duplication (though possibly the result
> could reduce similar duplication with the bitmap variant). Hmm, actually
> looking further, we already have add_object_entry_from_pack() for
> --stdin-packs.

I was trying to remember how I handled this for cruft packs (which want
to take a slightly different path to add_object_entry() in order to
record the object mtimes along the way). Indeed, in that case there is a
special add_cruft_object_entry() which uses pack and offset directly.

Having looked at all of these functions which are layered on top of
add_object_entry() recently, I tend to agree that there is some room
for clean-up there.

I plan on sending the cruft pack series soon, after I untangle all of
the `repack` + multi-pack bitmaps stuff, so perhaps that will be a good
time to pursue this.

Anyway, just to say that I probably agree that the pack mru cache is
helping us enough to make this negligible, but that it is on my mind and
we'll have a chance to look at it soon.

Thanks,
Taylor