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

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