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.
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?
> @@ -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.
-Peff
