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