{"thread":{"id":"58592","subject":"Git BUG 2.37.3 and 2.38.0","startedAt":"2022-10-10T14:33:58Z","lastAt":"2022-10-11T15:51:26Z","messageCount":18,"participants":["orygaw","rsbecker@nexbridge.com","Taylor Blau","Ævar Arnfjörð Bjarmason","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"464478","messageId":"o2ijhZAUIi7nPTwQmsJfpvPIEeLbhvn1AP8rhW2hCNbC380Z2ogDHzZigqJsHI6RwphsIZR3OSJSy-wYvyWv5un632tKynHKFLFPLPEDH2g=@protonmail.com","threadId":"58592","inReplyTo":null,"subject":"Git BUG 2.37.3 and 2.38.0","fromName":"orygaw","fromEmail":"orygaw@protonmail.com","sentAt":"2022-10-10T14:33:41Z","receivedAt":"2022-10-10T14:33:58Z","isPatch":false,"sender":{"key":"orygaw@protonmail.com","avatar":null},"body":"Hello, \n\n\nI found a bug with GIT with version 2.37.3 and 2.38.0\n\nMy system FreeBSD 13.0-RELEASE-p11:\n\ncommand:\n\ngit log -1 --invert-grep\n\n\n* thread #1, name = 'git', stop reason = signal SIGSEGV: invalid address (fault address: 0x0)\n  * frame #0: 0x00000000004fe580 git`free_pattern_expr(x=0x0000000000000000) at grep.c:755:13\n    frame #1: 0x00000000004fe501 git`free_grep_patterns(opt=0x00007fffffffdc20) at grep.c:795:2\n    frame #2: 0x00000000005edd16 git`release_revisions(revs=0x00007fffffffda58) at revision.c:3030:2\n    frame #3: 0x00000000003826c8 git`cmd_log_deinit(ret=0, rev=0x00007fffffffda58) at log.c:353:2\n    frame #4: 0x00000000003845ae git`cmd_log(argc=3, argv=0x00007fffffffe960, prefix=0x0000000000000000) at log.c:883:9\n    frame #5: 0x00000000002f8e8c git`run_builtin(p=0x00000000006a4b58, argc=3, argv=0x00007fffffffe960) at git.c:466:11\n    frame #6: 0x00000000002f7783 git`handle_builtin(argc=3, argv=0x00007fffffffe960) at git.c:721:3\n    frame #7: 0x00000000002f87c6 git`run_argv(argcp=0x00007fffffffe8a4, argv=0x00007fffffffe898) at git.c:788:4\n    frame #8: 0x00000000002f7521 git`cmd_main(argc=3, argv=0x00007fffffffe960) at git.c:921:19\n    frame #9: 0x000000000042b53a git`main(argc=4, argv=0x00007fffffffe958) at common-main.c:56:11\n    frame #10: 0x00000000002f6dd0 git`_start(ap=<unavailable>, cleanup=<unavailable>) at crt1_c.c:75:7\n\n"},{"id":"464480","messageId":"037801d8dcbe$9d585b20$d8091160$@nexbridge.com","threadId":"58592","inReplyTo":"o2ijhZAUIi7nPTwQmsJfpvPIEeLbhvn1AP8rhW2hCNbC380Z2ogDHzZigqJsHI6RwphsIZR3OSJSy-wYvyWv5un632tKynHKFLFPLPEDH2g=@protonmail.com","subject":"RE: Git BUG 2.37.3 and 2.38.0","fromName":"","fromEmail":"rsbecker@nexbridge.com","sentAt":"2022-10-10T15:40:18Z","receivedAt":"2022-10-10T15:40:40Z","isPatch":false,"sender":{"key":"randall.becker@nexbridge.ca","avatar":"https://avatars.githubusercontent.com/u/28956764?v=4"},"body":"On October 10, 2022 10:34 AM, orygaw\n>I found a bug with GIT with version 2.37.3 and 2.38.0\n>\n>My system FreeBSD 13.0-RELEASE-p11:\n>\n>command:\n>\n>git log -1 --invert-grep\n>\n>\n>* thread #1, name = 'git', stop reason = signal SIGSEGV: invalid address (fault\n>address: 0x0)\n>  * frame #0: 0x00000000004fe580 git`free_pattern_expr(x=0x0000000000000000)\n>at grep.c:755:13\n>    frame #1: 0x00000000004fe501 git`free_grep_patterns(opt=0x00007fffffffdc20)\n>at grep.c:795:2\n>    frame #2: 0x00000000005edd16 git`release_revisions(revs=0x00007fffffffda58)\n>at revision.c:3030:2\n>    frame #3: 0x00000000003826c8 git`cmd_log_deinit(ret=0,\n>rev=0x00007fffffffda58) at log.c:353:2\n>    frame #4: 0x00000000003845ae git`cmd_log(argc=3, argv=0x00007fffffffe960,\n>prefix=0x0000000000000000) at log.c:883:9\n>    frame #5: 0x00000000002f8e8c git`run_builtin(p=0x00000000006a4b58, argc=3,\n>argv=0x00007fffffffe960) at git.c:466:11\n>    frame #6: 0x00000000002f7783 git`handle_builtin(argc=3,\n>argv=0x00007fffffffe960) at git.c:721:3\n>    frame #7: 0x00000000002f87c6 git`run_argv(argcp=0x00007fffffffe8a4,\n>argv=0x00007fffffffe898) at git.c:788:4\n>    frame #8: 0x00000000002f7521 git`cmd_main(argc=3, argv=0x00007fffffffe960)\n>at git.c:921:19\n>    frame #9: 0x000000000042b53a git`main(argc=4, argv=0x00007fffffffe958) at\n>common-main.c:56:11\n>    frame #10: 0x00000000002f6dd0 git`_start(ap=<unavailable>,\n>cleanup=<unavailable>) at crt1_c.c:75:7\n\nI can confirm a similar situation on Cygwin at 2.38.0.\n\n$ git log -1 --invert-grep\ncommit 5385d4b84047b3c42cde36f1fab83ac57df17ca8 (HEAD -> topic, origin/topic)\nAuthor: Me <yesme@domain.com>\nDate:   Sun Oct 9 22:26:47 2022 -0400\n\n    rms.yaml: add path variable definition\nSegmentation fault (core dumped)\n\nMy history is extensive.\n-Randall\n\n"},{"id":"464481","messageId":"Y0Q+v0s5BPYEPq2K@nand.local","threadId":"58592","inReplyTo":"037801d8dcbe$9d585b20$d8091160$@nexbridge.com","subject":"Re: Git BUG 2.37.3 and 2.38.0","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-10-10T15:48:15Z","receivedAt":"2022-10-10T15:48:22Z","isPatch":false,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Oct 10, 2022 at 11:40:18AM -0400, rsbecker@nexbridge.com wrote:\n> I can confirm a similar situation on Cygwin at 2.38.0.\n>\n> $ git log -1 --invert-grep\n> commit 5385d4b84047b3c42cde36f1fab83ac57df17ca8 (HEAD -> topic, origin/topic)\n> Author: Me <yesme@domain.com>\n> Date:   Sun Oct 9 22:26:47 2022 -0400\n>\n>     rms.yaml: add path variable definition\n> Segmentation fault (core dumped)\n\nThanks, both. I can reproduce it, too. A quick bisection points to\nf41fb662f5 (revisions API: have release_revisions() release\n\"grep_filter\", 2022-04-13), which appears probable.\n\nI'll push up a patch shortly which fixes this issue.\n\n\nThanks,\nTaylor\n"},{"id":"464499","messageId":"patch-1.1-f4b90799fce-20221010T165711Z-avarab@gmail.com","threadId":"58592","inReplyTo":"o2ijhZAUIi7nPTwQmsJfpvPIEeLbhvn1AP8rhW2hCNbC380Z2ogDHzZigqJsHI6RwphsIZR3OSJSy-wYvyWv5un632tKynHKFLFPLPEDH2g=@protonmail.com","subject":"[PATCH] log: require --grep for --invert-grep and --all-match, fix segfault","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-10-10T16:57:42Z","receivedAt":"2022-10-10T16:57:54Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Neither the \"--invert-grep\" option added in [1] nor the earlier\n\"--all-match\" option added in [2] were intended to be used\nstand-alone.\n\nBut due to how the built-in and the revision API interacted those\noptions without the corresponding --grep would be ignored.\n\nThen in f41fb662f57 (revisions API: have release_revisions() release\n\"grep_filter\", 2022-04-13) this turned into a segfault, as we'd\nattempt to free() the non-existing --grep patterns.\n\nArguably it makes more sense to add this check to\ncompile_grep_patterns(), since it's possible to use the C API in the\nsame way and trigger this segfault. But in practice the revision.c API\nis the only user of \"no_body_match\", and by placing the check here we\ncan more sensibly emit a message that assumes that the user used\n\"--invert-grep\" without \"--grep\".\n\n1. 22dfa8a23de (log: teach --invert-grep option, 2015-01-12)\n2. 0ab7befa31d (grep --all-match, 2006-09-27)\n\nReported-by: orygaw <orygaw@protonmail.com>\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n revision.c      | 6 ++++++\n t/t4202-log.sh  | 4 ++++\n t/t7810-grep.sh | 4 ++++\n 3 files changed, 14 insertions(+)\n\ndiff --git a/revision.c b/revision.c\nindex 36e31942cee..a55ead48448 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -2986,6 +2986,12 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s\n \t\tdie(_(\"options '%s' and '%s' cannot be used together\"), \"--no-walk\", \"--graph\");\n \tif (!revs->reflog_info && revs->grep_filter.use_reflog_filter)\n \t\tdie(_(\"the option '%s' requires '%s'\"), \"--grep-reflog\", \"--walk-reflogs\");\n+\tif (!revs->grep_filter.pattern_expression) {\n+\t\tif (revs->grep_filter.no_body_match)\n+\t\t\tdie(_(\"the option '%s' requires '%s'\"), \"--invert-grep\", \"--grep\");\n+\t\tif (revs->grep_filter.all_match)\n+\t\t\tdie(_(\"the option '%s' requires '%s'\"), \"--all-match\", \"--grep\");\n+\t}\n \n \tif (revs->line_level_traverse &&\n \t    (revs->diffopt.output_format & ~(DIFF_FORMAT_PATCH | DIFF_FORMAT_NO_OUTPUT)))\ndiff --git a/t/t4202-log.sh b/t/t4202-log.sh\nindex cc15cb4ff62..298678fb7c8 100755\n--- a/t/t4202-log.sh\n+++ b/t/t4202-log.sh\n@@ -249,6 +249,10 @@ test_expect_success 'log --grep' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'log --invert-grep usage' '\n+\ttest_expect_code 128 git log --invert-grep\n+'\n+\n cat > expect << EOF\n second\n initial\ndiff --git a/t/t7810-grep.sh b/t/t7810-grep.sh\nindex 8eded6ab274..6dd750349e1 100755\n--- a/t/t7810-grep.sh\n+++ b/t/t7810-grep.sh\n@@ -914,6 +914,10 @@ test_expect_success 'log with multiple --grep uses union' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'log --all-match usage' '\n+\ttest_expect_code 128 git log --all-match\n+'\n+\n test_expect_success 'log --all-match with multiple --grep uses intersection' '\n \tgit log --all-match --grep=i --grep=r --format=%s >actual &&\n \t{\n-- \n2.38.0.971.ge79ff6d20e7\n\n"},{"id":"464502","messageId":"xmqqlepn4nnn.fsf@gitster.g","threadId":"58592","inReplyTo":"Y0Q+v0s5BPYEPq2K@nand.local","subject":"Re: Git BUG 2.37.3 and 2.38.0","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-10-10T17:02:52Z","receivedAt":"2022-10-10T17:03:17Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau <me@ttaylorr.com> writes:\n\n> On Mon, Oct 10, 2022 at 11:40:18AM -0400, rsbecker@nexbridge.com wrote:\n>> I can confirm a similar situation on Cygwin at 2.38.0.\n>>\n>> $ git log -1 --invert-grep\n>> commit 5385d4b84047b3c42cde36f1fab83ac57df17ca8 (HEAD -> topic, origin/topic)\n>> Author: Me <yesme@domain.com>\n>> Date:   Sun Oct 9 22:26:47 2022 -0400\n>>\n>>     rms.yaml: add path variable definition\n>> Segmentation fault (core dumped)\n>\n> Thanks, both. I can reproduce it, too. A quick bisection points to\n> f41fb662f5 (revisions API: have release_revisions() release\n> \"grep_filter\", 2022-04-13), which appears probable.\n>\n> I'll push up a patch shortly which fixes this issue.\n\nThanks.\n"},{"id":"464504","messageId":"xmqq7d174m6a.fsf@gitster.g","threadId":"58592","inReplyTo":"patch-1.1-f4b90799fce-20221010T165711Z-avarab@gmail.com","subject":"Re: [PATCH] log: require --grep for --invert-grep and --all-match, fix segfault","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-10-10T17:34:53Z","receivedAt":"2022-10-10T17:35:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:\n\n> Neither the \"--invert-grep\" option added in [1] nor the earlier\n> \"--all-match\" option added in [2] were intended to be used\n> stand-alone.\n\nMostly yes, but with \"to be used\" -> \"to take effect\".\n\n\"[alias] lga = log --all-match\" would be equivalent to \"log\"\nunless the command line starts talking about \"--grep\", which is\nquite handy.\n\nSo the real fix would be not to forbid the standalone passing of the\noption to the command, but to fix the over-eager freeing of an\nunallocated resource, introduced more recently, I would suspect.\n\nThanks.\n"},{"id":"464506","messageId":"cover.1665423686.git.me@ttaylorr.com","threadId":"58592","inReplyTo":"o2ijhZAUIi7nPTwQmsJfpvPIEeLbhvn1AP8rhW2hCNbC380Z2ogDHzZigqJsHI6RwphsIZR3OSJSy-wYvyWv5un632tKynHKFLFPLPEDH2g=@protonmail.com","subject":"[PATCH 0/2] grep: tolerate NULL argument to free_grep_expr()","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-10-10T17:41:27Z","receivedAt":"2022-10-10T17:41:32Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"This short series demonstrates and fixes a bug whereby a NULL\n`struct grep_expr*` argument (such as when running `git log\n--invert-grep` without a `--grep` argument) could cause us to segfault\nwhen trying to dereference that argument.\n\nAn alternative approach is to disallow `--invert-grep` without a\n`--grep` argument. But a more user-friendly approach is to permit this\ncombination by pretending as if neither argument was given. This patch\nseries takes the latter approach by teaching free_grep_expr() that\npassing a NULL argument should result in a noop, not a segfault.\n\nTaylor Blau (2):\n  t4202: demonstrate `git log --invert-grep` segfault\n  grep.c: tolerate NULL grep_expr in free_pattern_expr()\n\n grep.c         | 5 +++--\n t/t4202-log.sh | 6 ++++++\n 2 files changed, 9 insertions(+), 2 deletions(-)\n\n-- \n2.37.0.1.g1379af2e9d\n"},{"id":"464507","messageId":"fa27aca12c46d2934baa594c05b7b3d7672e0be6.1665423686.git.me@ttaylorr.com","threadId":"58592","inReplyTo":"cover.1665423686.git.me@ttaylorr.com","subject":"[PATCH 1/2] t4202: demonstrate `git log --invert-grep` segfault","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-10-10T17:41:30Z","receivedAt":"2022-10-10T17:41:35Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"When `--invert-grep` is given without a pattern, `git log` behaves as\nnormal. But as of f41fb662f5 (revisions API: have release_revisions()\nrelease \"grep_filter\", 2022-04-13), this doesn't quite work because we\ntry to dereference the NULL `grep_expr` pointer in\n`free_pattern_expr()`, leading to a sgefault.\n\nThe subsequent patch will explain the bug, provide a fix, and update\nthis test to expect success.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n t/t4202-log.sh | 6 ++++++\n 1 file changed, 6 insertions(+)\n\ndiff --git a/t/t4202-log.sh b/t/t4202-log.sh\nindex cc15cb4ff6..e3ec5f5661 100755\n--- a/t/t4202-log.sh\n+++ b/t/t4202-log.sh\n@@ -297,6 +297,12 @@ test_expect_success 'log --invert-grep --grep -i' '\n \tfi\n '\n \n+test_expect_failure 'log --invert-grep (no --grep)' '\n+\tgit log --pretty=\"tformat:%s\" >expect &&\n+\tgit log --invert-grep --pretty=\"tformat:%s\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'log --grep option parsing' '\n \techo second >expect &&\n \tgit log -1 --pretty=\"tformat:%s\" --grep sec >actual &&\n-- \n2.37.0.1.g1379af2e9d\n\n"},{"id":"464508","messageId":"7e094882c2a71894416089f894557a9eae07e8f8.1665423686.git.me@ttaylorr.com","threadId":"58592","inReplyTo":"cover.1665423686.git.me@ttaylorr.com","subject":"[PATCH 2/2] grep.c: tolerate NULL grep_expr in free_pattern_expr()","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-10-10T17:41:32Z","receivedAt":"2022-10-10T17:41:42Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"As demonstrated in the previous commit, `git log --invert-grep` without\na `--grep` argument causes a segfault after f41fb662f5 (revisions API:\nhave release_revisions() release \"grep_filter\", 2022-04-13).\n\nThe segfault occurs in `free_pattern_expr()`, which crashes on trying to\nswitch on `x->node` when given a NULL pointer. Usually we avoid calling\n`free_pattern_expr()` without a pattern as indicated by the `extended`\nbit being zero.\n\nBut it is possible to get into a state where the `extended` bit is\nnon-zero, but the `pattern_expression` is still NULL. This happens\nbecause the `--invert-grep` option sets the `no_body_match` bit. When we\ncall `compile_grep_patterns()`, we set `opt->extended = 1`. But the\n`pattern_expression` is left as NULL, since we return with a NULL\n`header_expr`.\n\nSo when we try to call `free_pattern_expr()`, things go awry, since\n`free_grep_patterns()` expects a non-NULL argument.\n\nInstead, teach `free_grep_patterns()` to tolerate a NULL argument\n(treating it as a noop), and avoid checking whether or not the\n`extended` bit is set, since `free_pattern_expr()` will handle its\nargument regardless.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n grep.c         | 5 +++--\n t/t4202-log.sh | 2 +-\n 2 files changed, 4 insertions(+), 3 deletions(-)\n\ndiff --git a/grep.c b/grep.c\nindex 52a894c989..bcc6e63365 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -752,6 +752,9 @@ void compile_grep_patterns(struct grep_opt *opt)\n \n static void free_pattern_expr(struct grep_expr *x)\n {\n+\tif (!x)\n+\t\treturn;\n+\n \tswitch (x->node) {\n \tcase GREP_NODE_TRUE:\n \tcase GREP_NODE_ATOM:\n@@ -790,8 +793,6 @@ void free_grep_patterns(struct grep_opt *opt)\n \t\tfree(p);\n \t}\n \n-\tif (!opt->extended)\n-\t\treturn;\n \tfree_pattern_expr(opt->pattern_expression);\n }\n \ndiff --git a/t/t4202-log.sh b/t/t4202-log.sh\nindex e3ec5f5661..44f7ef0ea2 100755\n--- a/t/t4202-log.sh\n+++ b/t/t4202-log.sh\n@@ -297,7 +297,7 @@ test_expect_success 'log --invert-grep --grep -i' '\n \tfi\n '\n \n-test_expect_failure 'log --invert-grep (no --grep)' '\n+test_expect_success 'log --invert-grep (no --grep)' '\n \tgit log --pretty=\"tformat:%s\" >expect &&\n \tgit log --invert-grep --pretty=\"tformat:%s\" >actual &&\n \ttest_cmp expect actual\n-- \n2.37.0.1.g1379af2e9d\n"},{"id":"464509","messageId":"xmqq35bv4lns.fsf@gitster.g","threadId":"58592","inReplyTo":"xmqq7d174m6a.fsf@gitster.g","subject":"Re: [PATCH] log: require --grep for --invert-grep and --all-match, fix segfault","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-10-10T17:45:59Z","receivedAt":"2022-10-10T17:46:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:\n>\n>> Neither the \"--invert-grep\" option added in [1] nor the earlier\n>> \"--all-match\" option added in [2] were intended to be used\n>> stand-alone.\n>\n> Mostly yes, but with \"to be used\" -> \"to take effect\".\n>\n> \"[alias] lga = log --all-match\" would be equivalent to \"log\"\n> unless the command line starts talking about \"--grep\", which is\n> quite handy.\n>\n> So the real fix would be not to forbid the standalone passing of the\n> option to the command, but to fix the over-eager freeing of an\n> unallocated resource, introduced more recently, I would suspect.\n\nOn the other hand, I do not think \"--invert-grep\" is useful in the\nsame way.  The only usage I can think of is to omit merges by\nlooking for substring \"^Merge\", but then we already have a more\nrobust \"--no-merges\" option for that purpose.\n\nBut both uses the same mechanism, I would say treating them the\nsame, i.e. silently ignoring these when --grep is not given, would\nbe the most sensible.\n\nThanks.\n"},{"id":"464510","messageId":"xmqqy1tn36pc.fsf@gitster.g","threadId":"58592","inReplyTo":"7e094882c2a71894416089f894557a9eae07e8f8.1665423686.git.me@ttaylorr.com","subject":"Re: [PATCH 2/2] grep.c: tolerate NULL grep_expr in free_pattern_expr()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-10-10T17:54:23Z","receivedAt":"2022-10-10T17:54:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau <me@ttaylorr.com> writes:\n\n> diff --git a/grep.c b/grep.c\n> index 52a894c989..bcc6e63365 100644\n> --- a/grep.c\n> +++ b/grep.c\n> @@ -752,6 +752,9 @@ void compile_grep_patterns(struct grep_opt *opt)\n>  \n>  static void free_pattern_expr(struct grep_expr *x)\n>  {\n> +\tif (!x)\n> +\t\treturn;\n> +\n>  \tswitch (x->node) {\n>  \tcase GREP_NODE_TRUE:\n>  \tcase GREP_NODE_ATOM:\n\nThis hunk makes sense, but\n\n> @@ -790,8 +793,6 @@ void free_grep_patterns(struct grep_opt *opt)\n>  \t\tfree(p);\n>  \t}\n>  \n> -\tif (!opt->extended)\n> -\t\treturn;\n>  \tfree_pattern_expr(opt->pattern_expression);\n>  }\n\nI do not know about this one.  We used to avoid freeing, even when\nthe .pattern_expression member is set, as long as the .extended bit\nis not set.  Now we unconditionally try to free it even when the bit\nsays it does not want to.  Why?\n\n> diff --git a/t/t4202-log.sh b/t/t4202-log.sh\n> index e3ec5f5661..44f7ef0ea2 100755\n> --- a/t/t4202-log.sh\n> +++ b/t/t4202-log.sh\n> @@ -297,7 +297,7 @@ test_expect_success 'log --invert-grep --grep -i' '\n>  \tfi\n>  '\n>  \n> -test_expect_failure 'log --invert-grep (no --grep)' '\n> +test_expect_success 'log --invert-grep (no --grep)' '\n>  \tgit log --pretty=\"tformat:%s\" >expect &&\n>  \tgit log --invert-grep --pretty=\"tformat:%s\" >actual &&\n>  \ttest_cmp expect actual\n\nEspecially for something this small, doing the \"failing test first\nand then fix with flipping the test to success\" is very much\nunwelcome.  For whoever gets curious (me included when accepting\nposted patch), it is easy to revert only the part of the commit\noutside t/ tentatively to see how the original code breaks.  Keeping\nthe fix and protection of the fix together will avoid mistakes.  In\nthis case, the whole test fits inside the post context of the patch,\nbut in general, this \"flip failure to success\" will hide the body of\nthe test that changes behaviour while reviewing the patch text,\nwhich is another downside.\n\nThanks.\n\n"},{"id":"464512","messageId":"Y0RgIGpxXXilh7YM@nand.local","threadId":"58592","inReplyTo":"xmqqy1tn36pc.fsf@gitster.g","subject":"Re: [PATCH 2/2] grep.c: tolerate NULL grep_expr in free_pattern_expr()","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-10-10T18:10:40Z","receivedAt":"2022-10-10T18:10:45Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Oct 10, 2022 at 10:54:23AM -0700, Junio C Hamano wrote:\n> Taylor Blau <me@ttaylorr.com> writes:\n> > @@ -790,8 +793,6 @@ void free_grep_patterns(struct grep_opt *opt)\n> >  \t\tfree(p);\n> >  \t}\n> >\n> > -\tif (!opt->extended)\n> > -\t\treturn;\n> >  \tfree_pattern_expr(opt->pattern_expression);\n> >  }\n>\n> I do not know about this one.  We used to avoid freeing, even when\n> the .pattern_expression member is set, as long as the .extended bit\n> is not set.  Now we unconditionally try to free it even when the bit\n> says it does not want to.  Why?\n\nIt's not \"does not want to\" be freed. As best I can tell, we conflate\n`opt->extended` with \"there is something in `opt->pattern_expression`\".\nSo checking whether or not `opt->extended` is non-zero isn't \"keep this\naround because I'm going to use it later\", but instead \"there is\nnothing to free, don't bother calling `free_pattern_expr()`\".\n\nA more direct way of saying the latter would have been to replace the\nif-statement with `if (opt->pattern_expression)`.\n\nI hinted at this in the commit message, but I will make it more direct\nto avoid future readers' confusion.\n\n> > diff --git a/t/t4202-log.sh b/t/t4202-log.sh\n> > index e3ec5f5661..44f7ef0ea2 100755\n> > --- a/t/t4202-log.sh\n> > +++ b/t/t4202-log.sh\n> > @@ -297,7 +297,7 @@ test_expect_success 'log --invert-grep --grep -i' '\n> >  \tfi\n> >  '\n> >\n> > -test_expect_failure 'log --invert-grep (no --grep)' '\n> > +test_expect_success 'log --invert-grep (no --grep)' '\n> >  \tgit log --pretty=\"tformat:%s\" >expect &&\n> >  \tgit log --invert-grep --pretty=\"tformat:%s\" >actual &&\n> >  \ttest_cmp expect actual\n>\n> Especially for something this small, doing the \"failing test first\n> and then fix with flipping the test to success\" is very much\n> unwelcome.  For whoever gets curious (me included when accepting\n> posted patch), it is easy to revert only the part of the commit\n> outside t/ tentatively to see how the original code breaks.  Keeping\n> the fix and protection of the fix together will avoid mistakes.  In\n> this case, the whole test fits inside the post context of the patch,\n> but in general, this \"flip failure to success\" will hide the body of\n> the test that changes behaviour while reviewing the patch text,\n> which is another downside.\n\nGood to know. I had considered it good practice, even for a small fix,\nas a way to show your work and prove that you had a legitimately broken\ntest case demonstrating the bug. But if it creates an extra hassle, I\ndon't mind squashing it down.\n\nI can send a squashed version of these two patches, but let's see if\nthere are any other comments, first.\n\nThanks,\nTaylor\n"},{"id":"464513","messageId":"xmqqsfjv35xi.fsf@gitster.g","threadId":"58592","inReplyTo":"xmqqy1tn36pc.fsf@gitster.g","subject":"Re: [PATCH 2/2] grep.c: tolerate NULL grep_expr in free_pattern_expr()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-10-10T18:11:05Z","receivedAt":"2022-10-10T18:11:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n>>  static void free_pattern_expr(struct grep_expr *x)\n>>  {\n>> +\tif (!x)\n>> +\t\treturn;\n>> +\n>>  \tswitch (x->node) {\n>>  \tcase GREP_NODE_TRUE:\n>>  \tcase GREP_NODE_ATOM:\n>\n> This hunk makes sense, but\n>\n>> @@ -790,8 +793,6 @@ void free_grep_patterns(struct grep_opt *opt)\n>>  \t\tfree(p);\n>>  \t}\n>>  \n>> -\tif (!opt->extended)\n>> -\t\treturn;\n>>  \tfree_pattern_expr(opt->pattern_expression);\n>>  }\n>\n> I do not know about this one.  We used to avoid freeing, even when\n> the .pattern_expression member is set, as long as the .extended bit\n> is not set.  Now we unconditionally try to free it even when the bit\n> says it does not want to.  Why?\n\nAh, grep.c::compile_grep_patterns() has the answer.  We only\npopulate the .pattern_expression member when we are doing a complex\nquery and leave it empty otherwise.  The .pattern_list member is\nused instead as a list of OR'ed patterns in grep.c::match_line()\nwhen .extended is not set.\n\nThe !opt->extended guard assumes that opt->pattern_expression exists\nonly when extended is set, which is correct, but forgets that even\nwhen extended is set, pattern_expression is not necessarily non-NULL.\n\nSo I think the right thing to do may be to allow free_pattern_expr()\nto take and ignore NULL silently?  Ah, that is already what you are\ndoing in the first hunk.  Is this second hunk even necessary?\n\nI wonder how calls to grep.c::match_line() with opt->extended true\nand opt->pattern_expression NULL, though.  It should die() at the\nbeginning of match_expr_eval(), which probably is OK, but somehow\nfeels unsatisfactory.\n"},{"id":"464516","messageId":"Y0Rg9My2EaWl/WCU@nand.local","threadId":"58592","inReplyTo":"xmqqsfjv35xi.fsf@gitster.g","subject":"Re: [PATCH 2/2] grep.c: tolerate NULL grep_expr in free_pattern_expr()","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-10-10T18:14:12Z","receivedAt":"2022-10-10T18:14:19Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Oct 10, 2022 at 11:11:05AM -0700, Junio C Hamano wrote:\n> Junio C Hamano <gitster@pobox.com> writes:\n>\n> So I think the right thing to do may be to allow free_pattern_expr()\n> to take and ignore NULL silently?  Ah, that is already what you are\n> doing in the first hunk.  Is this second hunk even necessary?\n\nRight. The fix in my patch is to have `free_pattern_expr()` treat its\narguments like `free()` does, where NULL is a silent noop.\n\nWe could do with or without the second hunk. I dropped it to avoid\nconfusion, but it seems to have had the opposite effect ;-). I'll keep\nthe if-statement there and instead drop the latter hunk.\n\n> I wonder how calls to grep.c::match_line() with opt->extended true\n> and opt->pattern_expression NULL, though.  It should die() at the\n> beginning of match_expr_eval(), which probably is OK, but somehow\n> feels unsatisfactory.\n\nIt does, and we can thank Linus for that: c922b01f54 (grep: fix segfault\nwhen \"git grep '('\" is given, 2009-04-27).\n\nThanks,\nTaylor\n"},{"id":"464522","messageId":"221010.86o7ujpl44.gmgdl@evledraar.gmail.com","threadId":"58592","inReplyTo":"xmqq35bv4lns.fsf@gitster.g","subject":"Re: [PATCH] log: require --grep for --invert-grep and --all-match, fix segfault","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-10-10T18:48:25Z","receivedAt":"2022-10-10T18:52:20Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Oct 10 2022, Junio C Hamano wrote:\n\n> Junio C Hamano <gitster@pobox.com> writes:\n>\n>> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:\n>>\n>>> Neither the \"--invert-grep\" option added in [1] nor the earlier\n>>> \"--all-match\" option added in [2] were intended to be used\n>>> stand-alone.\n>>\n>> Mostly yes, but with \"to be used\" -> \"to take effect\".\n>>\n>> \"[alias] lga = log --all-match\" would be equivalent to \"log\"\n>> unless the command line starts talking about \"--grep\", which is\n>> quite handy.\n>>\n>> So the real fix would be not to forbid the standalone passing of the\n>> option to the command, but to fix the over-eager freeing of an\n>> unallocated resource, introduced more recently, I would suspect.\n>\n> On the other hand, I do not think \"--invert-grep\" is useful in the\n> same way.  The only usage I can think of is to omit merges by\n> looking for substring \"^Merge\", but then we already have a more\n> robust \"--no-merges\" option for that purpose.\n>\n> But both uses the same mechanism, I would say treating them the\n> same, i.e. silently ignoring these when --grep is not given, would\n> be the most sensible.\n\nThe rationale for changing it this way is that the documentaion says:\n\t\n\t--all-match::\n\t        Limit the commits output to ones that match all given `--grep`,\n\t        instead of ones that match at least one.\n\t--invert-grep::\n\t        Limit the commits output to ones with log message that do not\n\t        match the pattern specified with `--grep=<pattern>`.\n\nEspecially as we also say:\n\n\t--grep-reflog=<pattern>::\n\t        Limit the commits output to ones with reflog entries that\n        \tmatch the specified pattern (regular expression) [...]\n\nI.e. pretty much the same wording, but there we'll die if the\ncorresponding other option is omitted, as the context shows.\n\nMaybe we want to post-hoc say we'd like to support it, in which case\nsome plasting over the issue (e.g. in the direction of Taylor's fix)\nwould do it.\n\n"},{"id":"464524","messageId":"Y0Rr0sQ91C/SgmBJ@nand.local","threadId":"58592","inReplyTo":"221010.86o7ujpl44.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH] log: require --grep for --invert-grep and --all-match, fix segfault","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-10-10T19:00:34Z","receivedAt":"2022-10-10T19:00:40Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Oct 10, 2022 at 08:48:25PM +0200, Ævar Arnfjörð Bjarmason wrote:\n> The rationale for changing it this way is that the documentaion says:\n>\n> \t--all-match::\n> \t        Limit the commits output to ones that match all given `--grep`,\n> \t        instead of ones that match at least one.\n> \t--invert-grep::\n> \t        Limit the commits output to ones with log message that do not\n> \t        match the pattern specified with `--grep=<pattern>`.\n\nThis does feel a little academic, but to me the documentation seems to\nsuggest that `--all-match` or `--invert-grep` should both support the\nabsence of a `--grep` argument.\n\nAt least in `--invert-grep`, my reading is \"the pattern specified with\n`--grep=<pattern>` [...if any]\".\n\nIn any case, the behavior has been as such for long enough that it feels\nlike our documentation needs to be changed, and not the behavior itself.\nOn the other hand, I have a hard time imagining that many/any people\ncare about this particular behavior.\n\nThanks,\nTaylor\n"},{"id":"464597","messageId":"patch-v2-1.1-6ad7627706f-20221011T094715Z-avarab@gmail.com","threadId":"58592","inReplyTo":"patch-1.1-f4b90799fce-20221010T165711Z-avarab@gmail.com","subject":"[PATCH v2] grep.c: remove \"extended\" in favor of \"pattern_expression\", fix segfault","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-10-11T09:48:45Z","receivedAt":"2022-10-11T09:48:55Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Since 79d3696cfb4 (git-grep: boolean expression on pattern matching.,\n2006-06-30) the \"pattern_expression\" member has been used for complex\nqueries (AND/OR...), with \"pattern_list\" being used for the simple OR\nqueries. Since then we've used both \"pattern_expression\" and its\nassociated boolean \"extended\" member to see if we have a complex\nexpression.\n\nSince f41fb662f57 (revisions API: have release_revisions() release\n\"grep_filter\", 2022-04-13) we've had a subtle bug relating to that: If\nwe supplied options that were only used for \"complex queries\", but\ndidn't supply the query itself we'd set \"opt->extended\", but would\nhave a NULL \"pattern_expression\". As a result these would segfault as\nwe tried to call \"free_grep_patterns()\" from \"release_revisions()\":\n\n\tgit -P log -1 --invert-grep\n\tgit -P log -1 --all-match\n\nThe root cause of this is that we were conflating the state management\nwe needed in \"compile_grep_patterns()\" itself with whether or not we\nhad an \"opt->pattern_expression\" later on.\n\nIn this cases as we're going through \"compile_grep_patterns()\" we have\nno \"opt->pattern_list\" but have \"opt->no_body_match\" or\n\"opt->all_match\". So we'd set \"opt->extended = 1\", but not \"return\" on\n\"opt->extended\" as that's an \"else if\" in the same \"if\" statement.\n\nThat behavior is intentional and required, as the common case is that\nwe have an \"opt->pattern_list\" that we're about to parse into the\n\"opt->pattern_expression\".\n\nBut we don't need to keep track of this \"extended\" flag beyond the\nstate management in compile_grep_patterns() itself. It needs it, but\nonce we're out of that function we can rely on\n\"opt->pattern_expression\" being non-NULL instead for using these\nextended patterns.\n\nAs 79d3696cfb4 itself shows we've assumed that there's a one-to-one\nmapping between the two since the very beginning. I.e. \"match_line()\"\nwould check \"opt->extended\" to see if it should call \"match_expr()\",\nand the first thing we do in that function is assume that we have a\n\"opt->pattern_expression\". We'd then call \"match_expr_eval()\", which\nwould have died if that \"opt->pattern_expression\" was NULL.\n\nThe \"die\" was added in c922b01f54c (grep: fix segfault when \"git grep\n'('\" is given, 2009-04-27), and can now be removed as it's now clearly\nunreachable. We still do the right thing in the case that prompted\nthat fix:\n\n\tgit grep '('\n\tfatal: unmatched parenthesis\n\nArguably neither the \"--invert-grep\" option added in [1] nor the\nearlier \"--all-match\" option added in [2] were intended to be used\nstand-alone, and another approach[3] would be to error out in those\ncases. But since we've been treating them as a NOOP when given without\n--grep for a long time let's keep doing that.\n\nWe could also return in \"free_pattern_expr()\" if the argument is\nnon-NULL, as an alternative fix for this segfault does [4]. That would\nbe more elegant in making the \"free_*()\" function behave like\n\"free()\", but it would also remove a sanity check: The\n\"free_pattern_expr()\" function calls itself recursively, and only the\ntop-level is allowed to be NULL, let's not conflate those two\nconditions.\n\n1. 22dfa8a23de (log: teach --invert-grep option, 2015-01-12)\n2. 0ab7befa31d (grep --all-match, 2006-09-27)\n3. https://lore.kernel.org/git/patch-1.1-f4b90799fce-20221010T165711Z-avarab@gmail.com/\n4. http://lore.kernel.org/git/7e094882c2a71894416089f894557a9eae07e8f8.1665423686.git.me@ttaylorr.com\n\nReported-by: orygaw <orygaw@protonmail.com>\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n\nPer the v1 feedback this v2 doesn't change any user-observable\nbehavior, except to address the segfault(s).\n\nPer https://lore.kernel.org/git/Y0Rg9My2EaWl%2FWCU@nand.local/ it\nsounds like Taylor's preparing his own v2 with a more narrow fix\nfocused on https://lore.kernel.org/git/Y0Rg9My2EaWl%2FWCU@nand.local/;\nwhich we may or may not want to have instead of this as a quicker\nband-aid.\n\nBut this attempts to address the root cause of the problem, which is\nthat grep.[ch] is effectively juggling two struct members that mean\nthe same thing, but whose state drifted apart.\n\nPassing CI at: https://github.com/avar/git/actions/runs/3225227595\n(actually https://github.com/avar/git/actions/runs/3225813488, but\nwhen I was about to submit this I made a trivial whitespace fix in the\nt/*.sh change).\n\nRange-diff against v1:\n1:  f4b90799fce ! 1:  6ad7627706f log: require --grep for --invert-grep and --all-match, fix segfault\n    @@ Metadata\n     Author: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n     \n      ## Commit message ##\n    -    log: require --grep for --invert-grep and --all-match, fix segfault\n    +    grep.c: remove \"extended\" in favor of \"pattern_expression\", fix segfault\n     \n    -    Neither the \"--invert-grep\" option added in [1] nor the earlier\n    -    \"--all-match\" option added in [2] were intended to be used\n    -    stand-alone.\n    +    Since 79d3696cfb4 (git-grep: boolean expression on pattern matching.,\n    +    2006-06-30) the \"pattern_expression\" member has been used for complex\n    +    queries (AND/OR...), with \"pattern_list\" being used for the simple OR\n    +    queries. Since then we've used both \"pattern_expression\" and its\n    +    associated boolean \"extended\" member to see if we have a complex\n    +    expression.\n     \n    -    But due to how the built-in and the revision API interacted those\n    -    options without the corresponding --grep would be ignored.\n    +    Since f41fb662f57 (revisions API: have release_revisions() release\n    +    \"grep_filter\", 2022-04-13) we've had a subtle bug relating to that: If\n    +    we supplied options that were only used for \"complex queries\", but\n    +    didn't supply the query itself we'd set \"opt->extended\", but would\n    +    have a NULL \"pattern_expression\". As a result these would segfault as\n    +    we tried to call \"free_grep_patterns()\" from \"release_revisions()\":\n     \n    -    Then in f41fb662f57 (revisions API: have release_revisions() release\n    -    \"grep_filter\", 2022-04-13) this turned into a segfault, as we'd\n    -    attempt to free() the non-existing --grep patterns.\n    +            git -P log -1 --invert-grep\n    +            git -P log -1 --all-match\n     \n    -    Arguably it makes more sense to add this check to\n    -    compile_grep_patterns(), since it's possible to use the C API in the\n    -    same way and trigger this segfault. But in practice the revision.c API\n    -    is the only user of \"no_body_match\", and by placing the check here we\n    -    can more sensibly emit a message that assumes that the user used\n    -    \"--invert-grep\" without \"--grep\".\n    +    The root cause of this is that we were conflating the state management\n    +    we needed in \"compile_grep_patterns()\" itself with whether or not we\n    +    had an \"opt->pattern_expression\" later on.\n    +\n    +    In this cases as we're going through \"compile_grep_patterns()\" we have\n    +    no \"opt->pattern_list\" but have \"opt->no_body_match\" or\n    +    \"opt->all_match\". So we'd set \"opt->extended = 1\", but not \"return\" on\n    +    \"opt->extended\" as that's an \"else if\" in the same \"if\" statement.\n    +\n    +    That behavior is intentional and required, as the common case is that\n    +    we have an \"opt->pattern_list\" that we're about to parse into the\n    +    \"opt->pattern_expression\".\n    +\n    +    But we don't need to keep track of this \"extended\" flag beyond the\n    +    state management in compile_grep_patterns() itself. It needs it, but\n    +    once we're out of that function we can rely on\n    +    \"opt->pattern_expression\" being non-NULL instead for using these\n    +    extended patterns.\n    +\n    +    As 79d3696cfb4 itself shows we've assumed that there's a one-to-one\n    +    mapping between the two since the very beginning. I.e. \"match_line()\"\n    +    would check \"opt->extended\" to see if it should call \"match_expr()\",\n    +    and the first thing we do in that function is assume that we have a\n    +    \"opt->pattern_expression\". We'd then call \"match_expr_eval()\", which\n    +    would have died if that \"opt->pattern_expression\" was NULL.\n    +\n    +    The \"die\" was added in c922b01f54c (grep: fix segfault when \"git grep\n    +    '('\" is given, 2009-04-27), and can now be removed as it's now clearly\n    +    unreachable. We still do the right thing in the case that prompted\n    +    that fix:\n    +\n    +            git grep '('\n    +            fatal: unmatched parenthesis\n    +\n    +    Arguably neither the \"--invert-grep\" option added in [1] nor the\n    +    earlier \"--all-match\" option added in [2] were intended to be used\n    +    stand-alone, and another approach[3] would be to error out in those\n    +    cases. But since we've been treating them as a NOOP when given without\n    +    --grep for a long time let's keep doing that.\n    +\n    +    We could also return in \"free_pattern_expr()\" if the argument is\n    +    non-NULL, as an alternative fix for this segfault does [4]. That would\n    +    be more elegant in making the \"free_*()\" function behave like\n    +    \"free()\", but it would also remove a sanity check: The\n    +    \"free_pattern_expr()\" function calls itself recursively, and only the\n    +    top-level is allowed to be NULL, let's not conflate those two\n    +    conditions.\n     \n         1. 22dfa8a23de (log: teach --invert-grep option, 2015-01-12)\n         2. 0ab7befa31d (grep --all-match, 2006-09-27)\n    +    3. https://lore.kernel.org/git/patch-1.1-f4b90799fce-20221010T165711Z-avarab@gmail.com/\n    +    4. http://lore.kernel.org/git/7e094882c2a71894416089f894557a9eae07e8f8.1665423686.git.me@ttaylorr.com\n     \n         Reported-by: orygaw <orygaw@protonmail.com>\n         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n     \n    - ## revision.c ##\n    -@@ revision.c: int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s\n    - \t\tdie(_(\"options '%s' and '%s' cannot be used together\"), \"--no-walk\", \"--graph\");\n    - \tif (!revs->reflog_info && revs->grep_filter.use_reflog_filter)\n    - \t\tdie(_(\"the option '%s' requires '%s'\"), \"--grep-reflog\", \"--walk-reflogs\");\n    -+\tif (!revs->grep_filter.pattern_expression) {\n    -+\t\tif (revs->grep_filter.no_body_match)\n    -+\t\t\tdie(_(\"the option '%s' requires '%s'\"), \"--invert-grep\", \"--grep\");\n    -+\t\tif (revs->grep_filter.all_match)\n    -+\t\t\tdie(_(\"the option '%s' requires '%s'\"), \"--all-match\", \"--grep\");\n    -+\t}\n    + ## grep.c ##\n    +@@ grep.c: void compile_grep_patterns(struct grep_opt *opt)\n    + {\n    + \tstruct grep_pat *p;\n    + \tstruct grep_expr *header_expr = prep_header_patterns(opt);\n    ++\tint extended = 0;\n    + \n    + \tfor (p = opt->pattern_list; p; p = p->next) {\n    + \t\tswitch (p->token) {\n    +@@ grep.c: void compile_grep_patterns(struct grep_opt *opt)\n    + \t\t\tcompile_regexp(p, opt);\n    + \t\t\tbreak;\n    + \t\tdefault:\n    +-\t\t\topt->extended = 1;\n    ++\t\t\textended = 1;\n    + \t\t\tbreak;\n    + \t\t}\n    + \t}\n    + \n    + \tif (opt->all_match || opt->no_body_match || header_expr)\n    +-\t\topt->extended = 1;\n    +-\telse if (!opt->extended)\n    ++\t\textended = 1;\n    ++\telse if (!extended)\n    + \t\treturn;\n    + \n    + \tp = opt->pattern_list;\n    +@@ grep.c: void free_grep_patterns(struct grep_opt *opt)\n    + \t\tfree(p);\n    + \t}\n      \n    - \tif (revs->line_level_traverse &&\n    - \t    (revs->diffopt.output_format & ~(DIFF_FORMAT_PATCH | DIFF_FORMAT_NO_OUTPUT)))\n    +-\tif (!opt->extended)\n    ++\tif (!opt->pattern_expression)\n    + \t\treturn;\n    + \tfree_pattern_expr(opt->pattern_expression);\n    + }\n    +@@ grep.c: static int match_expr_eval(struct grep_opt *opt, struct grep_expr *x,\n    + {\n    + \tint h = 0;\n    + \n    +-\tif (!x)\n    +-\t\tdie(\"Not a valid grep expression\");\n    + \tswitch (x->node) {\n    + \tcase GREP_NODE_TRUE:\n    + \t\th = 1;\n    +@@ grep.c: static int match_line(struct grep_opt *opt,\n    + \tstruct grep_pat *p;\n    + \tint hit = 0;\n    + \n    +-\tif (opt->extended)\n    ++\tif (opt->pattern_expression)\n    + \t\treturn match_expr(opt, bol, eol, ctx, col, icol,\n    + \t\t\t\t  collect_hits);\n    + \n    +@@ grep.c: static int should_lookahead(struct grep_opt *opt)\n    + {\n    + \tstruct grep_pat *p;\n    + \n    +-\tif (opt->extended)\n    ++\tif (opt->pattern_expression)\n    + \t\treturn 0; /* punt for too complex stuff */\n    + \tif (opt->invert)\n    + \t\treturn 0;\n    +\n    + ## grep.h ##\n    +@@ grep.h: struct grep_opt {\n    + #define GREP_BINARY_TEXT\t2\n    + \tint binary;\n    + \tint allow_textconv;\n    +-\tint extended;\n    + \tint use_reflog_filter;\n    + \tint relative;\n    + \tint pathname;\n     \n      ## t/t4202-log.sh ##\n     @@ t/t4202-log.sh: test_expect_success 'log --grep' '\n      \ttest_cmp expect actual\n      '\n      \n    -+test_expect_success 'log --invert-grep usage' '\n    -+\ttest_expect_code 128 git log --invert-grep\n    -+'\n    ++for noop_opt in --invert-grep --all-match\n    ++do\n    ++\ttest_expect_success \"log $noop_opt without --grep is a NOOP\" '\n    ++\t\tgit log >expect &&\n    ++\t\tgit log $noop_opt >actual &&\n    ++\t\ttest_cmp expect actual\n    ++\t'\n    ++done\n     +\n      cat > expect << EOF\n      second\n      initial\n    -\n    - ## t/t7810-grep.sh ##\n    -@@ t/t7810-grep.sh: test_expect_success 'log with multiple --grep uses union' '\n    - \ttest_cmp expect actual\n    - '\n    - \n    -+test_expect_success 'log --all-match usage' '\n    -+\ttest_expect_code 128 git log --all-match\n    -+'\n    -+\n    - test_expect_success 'log --all-match with multiple --grep uses intersection' '\n    - \tgit log --all-match --grep=i --grep=r --format=%s >actual &&\n    - \t{\n\n grep.c         | 15 +++++++--------\n grep.h         |  1 -\n t/t4202-log.sh |  9 +++++++++\n 3 files changed, 16 insertions(+), 9 deletions(-)\n\ndiff --git a/grep.c b/grep.c\nindex 52a894c9890..06eed694936 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -708,6 +708,7 @@ void compile_grep_patterns(struct grep_opt *opt)\n {\n \tstruct grep_pat *p;\n \tstruct grep_expr *header_expr = prep_header_patterns(opt);\n+\tint extended = 0;\n \n \tfor (p = opt->pattern_list; p; p = p->next) {\n \t\tswitch (p->token) {\n@@ -717,14 +718,14 @@ void compile_grep_patterns(struct grep_opt *opt)\n \t\t\tcompile_regexp(p, opt);\n \t\t\tbreak;\n \t\tdefault:\n-\t\t\topt->extended = 1;\n+\t\t\textended = 1;\n \t\t\tbreak;\n \t\t}\n \t}\n \n \tif (opt->all_match || opt->no_body_match || header_expr)\n-\t\topt->extended = 1;\n-\telse if (!opt->extended)\n+\t\textended = 1;\n+\telse if (!extended)\n \t\treturn;\n \n \tp = opt->pattern_list;\n@@ -790,7 +791,7 @@ void free_grep_patterns(struct grep_opt *opt)\n \t\tfree(p);\n \t}\n \n-\tif (!opt->extended)\n+\tif (!opt->pattern_expression)\n \t\treturn;\n \tfree_pattern_expr(opt->pattern_expression);\n }\n@@ -971,8 +972,6 @@ static int match_expr_eval(struct grep_opt *opt, struct grep_expr *x,\n {\n \tint h = 0;\n \n-\tif (!x)\n-\t\tdie(\"Not a valid grep expression\");\n \tswitch (x->node) {\n \tcase GREP_NODE_TRUE:\n \t\th = 1;\n@@ -1052,7 +1051,7 @@ static int match_line(struct grep_opt *opt,\n \tstruct grep_pat *p;\n \tint hit = 0;\n \n-\tif (opt->extended)\n+\tif (opt->pattern_expression)\n \t\treturn match_expr(opt, bol, eol, ctx, col, icol,\n \t\t\t\t  collect_hits);\n \n@@ -1370,7 +1369,7 @@ static int should_lookahead(struct grep_opt *opt)\n {\n \tstruct grep_pat *p;\n \n-\tif (opt->extended)\n+\tif (opt->pattern_expression)\n \t\treturn 0; /* punt for too complex stuff */\n \tif (opt->invert)\n \t\treturn 0;\ndiff --git a/grep.h b/grep.h\nindex bdcadce61b8..6075f997e68 100644\n--- a/grep.h\n+++ b/grep.h\n@@ -151,7 +151,6 @@ struct grep_opt {\n #define GREP_BINARY_TEXT\t2\n \tint binary;\n \tint allow_textconv;\n-\tint extended;\n \tint use_reflog_filter;\n \tint relative;\n \tint pathname;\ndiff --git a/t/t4202-log.sh b/t/t4202-log.sh\nindex cc15cb4ff62..2ce2b41174d 100755\n--- a/t/t4202-log.sh\n+++ b/t/t4202-log.sh\n@@ -249,6 +249,15 @@ test_expect_success 'log --grep' '\n \ttest_cmp expect actual\n '\n \n+for noop_opt in --invert-grep --all-match\n+do\n+\ttest_expect_success \"log $noop_opt without --grep is a NOOP\" '\n+\t\tgit log >expect &&\n+\t\tgit log $noop_opt >actual &&\n+\t\ttest_cmp expect actual\n+\t'\n+done\n+\n cat > expect << EOF\n second\n initial\n-- \n2.38.0.971.ge79ff6d20e7\n\n"},{"id":"464618","messageId":"xmqqczayweg4.fsf@gitster.g","threadId":"58592","inReplyTo":"patch-v2-1.1-6ad7627706f-20221011T094715Z-avarab@gmail.com","subject":"Re: [PATCH v2] grep.c: remove \"extended\" in favor of \"pattern_expression\", fix segfault","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-10-11T15:46:35Z","receivedAt":"2022-10-11T15:51:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:\n\n>  grep.c         | 15 +++++++--------\n>  grep.h         |  1 -\n>  t/t4202-log.sh |  9 +++++++++\n>  3 files changed, 16 insertions(+), 9 deletions(-)\n>\n> diff --git a/grep.c b/grep.c\n> index 52a894c9890..06eed694936 100644\n> --- a/grep.c\n> +++ b/grep.c\n> @@ -708,6 +708,7 @@ void compile_grep_patterns(struct grep_opt *opt)\n>  {\n>  \tstruct grep_pat *p;\n>  \tstruct grep_expr *header_expr = prep_header_patterns(opt);\n> +\tint extended = 0;\n>  \n>  \tfor (p = opt->pattern_list; p; p = p->next) {\n>  \t\tswitch (p->token) {\n> @@ -717,14 +718,14 @@ void compile_grep_patterns(struct grep_opt *opt)\n>  \t\t\tcompile_regexp(p, opt);\n>  \t\t\tbreak;\n>  \t\tdefault:\n> -\t\t\topt->extended = 1;\n> +\t\t\textended = 1;\n>  \t\t\tbreak;\n>  \t\t}\n>  \t}\n>  \n>  \tif (opt->all_match || opt->no_body_match || header_expr)\n> -\t\topt->extended = 1;\n> -\telse if (!opt->extended)\n> +\t\textended = 1;\n> +\telse if (!extended)\n>  \t\treturn;\n\nNice. I like this change to make \"!!opt->pattern_expression\" the\nauthoritative source of truth for opt->extended by getting rid of\nthe latter.  This function did need to have a handy way to tell \"do\nwe need to populate pattern_expression?\" while going over the list\nand also use of some features forced us to do so no matter how\nsimple the patterns on the list are, but after doing so and\npopulating the pattern_expression, there was no reason to keep it\naround by having it as a member in the opt structure.\n\n> @@ -790,7 +791,7 @@ void free_grep_patterns(struct grep_opt *opt)\n>  \t\tfree(p);\n>  \t}\n>  \n> -\tif (!opt->extended)\n> +\tif (!opt->pattern_expression)\n>  \t\treturn;\n>  \tfree_pattern_expr(opt->pattern_expression);\n>  }\n> @@ -971,8 +972,6 @@ static int match_expr_eval(struct grep_opt *opt, struct grep_expr *x,\n>  {\n>  \tint h = 0;\n>  \n> -\tif (!x)\n> -\t\tdie(\"Not a valid grep expression\");\n>  \tswitch (x->node) {\n>  \tcase GREP_NODE_TRUE:\n>  \t\th = 1;\n> @@ -1052,7 +1051,7 @@ static int match_line(struct grep_opt *opt,\n>  \tstruct grep_pat *p;\n>  \tint hit = 0;\n>  \n> -\tif (opt->extended)\n> +\tif (opt->pattern_expression)\n>  \t\treturn match_expr(opt, bol, eol, ctx, col, icol,\n>  \t\t\t\t  collect_hits);\n>  \n> @@ -1370,7 +1369,7 @@ static int should_lookahead(struct grep_opt *opt)\n>  {\n>  \tstruct grep_pat *p;\n>  \n> -\tif (opt->extended)\n> +\tif (opt->pattern_expression)\n>  \t\treturn 0; /* punt for too complex stuff */\n>  \tif (opt->invert)\n>  \t\treturn 0;\n\nAnd the necessary change for users is surprisingly small.\n\n> diff --git a/grep.h b/grep.h\n> index bdcadce61b8..6075f997e68 100644\n> --- a/grep.h\n> +++ b/grep.h\n> @@ -151,7 +151,6 @@ struct grep_opt {\n>  #define GREP_BINARY_TEXT\t2\n>  \tint binary;\n>  \tint allow_textconv;\n> -\tint extended;\n>  \tint use_reflog_filter;\n>  \tint relative;\n>  \tint pathname;\n> diff --git a/t/t4202-log.sh b/t/t4202-log.sh\n> index cc15cb4ff62..2ce2b41174d 100755\n> --- a/t/t4202-log.sh\n> +++ b/t/t4202-log.sh\n> @@ -249,6 +249,15 @@ test_expect_success 'log --grep' '\n>  \ttest_cmp expect actual\n>  '\n>  \n> +for noop_opt in --invert-grep --all-match\n> +do\n> +\ttest_expect_success \"log $noop_opt without --grep is a NOOP\" '\n> +\t\tgit log >expect &&\n> +\t\tgit log $noop_opt >actual &&\n> +\t\ttest_cmp expect actual\n> +\t'\n> +done\n\nOK.\n\nThanks, will queue.\n"}]}