# [PATCH 0/5] ref-filter: small cleanups and fixes 22 messages from 2025-07-24 to 2025-07-29. Participants: Karthik Nayak, Junio C Hamano, Patrick Steinhardt. Thread: https://gitlist.dev/t/63835 ## Karthik Nayak, 2025-07-24 08:14 Subject: [PATCH 0/5] ref-filter: small cleanups and fixes Message-ID: <20250724-kn-small-cleanups-v1-0-0c70f591de3e@gmail.com> URL: https://gitlist.dev/e/20250724-kn-small-cleanups-v1-0-0c70f591de3e%40gmail.com ``` This series contains a few of the small fixes and comments which I've gathered from reviews of my earlier series [1] to add the '--start-after' flag to 'git-for-each-ref(1)'. Individually each patch doesn't hold too much weight on its own, but together these small improvements add up. That said, if these patches are too small for the noise generated, we could simply drop it or combine some commits together. This is based on top of 3f2a94875d (The twelfth batch, 2025-07-21) with 'kn/for-each-ref-skip' merged in. [1]: https://lore.kernel.org/r/20250701-306-git-for-each-ref-pagination-v1-0-4f0ae7c0688f@gmail.com Signed-off-by: Karthik Nayak --- Documentation/git-for-each-ref.adoc | 9 +++++---- builtin/for-each-ref.c | 2 +- ref-filter.c | 5 +++-- refs/ref-cache.c | 5 +++-- t/t6302-for-each-ref-filter.sh | 19 +++++++++++++++++++ 5 files changed, 31 insertions(+), 9 deletions(-) Karthik Nayak (5): ref-cache: use 'size_t' instead of int for length for-each-ref: fix documentation argument ordering for-each-ref: reword the documentation for '--start-after' t6302: add test combining '--start-after' with '--exclude' ref-filter: use REF_ITERATOR_SEEK_SET_PREFIX instead of '1' base-commit: 15fcbd8f16a2c119a5319b0657e52fe0f387df46 change-id: 20250721-kn-small-cleanups-a01fe5d2756d Thanks - Karthik ``` ## Karthik Nayak, 2025-07-24 08:14 Subject: [PATCH 1/5] ref-cache: use 'size_t' instead of int for length Message-ID: <20250724-kn-small-cleanups-v1-1-0c70f591de3e@gmail.com> URL: https://gitlist.dev/e/20250724-kn-small-cleanups-v1-1-0c70f591de3e%40gmail.com In-Reply-To: <20250724-kn-small-cleanups-v1-0-0c70f591de3e@gmail.com> ``` The commit 090eb5336c (refs: selectively set prefix in the seek functions, 2025-07-15) modified the ref-cache iterator to support seeking to a specified marker without setting the prefix. The commit adds and uses an integer 'len' to capture the length of the seek marker to compare with the entries of a given directory. Since the type of the variable is 'int', this is met with a typecast of converting a `strlen` to 'int' so it can be assigned to the 'len' variable. This is whole operation is a bit wrong: 1. Since the 'len' variable is eventually used in a 'strncmp', it should have been of type 'size_t'. 2. This also truncates the value provided from 'strlen' to an int, which could cause a large refname to produce a negative number. Let's do the correct thing here and simply use 'size_t' for `len`. Signed-off-by: Karthik Nayak --- refs/ref-cache.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/refs/ref-cache.c b/refs/ref-cache.c index 1d95b56d40..8df7ae43e5 100644 --- a/refs/ref-cache.c +++ b/refs/ref-cache.c @@ -498,13 +498,14 @@ static int cache_ref_iterator_seek(struct ref_iterator *ref_iterator, * indexing to each level as needed. */ do { - int len, idx; + int idx; + size_t len; int cmp = 0; sort_ref_dir(dir); slash = strchr(slash, '/'); - len = slash ? slash - refname : (int)strlen(refname); + len = slash ? (size_t)(slash - refname) : strlen(refname); for (idx = 0; idx < dir->nr; idx++) { cmp = strncmp(refname, dir->entries[idx]->name, len); -- 2.49.0 ``` ## Karthik Nayak, 2025-07-24 08:14 Subject: [PATCH 2/5] for-each-ref: fix documentation argument ordering Message-ID: <20250724-kn-small-cleanups-v1-2-0c70f591de3e@gmail.com> URL: https://gitlist.dev/e/20250724-kn-small-cleanups-v1-2-0c70f591de3e%40gmail.com In-Reply-To: <20250724-kn-small-cleanups-v1-0-0c70f591de3e@gmail.com> ``` Improve the 'git-for-each-ref(1)' documentation with two corrections: 1. Add parentheses around `--exclude=` to indicate this option can be repeated as a complete unit. 2. Move `--stdin | ...` to the end, after all flags, since `` is a positional argument that should appear last in the argument list. Signed-off-by: Karthik Nayak --- Documentation/git-for-each-ref.adoc | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/Documentation/git-for-each-ref.adoc b/Documentation/git-for-each-ref.adoc index ae61ba642a..a170de4209 100644 --- a/Documentation/git-for-each-ref.adoc +++ b/Documentation/git-for-each-ref.adoc @@ -10,11 +10,11 @@ SYNOPSIS [verse] 'git for-each-ref' [--count=] [--shell|--perl|--python|--tcl] [(--sort=)...] [--format=] - [--include-root-refs] [ --stdin | ... ] - [--points-at=] + [--include-root-refs] [--points-at=] [--merged[=]] [--no-merged[=]] [--contains[=]] [--no-contains[=]] - [--exclude= ...] [--start-after=] + [(--exclude=)...] [--start-after=] + [ --stdin | ... ] DESCRIPTION ----------- -- 2.49.0 ``` ## Karthik Nayak, 2025-07-24 08:14 Subject: [PATCH 3/5] for-each-ref: reword the documentation for '--start-after' Message-ID: <20250724-kn-small-cleanups-v1-3-0c70f591de3e@gmail.com> URL: https://gitlist.dev/e/20250724-kn-small-cleanups-v1-3-0c70f591de3e%40gmail.com In-Reply-To: <20250724-kn-small-cleanups-v1-0-0c70f591de3e@gmail.com> ``` The documentation for '--start-after' states that the flag cannot be used with general pattern matching. This is a bit vague, since there is no clear understanding about what 'general' means here. Rewrite the sentence to be more specific. While here, fix a typo in the 'OPT_STRING'. Helped-by: Junio C Hamano Signed-off-by: Karthik Nayak --- Documentation/git-for-each-ref.adoc | 3 ++- builtin/for-each-ref.c | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/Documentation/git-for-each-ref.adoc b/Documentation/git-for-each-ref.adoc index a170de4209..48af302a6f 100644 --- a/Documentation/git-for-each-ref.adoc +++ b/Documentation/git-for-each-ref.adoc @@ -114,7 +114,8 @@ TAB %(refname)`. deleted, modified or added between invocations. Output will only yield those references which follow the marker lexicographically. Output begins from the first reference that would come after the marker alphabetically. Cannot be - used with general pattern matching or custom sort options. + used with `--sort=` or `--stdin` options, or the __ argument(s) + to limit the refs. FIELD NAMES ----------- diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c index 3f21598046..79a79212c9 100644 --- a/builtin/for-each-ref.c +++ b/builtin/for-each-ref.c @@ -45,7 +45,7 @@ int cmd_for_each_ref(int argc, OPT_GROUP(""), OPT_INTEGER( 0 , "count", &format.array_opts.max_count, N_("show only matched refs")), OPT_STRING( 0 , "format", &format.format, N_("format"), N_("format to use for the output")), - OPT_STRING( 0 , "start-after", &filter.start_after, N_("start-start"), N_("start iteration after the provided marker")), + OPT_STRING( 0 , "start-after", &filter.start_after, N_("start-after"), N_("start iteration after the provided marker")), OPT__COLOR(&format.use_color, N_("respect format colors")), OPT_REF_FILTER_EXCLUDE(&filter), OPT_REF_SORT(&sorting_options), -- 2.49.0 ``` ## Karthik Nayak, 2025-07-24 08:14 Subject: [PATCH 4/5] t6302: add test combining '--start-after' with '--exclude' Message-ID: <20250724-kn-small-cleanups-v1-4-0c70f591de3e@gmail.com> URL: https://gitlist.dev/e/20250724-kn-small-cleanups-v1-4-0c70f591de3e%40gmail.com In-Reply-To: <20250724-kn-small-cleanups-v1-0-0c70f591de3e@gmail.com> ``` The '--start-after' doesn't explicitly mention being compatible with the '--exclude' flag, generally only incompatibility is explicitly called out. However, it would be nice to test the compatibility between the two to avoid future regressions. Let's do that. Signed-off-by: Karthik Nayak --- t/t6302-for-each-ref-filter.sh | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh index e097db6b02..9b80ea1e3b 100755 --- a/t/t6302-for-each-ref-filter.sh +++ b/t/t6302-for-each-ref-filter.sh @@ -712,6 +712,25 @@ test_expect_success 'start after, overflow specific reference path' ' test_cmp expect actual ' +test_expect_success 'start after, with exclude pattern' ' + cat >expect <<-\EOF && + refs/tags/annotated-tag + refs/tags/doubly-annotated-tag + refs/tags/doubly-signed-tag + refs/tags/foo1.10 + refs/tags/foo1.3 + refs/tags/foo1.6 + refs/tags/four + refs/tags/one + refs/tags/signed-tag + refs/tags/three + refs/tags/two + EOF + git for-each-ref --format="%(refname)" --start-after=refs/odd/spot \ + --exclude=refs/tags/foo >actual && + test_cmp expect actual +' + test_expect_success 'start after, last reference' ' cat >expect <<-\EOF && EOF -- 2.49.0 ``` ## Karthik Nayak, 2025-07-24 08:14 Subject: [PATCH 5/5] ref-filter: use REF_ITERATOR_SEEK_SET_PREFIX instead of '1' Message-ID: <20250724-kn-small-cleanups-v1-5-0c70f591de3e@gmail.com> URL: https://gitlist.dev/e/20250724-kn-small-cleanups-v1-5-0c70f591de3e%40gmail.com In-Reply-To: <20250724-kn-small-cleanups-v1-0-0c70f591de3e@gmail.com> ``` In the commit 51511d68f4 (for-each-ref: introduce a '--start-after' option, 2025-07-15), for introducing the '--start-after' flag, the `ref_iterator_seek()` was modified to also accept a flag. This was to allow the function to also set the prefix when 'REF_ITERATOR_SEEK_SET_PREFIX' was set. In `do_filter_refs()` instead of passing the flag, we pass in '1' which is the value of the flag. While this works, this is definitely hard to read and introduces inconsistency. Change it to use the flag. While here, remove the unnecessary 'if (prefix)' clause in the 'else' statement, since the block already checks for 'prefix'. Reported-by: Junio C Hamano Signed-off-by: Karthik Nayak --- ref-filter.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/ref-filter.c b/ref-filter.c index d5a146de87..4edf0df4cc 100644 --- a/ref-filter.c +++ b/ref-filter.c @@ -3254,8 +3254,9 @@ static int do_filter_refs(struct ref_filter *filter, unsigned int type, each_ref if (filter->start_after) ret = start_ref_iterator_after(iter, filter->start_after); - else if (prefix) - ret = ref_iterator_seek(iter, prefix, 1); + else + ret = ref_iterator_seek(iter, prefix, + REF_ITERATOR_SEEK_SET_PREFIX); if (!ret) ret = do_for_each_ref_iterator(iter, fn, cb_data); -- 2.49.0 ``` ## Junio C Hamano, 2025-07-24 17:17 Subject: Re: [PATCH 1/5] ref-cache: use 'size_t' instead of int for length Message-ID: URL: https://gitlist.dev/e/xmqq7bzxik3f.fsf%40gitster.g In-Reply-To: <20250724-kn-small-cleanups-v1-1-0c70f591de3e@gmail.com> ``` Karthik Nayak writes: > The commit 090eb5336c (refs: selectively set prefix in the seek > functions, 2025-07-15) modified the ref-cache iterator to support > seeking to a specified marker without setting the prefix. > > The commit adds and uses an integer 'len' to capture the length of the > seek marker to compare with the entries of a given directory. Since the > type of the variable is 'int', this is met with a typecast of converting > a `strlen` to 'int' so it can be assigned to the 'len' variable. > > This is whole operation is a bit wrong: > 1. Since the 'len' variable is eventually used in a 'strncmp', it should > have been of type 'size_t'. > 2. This also truncates the value provided from 'strlen' to an int, which > could cause a large refname to produce a negative number. > > Let's do the correct thing here and simply use 'size_t' for `len`. > > Signed-off-by: Karthik Nayak > --- > refs/ref-cache.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/refs/ref-cache.c b/refs/ref-cache.c > index 1d95b56d40..8df7ae43e5 100644 > --- a/refs/ref-cache.c > +++ b/refs/ref-cache.c > @@ -498,13 +498,14 @@ static int cache_ref_iterator_seek(struct ref_iterator *ref_iterator, > * indexing to each level as needed. > */ > do { > - int len, idx; > + int idx; > + size_t len; > int cmp = 0; > > sort_ref_dir(dir); > > slash = strchr(slash, '/'); > - len = slash ? slash - refname : (int)strlen(refname); > + len = slash ? (size_t)(slash - refname) : strlen(refname); The "strlen()" side is good, but was there recently a discussion on how to safely convert (slash - refname) that is ptrdiff_t to size_t? My archive search found a rather old ptrdiff_to_size() proposal https://lore.kernel.org/git/20241227213729.GA796141@coredump.intra.peff.net/ but I thought there were another discussion thread about casting to size_t recently. I _think_ a vanilla cast is safe here, as slash sits always right to refname (if not NULL, that is), and the difference should fit within size_t (because the difference is smaller than the size of the memory block pointed at by slash). So in short, this looks good. Will queue. ``` ## Junio C Hamano, 2025-07-24 17:27 Subject: Re: [PATCH 2/5] for-each-ref: fix documentation argument ordering Message-ID: URL: https://gitlist.dev/e/xmqqy0sdh529.fsf%40gitster.g In-Reply-To: <20250724-kn-small-cleanups-v1-2-0c70f591de3e@gmail.com> ``` Karthik Nayak writes: > Improve the 'git-for-each-ref(1)' documentation with two corrections: > > 1. Add parentheses around `--exclude=` to indicate this option > can be repeated as a complete unit. > > 2. Move `--stdin | ...` to the end, after all flags, since > `` is a positional argument that should appear last in the > argument list. > > Signed-off-by: Karthik Nayak > --- > Documentation/git-for-each-ref.adoc | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/Documentation/git-for-each-ref.adoc b/Documentation/git-for-each-ref.adoc > index ae61ba642a..a170de4209 100644 > --- a/Documentation/git-for-each-ref.adoc > +++ b/Documentation/git-for-each-ref.adoc > @@ -10,11 +10,11 @@ SYNOPSIS > [verse] > 'git for-each-ref' [--count=] [--shell|--perl|--python|--tcl] > [(--sort=)...] [--format=] > - [--include-root-refs] [ --stdin | ... ] > - [--points-at=] > + [--include-root-refs] [--points-at=] > [--merged[=]] [--no-merged[=]] > [--contains[=]] [--no-contains[=]] > - [--exclude= ...] [--start-after=] > + [(--exclude=)...] [--start-after=] > + [ --stdin | ... ] Shouldn't the last line align with the others? ``` ## Junio C Hamano, 2025-07-24 17:41 Subject: Re: [PATCH 0/5] ref-filter: small cleanups and fixes Message-ID: URL: https://gitlist.dev/e/xmqqtt31h4fu.fsf%40gitster.g In-Reply-To: <20250724-kn-small-cleanups-v1-0-0c70f591de3e@gmail.com> ``` Karthik Nayak writes: > This series contains a few of the small fixes and comments which I've > gathered from reviews of my earlier series [1] to add the > '--start-after' flag to 'git-for-each-ref(1)'. > > Individually each patch doesn't hold too much weight on its own, but > together these small improvements add up. That said, if these patches > are too small for the noise generated, we could simply drop it or > combine some commits together. > > This is based on top of 3f2a94875d (The twelfth batch, 2025-07-21) with > 'kn/for-each-ref-skip' merged in. > > [1]: https://lore.kernel.org/r/20250701-306-git-for-each-ref-pagination-v1-0-4f0ae7c0688f@gmail.com > > Signed-off-by: Karthik Nayak > --- > Documentation/git-for-each-ref.adoc | 9 +++++---- > builtin/for-each-ref.c | 2 +- > ref-filter.c | 5 +++-- > refs/ref-cache.c | 5 +++-- > t/t6302-for-each-ref-filter.sh | 19 +++++++++++++++++++ > 5 files changed, 31 insertions(+), 9 deletions(-) Will queue. All of them made sense. Thanks. ``` ## Karthik Nayak, 2025-07-24 22:14 Subject: Re: [PATCH 2/5] for-each-ref: fix documentation argument ordering Message-ID: URL: https://gitlist.dev/e/CAOLa%3DZRvyBK9vVz4%2BOhLUgAyktdVTb44VC%3De8KQNB6BszJ0ixA%40mail.gmail.com In-Reply-To: ``` Junio C Hamano writes: > Karthik Nayak writes: > >> Improve the 'git-for-each-ref(1)' documentation with two corrections: >> >> 1. Add parentheses around `--exclude=` to indicate this option >> can be repeated as a complete unit. >> >> 2. Move `--stdin | ...` to the end, after all flags, since >> `` is a positional argument that should appear last in the >> argument list. >> >> Signed-off-by: Karthik Nayak >> --- >> Documentation/git-for-each-ref.adoc | 6 +++--- >> 1 file changed, 3 insertions(+), 3 deletions(-) >> >> diff --git a/Documentation/git-for-each-ref.adoc b/Documentation/git-for-each-ref.adoc >> index ae61ba642a..a170de4209 100644 >> --- a/Documentation/git-for-each-ref.adoc >> +++ b/Documentation/git-for-each-ref.adoc >> @@ -10,11 +10,11 @@ SYNOPSIS >> [verse] >> 'git for-each-ref' [--count=] [--shell|--perl|--python|--tcl] >> [(--sort=)...] [--format=] >> - [--include-root-refs] [ --stdin | ... ] >> - [--points-at=] >> + [--include-root-refs] [--points-at=] >> [--merged[=]] [--no-merged[=]] >> [--contains[=]] [--no-contains[=]] >> - [--exclude= ...] [--start-after=] >> + [(--exclude=)...] [--start-after=] >> + [ --stdin | ... ] > > Shouldn't the last line align with the others? Indeed, I blotched it up somehow. Will add it locally and wait a ~day before sending in a new version. ``` ## Junio C Hamano, 2025-07-24 22:32 Subject: Re: [PATCH 2/5] for-each-ref: fix documentation argument ordering Message-ID: URL: https://gitlist.dev/e/xmqqo6t9xlr8.fsf%40gitster.g In-Reply-To: ``` Karthik Nayak writes: > Indeed, I blotched it up somehow. Will add it locally and wait a ~day > before sending in a new version. Hopefully no need, as I've tweaked a bit locally while queuing. ``` ## Patrick Steinhardt, 2025-07-25 10:55 Subject: Re: [PATCH 2/5] for-each-ref: fix documentation argument ordering Message-ID: URL: https://gitlist.dev/e/aINitJW7Mbxswy4b%40pks.im In-Reply-To: <20250724-kn-small-cleanups-v1-2-0c70f591de3e@gmail.com> ``` On Thu, Jul 24, 2025 at 10:14:43AM +0200, Karthik Nayak wrote: > diff --git a/Documentation/git-for-each-ref.adoc b/Documentation/git-for-each-ref.adoc > index ae61ba642a..a170de4209 100644 > --- a/Documentation/git-for-each-ref.adoc > +++ b/Documentation/git-for-each-ref.adoc > @@ -10,11 +10,11 @@ SYNOPSIS > [verse] > 'git for-each-ref' [--count=] [--shell|--perl|--python|--tcl] > [(--sort=)...] [--format=] > - [--include-root-refs] [ --stdin | ... ] > - [--points-at=] > + [--include-root-refs] [--points-at=] > [--merged[=]] [--no-merged[=]] > [--contains[=]] [--no-contains[=]] > - [--exclude= ...] [--start-after=] > + [(--exclude=)...] [--start-after=] > + [ --stdin | ... ] While at it we could also convert this to use `[synopsis]`. Patrick ``` ## Patrick Steinhardt, 2025-07-25 10:55 Subject: Re: [PATCH 3/5] for-each-ref: reword the documentation for '--start-after' Message-ID: URL: https://gitlist.dev/e/aINiuo4GWlW_1Fd-%40pks.im In-Reply-To: <20250724-kn-small-cleanups-v1-3-0c70f591de3e@gmail.com> ``` On Thu, Jul 24, 2025 at 10:14:44AM +0200, Karthik Nayak wrote: > diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c > index 3f21598046..79a79212c9 100644 > --- a/builtin/for-each-ref.c > +++ b/builtin/for-each-ref.c > @@ -45,7 +45,7 @@ int cmd_for_each_ref(int argc, > OPT_GROUP(""), > OPT_INTEGER( 0 , "count", &format.array_opts.max_count, N_("show only matched refs")), > OPT_STRING( 0 , "format", &format.format, N_("format"), N_("format to use for the output")), > - OPT_STRING( 0 , "start-after", &filter.start_after, N_("start-start"), N_("start iteration after the provided marker")), > + OPT_STRING( 0 , "start-after", &filter.start_after, N_("start-after"), N_("start iteration after the provided marker")), Funny how typos like this frequently go through reviews without anybody noticing :) Patrick ``` ## Karthik Nayak, 2025-07-28 20:18 Subject: Re: [PATCH 2/5] for-each-ref: fix documentation argument ordering Message-ID: URL: https://gitlist.dev/e/CAOLa%3DZRB6VJU4Km8jotcEsGMbQeb1Vq%2BwaZriHS9c2RVxiqJhA%40mail.gmail.com In-Reply-To: ``` Patrick Steinhardt writes: > On Thu, Jul 24, 2025 at 10:14:43AM +0200, Karthik Nayak wrote: >> diff --git a/Documentation/git-for-each-ref.adoc b/Documentation/git-for-each-ref.adoc >> index ae61ba642a..a170de4209 100644 >> --- a/Documentation/git-for-each-ref.adoc >> +++ b/Documentation/git-for-each-ref.adoc >> @@ -10,11 +10,11 @@ SYNOPSIS >> [verse] >> 'git for-each-ref' [--count=] [--shell|--perl|--python|--tcl] >> [(--sort=)...] [--format=] >> - [--include-root-refs] [ --stdin | ... ] >> - [--points-at=] >> + [--include-root-refs] [--points-at=] >> [--merged[=]] [--no-merged[=]] >> [--contains[=]] [--no-contains[=]] >> - [--exclude= ...] [--start-after=] >> + [(--exclude=)...] [--start-after=] >> + [ --stdin | ... ] > > While at it we could also convert this to use `[synopsis]`. > > Patrick That's a good point, Let me do that. ``` ## Karthik Nayak, 2025-07-28 20:19 Subject: Re: [PATCH 2/5] for-each-ref: fix documentation argument ordering Message-ID: URL: https://gitlist.dev/e/CAOLa%3DZRTL-V6nmRv4HM8LWu3q52V4Exf50KpF8AHt%2BhoV4n9Mg%40mail.gmail.com In-Reply-To: ``` Junio C Hamano writes: > Karthik Nayak writes: > >> Indeed, I blotched it up somehow. Will add it locally and wait a ~day >> before sending in a new version. > > Hopefully no need, as I've tweaked a bit locally while queuing. Thanks, but since Patrick also pointed out about the 'synopsis' block, I'll send in a new version with the fix. ``` ## Karthik Nayak, 2025-07-28 20:20 Subject: [PATCH v2 0/5] ref-filter: small cleanups and fixes Message-ID: <20250728-kn-small-cleanups-v2-0-d3021c8bf471@gmail.com> URL: https://gitlist.dev/e/20250728-kn-small-cleanups-v2-0-d3021c8bf471%40gmail.com In-Reply-To: <20250724-kn-small-cleanups-v1-0-0c70f591de3e@gmail.com> ``` This series contains a few of the small fixes and comments which I've gathered from reviews of my earlier series [1] to add the '--start-after' flag to 'git-for-each-ref(1)'. Individually each patch doesn't hold too much weight on its own, but together these small improvements add up. That said, if these patches are too small for the noise generated, we could simply drop it or combine some commits together. This is based on top of 3f2a94875d (The twelfth batch, 2025-07-21) with 'kn/for-each-ref-skip' merged in. [1]: https://lore.kernel.org/r/20250701-306-git-for-each-ref-pagination-v1-0-4f0ae7c0688f@gmail.com Signed-off-by: Karthik Nayak --- Changes in v2: - In Patch 2/5, fix a whitespace issue and convert 'git-for-each-ref(1)' documentation to use the new synopsis block. - Link to v1: https://lore.kernel.org/r/20250724-kn-small-cleanups-v1-0-0c70f591de3e@gmail.com --- Documentation/git-for-each-ref.adoc | 13 +++++++------ builtin/for-each-ref.c | 2 +- ref-filter.c | 5 +++-- refs/ref-cache.c | 5 +++-- t/t6302-for-each-ref-filter.sh | 19 +++++++++++++++++++ 5 files changed, 33 insertions(+), 11 deletions(-) Karthik Nayak (5): ref-cache: use 'size_t' instead of int for length for-each-ref: fix documentation argument ordering for-each-ref: reword the documentation for '--start-after' t6302: add test combining '--start-after' with '--exclude' ref-filter: use REF_ITERATOR_SEEK_SET_PREFIX instead of '1' Range-diff versus v1: 1: 449473efe6 = 1: cb34545b3a ref-cache: use 'size_t' instead of int for length 2: b5aec572bc < -: ---------- for-each-ref: fix documentation argument ordering -: ---------- > 2: 3fab0eda31 for-each-ref: fix documentation argument ordering 3: 005a35a4b3 = 3: 49e5aa69f2 for-each-ref: reword the documentation for '--start-after' 4: 162f5de259 = 4: 93babd3e71 t6302: add test combining '--start-after' with '--exclude' 5: 3eb8591385 = 5: ce90149919 ref-filter: use REF_ITERATOR_SEEK_SET_PREFIX instead of '1' base-commit: 15fcbd8f16a2c119a5319b0657e52fe0f387df46 change-id: 20250721-kn-small-cleanups-a01fe5d2756d Thanks - Karthik ``` ## Karthik Nayak, 2025-07-28 20:20 Subject: [PATCH v2 1/5] ref-cache: use 'size_t' instead of int for length Message-ID: <20250728-kn-small-cleanups-v2-1-d3021c8bf471@gmail.com> URL: https://gitlist.dev/e/20250728-kn-small-cleanups-v2-1-d3021c8bf471%40gmail.com In-Reply-To: <20250728-kn-small-cleanups-v2-0-d3021c8bf471@gmail.com> ``` The commit 090eb5336c (refs: selectively set prefix in the seek functions, 2025-07-15) modified the ref-cache iterator to support seeking to a specified marker without setting the prefix. The commit adds and uses an integer 'len' to capture the length of the seek marker to compare with the entries of a given directory. Since the type of the variable is 'int', this is met with a typecast of converting a `strlen` to 'int' so it can be assigned to the 'len' variable. This is whole operation is a bit wrong: 1. Since the 'len' variable is eventually used in a 'strncmp', it should have been of type 'size_t'. 2. This also truncates the value provided from 'strlen' to an int, which could cause a large refname to produce a negative number. Let's do the correct thing here and simply use 'size_t' for `len`. Signed-off-by: Karthik Nayak --- refs/ref-cache.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/refs/ref-cache.c b/refs/ref-cache.c index 1d95b56d40..8df7ae43e5 100644 --- a/refs/ref-cache.c +++ b/refs/ref-cache.c @@ -498,13 +498,14 @@ static int cache_ref_iterator_seek(struct ref_iterator *ref_iterator, * indexing to each level as needed. */ do { - int len, idx; + int idx; + size_t len; int cmp = 0; sort_ref_dir(dir); slash = strchr(slash, '/'); - len = slash ? slash - refname : (int)strlen(refname); + len = slash ? (size_t)(slash - refname) : strlen(refname); for (idx = 0; idx < dir->nr; idx++) { cmp = strncmp(refname, dir->entries[idx]->name, len); -- 2.50.0 ``` ## Karthik Nayak, 2025-07-28 20:20 Subject: [PATCH v2 2/5] for-each-ref: fix documentation argument ordering Message-ID: <20250728-kn-small-cleanups-v2-2-d3021c8bf471@gmail.com> URL: https://gitlist.dev/e/20250728-kn-small-cleanups-v2-2-d3021c8bf471%40gmail.com In-Reply-To: <20250728-kn-small-cleanups-v2-0-d3021c8bf471@gmail.com> ``` Improve the 'git-for-each-ref(1)' documentation with two corrections: 1. Add parentheses around `--exclude=` to indicate this option can be repeated as a complete unit. 2. Move `--stdin | ...` to the end, after all flags, since `` is a positional argument that should appear last in the argument list. While here, change to using the synopsis block which will automatically format placeholders in italics and keywords in monospace. Signed-off-by: Karthik Nayak --- Documentation/git-for-each-ref.adoc | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/Documentation/git-for-each-ref.adoc b/Documentation/git-for-each-ref.adoc index ae61ba642a..ec3b10e14a 100644 --- a/Documentation/git-for-each-ref.adoc +++ b/Documentation/git-for-each-ref.adoc @@ -7,14 +7,14 @@ git-for-each-ref - Output information on each ref SYNOPSIS -------- -[verse] -'git for-each-ref' [--count=] [--shell|--perl|--python|--tcl] +[synopsis] +git for-each-ref [--count=] [--shell|--perl|--python|--tcl] [(--sort=)...] [--format=] - [--include-root-refs] [ --stdin | ... ] - [--points-at=] + [--include-root-refs] [--points-at=] [--merged[=]] [--no-merged[=]] [--contains[=]] [--no-contains[=]] - [--exclude= ...] [--start-after=] + [(--exclude=)...] [--start-after=] + [ --stdin | ... ] DESCRIPTION ----------- -- 2.50.0 ``` ## Karthik Nayak, 2025-07-28 20:20 Subject: [PATCH v2 3/5] for-each-ref: reword the documentation for '--start-after' Message-ID: <20250728-kn-small-cleanups-v2-3-d3021c8bf471@gmail.com> URL: https://gitlist.dev/e/20250728-kn-small-cleanups-v2-3-d3021c8bf471%40gmail.com In-Reply-To: <20250728-kn-small-cleanups-v2-0-d3021c8bf471@gmail.com> ``` The documentation for '--start-after' states that the flag cannot be used with general pattern matching. This is a bit vague, since there is no clear understanding about what 'general' means here. Rewrite the sentence to be more specific. While here, fix a typo in the 'OPT_STRING'. Helped-by: Junio C Hamano Signed-off-by: Karthik Nayak --- Documentation/git-for-each-ref.adoc | 3 ++- builtin/for-each-ref.c | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/Documentation/git-for-each-ref.adoc b/Documentation/git-for-each-ref.adoc index ec3b10e14a..060940904d 100644 --- a/Documentation/git-for-each-ref.adoc +++ b/Documentation/git-for-each-ref.adoc @@ -114,7 +114,8 @@ TAB %(refname)`. deleted, modified or added between invocations. Output will only yield those references which follow the marker lexicographically. Output begins from the first reference that would come after the marker alphabetically. Cannot be - used with general pattern matching or custom sort options. + used with `--sort=` or `--stdin` options, or the __ argument(s) + to limit the refs. FIELD NAMES ----------- diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c index 3f21598046..79a79212c9 100644 --- a/builtin/for-each-ref.c +++ b/builtin/for-each-ref.c @@ -45,7 +45,7 @@ int cmd_for_each_ref(int argc, OPT_GROUP(""), OPT_INTEGER( 0 , "count", &format.array_opts.max_count, N_("show only matched refs")), OPT_STRING( 0 , "format", &format.format, N_("format"), N_("format to use for the output")), - OPT_STRING( 0 , "start-after", &filter.start_after, N_("start-start"), N_("start iteration after the provided marker")), + OPT_STRING( 0 , "start-after", &filter.start_after, N_("start-after"), N_("start iteration after the provided marker")), OPT__COLOR(&format.use_color, N_("respect format colors")), OPT_REF_FILTER_EXCLUDE(&filter), OPT_REF_SORT(&sorting_options), -- 2.50.0 ``` ## Karthik Nayak, 2025-07-28 20:20 Subject: [PATCH v2 4/5] t6302: add test combining '--start-after' with '--exclude' Message-ID: <20250728-kn-small-cleanups-v2-4-d3021c8bf471@gmail.com> URL: https://gitlist.dev/e/20250728-kn-small-cleanups-v2-4-d3021c8bf471%40gmail.com In-Reply-To: <20250728-kn-small-cleanups-v2-0-d3021c8bf471@gmail.com> ``` The '--start-after' doesn't explicitly mention being compatible with the '--exclude' flag, generally only incompatibility is explicitly called out. However, it would be nice to test the compatibility between the two to avoid future regressions. Let's do that. Signed-off-by: Karthik Nayak --- t/t6302-for-each-ref-filter.sh | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh index e097db6b02..9b80ea1e3b 100755 --- a/t/t6302-for-each-ref-filter.sh +++ b/t/t6302-for-each-ref-filter.sh @@ -712,6 +712,25 @@ test_expect_success 'start after, overflow specific reference path' ' test_cmp expect actual ' +test_expect_success 'start after, with exclude pattern' ' + cat >expect <<-\EOF && + refs/tags/annotated-tag + refs/tags/doubly-annotated-tag + refs/tags/doubly-signed-tag + refs/tags/foo1.10 + refs/tags/foo1.3 + refs/tags/foo1.6 + refs/tags/four + refs/tags/one + refs/tags/signed-tag + refs/tags/three + refs/tags/two + EOF + git for-each-ref --format="%(refname)" --start-after=refs/odd/spot \ + --exclude=refs/tags/foo >actual && + test_cmp expect actual +' + test_expect_success 'start after, last reference' ' cat >expect <<-\EOF && EOF -- 2.50.0 ``` ## Karthik Nayak, 2025-07-28 20:20 Subject: [PATCH v2 5/5] ref-filter: use REF_ITERATOR_SEEK_SET_PREFIX instead of '1' Message-ID: <20250728-kn-small-cleanups-v2-5-d3021c8bf471@gmail.com> URL: https://gitlist.dev/e/20250728-kn-small-cleanups-v2-5-d3021c8bf471%40gmail.com In-Reply-To: <20250728-kn-small-cleanups-v2-0-d3021c8bf471@gmail.com> ``` In the commit 51511d68f4 (for-each-ref: introduce a '--start-after' option, 2025-07-15), for introducing the '--start-after' flag, the `ref_iterator_seek()` was modified to also accept a flag. This was to allow the function to also set the prefix when 'REF_ITERATOR_SEEK_SET_PREFIX' was set. In `do_filter_refs()` instead of passing the flag, we pass in '1' which is the value of the flag. While this works, this is definitely hard to read and introduces inconsistency. Change it to use the flag. While here, remove the unnecessary 'if (prefix)' clause in the 'else' statement, since the block already checks for 'prefix'. Reported-by: Junio C Hamano Signed-off-by: Karthik Nayak --- ref-filter.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/ref-filter.c b/ref-filter.c index d5a146de87..4edf0df4cc 100644 --- a/ref-filter.c +++ b/ref-filter.c @@ -3254,8 +3254,9 @@ static int do_filter_refs(struct ref_filter *filter, unsigned int type, each_ref if (filter->start_after) ret = start_ref_iterator_after(iter, filter->start_after); - else if (prefix) - ret = ref_iterator_seek(iter, prefix, 1); + else + ret = ref_iterator_seek(iter, prefix, + REF_ITERATOR_SEEK_SET_PREFIX); if (!ret) ret = do_for_each_ref_iterator(iter, fn, cb_data); -- 2.50.0 ``` ## Patrick Steinhardt, 2025-07-29 08:04 Subject: Re: [PATCH v2 0/5] ref-filter: small cleanups and fixes Message-ID: URL: https://gitlist.dev/e/aIiAj8COJOAXv-xe%40pks.im In-Reply-To: <20250728-kn-small-cleanups-v2-0-d3021c8bf471@gmail.com> ``` On Mon, Jul 28, 2025 at 10:20:45PM +0200, Karthik Nayak wrote: > This series contains a few of the small fixes and comments which I've > gathered from reviews of my earlier series [1] to add the > '--start-after' flag to 'git-for-each-ref(1)'. > > Individually each patch doesn't hold too much weight on its own, but > together these small improvements add up. That said, if these patches > are too small for the noise generated, we could simply drop it or > combine some commits together. > > This is based on top of 3f2a94875d (The twelfth batch, 2025-07-21) with > 'kn/for-each-ref-skip' merged in. > > [1]: https://lore.kernel.org/r/20250701-306-git-for-each-ref-pagination-v1-0-4f0ae7c0688f@gmail.com > > Signed-off-by: Karthik Nayak > --- > Changes in v2: > - In Patch 2/5, fix a whitespace issue and convert 'git-for-each-ref(1)' > documentation to use the new synopsis block. > - Link to v1: https://lore.kernel.org/r/20250724-kn-small-cleanups-v1-0-0c70f591de3e@gmail.com Thanks, this version looks good to me. Patrick ```