Re: [PATCH v2 4/5] fetch: teach independent negotiation (no packfile)

From: Junio C Hamano <gitster@pobox.com>
Date: May 6, 2021, 02:12 UTC
Message-ID: <xmqqbl9o1ugy.fsf@gitster.g>
In-Reply-To: <9f650594-c8a6-4d89-2686-49de48aabb62@gmail.com>

Derrick Stolee <stolee@gmail.com> writes:

Show 14 quoted lines

>> Moving to lookup_object() made me look around, but at this point the
>> object in question (which comes from the negotiation_tips) has been
>> instantiated, so it is OK.
>> 
>>     Side note. The big difference between lookup and parse is what
>>     happens when this process hasn't seen the object yet---the
>>     former will just return NULL instead of instantiating the
>>     in-core copy; for that reason, it is easier on the readers to
>>     use parse_object() if we know we want an in-core object *and*
>>     when we use it we want to see it parsed already).
>
> Please forgive my incorrect recommendation here. I was expecting
> lookup_object() to behave like lookup_commit(), which creates the
> object if it is not already in the cached set.

It is not wrong per-se to recommend or use lookup_object() in this case, as long as who recommends or uses the function is certain that the object should already exist in-core.

But it is just that the lookup_object() is often not what the caller wants to use; it is a very rare use case for a caller to be able to say "I am OK for you to return NULL to me for this object, even if it is in our object store, when we do not yet have it in-core". The only exception is the implementation of lookup_<type>() helpers, where they handle "not loaded yet" answer from lookup_object() with create_object() and always return usable in-core object to the caller.

So the sole practical use case for lookup_object() outside the lookup_<type>() implementation becomes "I am 100% sure that an object with this object name has already been loaded in-core, so please let me access it", which may be optimized for a wrong case (because most of the API pass in-core objects, not object names, around). I am not sure if this particular API strangeness can be remedied, though.

Thanks.