From: Tamir Duberstein Date: Wed, 10 Jun 2026 12:25:03 GMT Subject: Re: [PATCH v2] ref-filter: restore prefix-scoped iteration Message-ID: In-Reply-To: On Wed, Jun 10, 2026 at 3:50 AM Karthik Nayak wrote: > > Tamir Duberstein 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. Agreed. I will explain that `cache_ref_iterator_set_prefix()` primes the loose-ref cache during iterator construction, before the later seek can narrow it. > > > > > 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 > > --- > > 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? We do not. Your version is simpler and preserves the existing iterator flow. I have adopted it for v3. Thanks! > [...] > > Thanks for the patch, this is indeed a regression we must fix and the > benchmarks are a clear indication of it. Thank you! I'll try not to break threading on the next roll.