On Wed, Apr 10, 2024 at 08:26:13AM -0700, Junio C Hamano wrote: > Patrick Steinhardt <ps@xxxxxx> writes: > > >> 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. When iterating through refs this would incur one allocation per ref though, whereas using a `struct strbuf` would only require a single one. Patrick
Attachment:
signature.asc
Description: PGP signature
