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

From: Taylor Blau <me@ttaylorr.com>
Date: Sep 3, 2021, 02:33 UTC
Message-ID: <YTGJbYmq7O2OvqIJ@nand.local>
In-Reply-To: <YTFYhG+nK49/jR/v@coredump.intra.peff.net>

On Thu, Sep 02, 2021 at 07:04:36PM -0400, Jeff King wrote:

Show 29 quoted lines

> 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