From 65855751d190d66d15bc2c7e17e93fe00d04c539 Mon Sep 17 00:00:00 2001 From: Karthik Nayak Date: Mon, 28 Jul 2025 22:20:46 +0200 Subject: [PATCH 1/5] ref-cache: use 'size_t' instead of int for length 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 Signed-off-by: Junio C Hamano --- 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 ceef3a2008..c180e0aad7 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); From a7c8a4c5f5bbe6c232c8aefc02499fe1c0b6d221 Mon Sep 17 00:00:00 2001 From: Karthik Nayak Date: Mon, 28 Jul 2025 22:20:47 +0200 Subject: [PATCH 2/5] for-each-ref: fix documentation argument ordering 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 Signed-off-by: Junio C Hamano --- 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 ----------- From fa0f4e46f5db22d0be63e66228d5f27fa1cb7ee6 Mon Sep 17 00:00:00 2001 From: Karthik Nayak Date: Mon, 28 Jul 2025 22:20:48 +0200 Subject: [PATCH 3/5] for-each-ref: reword the documentation for '--start-after' 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 Signed-off-by: Junio C Hamano --- 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), From ed9cc2144c97b3ad609d71b6033a95079179fc0e Mon Sep 17 00:00:00 2001 From: Karthik Nayak Date: Mon, 28 Jul 2025 22:20:49 +0200 Subject: [PATCH 4/5] t6302: add test combining '--start-after' with '--exclude' 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 Signed-off-by: Junio C Hamano --- 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 From 444ad14e02edc59e61f7d53ae3b9f8ebe90860fd Mon Sep 17 00:00:00 2001 From: Karthik Nayak Date: Mon, 28 Jul 2025 22:20:50 +0200 Subject: [PATCH 5/5] ref-filter: use REF_ITERATOR_SEEK_SET_PREFIX instead of '1' 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 Signed-off-by: Junio C Hamano --- ref-filter.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/ref-filter.c b/ref-filter.c index c8a6b7f1af..62ffdcdcad 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);