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

From: Andy Koppe <andy.koppe@gmail.com>
Date: Oct 15, 2026, 19:43 UTC
Message-ID: <CAHWeT-Z4n=txTGaBNuGPj3jcOou91owB59gvaPvF=2_p50eFHA@mail.gmail.com>
In-Reply-To: <xmqqmrseooor.fsf@gitster.g>

Junio C Hamano <gitster@pobox.com> wrote:

Show 35 quoted lines

> 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