Re: [PATCH v2 05/10] cat-file: use delta_base_cache entries directly

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 26, 2024, 23:05 UTC
Message-ID: <xmqqcyluga1e.fsf@gitster.g>
In-Reply-To: <xmqqjzg3geea.fsf@gitster.g>

Junio C Hamano <gitster@pobox.com> writes:

Show 19 quoted lines

>> +	/*
>> +	 * Set if caller is able to use OI_DBCACHED entries without copying.
>> +	 * This only applies to OI_DBCACHED entries at the moment,
>> +	 * not OI_CACHED or any other type of entry.
>> +	 */
>> +	unsigned direct_cache:1;
> ...
> You seem to set this bit in batch_objects(), so it does sound like
> that the bit is expected to be set by the caller to tell the API
> something.  If that is the case, then (1) move it to "request" part
> of the object_info structure, and (2) define what it means to be
> "able to use ... without copying".  Mechanically, it may mean
> "contentp directly points into the delta base cache", but what
> implication does it have to callers?  If the caller obtains such a
> pointer in .contentp, what is required for its access pattern to
> make accessing the pointer safe?  The caller cannot use the pointed
> memory after it accesses another object?  What is the definition of
> "access" in the context of this discussion?  Does "checking for
> existence of an object" count as an access?

Another thing that makes me worry about this approach (as opposed to a much simpler to reason about alternative like "transfer ownership to the caller") is that it is very hard to guarantee that other object access that is not under caller's control will never happen, and it is even harder to make sure that the code will keep giving such a guarantee.

In other words, the arrangement smells a bit too brittle.

For example, would it be possible for a lazy fetching of (unrelated) objects triggers after the caller asks about an object and borrows a pointer into delta base cache, and would it scramble the entries of delta base cache, silently invalidating the borrowed piece of memory? Would it be possible for the textconv and other filtering mechanism driven by the attribute system trigger access to configured attribute blob, which has to be lazily fetched from other place?

Thanks.