On Mon, Apr 08, 2024 at 05:38:11PM +0000, John Cai via GitGitGadget wrote:
> From: John Cai <johncai86@xxxxxxxxx>
[snip]
> @@ -1928,6 +1928,7 @@ int refs_read_symbolic_ref(struct ref_store *ref_store, const char *refname,
>
> 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.
[snip]
> diff --git a/refs/ref-cache.c b/refs/ref-cache.c
> index 9f9797209a4..4c23b92414a 100644
> --- a/refs/ref-cache.c
> +++ b/refs/ref-cache.c
> @@ -34,12 +34,14 @@ struct ref_dir *get_ref_dir(struct ref_entry *entry)
> }
>
> struct ref_entry *create_ref_entry(const char *refname,
> + const char *referent,
> const struct object_id *oid, int flag)
> {
> struct ref_entry *ref;
>
> FLEX_ALLOC_STR(ref, name, refname);
> oidcpy(&ref->u.value.oid, oid);
> + ref->u.value.referent = referent;
> ref->flag = flag;
> return ref;
> }
It's kind of hard to spot the actual changes inbetween all those changes
to callsites. Is it possible to split the patch up such that we have one
patch that changes all the callsites and one patch that does the actual
changes to implement this?
Patrick
Attachment:
signature.asc
Description: PGP signature
