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

[Patrick Steinhardt <ps@pks.im>]

[-- Attachment #1: Type: text/plain, Size: 1579 bytes --]

On Wed, Apr 10, 2024 at 08:26:13AM -0700, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> 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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]