[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