[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

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

[Junio C Hamano]

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

> 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.

The commit log message explains the motivation well, and the attempt
to address the feedback from the previous round by handling all 19
options from revision.c is appreciated.

But the approach of hardcoding an array of option strings that take
arguments is inherently fragile and scales poorly.  By duplicating
parser knowledge into the early scan, this patch falls into the
exact trap described by the NEEDSWORK comment just below it.

> 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",
> +	};

Is this hardcoded list complete?  It misses options parsed via
diff_merges_parse_opts() in diff-merges.c (e.g., --diff-merges), as
well as options handled in diff.c (e.g., --src-prefix and
--dst-prefix), which also take separate arguments and are accepted
by git rev-list.  Would

    $ git rev-list --src-prefix --missing-only HEAD

take --missing-only as an extra option or a prefix?

Thanks.