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

Commit Message

John Cai April 8, 2024, 5:38 p.m. UTC

From: John Cai <[e-mail]>

Since ref iterators do not hold onto the direct value of a reference
without resolving it, the only way to get ahold of a direct value of a
symbolic ref is to make a separate call to refs_read_symbolic_ref.

To make accessing the direct value of a symbolic ref more efficient,
let's save the direct value of the ref in the iterator for both the
files backend and reftable backend.

To do so, we also need to add an argument to refs_resolve_ref_unsafe to
save the direct value of the reference somewhere.

Signed-off-by: John Cai <[e-mail]>
---
 builtin/submodule--helper.c |  2 +-
 refs.c                      | 23 +++++++++++++----------
 refs.h                      |  3 ++-
 refs/files-backend.c        | 20 +++++++++++---------
 refs/iterator.c             |  1 +
 refs/ref-cache.c            |  3 +++
 refs/ref-cache.h            |  2 ++
 refs/refs-internal.h        |  1 +
 refs/reftable-backend.c     | 13 +++++++++----
 remote.c                    |  2 +-
 sequencer.c                 |  4 ++--
 t/helper/test-ref-store.c   |  2 +-
 worktree.c                  |  4 +++-
 13 files changed, 50 insertions(+), 30 deletions(-)

Comments

Junio C Hamano April 8, 2024, 11:02 p.m. UTC | #1

> diff --git a/refs.h b/refs.h
> index 298caf6c618..2e740c692ac 100644
> --- a/refs.h
> +++ b/refs.h
> @@ -71,9 +71,10 @@ struct pack_refs_opts {
>  	struct ref_exclusions *exclusions;
>  	struct string_list *includes;
>  };
> -
>  const char *refs_resolve_ref_unsafe(struct ref_store *refs,
> +
>  				    const char *refname,
> +				    char **referent,
>  				    int resolve_flags,
>  				    struct object_id *oid,
>  				    int *flags);

If referent is meant to be an out-parameter, it should sit next to
oid that is also an out-parameter.  And as a late-comer sibling, it
should sit after its elder brother.

Also, I do not see the reason for the shuffling of blank lines.
Shouldn't it be the other way around, even?  After an unrelated
definition of "struct pack_refs_opts", there is (and should be)
a blank line, then the first line of the declaration of function.

Perhaps some fat-fingering.

> @@ -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)
> @@ -1989,6 +1990,8 @@ 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;

Is this safe?  After this assignment, which "return" in this loop
are you expecting to return from this function?  If you fail to
return from the function during this iteration, you'll clobber the
same strbuf with the next refs_read_raw_ref(), but I do not see how
you are ensuring that you'll return from the function without such
corruption happening.

This assignment happens only when read_flags has REF_ISSYMREF set,
so the next "if it is not, then return refname" would not even
trigger.  If RESOLVE_REF_NO_RECURSE bit is on in resolve_flags,
then we'd return without further dereferencing, but if that is the
only safe exit from the function, shouldn't you be guarding the
function with something like

	if (referent && !(resolve_flags & RESOLVE_REF_NO_RECURSE))
		BUG("recursive dereference can will clobber *referent");

to protect its callers from their own mistakes?

Another return before we start the next iteration of the loop and
clobber sb_refname with another call to refs_read_raw_ref() is the
error return codepath at the end of the loop, but that is a totally
uninteresting case.

Or am I totally confused?

John Cai April 9, 2024, 8:29 p.m. UTC | #2

Hi Junio

On 8 Apr 2024, at 19:02, Junio C Hamano wrote:

>> diff --git a/refs.h b/refs.h
>> index 298caf6c618..2e740c692ac 100644
>> --- a/refs.h
>> +++ b/refs.h
>> @@ -71,9 +71,10 @@ struct pack_refs_opts {
>>  	struct ref_exclusions *exclusions;
>>  	struct string_list *includes;
>>  };
>> -
>>  const char *refs_resolve_ref_unsafe(struct ref_store *refs,
>> +
>>  				    const char *refname,
>> +				    char **referent,
>>  				    int resolve_flags,
>>  				    struct object_id *oid,
>>  				    int *flags);
>
> If referent is meant to be an out-parameter, it should sit next to
> oid that is also an out-parameter.  And as a late-comer sibling, it
> should sit after its elder brother.
>
> Also, I do not see the reason for the shuffling of blank lines.
> Shouldn't it be the other way around, even?  After an unrelated
> definition of "struct pack_refs_opts", there is (and should be)
> a blank line, then the first line of the declaration of function.
>
> Perhaps some fat-fingering.

This was indeed a case of fat-fingering.

>
>> @@ -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)
>> @@ -1989,6 +1990,8 @@ 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;
>
> Is this safe?  After this assignment, which "return" in this loop
> are you expecting to return from this function?  If you fail to
> return from the function during this iteration, you'll clobber the
> same strbuf with the next refs_read_raw_ref(), but I do not see how
> you are ensuring that you'll return from the function without such
> corruption happening.
>
> This assignment happens only when read_flags has REF_ISSYMREF set,
> so the next "if it is not, then return refname" would not even
> trigger.  If RESOLVE_REF_NO_RECURSE bit is on in resolve_flags,
> then we'd return without further dereferencing, but if that is the
> only safe exit from the function, shouldn't you be guarding the
> function with something like
>
> 	if (referent && !(resolve_flags & RESOLVE_REF_NO_RECURSE))
> 		BUG("recursive dereference can will clobber *referent");
>
> to protect its callers from their own mistakes?
>
> Another return before we start the next iteration of the loop and
> clobber sb_refname with another call to refs_read_raw_ref() is the
> error return codepath at the end of the loop, but that is a totally
> uninteresting case.
>
> Or am I totally confused?

Yeah good point. This probably not a good idea. In fact, perhaps adding another
argument to this function is unnecessary. We already have refs_read_symbolic_ref
and we can just make a separate call to that once we know that a ref is a
symbolic reference. Though a separate call is less efficient, I'm not sure it's
worth adding this parameter.

thanks
John

Patrick Steinhardt April 10, 2024, 6:52 a.m. UTC | #3

On Mon, Apr 08, 2024 at 05:38:11PM +0000, John Cai via GitGitGadget wrote:
> From: John Cai <[e-mail]>
[snip]
> @@ -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]
> 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

Junio C Hamano April 10, 2024, 3:26 p.m. UTC | #4

Patrick Steinhardt <[e-mail]> writes:

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

Or return an allocated piece of memory via "char **".  I think an
interface that _requires_ the caller to use strbuf is not nice, if
the caller is not expected to further _edit_ the returned contents
using the strbuf API.  If it is likely that the caller would want to
perform further post-processing on the result, an interface based on
strbuf is nice, but I do not think it applies to this case.

Patrick Steinhardt April 11, 2024, 9:11 a.m. UTC | #5

On Wed, Apr 10, 2024 at 08:26:13AM -0700, Junio C Hamano wrote:
> Patrick Steinhardt <[e-mail]> writes:
> 
> >>  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.
> 
> Or return an allocated piece of memory via "char **".  I think an
> interface that _requires_ the caller to use strbuf is not nice, if
> the caller is not expected to further _edit_ the returned contents
> using the strbuf API.  If it is likely that the caller would want to
> perform further post-processing on the result, an interface based on
> strbuf is nice, but I do not think it applies to this case.

When iterating through refs this would incur one allocation per ref
though, whereas using a `struct strbuf` would only require a single one.

Patrick

Patch

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index fda50f2af1e..cf9966b7827 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -45,7 +45,7 @@  static int repo_get_default_remote(struct repository *repo, char **default_remot
 	char *dest = NULL;
 	struct strbuf sb = STRBUF_INIT;
 	struct ref_store *store = get_main_ref_store(repo);
-	const char *refname = refs_resolve_ref_unsafe(store, "HEAD", 0, NULL,
+	const char *refname = refs_resolve_ref_unsafe(store, "HEAD", NULL, 0, NULL,
 						      NULL);
 
 	if (!refname)
diff --git a/refs.c b/refs.c
index 55d2e0b2cb9..b87a680249e 100644
--- a/refs.c
+++ b/refs.c
@@ -379,7 +379,7 @@  char *refs_resolve_refdup(struct ref_store *refs,
 {
 	const char *result;
 
-	result = refs_resolve_ref_unsafe(refs, refname, resolve_flags,
+	result = refs_resolve_ref_unsafe(refs, refname, NULL, resolve_flags,
 					 oid, flags);
 	return xstrdup_or_null(result);
 }
@@ -404,7 +404,7 @@  int read_ref_full(const char *refname, int resolve_flags, struct object_id *oid,
 {
 	struct ref_store *refs = get_main_ref_store(the_repository);
 
-	if (refs_resolve_ref_unsafe(refs, refname, resolve_flags,
+	if (refs_resolve_ref_unsafe(refs, refname, NULL, resolve_flags,
 				    oid, flags))
 		return 0;
 	return -1;
@@ -417,7 +417,7 @@  int read_ref(const char *refname, struct object_id *oid)
 
 int refs_ref_exists(struct ref_store *refs, const char *refname)
 {
-	return !!refs_resolve_ref_unsafe(refs, refname, RESOLVE_REF_READING,
+	return !!refs_resolve_ref_unsafe(refs, refname, NULL, RESOLVE_REF_READING,
 					 NULL, NULL);
 }
 
@@ -773,7 +773,7 @@  int expand_ref(struct repository *repo, const char *str, int len,
 		this_result = refs_found ? &oid_from_ref : oid;
 		strbuf_reset(&fullref);
 		strbuf_addf(&fullref, *p, len, str);
-		r = refs_resolve_ref_unsafe(refs, fullref.buf,
+		r = refs_resolve_ref_unsafe(refs, fullref.buf, NULL,
 					    RESOLVE_REF_READING,
 					    this_result, &flag);
 		if (r) {
@@ -807,7 +807,7 @@  int repo_dwim_log(struct repository *r, const char *str, int len,
 
 		strbuf_reset(&path);
 		strbuf_addf(&path, *p, len, str);
-		ref = refs_resolve_ref_unsafe(refs, path.buf,
+		ref = refs_resolve_ref_unsafe(refs, path.buf, NULL,
 					      RESOLVE_REF_READING,
 					      oid ? &hash : NULL, NULL);
 		if (!ref)
@@ -876,7 +876,7 @@  int is_pseudoref(struct ref_store *refs, const char *refname)
 		return 0;
 
 	if (ends_with(refname, "_HEAD")) {
-		refs_resolve_ref_unsafe(refs, refname,
+		refs_resolve_ref_unsafe(refs, refname, NULL,
 					RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE,
 					&oid, NULL);
 		return !is_null_oid(&oid);
@@ -884,7 +884,7 @@  int is_pseudoref(struct ref_store *refs, const char *refname)
 
 	for (i = 0; i < ARRAY_SIZE(irregular_pseudorefs); i++)
 		if (!strcmp(refname, irregular_pseudorefs[i])) {
-			refs_resolve_ref_unsafe(refs, refname,
+			refs_resolve_ref_unsafe(refs, refname, NULL,
 						RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE,
 						&oid, NULL);
 			return !is_null_oid(&oid);
@@ -1590,7 +1590,7 @@  int refs_head_ref(struct ref_store *refs, each_ref_fn fn, void *cb_data)
 	struct object_id oid;
 	int flag;
 
-	if (refs_resolve_ref_unsafe(refs, "HEAD", RESOLVE_REF_READING,
+	if (refs_resolve_ref_unsafe(refs, "HEAD", NULL, RESOLVE_REF_READING,
 				    &oid, &flag))
 		return fn("HEAD", &oid, flag, cb_data);
 
@@ -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)
@@ -1989,6 +1990,8 @@  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) {
@@ -2024,7 +2027,7 @@  int refs_init_db(struct ref_store *refs, int flags, struct strbuf *err)
 const char *resolve_ref_unsafe(const char *refname, int resolve_flags,
 			       struct object_id *oid, int *flags)
 {
-	return refs_resolve_ref_unsafe(get_main_ref_store(the_repository), refname,
+	return refs_resolve_ref_unsafe(get_main_ref_store(the_repository), refname, NULL,
 				       resolve_flags, oid, flags);
 }
 
@@ -2039,7 +2042,7 @@  int resolve_gitlink_ref(const char *submodule, const char *refname,
 	if (!refs)
 		return -1;
 
-	if (!refs_resolve_ref_unsafe(refs, refname, 0, oid, &flags) ||
+	if (!refs_resolve_ref_unsafe(refs, refname, NULL, 0, oid, &flags) ||
 	    is_null_oid(oid))
 		return -1;
 	return 0;
diff --git a/refs.h b/refs.h
index 298caf6c618..2e740c692ac 100644
--- a/refs.h
+++ b/refs.h
@@ -71,9 +71,10 @@  struct pack_refs_opts {
 	struct ref_exclusions *exclusions;
 	struct string_list *includes;
 };
-
 const char *refs_resolve_ref_unsafe(struct ref_store *refs,
+
 				    const char *refname,
+				    char **referent,
 				    int resolve_flags,
 				    struct object_id *oid,
 				    int *flags);
diff --git a/refs/files-backend.c b/refs/files-backend.c
index a098d14ea00..90342549af9 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -235,8 +235,9 @@  static void loose_fill_ref_dir_regular_file(struct files_ref_store *refs,
 {
 	struct object_id oid;
 	int flag;
+	char *referent;
 
-	if (!refs_resolve_ref_unsafe(&refs->base, refname, RESOLVE_REF_READING,
+	if (!refs_resolve_ref_unsafe(&refs->base, refname, &referent, RESOLVE_REF_READING,
 				     &oid, &flag)) {
 		oidclr(&oid);
 		flag |= REF_ISBROKEN;
@@ -258,7 +259,7 @@  static void loose_fill_ref_dir_regular_file(struct files_ref_store *refs,
 		oidclr(&oid);
 		flag |= REF_BAD_NAME | REF_ISBROKEN;
 	}
-	add_entry_to_dir(dir, create_ref_entry(refname, &oid, flag));
+	add_entry_to_dir(dir, create_ref_entry(refname, referent, &oid, flag));
 }
 
 /*
@@ -843,6 +844,7 @@  static int files_ref_iterator_advance(struct ref_iterator *ref_iterator)
 			continue;
 
 		iter->base.refname = iter->iter0->refname;
+		iter->base.referent = iter->iter0->referent;
 		iter->base.oid = iter->iter0->oid;
 		iter->base.flags = iter->iter0->flags;
 		return ITER_OK;
@@ -1109,7 +1111,7 @@  static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs,
 		goto error_return;
 	}
 
-	if (!refs_resolve_ref_unsafe(&refs->base, lock->ref_name, 0,
+	if (!refs_resolve_ref_unsafe(&refs->base, lock->ref_name, NULL, 0,
 				     &lock->old_oid, NULL))
 		oidclr(&lock->old_oid);
 	goto out;
@@ -1444,7 +1446,7 @@  static int files_copy_or_rename_ref(struct ref_store *ref_store,
 		goto out;
 	}
 
-	if (!refs_resolve_ref_unsafe(&refs->base, oldrefname,
+	if (!refs_resolve_ref_unsafe(&refs->base, oldrefname, NULL,
 				     RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE,
 				     &orig_oid, &flag)) {
 		ret = error("refname %s not found", oldrefname);
@@ -1490,7 +1492,7 @@  static int files_copy_or_rename_ref(struct ref_store *ref_store,
 	 * the safety anyway; we want to delete the reference whatever
 	 * its current value.
 	 */
-	if (!copy && refs_resolve_ref_unsafe(&refs->base, newrefname,
+	if (!copy && refs_resolve_ref_unsafe(&refs->base, newrefname, NULL,
 					     RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE,
 					     NULL, NULL) &&
 	    refs_delete_ref(&refs->base, NULL, newrefname,
@@ -1863,7 +1865,7 @@  static int commit_ref_update(struct files_ref_store *refs,
 		int head_flag;
 		const char *head_ref;
 
-		head_ref = refs_resolve_ref_unsafe(&refs->base, "HEAD",
+		head_ref = refs_resolve_ref_unsafe(&refs->base, "HEAD", NULL,
 						   RESOLVE_REF_READING,
 						   NULL, &head_flag);
 		if (head_ref && (head_flag & REF_ISSYMREF) &&
@@ -1911,7 +1913,7 @@  static void update_symref_reflog(struct files_ref_store *refs,
 	struct object_id new_oid;
 
 	if (logmsg &&
-	    refs_resolve_ref_unsafe(&refs->base, target,
+	    refs_resolve_ref_unsafe(&refs->base, target, NULL,
 				    RESOLVE_REF_READING, &new_oid, NULL) &&
 	    files_log_ref_write(refs, refname, &lock->old_oid,
 				&new_oid, logmsg, 0, &err)) {
@@ -2505,7 +2507,7 @@  static int lock_ref_for_update(struct files_ref_store *refs,
 			 * to record and possibly check old_oid:
 			 */
 			if (!refs_resolve_ref_unsafe(&refs->base,
-						     referent.buf, 0,
+						     referent.buf, NULL, 0,
 						     &lock->old_oid, NULL)) {
 				if (update->flags & REF_HAVE_OLD) {
 					strbuf_addf(err, "cannot lock ref '%s': "
@@ -3200,7 +3202,7 @@  static int files_reflog_expire(struct ref_store *ref_store,
 			int type;
 			const char *ref;
 
-			ref = refs_resolve_ref_unsafe(&refs->base, refname,
+			ref = refs_resolve_ref_unsafe(&refs->base, refname, NULL,
 						      RESOLVE_REF_NO_RECURSE,
 						      NULL, &type);
 			update = !!(ref && !(type & REF_ISSYMREF));
diff --git a/refs/iterator.c b/refs/iterator.c
index 9db8b056d56..26ca6f645ee 100644
--- a/refs/iterator.c
+++ b/refs/iterator.c
@@ -199,6 +199,7 @@  static int merge_ref_iterator_advance(struct ref_iterator *ref_iterator)
 		}
 
 		if (selection & ITER_YIELD_CURRENT) {
+			iter->base.referent = (*iter->current)->referent;
 			iter->base.refname = (*iter->current)->refname;
 			iter->base.oid = (*iter->current)->oid;
 			iter->base.flags = (*iter->current)->flags;
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;
 }
@@ -428,6 +430,7 @@  static int cache_ref_iterator_advance(struct ref_iterator *ref_iterator)
 			level->prefix_state = entry_prefix_state;
 			level->index = -1;
 		} else {
+			iter->base.referent = entry->u.value.referent;
 			iter->base.refname = entry->name;
 			iter->base.oid = &entry->u.value.oid;
 			iter->base.flags = entry->flag;
diff --git a/refs/ref-cache.h b/refs/ref-cache.h
index 95c76e27c83..12ddee4fddc 100644
--- a/refs/ref-cache.h
+++ b/refs/ref-cache.h
@@ -42,6 +42,7 @@  struct ref_value {
 	 * referred to by the last reference in the symlink chain.
 	 */
 	struct object_id oid;
+	const char *referent;
 };
 
 /*
@@ -173,6 +174,7 @@  struct ref_entry *create_dir_entry(struct ref_cache *cache,
 				   const char *dirname, size_t len);
 
 struct ref_entry *create_ref_entry(const char *refname,
+				   const char *referent,
 				   const struct object_id *oid, int flag);
 
 /*
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index 56641aa57a1..a10363eccf4 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -319,6 +319,7 @@  enum do_for_each_ref_flags {
 struct ref_iterator {
 	struct ref_iterator_vtable *vtable;
 	const char *refname;
+	const char *referent;
 	const struct object_id *oid;
 	unsigned int flags;
 };
diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c
index e206d5a073c..7e8d15cc6a0 100644
--- a/refs/reftable-backend.c
+++ b/refs/reftable-backend.c
@@ -342,6 +342,7 @@  static int reftable_ref_iterator_advance(struct ref_iterator *ref_iterator)
 
 	while (!iter->err) {
 		int flags = 0;
+		char **symref = NULL;
 
 		iter->err = reftable_iterator_next_ref(&iter->iter, &iter->ref);
 		if (iter->err)
@@ -377,7 +378,10 @@  static int reftable_ref_iterator_advance(struct ref_iterator *ref_iterator)
 			oidread(&iter->oid, iter->ref.value.val2.value);
 			break;
 		case REFTABLE_REF_SYMREF:
-			if (!refs_resolve_ref_unsafe(&iter->refs->base, iter->ref.refname,
+			if (!iter->ref.value.symref)
+				symref = &iter->ref.value.symref;
+
+			if (!refs_resolve_ref_unsafe(&iter->refs->base, iter->ref.refname, symref,
 						     RESOLVE_REF_READING, &iter->oid, &flags))
 				oidclr(&iter->oid);
 			break;
@@ -406,6 +410,7 @@  static int reftable_ref_iterator_advance(struct ref_iterator *ref_iterator)
 				continue;
 
 		iter->base.refname = iter->ref.refname;
+		iter->base.referent = iter->ref.value.symref;
 		iter->base.oid = &iter->oid;
 		iter->base.flags = flags;
 
@@ -877,7 +882,7 @@  static int reftable_be_transaction_prepare(struct ref_store *ref_store,
 			 * so it is safe to call `refs_resolve_ref_unsafe()`
 			 * here without causing races.
 			 */
-			const char *resolved = refs_resolve_ref_unsafe(&refs->base, u->refname, 0,
+			const char *resolved = refs_resolve_ref_unsafe(&refs->base, u->refname, NULL, 0,
 								       &current_oid, NULL);
 
 			if (u->flags & REF_NO_DEREF) {
@@ -1252,7 +1257,7 @@  static int write_create_symref_table(struct reftable_writer *writer, void *cb_da
 	 * never happens.
 	 */
 	if (!create->logmsg ||
-	    !refs_resolve_ref_unsafe(&create->refs->base, create->target,
+	    !refs_resolve_ref_unsafe(&create->refs->base, create->target, NULL,
 				     RESOLVE_REF_READING, &new_oid, NULL) ||
 	    !should_write_log(&create->refs->base, create->refname))
 		return 0;
@@ -1263,7 +1268,7 @@  static int write_create_symref_table(struct reftable_writer *writer, void *cb_da
 	log.value.update.message = xstrndup(create->logmsg,
 					    create->refs->write_options.block_size / 2);
 	memcpy(log.value.update.new_hash, new_oid.hash, GIT_MAX_RAWSZ);
-	if (refs_resolve_ref_unsafe(&create->refs->base, create->refname,
+	if (refs_resolve_ref_unsafe(&create->refs->base, create->refname, NULL,
 				    RESOLVE_REF_READING, &old_oid, NULL))
 		memcpy(log.value.update.old_hash, old_oid.hash, GIT_MAX_RAWSZ);
 
diff --git a/remote.c b/remote.c
index 2b650b813b7..36525451e9c 100644
--- a/remote.c
+++ b/remote.c
@@ -518,7 +518,7 @@  static void read_config(struct repository *repo, int early)
 	repo->remote_state->current_branch = NULL;
 	if (startup_info->have_repository && !early) {
 		const char *head_ref = refs_resolve_ref_unsafe(
-			get_main_ref_store(repo), "HEAD", 0, NULL, &flag);
+			get_main_ref_store(repo), "HEAD", NULL, 0, NULL, &flag);
 		if (head_ref && (flag & REF_ISSYMREF) &&
 		    skip_prefix(head_ref, "refs/heads/", &head_ref)) {
 			repo->remote_state->current_branch = make_branch(
diff --git a/sequencer.c b/sequencer.c
index fa838f264f5..e9cc2a4fdf4 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1430,7 +1430,7 @@  void print_commit_summary(struct repository *r,
 	diff_setup_done(&rev.diffopt);
 
 	refs = get_main_ref_store(r);
-	head = refs_resolve_ref_unsafe(refs, "HEAD", 0, NULL, NULL);
+	head = refs_resolve_ref_unsafe(refs, "HEAD", NULL, 0, NULL, NULL);
 	if (!head)
 		die(_("unable to resolve HEAD after creating commit"));
 	if (!strcmp(head, "HEAD"))
@@ -4651,7 +4651,7 @@  static int apply_save_autostash_ref(struct repository *r, const char *refname,
 	if (!refs_ref_exists(get_main_ref_store(r), refname))
 		return 0;
 
-	if (!refs_resolve_ref_unsafe(get_main_ref_store(r), refname,
+	if (!refs_resolve_ref_unsafe(get_main_ref_store(r), refname, NULL,
 				     RESOLVE_REF_READING, &stash_oid, &flag))
 		return -1;
 	if (flag & REF_ISSYMREF)
diff --git a/t/helper/test-ref-store.c b/t/helper/test-ref-store.c
index 7a0f6cac53d..bc501c5253c 100644
--- a/t/helper/test-ref-store.c
+++ b/t/helper/test-ref-store.c
@@ -203,7 +203,7 @@  static int cmd_resolve_ref(struct ref_store *refs, const char **argv)
 	int flags;
 	const char *ref;
 
-	ref = refs_resolve_ref_unsafe(refs, refname, resolve_flags,
+	ref = refs_resolve_ref_unsafe(refs, refname, NULL, resolve_flags,
 				      &oid, &flags);
 	printf("%s %s 0x%x\n", oid_to_hex(&oid), ref ? ref : "(null)", flags);
 	return ref ? 0 : 1;
diff --git a/worktree.c b/worktree.c
index b02a05a74a3..1fb865c2ae7 100644
--- a/worktree.c
+++ b/worktree.c
@@ -42,6 +42,7 @@  static void add_head_info(struct worktree *wt)
 
 	target = refs_resolve_ref_unsafe(get_worktree_ref_store(wt),
 					 "HEAD",
+					 NULL,
 					 0,
 					 &wt->head_oid, &flags);
 	if (!target)
@@ -446,7 +447,7 @@  int is_shared_symref(const struct worktree *wt, const char *symref,
 	}
 
 	refs = get_worktree_ref_store(wt);
-	symref_target = refs_resolve_ref_unsafe(refs, symref, 0,
+	symref_target = refs_resolve_ref_unsafe(refs, symref, NULL, 0,
 						NULL, &flags);
 	if ((flags & REF_ISSYMREF) &&
 	    symref_target && !strcmp(symref_target, target))
@@ -547,6 +548,7 @@  int other_head_refs(each_ref_fn fn, void *cb_data)
 		strbuf_worktree_ref(wt, &refname, "HEAD");
 		if (refs_resolve_ref_unsafe(get_main_ref_store(the_repository),
 					    refname.buf,
+					    NULL,
 					    RESOLVE_REF_READING,
 					    &oid, &flag))
 			ret = fn(refname.buf, &oid, flag, cb_data);