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

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:

quoted

+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