Re: [PATCH v2 1/3] refs: keep track of unresolved reference value in iterator

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 10, 2024, 15:26 UTC
Message-ID: <xmqqwmp5xmqy.fsf@gitster.g>
In-Reply-To: <ZhY3SUdrX2WifyZf@tanuki>

Patrick Steinhardt <ps@pks.im> writes:

Show 17 quoted lines

>>  const char *refs_resolve_ref_unsafe(struct ref_store *refs,
>>  				    const char *refname,
>> +				    char **referent,
>>  				    int resolve_flags,
>>  				    struct object_id *oid,
>>  				    int *flags)
>
> I wonder whether this really should be a `char **`. You don't seem to be
> free'ing returned pointers anywhere, so this should probably at least be
> `const char **` to indicate that memory is owned by the ref store. But
> that would require the ref store to inevitably release it, which may
> easily lead to bugs when the caller expects a different lifetime.
>
> So how about we instead make this a `struct strbuf *referent`? This
> makes it quite clear who owns the memory. Furthermore, if the caller
> wants to resolve multiple refs, it would allow them to reuse the buffer
> for better-optimized allocation patterns.

Or return an allocated piece of memory via "char **". I think an interface that _requires_ the caller to use strbuf is not nice, if the caller is not expected to further _edit_ the returned contents using the strbuf API. If it is likely that the caller would want to perform further post-processing on the result, an interface based on strbuf is nice, but I do not think it applies to this case.