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

[John Cai <johncai86@gmail.com>]

Hey Peff,

On 11 Jun 2024, at 4:50, Jeff King wrote:

> On Thu, Jun 06, 2024 at 05:26:37PM +0000, John Cai via GitGitGadget wrote:
>
>> 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.

Yes, exactly. I think you're right that it'd be preferable to just use the
output of refs_resolve_ref_unsafe() to get the value of the referent.
>
> 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?

The goal is to keep track of the value that %(symref) would need in the
iterator so that a separate call doesn't need to be made.

>
> 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?

Right, the final referent is the right one to pass down.

>
>> @@ -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:
>
>> @@ -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.

Going to include changes to remove the out-parameter which will simplify things
quite a bit.

>
> -Peff

thanks for the review!
John