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

[Jeff King <peff@peff.net>]

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

We could shortcut that by providing it with the extra information, the
way add_object_entry_from_bitmap() does. The original code before your
series could have done the same optimization, but it became much more
obvious after your series, since -Wunused-parameters notices that we do
not look at the "pack" or "pos" parameters at all. :)

It may not be that exciting an optimization, though. Pack lookups aren't
_that_ expensive, and the pack-mru code would mean we always find it in
the first pack (since by definition we're iterating through the objects
in whole packs, our locality is perfect).

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.

So I offer it mainly as an observation, in case somebody wants to look
into it further (both for the optimization and the possibility of
simplifying the code).

-Peff