Re: [PATCH 1/4] refs: add referent parameter to refs_resolve_ref_unsafe

From: Jeff King <peff@peff.net>
Date: Jun 11, 2024, 08:50 UTC
Message-ID: <20240611085058.GJ3248245@coredump.intra.peff.net>
In-Reply-To: <011c10f488610b0a795a843bff66723477783761.1717694801.git.gitgitgadget@gmail.com>

On Thu, Jun 06, 2024 at 05:26:37PM +0000, John Cai via GitGitGadget wrote:

Show 6 quoted lines

> From: John Cai <johncai86@gmail.com>
> 
> refs_resolve_ref_unsafe retrieves the referent, the unresolved value of
> a reference. Add a parameter to allow refs_resolve_ref_unsafe to pass up
> the value of referent to the caller so it can save this value in ref
> iterators for more efficient access.

This commit message left me with a lot of questions.

For one, it wasn't immediately obvious to me what a "referent" is. ;) I think an example could help. If I understand, you mean that if you have a situation like:

  - refs/heads/one is a symref pointing to refs/heads/two
  - refs/heads/two is a regular ref

and we resolve "one", then "two" is the referent? And the caller might want to know that?

But I think we already pass that out as the return value from refs_resolve_ref_unsafe(). That is how something like "rev-parse --symbolic-full-name" works now.

But there are some subtleties. In a chain of symbolic refs (say, "two" is a symbolic ref to "three"), we return only the final name ("three"). And you might want to know about "two".

You can pass RESOLVE_REF_NO_RECURSE to inhibit this, and get back just "two". You can see that now with "git symbolic-ref --no-recurse". The downside is that we never look at the referent at all, so you get only the symref value (and no information about the actual oid, or if the referent even exists). You would still get an oid for any non-symrefs you examine.

So reading between the lines, you have a caller in mind which wants to know the immediate referent in addition to the final recursive oid?

Looking at the rest of your series, I guess that caller is the one in loose_fill_ref_dir_regular_file(), so that it can get passed to the for-each-ref callback. But why is it right thing for it to record and pass along the immediate referent there, and not the final one? For that matter, would a caller ever want to see the whole chain of one/two/three?

Show 8 quoted lines

> @@ -1761,6 +1761,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,
> +				    const char *referent,
>  				    int resolve_flags,
>  				    struct object_id *oid,
>  				    int *flags)

Unless I am misunderstanding the purpose of your patch completely, this "referent" is meant to be an out-parameter, right? In which case, shouldn't it be "const char **referent"?

As the code is now:

Show 10 quoted lines

> @@ -1822,6 +1823,9 @@ const char *refs_resolve_ref_unsafe(struct ref_store *refs,
>  		}
>  
>  		*flags |= read_flags;
> +		if (referent && (read_flags & REF_ISSYMREF) &&
> +		    sb_refname.len > 0)
> +			referent = sb_refname.buf;
>  
>  		if (!(read_flags & REF_ISSYMREF)) {
>  			if (*flags & REF_BAD_NAME) {

...we'd assign the local "referent" pointer to our refname buf, but the caller would never see that. Plus doing so would not help you anyway, since sb_refname will be used again as we recurse. So at best, you end up with the final name in the chain anyway. Or at worst, sb_refname gets reallocated and "referent" is left as a dangling pointer.

-Peff