Re: [PATCH v2] ref-filter: restore prefix-scoped iteration

>
> Tamir Duberstein <tamird@gmail.com> writes:
>
> > Commit dabecb9db2 (for-each-ref: introduce a '--start-after' option,
> > 2025-07-15) changed single-kind branch, remote-tracking branch, and tag
> > enumeration in do_filter_refs() from constructing an iterator with the
> > namespace prefix to constructing an unscoped iterator and applying the
> > prefix with ref_iterator_seek().
> >
> > Before that change, refs_for_each_fullref_in() passed the namespace
> > prefix during iterator construction. That helper has since been
> > replaced by refs_for_each_ref_ext().
> >
> > The files backend primes its loose-ref cache for the construction
> > prefix before it opens packed refs. An empty construction prefix
> > therefore reads every loose ref, and a later seek cannot undo that I/O.
> > Consequently, git branch, git branch --remotes, and git tag scale with
> > unrelated loose refs.
> >
>
> And this is the crux of the issue. Currently we do
>
> - refs_ref_iterator_begin()
>   - ref_iterator_seek()
>
> And between the two `cache_ref_iterator_set_prefix()` is already called
> which caches all the loose refs. This is the IO intensive operation this
> patch tries to avoid.
>
> I think it would be worthwhile to add this information in the commit
> message.

>
> >
> > Patrick Steinhardt observed during review that iterator construction
> > and seeking accepted similar strings but assigned them different state
> > semantics. Junio C Hamano then pointed out that no current command can
> > combine start_after with this single-kind path, but future branch or
> > tag support would need to keep the namespace while moving the cursor.
> >
> > Keep the existing start_after path unchanged. The iterator API cannot
> > currently seek to one string while retaining another as its prefix:
> > an unflagged seek clears the prefix, while REF_ITERATOR_SEEK_SET_PREFIX
> > replaces it with the seek string.
> >
> > For the commands affected by this regression, which do not set
> > start_after, pass the namespace prefix during iterator construction so
> > that loose refs are scoped before the packed-refs snapshot is opened.
> > This fixes the current regression without deleting the ref-filter state
> > discussed during review or changing its dormant behavior.
> >
> > Add REFFILES-gated performance cases with one branch, one
> > remote-tracking branch, one tag, and 10,000 unrelated loose refs. The
> > benchmarks were run with:
> >
> >     GIT_PERF_REPEAT_COUNT=5 GIT_PERF_MAKE_OPTS=-j8 \
> >         t/perf/run a89346e34a . -- p6300-for-each-ref.sh
> >
> > The following are the best of five runs, with each run invoking the
> > command ten times. Times are elapsed seconds with user and system CPU
> > seconds in parentheses:
> >
> >                                   a89346e34a       this commit
> >   branch                       2.74(0.13+2.56)   0.11(0.04+0.04)
> >   branch --remotes             2.81(0.13+2.62)   0.12(0.04+0.04)
> >   tag                          3.01(0.14+2.82)   0.11(0.04+0.04)
> >
> > Both revisions used the default -O2 build flags and a config.mak
> > containing only "NO_REGEX = NeedsStartEnd". They were built with Apple
> > clang 21.0.0 on macOS 26.5. The machine was a MacBook Pro (Mac16,6)
> > with a 16-core Apple M4 Max (12 performance and four efficiency cores)
> > and 128 GB RAM.
> >
> > Link: https://lore.kernel.org/git/aGZidwwlToWThkn8@pks.im/
> > Link: https://lore.kernel.org/git/xmqqikjq7s16.fsf@gitster.g/
> > Fixes: dabecb9db2b2 ("for-each-ref: introduce a '--start-after' option")
> > Assisted-by: Codex gpt-5.5
> > Signed-off-by: Tamir Duberstein <tamird@gmail.com>
> > ---
> > The series is based on a89346e34a (maint) because the regression has
> > been present in released versions since Git 2.51.0.
> > ---
> > Changes in v2:
> > - Extract local variable `store`.
> > - Link to v1: https://patch.msgid.link/20260605-fix-git-branch-regression-v1-1-02f40ad40929@gmail.com
> > ---
> >  ref-filter.c                 | 28 +++++++++++++++++++---------
> >  t/perf/p6300-for-each-ref.sh | 39 ++++++++++++++++++++++++++++++++++++++-
> >  2 files changed, 57 insertions(+), 10 deletions(-)
> >
> > diff --git a/ref-filter.c b/ref-filter.c
> > index 1da4c0e60d..5cbc007d64 100644
> > --- a/ref-filter.c
> > +++ b/ref-filter.c
> > @@ -3315,19 +3315,29 @@ static int do_filter_refs(struct ref_filter *filter, unsigned int type, refs_for
> >               prefix = "refs/tags/";
> >
> >       if (prefix) {
> > -             struct ref_iterator *iter;
> > +             struct ref_store *store = get_main_ref_store(the_repository);
> >
> > -             iter = refs_ref_iterator_begin(get_main_ref_store(the_repository),
> > -                                            "", NULL, 0, 0);
> > +             if (filter->start_after) {
> > +                     struct ref_iterator *iter;
> > +
> > +                     iter = refs_ref_iterator_begin(store, "", NULL, 0, 0);
> >
> > -             if (filter->start_after)
> >                       ret = start_ref_iterator_after(iter, filter->start_after);
> > -             else
> > -                     ret = ref_iterator_seek(iter, prefix,
> > -                                             REF_ITERATOR_SEEK_SET_PREFIX);
> > +                     if (!ret)
> > +                             ret = do_for_each_ref_iterator(iter, fn,
> > +                                                            cb_data);
> > +             } else {
> > +                     /*
> > +                      * Pass the prefix during construction because the files
> > +                      * backend primes loose refs before a later seek can
> > +                      * narrow the iterator.
> > +                      */
> > +                     struct refs_for_each_ref_options opts = {
> > +                             .prefix = prefix,
> > +                     };
> >
> > -             if (!ret)
> > -                     ret = do_for_each_ref_iterator(iter, fn, cb_data);
> > +                     ret = refs_for_each_ref_ext(store, fn, cb_data, &opts);
> > +             }
>
> This would work, as now we separate out the regular path to use
> `do_for_each_ref_iterator()` instead.
>
> But this causes a bit of confusion, why do we need to use
> `do_for_each_ref_iterator()` and why not simply provide the prefix to
> `refs_ref_iterator_begin()`, like before?