[PATCH] refs/files: keep root refs out of loose ref cache

[PATCH] refs/files: keep root refs out of loose ref cache

[Andy Koppe]

With the files backend, root refs such as HEAD or ORIG_HEAD are only
added to the loose ref cache if REFS_FOR_EACH_INCLUDE_ROOT_REFS is
passed to the iteration that creates the cache. If the cache has already
been created by an earlier iteration, root refs are silently omitted.
Conversely, once they have been added, any later iteration also yields
them, whether or not it asked for them. This has been the case since
d0f00c1ac17 (refs: introduce `refs_for_each_include_root_refs()`).

For example, `git for-each-ref --include-root-refs --merged=main` omits
root refs that are merged into main, because looking up the given commit
loads the replace refs, which populates the loose ref cache first. The
reftable backend is not affected.

Fix this by keeping the root refs out of the shared loose ref cache.
Instead, read them into a separate ref cache whenever an iteration asks
for them, and overlay its iterator on the loose ref iterator. The cache
is freed when the iteration finishes.

Signed-off-by: Andy Koppe <andy.koppe@gmail.com>
---
 refs/files-backend.c           | 38 +++++++++++++++++++++++++++-------
 t/t6302-for-each-ref-filter.sh | 12 +++++++++++
 2 files changed, 42 insertions(+), 8 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 71628550f2..74409878b6 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -481,8 +481,21 @@ static void add_root_refs(struct files_ref_store *refs,
 	for_each_root_ref(refs, fill_root_ref, &data);
 }
 
-static struct ref_cache *get_loose_ref_cache(struct files_ref_store *refs,
-					     unsigned int flags)
+/*
+ * Create a separate ref cache containing only the root refs. These are kept
+ * out of the loose ref cache, which may already exist and is shared by
+ * iterations that should not include them.
+ */
+static struct ref_cache *create_root_ref_cache(struct files_ref_store *refs)
+{
+	struct ref_cache *cache = create_ref_cache(&refs->base, NULL);
+
+	cache->root->flag &= ~REF_INCOMPLETE;
+	add_root_refs(refs, get_ref_dir(cache->root));
+	return cache;
+}
+
+static struct ref_cache *get_loose_ref_cache(struct files_ref_store *refs)
 {
 	if (!refs->loose) {
 		struct ref_dir *dir;
@@ -499,9 +512,6 @@ static struct ref_cache *get_loose_ref_cache(struct files_ref_store *refs,
 
 		dir = get_ref_dir(refs->loose->root);
 
-		if (flags & REFS_FOR_EACH_INCLUDE_ROOT_REFS)
-			add_root_refs(refs, dir);
-
 		/*
 		 * Add an incomplete entry for "refs/" (to be filled
 		 * lazily):
@@ -1004,6 +1014,7 @@ struct files_ref_iterator {
 	struct ref_iterator base;
 
 	struct ref_iterator *iter0;
+	struct ref_cache *root_refs;
 	struct repository *repo;
 	unsigned int flags;
 };
@@ -1053,6 +1064,7 @@ static void files_ref_iterator_release(struct ref_iterator *ref_iterator)
 	struct files_ref_iterator *iter =
 		(struct files_ref_iterator *)ref_iterator;
 	ref_iterator_free(iter->iter0);
+	free_ref_cache(iter->root_refs);
 }
 
 static struct ref_iterator_vtable files_ref_iterator_vtable = {
@@ -1068,6 +1080,7 @@ static struct ref_iterator *files_ref_iterator_begin(
 {
 	struct files_ref_store *refs;
 	struct ref_iterator *loose_iter, *packed_iter, *overlay_iter;
+	struct ref_cache *root_refs = NULL;
 	struct files_ref_iterator *iter;
 	struct ref_iterator *ref_iterator;
 	unsigned int required_flags = REF_STORE_READ;
@@ -1094,9 +1107,17 @@ static struct ref_iterator *files_ref_iterator_begin(
 	 * disk, and re-reads it if not.
 	 */
 
-	loose_iter = cache_ref_iterator_begin(get_loose_ref_cache(refs, flags),
+	loose_iter = cache_ref_iterator_begin(get_loose_ref_cache(refs),
 					      prefix, ref_store->repo, 1);
 
+	if (flags & REFS_FOR_EACH_INCLUDE_ROOT_REFS) {
+		root_refs = create_root_ref_cache(refs);
+		loose_iter = overlay_ref_iterator_begin(
+				cache_ref_iterator_begin(root_refs, prefix,
+							 ref_store->repo, 0),
+				loose_iter);
+	}
+
 	/*
 	 * The packed-refs file might contain broken references, for
 	 * example an old version of a reference that points at an
@@ -1118,6 +1139,7 @@ static struct ref_iterator *files_ref_iterator_begin(
 	ref_iterator = &iter->base;
 	base_ref_iterator_init(ref_iterator, &files_ref_iterator_vtable);
 	iter->iter0 = overlay_iter;
+	iter->root_refs = root_refs;
 	iter->repo = ref_store->repo;
 	iter->flags = flags;
 
@@ -1490,7 +1512,7 @@ static int should_pack_refs(struct files_ref_store *refs,
 	if (limit < 16)
 		limit = 16;
 
-	iter = cache_ref_iterator_begin(get_loose_ref_cache(refs, 0), NULL,
+	iter = cache_ref_iterator_begin(get_loose_ref_cache(refs), NULL,
 					refs->base.repo, 0);
 	while ((ret = ref_iterator_advance(iter)) == ITER_OK) {
 		if (should_pack_ref(refs, &iter->ref, opts))
@@ -1530,7 +1552,7 @@ static int files_optimize(struct ref_store *ref_store,
 
 	packed_refs_lock(refs->packed_ref_store, LOCK_DIE_ON_ERROR, &err);
 
-	iter = cache_ref_iterator_begin(get_loose_ref_cache(refs, 0), NULL,
+	iter = cache_ref_iterator_begin(get_loose_ref_cache(refs), NULL,
 					refs->base.repo, 0);
 	while ((ok = ref_iterator_advance(iter)) == ITER_OK) {
 		/*
diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh
index 7f060d97bf..a2fc257f98 100755
--- a/t/t6302-for-each-ref-filter.sh
+++ b/t/t6302-for-each-ref-filter.sh
@@ -96,6 +96,18 @@ test_expect_success '--include-root-refs omits dangling symrefs' '
 	)
 '
 
+test_expect_success '--include-root-refs with --merged' '
+	cat >expect <<-\EOF &&
+	HEAD
+	ORIG_HEAD
+	EOF
+	test_when_finished "git update-ref -d ORIG_HEAD" &&
+	git update-ref ORIG_HEAD main &&
+	git for-each-ref --format="%(refname)" --include-root-refs \
+		--merged=main "*HEAD" >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'filtering with --points-at' '
 	cat >expect <<-\EOF &&
 	refs/heads/main
-- 
2.56.0

Re: [PATCH] refs/files: keep root refs out of loose ref cache

[Patrick Steinhardt]

On Wed, Oct 14, 2026 at 04:32:08PM +0100, Andy Koppe wrote:
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index 71628550f2..74409878b6 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -481,8 +481,21 @@ static void add_root_refs(struct files_ref_store *refs,
>  	for_each_root_ref(refs, fill_root_ref, &data);
>  }
>  
> -static struct ref_cache *get_loose_ref_cache(struct files_ref_store *refs,
> -					     unsigned int flags)
> +/*
> + * Create a separate ref cache containing only the root refs. These are kept
> + * out of the loose ref cache, which may already exist and is shared by
> + * iterations that should not include them.
> + */

This raises the question whether an alternative fix could have been to
teach the cache itself to skip over root refs in case the caller is not
interested in them. The answer may very well be a "no", but I would be
curious to learn why you settled on the proposed fix instead.

> +static struct ref_cache *create_root_ref_cache(struct files_ref_store *refs)
> +{
> +	struct ref_cache *cache = create_ref_cache(&refs->base, NULL);
> +
> +	cache->root->flag &= ~REF_INCOMPLETE;
> +	add_root_refs(refs, get_ref_dir(cache->root));
> +	return cache;
> +}
> +
> +static struct ref_cache *get_loose_ref_cache(struct files_ref_store *refs)
>  {
>  	if (!refs->loose) {
>  		struct ref_dir *dir;
> @@ -1094,9 +1107,17 @@ static struct ref_iterator *files_ref_iterator_begin(
>  	 * disk, and re-reads it if not.
>  	 */
>  
> -	loose_iter = cache_ref_iterator_begin(get_loose_ref_cache(refs, flags),
> +	loose_iter = cache_ref_iterator_begin(get_loose_ref_cache(refs),
>  					      prefix, ref_store->repo, 1);
>  
> +	if (flags & REFS_FOR_EACH_INCLUDE_ROOT_REFS) {
> +		root_refs = create_root_ref_cache(refs);
> +		loose_iter = overlay_ref_iterator_begin(
> +				cache_ref_iterator_begin(root_refs, prefix,
> +							 ref_store->repo, 0),
> +				loose_iter);
> +	}

It's a bit weird that we recreate the root ref "cache" for every single
ref iterator we create that asks for root refs. Is there a reason why
you have decided to not actually cache the root refs the same way as we
cache the loose refs?

Thanks!

Patrick

Re: [PATCH] refs/files: keep root refs out of loose ref cache

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

>> +/*
>> + * Create a separate ref cache containing only the root refs. These are kept
>> + * out of the loose ref cache, which may already exist and is shared by
>> + * iterations that should not include them.
>> + */
>
> This raises the question whether an alternative fix could have been to
> teach the cache itself to skip over root refs in case the caller is not
> interested in them. The answer may very well be a "no", but I would be
> curious to learn why you settled on the proposed fix instead.
> ...
>> -	loose_iter = cache_ref_iterator_begin(get_loose_ref_cache(refs, flags),
>> +	loose_iter = cache_ref_iterator_begin(get_loose_ref_cache(refs),
>>  					      prefix, ref_store->repo, 1);
>>  
>> +	if (flags & REFS_FOR_EACH_INCLUDE_ROOT_REFS) {
>> +		root_refs = create_root_ref_cache(refs);
>> +		loose_iter = overlay_ref_iterator_begin(
>> +				cache_ref_iterator_begin(root_refs, prefix,
>> +							 ref_store->repo, 0),
>> +				loose_iter);
>> +	}
>
> It's a bit weird that we recreate the root ref "cache" for every single
> ref iterator we create that asks for root refs. Is there a reason why
> you have decided to not actually cache the root refs the same way as we
> cache the loose refs?

Curious.

So an alternative would be to have two caches, the primary one for
things under refs/ hierarchy and the other one for those outside,
and callers can choose to use only the primary one or use both of
them?

Re: [PATCH] refs/files: keep root refs out of loose ref cache

[Andy Koppe]

Junio C Hamano <gitster@pobox.com> wrote:
> Patrick Steinhardt <ps@pks.im> writes:
>>> +/*
>>> + * Create a separate ref cache containing only the root refs. These are kept
>>> + * out of the loose ref cache, which may already exist and is shared by
>>> + * iterations that should not include them.
>>> + */
>>
>> This raises the question whether an alternative fix could have been to
>> teach the cache itself to skip over root refs in case the caller is not
>> interested in them. The answer may very well be a "no", but I would be
>> curious to learn why you settled on the proposed fix instead.
>> ...
>>> -    loose_iter = cache_ref_iterator_begin(get_loose_ref_cache(refs, flags),
>>> +    loose_iter = cache_ref_iterator_begin(get_loose_ref_cache(refs),
>>>                                            prefix, ref_store->repo, 1);
>>>
>>> +    if (flags & REFS_FOR_EACH_INCLUDE_ROOT_REFS) {
>>> +            root_refs = create_root_ref_cache(refs);
>>> +            loose_iter = overlay_ref_iterator_begin(
>>> +                            cache_ref_iterator_begin(root_refs, prefix,
>>> +                                                     ref_store->repo, 0),
>>> +                            loose_iter);
>>> +    }
>>
>> It's a bit weird that we recreate the root ref "cache" for every single
>> ref iterator we create that asks for root refs. Is there a reason why
>> you have decided to not actually cache the root refs the same way as we
>> cache the loose refs?
>
> Curious.
>
> So an alternative would be to have two caches, the primary one for
> things under refs/ hierarchy and the other one for those outside,
> and callers can choose to use only the primary one or use both of
> them?

Thanks for considering this.

I think it makes for a simpler change with less overhead to not
actually cache them, as there are only two uses of the
REFS_FOR_EACH_INCLUDE_ROOT_REFS flag: in builtin/for-each-ref.c for
`git for-each-ref --include-root-refs`, and in builtin/refs.c for `git
refs migrate`. And both only invoke it once.

I considered keeping the root refs in the loose ref cache but skipping
them during iteration if the flag is unset. But that would mean more
impact on that much more common case.

A cleaner approach might be to drop the flag and change all call sites
that currently ask for all refs without setting the flag, so that they
explicitly ask for "refs/" only. That way root refs wouldn't need as
much special handling. But of course that would be a much bigger and
more risky change.

Andy

Re: [PATCH] refs/files: keep root refs out of loose ref cache

[Patrick Steinhardt]

On Thu, Oct 15, 2026 at 08:43:06PM +0100, Andy Koppe wrote:
> Junio C Hamano <gitster@pobox.com> wrote:
> > Patrick Steinhardt <ps@pks.im> writes:
> >>> +/*
> >>> + * Create a separate ref cache containing only the root refs. These are kept
> >>> + * out of the loose ref cache, which may already exist and is shared by
> >>> + * iterations that should not include them.
> >>> + */
> >>
> >> This raises the question whether an alternative fix could have been to
> >> teach the cache itself to skip over root refs in case the caller is not
> >> interested in them. The answer may very well be a "no", but I would be
> >> curious to learn why you settled on the proposed fix instead.
> >> ...
> >>> -    loose_iter = cache_ref_iterator_begin(get_loose_ref_cache(refs, flags),
> >>> +    loose_iter = cache_ref_iterator_begin(get_loose_ref_cache(refs),
> >>>                                            prefix, ref_store->repo, 1);
> >>>
> >>> +    if (flags & REFS_FOR_EACH_INCLUDE_ROOT_REFS) {
> >>> +            root_refs = create_root_ref_cache(refs);
> >>> +            loose_iter = overlay_ref_iterator_begin(
> >>> +                            cache_ref_iterator_begin(root_refs, prefix,
> >>> +                                                     ref_store->repo, 0),
> >>> +                            loose_iter);
> >>> +    }
> >>
> >> It's a bit weird that we recreate the root ref "cache" for every single
> >> ref iterator we create that asks for root refs. Is there a reason why
> >> you have decided to not actually cache the root refs the same way as we
> >> cache the loose refs?
> >
> > Curious.
> >
> > So an alternative would be to have two caches, the primary one for
> > things under refs/ hierarchy and the other one for those outside,
> > and callers can choose to use only the primary one or use both of
> > them?
> 
> Thanks for considering this.
> 
> I think it makes for a simpler change with less overhead to not
> actually cache them, as there are only two uses of the
> REFS_FOR_EACH_INCLUDE_ROOT_REFS flag: in builtin/for-each-ref.c for
> `git for-each-ref --include-root-refs`, and in builtin/refs.c for `git
> refs migrate`. And both only invoke it once.
> 
> I considered keeping the root refs in the loose ref cache but skipping
> them during iteration if the flag is unset. But that would mean more
> impact on that much more common case.
> 
> A cleaner approach might be to drop the flag and change all call sites
> that currently ask for all refs without setting the flag, so that they
> explicitly ask for "refs/" only. That way root refs wouldn't need as
> much special handling. But of course that would be a much bigger and
> more risky change.

That's fair. I guess the part that triggers me is that it's called a
"cache" whereas we don't really use it as one in this instance. But
that's a historical wart, and I agree that the proposed solution is good
enough.

Thanks!

Patrick

Re: [PATCH] refs/files: keep root refs out of loose ref cache

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> On Thu, Oct 15, 2026 at 08:43:06PM +0100, Andy Koppe wrote:
>> Junio C Hamano <gitster@pobox.com> wrote:
>> > Patrick Steinhardt <ps@pks.im> writes:
>> > ...

So in short ...

>> >> This raises the question whether an alternative fix could have been to
>> >> teach the cache itself to skip over root refs in case the caller is not
>> >> interested in them. The answer may very well be a "no", but I would be
>> >> curious to learn why you settled on the proposed fix instead.

... with this cleared in the proposed log message of v2 iteration, ...

>> A cleaner approach might be to drop the flag and change all call sites
>> that currently ask for all refs without setting the flag, so that they
>> explicitly ask for "refs/" only. That way root refs wouldn't need as
>> much special handling. But of course that would be a much bigger and
>> more risky change.
>
> That's fair. I guess the part that triggers me is that it's called a
> "cache" whereas we don't really use it as one in this instance. But
> that's a historical wart, and I agree that the proposed solution is good
> enough.

... the topic will be ready to go?

Thanks.