{"thread":{"id":"65760","subject":"[PATCH] ref-filter: restore prefix-scoped iteration","startedAt":"2026-06-05T16:43:08Z","lastAt":"2026-06-09T06:05:19Z","messageCount":4,"participants":["Tamir Duberstein","Junio C Hamano","Patrick Steinhardt"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"544805","messageId":"20260605-fix-git-branch-regression-v1-1-02f40ad40929@gmail.com","threadId":"65760","inReplyTo":null,"subject":"[PATCH] ref-filter: restore prefix-scoped iteration","fromName":"Tamir Duberstein","fromEmail":"tamird@gmail.com","sentAt":"2026-06-05T16:43:03Z","receivedAt":"2026-06-05T16:43:08Z","isPatch":true,"body":"Commit dabecb9db2 (for-each-ref: introduce a '--start-after' option,\n2025-07-15) changed single-kind branch, remote-tracking branch, and tag\nenumeration in do_filter_refs() from constructing an iterator with the\nnamespace prefix to constructing an unscoped iterator and applying the\nprefix with ref_iterator_seek().\n\nBefore that change, refs_for_each_fullref_in() passed the namespace\nprefix during iterator construction. That helper has since been\nreplaced by refs_for_each_ref_ext().\n\nThe files backend primes its loose-ref cache for the construction\nprefix before it opens packed refs. An empty construction prefix\ntherefore reads every loose ref, and a later seek cannot undo that I/O.\nConsequently, git branch, git branch --remotes, and git tag scale with\nunrelated loose refs.\n\nPatrick Steinhardt observed during review that iterator construction\nand seeking accepted similar strings but assigned them different state\nsemantics. Junio C Hamano then pointed out that no current command can\ncombine start_after with this single-kind path, but future branch or\ntag support would need to keep the namespace while moving the cursor.\n\nKeep the existing start_after path unchanged. The iterator API cannot\ncurrently seek to one string while retaining another as its prefix:\nan unflagged seek clears the prefix, while REF_ITERATOR_SEEK_SET_PREFIX\nreplaces it with the seek string.\n\nFor the commands affected by this regression, which do not set\nstart_after, pass the namespace prefix during iterator construction so\nthat loose refs are scoped before the packed-refs snapshot is opened.\nThis fixes the current regression without deleting the ref-filter state\ndiscussed during review or changing its dormant behavior.\n\nAdd REFFILES-gated performance cases with one branch, one\nremote-tracking branch, one tag, and 10,000 unrelated loose refs. The\nbenchmarks were run with:\n\n    GIT_PERF_REPEAT_COUNT=5 GIT_PERF_MAKE_OPTS=-j8 \\\n        t/perf/run a89346e34a . -- p6300-for-each-ref.sh\n\nThe following are the best of five runs, with each run invoking the\ncommand ten times. Times are elapsed seconds with user and system CPU\nseconds in parentheses:\n\n                                  a89346e34a       this commit\n  branch                       2.74(0.13+2.56)   0.11(0.04+0.04)\n  branch --remotes             2.81(0.13+2.62)   0.12(0.04+0.04)\n  tag                          3.01(0.14+2.82)   0.11(0.04+0.04)\n\nBoth revisions used the default -O2 build flags and a config.mak\ncontaining only \"NO_REGEX = NeedsStartEnd\". They were built with Apple\nclang 21.0.0 on macOS 26.5. The machine was a MacBook Pro (Mac16,6)\nwith a 16-core Apple M4 Max (12 performance and four efficiency cores)\nand 128 GB RAM.\n\nLink: https://lore.kernel.org/git/aGZidwwlToWThkn8@pks.im/\nLink: https://lore.kernel.org/git/xmqqikjq7s16.fsf@gitster.g/\nFixes: dabecb9db2b2 (\"for-each-ref: introduce a '--start-after' option\")\nAssisted-by: Codex gpt-5.5\nSigned-off-by: Tamir Duberstein <tamird@gmail.com>\n---\nThe series is based on a89346e34a (maint) because the regression has\nbeen present in released versions since Git 2.51.0.\n---\n ref-filter.c                 | 30 +++++++++++++++++++++---------\n t/perf/p6300-for-each-ref.sh | 39 ++++++++++++++++++++++++++++++++++++++-\n 2 files changed, 59 insertions(+), 10 deletions(-)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex 1da4c0e60d..2388a57b39 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -3315,19 +3315,31 @@ static int do_filter_refs(struct ref_filter *filter, unsigned int type, refs_for\n \t\tprefix = \"refs/tags/\";\n \n \tif (prefix) {\n-\t\tstruct ref_iterator *iter;\n+\t\tif (filter->start_after) {\n+\t\t\tstruct ref_iterator *iter;\n \n-\t\titer = refs_ref_iterator_begin(get_main_ref_store(the_repository),\n-\t\t\t\t\t       \"\", NULL, 0, 0);\n+\t\t\titer = refs_ref_iterator_begin(\n+\t\t\t\tget_main_ref_store(the_repository), \"\", NULL, 0,\n+\t\t\t\t0);\n \n-\t\tif (filter->start_after)\n \t\t\tret = start_ref_iterator_after(iter, filter->start_after);\n-\t\telse\n-\t\t\tret = ref_iterator_seek(iter, prefix,\n-\t\t\t\t\t\tREF_ITERATOR_SEEK_SET_PREFIX);\n+\t\t\tif (!ret)\n+\t\t\t\tret = do_for_each_ref_iterator(iter, fn,\n+\t\t\t\t\t\t\t       cb_data);\n+\t\t} else {\n+\t\t\t/*\n+\t\t\t * Pass the prefix during construction because the files\n+\t\t\t * backend primes loose refs before a later seek can\n+\t\t\t * narrow the iterator.\n+\t\t\t */\n+\t\t\tstruct refs_for_each_ref_options opts = {\n+\t\t\t\t.prefix = prefix,\n+\t\t\t};\n \n-\t\tif (!ret)\n-\t\t\tret = do_for_each_ref_iterator(iter, fn, cb_data);\n+\t\t\tret = refs_for_each_ref_ext(\n+\t\t\t\tget_main_ref_store(the_repository), fn, cb_data,\n+\t\t\t\t&opts);\n+\t\t}\n \t} else if (filter->kind & FILTER_REFS_REGULAR) {\n \t\tret = for_each_fullref_in_pattern(filter, fn, cb_data);\n \t}\ndiff --git a/t/perf/p6300-for-each-ref.sh b/t/perf/p6300-for-each-ref.sh\nindex fa7289c752..ed9c1c6a19 100755\n--- a/t/perf/p6300-for-each-ref.sh\n+++ b/t/perf/p6300-for-each-ref.sh\n@@ -1,6 +1,6 @@\n #!/bin/sh\n \n-test_description='performance of for-each-ref'\n+test_description='performance of ref-filter users'\n . ./perf-lib.sh\n \n test_perf_fresh_repo\n@@ -84,4 +84,41 @@ test_expect_success 'pack refs' '\n '\n run_tests \"packed\"\n \n+test_expect_success REFFILES 'setup many unrelated loose refs' '\n+\tgit init scoped &&\n+\ttest_commit -C scoped --no-tag base &&\n+\ttest_seq $ref_count_per_type |\n+\t\tsed \"s,.*,update refs/custom/unrelated_& HEAD,\" |\n+\t\tgit -C scoped update-ref --stdin &&\n+\tgit -C scoped update-ref refs/remotes/origin/main HEAD &&\n+\tgit -C scoped update-ref refs/tags/only HEAD\n+'\n+\n+test_perf \"branch (many unrelated loose refs)\" --prereq REFFILES \"\n+\t(\n+\t\tcd scoped &&\n+\t\tfor i in \\$(test_seq $test_iteration_count); do\n+\t\t\tgit branch --format='%(refname)' >/dev/null\n+\t\tdone\n+\t)\n+\"\n+\n+test_perf \"branch --remotes (many unrelated loose refs)\" --prereq REFFILES \"\n+\t(\n+\t\tcd scoped &&\n+\t\tfor i in \\$(test_seq $test_iteration_count); do\n+\t\t\tgit branch --remotes --format='%(refname)' >/dev/null\n+\t\tdone\n+\t)\n+\"\n+\n+test_perf \"tag (many unrelated loose refs)\" --prereq REFFILES \"\n+\t(\n+\t\tcd scoped &&\n+\t\tfor i in \\$(test_seq $test_iteration_count); do\n+\t\t\tgit tag --format='%(refname)' >/dev/null\n+\t\tdone\n+\t)\n+\"\n+\n test_done\n\n---\nbase-commit: a89346e34a937f001e5d397ee62224e3e9852040\nchange-id: 20260605-fix-git-branch-regression-9e4236f18091\n\nBest regards,\n--  \nTamir Duberstein <tamird@gmail.com>\n\n"},{"id":"544967","messageId":"xmqqpl20vhni.fsf@gitster.g","threadId":"65760","inReplyTo":"20260605-fix-git-branch-regression-v1-1-02f40ad40929@gmail.com","subject":"Re: [PATCH] ref-filter: restore prefix-scoped iteration","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-06-08T21:36:33Z","receivedAt":"2026-06-08T21:36:36Z","isPatch":true,"body":"Tamir Duberstein <tamird@gmail.com> writes:\n\n> diff --git a/ref-filter.c b/ref-filter.c\n> index 1da4c0e60d..2388a57b39 100644\n> --- a/ref-filter.c\n> +++ b/ref-filter.c\n> @@ -3315,19 +3315,31 @@ static int do_filter_refs(struct ref_filter *filter, unsigned int type, refs_for\n>  \t\tprefix = \"refs/tags/\";\n>  \n>  \tif (prefix) {\n\nBelow, adding an extra call to get_main_ref_store(the_repository)\nmakes one line unnecessarily split and harder to read.  How about\ndoing\n\n\t\tstruct ref_store *store = get_main_ref_store(the_repository);\n\nupfront here, and then use that to replace these two calls of\nget_main_ref_store(the_repository)?\n\n> +\t\tif (filter->start_after) {\n> +\t\t\tstruct ref_iterator *iter;\n>  \n> +\t\t\titer = refs_ref_iterator_begin(\n> +\t\t\t\tget_main_ref_store(the_repository), \"\", NULL, 0,\n> +\t\t\t\t0);\n>  \n>  \t\t\tret = start_ref_iterator_after(iter, filter->start_after);\n> +\t\t\tif (!ret)\n> +\t\t\t\tret = do_for_each_ref_iterator(iter, fn,\n> +\t\t\t\t\t\t\t       cb_data);\n> +\t\t} else {\n> +\t\t\t/*\n> +\t\t\t * Pass the prefix during construction because the files\n> +\t\t\t * backend primes loose refs before a later seek can\n> +\t\t\t * narrow the iterator.\n> +\t\t\t */\n> +\t\t\tstruct refs_for_each_ref_options opts = {\n> +\t\t\t\t.prefix = prefix,\n> +\t\t\t};\n>  \n> +\t\t\tret = refs_for_each_ref_ext(\n> +\t\t\t\tget_main_ref_store(the_repository), fn, cb_data,\n> +\t\t\t\t&opts);\n> +\t\t}\n>  \t} else if (filter->kind & FILTER_REFS_REGULAR) {\n"},{"id":"544974","messageId":"CAJ-ks9m9gq-=JB-gqeKaL4YOLSfrP2Cm0DytZjuC3OetG-UVbA@mail.gmail.com","threadId":"65760","inReplyTo":"xmqqpl20vhni.fsf@gitster.g","subject":"Re: [PATCH] ref-filter: restore prefix-scoped iteration","fromName":"Tamir Duberstein","fromEmail":"tamird@gmail.com","sentAt":"2026-06-08T22:39:48Z","receivedAt":"2026-06-08T22:40:27Z","isPatch":true,"body":"On Mon, Jun 8, 2026 at 2:36 PM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Tamir Duberstein <tamird@gmail.com> writes:\n>\n> > diff --git a/ref-filter.c b/ref-filter.c\n> > index 1da4c0e60d..2388a57b39 100644\n> > --- a/ref-filter.c\n> > +++ b/ref-filter.c\n> > @@ -3315,19 +3315,31 @@ static int do_filter_refs(struct ref_filter *filter, unsigned int type, refs_for\n> >               prefix = \"refs/tags/\";\n> >\n> >       if (prefix) {\n>\n> Below, adding an extra call to get_main_ref_store(the_repository)\n> makes one line unnecessarily split and harder to read.  How about\n> doing\n>\n>                 struct ref_store *store = get_main_ref_store(the_repository);\n>\n> upfront here, and then use that to replace these two calls of\n> get_main_ref_store(the_repository)?\n\nYep, done in v2.\n\nThanks for the review!\n\nBy the way, how long should I wait before sending new versions of my\npatches? I have 4 outstanding at the moment.\n"},{"id":"545010","messageId":"aietF4BX1Ewt3cpG@pks.im","threadId":"65760","inReplyTo":"CAJ-ks9m9gq-=JB-gqeKaL4YOLSfrP2Cm0DytZjuC3OetG-UVbA@mail.gmail.com","subject":"Re: [PATCH] ref-filter: restore prefix-scoped iteration","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-06-09T06:05:11Z","receivedAt":"2026-06-09T06:05:19Z","isPatch":true,"body":"On Mon, Jun 08, 2026 at 06:39:48PM -0400, Tamir Duberstein wrote:\n> On Mon, Jun 8, 2026 at 2:36 PM Junio C Hamano <gitster@pobox.com> wrote:\n> >\n> > Tamir Duberstein <tamird@gmail.com> writes:\n> >\n> > > diff --git a/ref-filter.c b/ref-filter.c\n> > > index 1da4c0e60d..2388a57b39 100644\n> > > --- a/ref-filter.c\n> > > +++ b/ref-filter.c\n> > > @@ -3315,19 +3315,31 @@ static int do_filter_refs(struct ref_filter *filter, unsigned int type, refs_for\n> > >               prefix = \"refs/tags/\";\n> > >\n> > >       if (prefix) {\n> >\n> > Below, adding an extra call to get_main_ref_store(the_repository)\n> > makes one line unnecessarily split and harder to read.  How about\n> > doing\n> >\n> >                 struct ref_store *store = get_main_ref_store(the_repository);\n> >\n> > upfront here, and then use that to replace these two calls of\n> > get_main_ref_store(the_repository)?\n> \n> Yep, done in v2.\n> \n> Thanks for the review!\n> \n> By the way, how long should I wait before sending new versions of my\n> patches? I have 4 outstanding at the moment.\n\nI typically aim to send at most one version per day per patch series.\nThis avoids that you're \"flooding\" the mailing list with too many\nversions of the same series, allows you to address feedback from\nmultiple folks in batches, and it gives you enough time to think about\nthe feedback without having to rush anything.\n\nWhether I actually do end up sending a series depends on a couple of\nfactors:\n\n  - How big is the series? The bigger it is the more time I give folks\n    to perform reviews.\n\n  - How substantial were the reviews you received? Is it just a couple\n    of small typos? Then it probably makes sense to wait one or two more\n    days to get some more involved reviews. Is it something that\n    requires signifciant rework? Then I'd send out soon so that others\n    don't review a patch series that will change significantly anyway.\n\n  - How close to being merged is the series? The closer it is the less\n    substantial the reviews will (hopefully) get, so it makes sense to\n    reroll a bit faster even if you only received minor feedback.\n\nSo there isn't really a golden rule to follow here, but a lot of this\ndepends on gut feeling. You probably won't have that feeling yet when\nstarting out in a new project, but that's fine. In case we see that\nbehaviour doesn't quite match the norm we'll typically give a hint that\nthe contributor should slow down or maybe send a new iteration.\n\nPatrick\n"}]}