patch, 5 partsref-filter: small cleanups and fixes

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 <karthik.188@gmail.com>
---
 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

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 <karthik.188@gmail.com>
---
 refs/ref-cache.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Karthik Nayak <karthik.188@gmail.com> writes:

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.

Improve the 'git-for-each-ref(1)' documentation with two corrections:

1. Add parentheses around `--exclude=<pattern>` to indicate this option
   can be repeated as a complete unit.

2. Move `--stdin | <pattern> ...` to the end, after all flags, since
   `<pattern>` is a positional argument that should appear last in the
   argument list.

Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 Documentation/git-for-each-ref.adoc | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Karthik Nayak <karthik.188@gmail.com> writes:

Shouldn't the last line align with the others?

Junio C Hamano <gitster@pobox.com> writes:

Indeed, I blotched it up somehow. Will add it locally and wait a ~day before sending in a new version.

Karthik Nayak <karthik.188@gmail.com> writes:

Hopefully no need, as I've tweaked a bit locally while queuing.

Junio C Hamano <gitster@pobox.com> writes:

Thanks, but since Patrick also pointed out about the 'synopsis' block, I'll send in a new version with the fix.

On Thu, Jul 24, 2025 at 10:14:43AM +0200, Karthik Nayak wrote:

While at it we could also convert this to use `[synopsis]`.

Patrick

Patrick Steinhardt <ps@pks.im> writes:

That's a good point, Let me do that.

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 <gitster@pobox.com>
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 Documentation/git-for-each-ref.adoc | 3 ++-
 builtin/for-each-ref.c              | 2 +-
 2 files changed, 3 insertions(+), 2 deletions(-)

On Thu, Jul 24, 2025 at 10:14:44AM +0200, Karthik Nayak wrote:

Funny how typos like this frequently go through reviews without anybody noticing :)

Patrick

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 <karthik.188@gmail.com>
---
 t/t6302-for-each-ref-filter.sh | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

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 <gitster@pobox.com>
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 ref-filter.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Karthik Nayak <karthik.188@gmail.com> writes:

Will queue.  All of them made sense.

Thanks.

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 <karthik.188@gmail.com>
---
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

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 <karthik.188@gmail.com>
---
 refs/ref-cache.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Improve the 'git-for-each-ref(1)' documentation with two corrections:

1. Add parentheses around `--exclude=<pattern>` to indicate this option
   can be repeated as a complete unit.

2. Move `--stdin | <pattern> ...` to the end, after all flags, since
   `<pattern>` 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 <karthik.188@gmail.com>
---
 Documentation/git-for-each-ref.adoc | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

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 <gitster@pobox.com>
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 Documentation/git-for-each-ref.adoc | 3 ++-
 builtin/for-each-ref.c              | 2 +-
 2 files changed, 3 insertions(+), 2 deletions(-)

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 <karthik.188@gmail.com>
---
 t/t6302-for-each-ref-filter.sh | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

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 <gitster@pobox.com>
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 ref-filter.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

On Mon, Jul 28, 2025 at 10:20:45PM +0200, Karthik Nayak wrote:

Thanks, this version looks good to me.

Patrick