[PATCH] rev-list: skip --grep argument in early scan

[PATCH] rev-list: skip --grep argument in early scan

[Azuama Odinaka Maximus]

Signed-off-by: Azuama Odinaka Maximus <azuama69@gmail.com>
---
 builtin/rev-list.c          | 2 ++
 t/t6022-rev-list-missing.sh | 6 ++++++
 2 files changed, 8 insertions(+)

diff --git a/builtin/rev-list.c b/builtin/rev-list.c
index 0faa833fac..453d82d25c 100644
--- a/builtin/rev-list.c
+++ b/builtin/rev-list.c
@@ -771,6 +771,8 @@ int cmd_rev_list(int argc,
 			revs.exclude_promisor_objects = 1;
 		} else if (skip_prefix(arg, "--missing=", &arg)) {
 			parse_missing_action_value(repo, arg);
+		} else if (!strcmp(arg, "--grep")) {
+			i++;
 		} else if (!strcmp(arg, "--missing-only")) {
 			arg_missing_only = 1;
 		} else if (!strcmp(arg, "-z")) {
diff --git a/t/t6022-rev-list-missing.sh b/t/t6022-rev-list-missing.sh
index 1bd2c3bc4f..e87678e468 100755
--- a/t/t6022-rev-list-missing.sh
+++ b/t/t6022-rev-list-missing.sh
@@ -215,6 +215,12 @@ do
 	'
 done
 
+test_expect_success "--grep accepts a pattern beginning with --" '
+	git rev-list --grep=--missing-only --max-count=1 HEAD >expect &&
+	git rev-list --grep --missing-only --max-count=1 HEAD >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success "--missing-only requires --missing=print or --missing=print-info" '
 	test_must_fail git rev-list --missing-only --objects HEAD 2>err &&
 	test_grep "requires --missing=print" err
-- 
2.53.0

Re: [PATCH] rev-list: skip --grep argument in early scan

[Pushkar Singh]

Hi,

Thanks for the patch!

One thing I noticed is that the commit message doesn't have a description.
Could you add a bit of context about the issue you're fixing and why this
change is needed? It'll make it easier for others to understand the reason
behind the patch.

> +               } else if (!strcmp(arg, "--grep")) {
> +                       i++;

Also, could you check if the first scan needs similar handling? With --grep
--exclude-promisor-objects, the first scan might treat
--exclude-promisor-objects
as an option instead of the pattern argument to --grep. A test for this case
would help confirm whether the fix covers both scans.

Thanks!

- Pushkar

[PATCH v2] rev-list: skip --grep argument in early scan

[Azuama Odinaka Maximus]

The early option scan in rev-list does not account for options
that take a separate argument. As a result, a pattern passed to
--grep can be mistaken for another option.

Skip the argument to --grep so that it is not interpreted as an
option during the early scan.
---
 builtin/rev-list.c          |  2 ++
 t/t6022-rev-list-missing.sh | 11 +++++++++++
 2 files changed, 13 insertions(+)

diff --git a/builtin/rev-list.c b/builtin/rev-list.c
index 0faa833fac..453d82d25c 100644
--- a/builtin/rev-list.c
+++ b/builtin/rev-list.c
@@ -771,6 +771,8 @@ int cmd_rev_list(int argc,
 			revs.exclude_promisor_objects = 1;
 		} else if (skip_prefix(arg, "--missing=", &arg)) {
 			parse_missing_action_value(repo, arg);
+		} else if (!strcmp(arg, "--grep")) {
+			i++;
 		} else if (!strcmp(arg, "--missing-only")) {
 			arg_missing_only = 1;
 		} else if (!strcmp(arg, "-z")) {
diff --git a/t/t6022-rev-list-missing.sh b/t/t6022-rev-list-missing.sh
index 1bd2c3bc4f..180e6ac766 100755
--- a/t/t6022-rev-list-missing.sh
+++ b/t/t6022-rev-list-missing.sh
@@ -215,6 +215,17 @@ do
 	'
 done
 
+test_expect_success "--grep accepts a pattern beginning with --" '
+	git rev-list --grep=--missing-only --max-count=1 HEAD >expect &&
+	git rev-list --grep --missing-only --max-count=1 HEAD >actual &&
+	test_cmp expect actual
+'
+test_expect_success "--grep does not trigger --exclude-promisor-objects in early scan" '
+    git rev-list --grep=--exclude-promisor-objects --missing=print HEAD >expect &&
+    git rev-list --grep --exclude-promisor-objects --missing=print HEAD >actual &&
+    test_cmp expect actual
+'
+
 test_expect_success "--missing-only requires --missing=print or --missing=print-info" '
 	test_must_fail git rev-list --missing-only --objects HEAD 2>err &&
 	test_grep "requires --missing=print" err
-- 
2.53.0

Re: [PATCH v2] rev-list: skip --grep argument in early scan

[Pushkar Singh]

Hi Azuma,

>
> Skip the argument to --grep so that it is not interpreted as an
> option during the early scan.
> ---

Can you add a "Changes in v2" section after the --- here, summarizing
what changed from the previous version?
Also, I noticed the "Signed-off-by" line is missing in v2. Could you
add it back?

Other than that, it looks good to me.

- Pushkar

Re: [PATCH v2] rev-list: skip --grep argument in early scan

[Junio C Hamano]

Azuama Odinaka Maximus <azuama69@gmail.com> writes:

> +test_expect_success "--grep accepts a pattern beginning with --" '
> +	git rev-list --grep=--missing-only --max-count=1 HEAD >expect &&
> +	git rev-list --grep --missing-only --max-count=1 HEAD >actual &&
> +	test_cmp expect actual
> +'
> +test_expect_success "--grep does not trigger --exclude-promisor-objects in early scan" '
> +    git rev-list --grep=--exclude-promisor-objects --missing=print HEAD >expect &&
> +    git rev-list --grep --exclude-promisor-objects --missing=print HEAD >actual &&
> +    test_cmp expect actual
> +'

Have a blank line between tests.

Do these tests truly verify anything beyond "the command line parser
is not broken by my commit"?  There is nothing in the history that
would trigger '--missing-only'; there is no positive indication that
the pattern starting with "--" was used by '--grep' at all.  The only
clue that '--missing-only' was not taken as a command line option is
that the command did not fail due to a lack of '--missing=<how>'.

The same issue exists in the second added test.

Thanks.

[PATCH v3] rev-list: skip --grep argument in early scan

[Azuama Odinaka Maximus]

The early option scan in rev-list does not account for options
that take a separate argument. As a result, a pattern passed to
--grep can be mistaken for another option.

Skip the argument to --grep so that it is not interpreted as an
option during the early scan.

The tests check that patterns beginning with "--" are actually
matched by --grep, including when the pattern resembles an option
that the early scan handles specially.

Signed-off-by: Azuama Odinaka Maximus <azuama69@gmail.com>
---
Changes in v3:
- Improve both regression tests to verify that --grep matches the
  expected commit when its pattern begins with "--".
- Add a blank line between the tests.

 builtin/rev-list.c          |  2 ++
 t/t6022-rev-list-missing.sh | 20 ++++++++++++++++++++
 2 files changed, 22 insertions(+)

diff --git a/builtin/rev-list.c b/builtin/rev-list.c
index 0faa833fac..453d82d25c 100644
--- a/builtin/rev-list.c
+++ b/builtin/rev-list.c
@@ -771,6 +771,8 @@ int cmd_rev_list(int argc,
 			revs.exclude_promisor_objects = 1;
 		} else if (skip_prefix(arg, "--missing=", &arg)) {
 			parse_missing_action_value(repo, arg);
+		} else if (!strcmp(arg, "--grep")) {
+			i++;
 		} else if (!strcmp(arg, "--missing-only")) {
 			arg_missing_only = 1;
 		} else if (!strcmp(arg, "-z")) {
diff --git a/t/t6022-rev-list-missing.sh b/t/t6022-rev-list-missing.sh
index 1bd2c3bc4f..3757bde708 100755
--- a/t/t6022-rev-list-missing.sh
+++ b/t/t6022-rev-list-missing.sh
@@ -215,6 +215,26 @@ do
 	'
 done
 
+test_expect_success "--grep accepts a pattern beginning with --" '
+        test_commit --no-tag --missing-only &&
+        commit=$(git rev-parse HEAD) &&
+        echo "$commit" >expect &&
+        git rev-list --grep=--missing-only HEAD >actual &&
+        test_cmp expect actual &&
+        git rev-list --grep --missing-only HEAD >actual &&
+        test_cmp expect actual
+'
+
+test_expect_success "--grep does not trigger --exclude-promisor-objects in early scan" '
+        test_commit --no-tag --exclude-promisor-objects &&
+        commit=$(git rev-parse HEAD) &&
+        echo "$commit" >expect &&
+        git rev-list --grep=--exclude-promisor-objects --missing=print HEAD >actual &&
+        test_cmp expect actual &&
+        git rev-list --grep --exclude-promisor-objects --missing=print HEAD >actual &&
+        test_cmp expect actual
+'
+
 test_expect_success "--missing-only requires --missing=print or --missing=print-info" '
 	test_must_fail git rev-list --missing-only --objects HEAD 2>err &&
 	test_grep "requires --missing=print" err
-- 
2.53.0

Re: [PATCH v3] rev-list: skip --grep argument in early scan

[Tanga Joris]

Hello Azuama,

Thanks for working on this.

I applied v3 on top of master (0ffa3e35c7) with "git am -3", because
builtin/rev-list.c has changed since you sent it, so running "git am"
fails. Rebasing on master would help. I built it with NO_RUST=1.
t6022 passes, and both new tests fail when I revert only the change
in builtin/rev-list.c, so they do catch the problem.

On Mon, 26 Oct 2026 at 11:30, Azuama Odinaka Maximus <azuama69@gmail.com> wrote:
> +test_expect_success "--grep accepts a pattern beginning with --" '
> +        test_commit --no-tag --missing-only &&
> +        commit=$(git rev-parse HEAD) &&

These lines are indented with spaces instead of a tab. "git diff
--check" reports "indent with spaces" for all the new test lines.

> +             } else if (!strcmp(arg, "--grep")) {
> +                     i++;

This fixes --grep, but other options that take a separate value
still have the same problem. In a repository whose only commit has
"--missing-only" as both its subject and its author name, with this
patch:

  $ git rev-list --grep --missing-only HEAD
  <the commit>
  $ git rev-list --author --missing-only HEAD
  fatal: --missing-only requires --missing=print or --missing=print-info

--committer and --grep-reflog fail the same way. In revision.c, 19
options are parsed with parse_long_opt(), which accepts the value as
a separate argument (--author, --committer, --since, --until, --skip,
--exclude and others). Should the early scan skip the value of all
of them, or is there a reason to handle only --grep? If this only fixes
--grep for now, the commit message could say so, because its first
paragraph describes the general problem.

Also, the NEEDSWORK comment just above this loop uses "--grep
--exclude-promisor-objects" as its example of what the loop gets
wrong. With this patch that example works, so the comment needs a
new example (--author would do), or it simply becomes misleading.

Thanks,
Tanga

[PATCH v4] rev-list: handle arguments in early option scan

[Azuama Odinaka Maximus]

The early option scan in cmd_rev_list() looks for options that
affect missing-object handling before setup_revisions() parses the
command line. It must not interpret an option's separate argument
as another option.

Use parse_long_opt() to recognize revision options that take
separate arguments, and skip those arguments. Handle -n and
--default as well. Stop scanning at --, where subsequent arguments
are pathspecs.

Add tests to ensure that --grep and --author arguments beginning
with -- are not mistaken for options during the early scan.

Signed-off-by: Azuama Odinaka Maximus <azuama69@gmail.com>
---
 builtin/rev-list.c          | 55 +++++++++++++++++++++++++++++++++++--
 t/t6022-rev-list-missing.sh | 26 ++++++++++++++++++
 2 files changed, 78 insertions(+), 3 deletions(-)

diff --git a/builtin/rev-list.c b/builtin/rev-list.c
index 071a0e3cb5..4dd646050a 100644
--- a/builtin/rev-list.c
+++ b/builtin/rev-list.c
@@ -744,6 +744,47 @@ static void prepare_maximal_independent(struct rev_info *revs)
 	revs->no_walk = 1;
 }
 
+static int rev_list_option_takes_argument(const char **argv)
+{
+	static const char * const options[] = {
+		"max-count",
+		"max-count-oldest",
+		"skip",
+		"max-age",
+		"since",
+		"since-as-filter",
+		"after",
+		"min-age",
+		"before",
+		"until",
+		"date",
+		"author",
+		"committer",
+		"grep-reflog",
+		"grep",
+		"encoding",
+		"glob",
+		"exclude",
+		"exclude-hidden",
+	};
+	const char *optarg;
+	size_t i;
+
+	if (!argv[1])
+		return 0;
+
+	if (!strcmp(argv[0], "-n") ||
+	    !strcmp(argv[0], "--default"))
+		return 1;
+
+	for (i = 0; i < ARRAY_SIZE(options); i++) {
+		if (parse_long_opt(options[i], argv, &optarg) == 2)
+			return 1;
+	}
+
+	return 0;
+}
+
 int cmd_rev_list(int argc,
 		 const char **argv,
 		 const char *prefix,
@@ -789,9 +830,8 @@ int cmd_rev_list(int argc,
 	 * NEEDSWORK: The next loop is utterly broken.  It tries to
 	 * notice an option is used, but without understanding if each
 	 * option takes an argument, which fundamentally would not
-	 * work.  It would not know "--grep
-	 * --exclude-promisor-objects" is not triggering
-	 * "--exclude-promisor-objects" option, for example.
+	 * work.  It would not know that "--author --missing-only" is not
+	 * triggering "--missing-only" option, for example.
 	 *
 	 * We really need setup_revisions() to have a mechanism to
 	 * allow and disallow some sets of options for different
@@ -802,6 +842,15 @@ int cmd_rev_list(int argc,
 	 */
 	for (i = 1; i < argc; i++) {
 		const char *arg = argv[i];
+
+		if (!strcmp(arg, "--"))
+			break;
+
+		if (rev_list_option_takes_argument(&argv[i])) {
+			i++;
+			continue;
+		}
+
 		if (!strcmp(arg, "--exclude-promisor-objects")) {
 			repo->fetch_if_missing = 0;
 			revs.exclude_promisor_objects = 1;
diff --git a/t/t6022-rev-list-missing.sh b/t/t6022-rev-list-missing.sh
index 1bd2c3bc4f..9f995968fd 100755
--- a/t/t6022-rev-list-missing.sh
+++ b/t/t6022-rev-list-missing.sh
@@ -215,6 +215,32 @@ do
 	'
 done
 
+test_expect_success "--grep accepts a pattern beginning with --" '
+	test_commit --no-tag --missing-only &&
+	commit=$(git rev-parse HEAD) &&
+	echo "$commit" >expect &&
+	git rev-list --grep=--missing-only HEAD >actual &&
+	test_cmp expect actual &&
+	git rev-list --grep --missing-only HEAD >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success "--grep does not trigger --exclude-promisor-objects in early scan" '
+	test_commit --no-tag --exclude-promisor-objects &&
+	commit=$(git rev-parse HEAD) &&
+	echo "$commit" >expect &&
+	git rev-list --grep=--exclude-promisor-objects --missing=print HEAD >actual &&
+	test_cmp expect actual &&
+	git rev-list --grep --exclude-promisor-objects --missing=print HEAD >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success "--author argument is not treated as an option in early scan" '
+	test_commit --no-tag author-test &&
+	git rev-list --author --missing-only HEAD >actual &&
+	test_must_be_empty actual
+'
+
 test_expect_success "--missing-only requires --missing=print or --missing=print-info" '
 	test_must_fail git rev-list --missing-only --objects HEAD 2>err &&
 	test_grep "requires --missing=print" err
-- 
2.56.0.386.g979eb7e29a