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

From: Patrick Steinhardt <ps@pks.im>
Date: Apr 10, 2024, 06:52 UTC
Message-ID: <ZhY3SUdrX2WifyZf@tanuki>
In-Reply-To: <6adc9dd26da4459d246591ce148c960b33bde336.1712597893.git.gitgitgadget@gmail.com>

On Mon, Apr 08, 2024 at 05:38:11PM +0000, John Cai via GitGitGadget wrote:

> From: John Cai <johncai86@gmail.com>

[snip]

Show 8 quoted lines

> @@ -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]

Show 19 quoted lines

> 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