{"thread":{"id":"57083","subject":"Git bug: Filter ignored when \"--invert-grep\" option is used.","startedAt":"2021-12-15T09:50:59Z","lastAt":"2021-12-17T18:16:07Z","messageCount":6,"participants":["Dotan Cohen","Junio C Hamano","René Scharfe"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"444141","messageId":"CAKDXFkMvXJm5+5Qxz2N5NH-s+nptayG_7+yTSPxynZxkOcaVKw@mail.gmail.com","threadId":"57083","inReplyTo":null,"subject":"Git bug: Filter ignored when \"--invert-grep\" option is used.","fromName":"Dotan Cohen","fromEmail":"dotancohen@gmail.com","sentAt":"2021-12-15T09:50:44Z","receivedAt":"2021-12-15T09:50:59Z","isPatch":false,"sender":{"key":"dotancohen@gmail.com","avatar":null},"body":"What did you do before the bug happened?\n$ git log -8 --author=Shachar --grep=Revert --invert-grep\n\nWhat did you expect to happen?\nI expected to see the last 8 commits from Shachar that did not have\nthe string \"Revert\" in the commit message.\n\nWhat happened instead?\nThe list of commits included commits by authors other than Shachar.\n\nWhat's different between what you expected and what actually happened?\nThe \"--author\" filter seems to be ignored when the \"--invert-grep\"\noption is used.\nI also tried to change the order of the options, but the results\nremained the same.\n\n[System Info]\ngit version:\ngit version 2.34.1\ncpu: x86_64\nno commit associated with this build\nsizeof-long: 8\nsizeof-size_t: 8\nshell-path: /bin/sh\nuname: Linux 5.11.0-41-generic #45~20.04.1-Ubuntu SMP Wed Nov 10\n10:20:10 UTC 2021 x86_64\ncompiler info: gnuc: 9.3\nlibc info: glibc: 2.31\n$SHELL (typically, interactive shell): /bin/bash\n\n\n[Enabled Hooks]\npre-commit\npre-push\n"},{"id":"444176","messageId":"xmqqzgp134i4.fsf@gitster.g","threadId":"57083","inReplyTo":"CAKDXFkMvXJm5+5Qxz2N5NH-s+nptayG_7+yTSPxynZxkOcaVKw@mail.gmail.com","subject":"Re: Git bug: Filter ignored when \"--invert-grep\" option is used.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-12-15T22:08:51Z","receivedAt":"2021-12-15T22:08:59Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Dotan Cohen <dotancohen@gmail.com> writes:\n\n> What did you do before the bug happened?\n> $ git log -8 --author=Shachar --grep=Revert --invert-grep\n>\n> What did you expect to happen?\n> I expected to see the last 8 commits from Shachar that did not have\n> the string \"Revert\" in the commit message.\n>\n> What happened instead?\n> The list of commits included commits by authors other than Shachar.\n>\n> What's different between what you expected and what actually happened?\n> The \"--author\" filter seems to be ignored when the \"--invert-grep\"\n> option is used.\n> I also tried to change the order of the options, but the results\n> remained the same.\n\nI think --author and --grep uses the same internal pattern matching\nengine, so with --invert-grep, I would not be surprised if the\ncommand looks for commits that do not have Revert and (or is that\nor?  I dunno) not authored by Shachar.\n"},{"id":"444230","messageId":"CAKDXFkOXNPTjQRvj7sy54YhH1QGFUsEXYeLKCShJP7_xueRseQ@mail.gmail.com","threadId":"57083","inReplyTo":"xmqqzgp134i4.fsf@gitster.g","subject":"Re: Git bug: Filter ignored when \"--invert-grep\" option is used.","fromName":"Dotan Cohen","fromEmail":"dotancohen@gmail.com","sentAt":"2021-12-16T14:54:39Z","receivedAt":"2021-12-16T14:54:53Z","isPatch":false,"sender":{"key":"dotancohen@gmail.com","avatar":null},"body":"> I think --author and --grep uses the same internal pattern matching\n> engine, so with --invert-grep, I would not be surprised if the\n> command looks for commits that do not have Revert and (or is that\n> or?  I dunno) not authored by Shachar.\n\nPossibly, but the flag is called --invert-grep not --invert-matches so\none would expect it to revert grep only. Though behaviour contrary to\nuser expectations is not an unusual property of git :)\n\nOther than piping to e.g. awk or worse, how would one get the commits\nby a particular author that do not have a specific string in the\ncommit message? Prettying to oneline would make the piping easier to\nat least get the commit ids, but I'd like to see the whole commit\nmessage and affected files.\n\nThanks, Junio.\n"},{"id":"444253","messageId":"xmqqee6cbalb.fsf@gitster.g","threadId":"57083","inReplyTo":"CAKDXFkOXNPTjQRvj7sy54YhH1QGFUsEXYeLKCShJP7_xueRseQ@mail.gmail.com","subject":"Re: Git bug: Filter ignored when \"--invert-grep\" option is used.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-12-16T19:42:24Z","receivedAt":"2021-12-16T19:42:30Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Dotan Cohen <dotancohen@gmail.com> writes:\n\n>> I think --author and --grep uses the same internal pattern matching\n>> engine, so with --invert-grep, I would not be surprised if the\n>> command looks for commits that do not have Revert and (or is that\n>> or?  I dunno) not authored by Shachar.\n>\n> Possibly, but the flag is called --invert-grep not --invert-matches so\n> one would expect it to revert grep only.\n\nThat is an actionable improvement idea to introduce a synonym ;-)\n\nBut in general, the way the internal \"git grep\" machinery is exposed\nto the commands in the \"git log\" family is very limited.  With \"git\ngrep\", it is quite straight-forward to say \"report hits for lines\nthat has this but not that\"\n\n    $ git grep -e this --and --not -e that\n\nbut because that the commands in the \"log\" family already use\n\"--not\" for a quite different purpose, \"git log --grep\" cannot even\nexpress something similar, even to find hits on a single line, let\nalone finding hits on two different lines (i.e. one on the \"author\"\nheader, the other in the message part, of the commit object).\n\n\n"},{"id":"444380","messageId":"e2e7759e-aa97-1117-6df2-f93a12afb094@web.de","threadId":"57083","inReplyTo":"xmqqee6cbalb.fsf@gitster.g","subject":"Re: Git bug: Filter ignored when \"--invert-grep\" option is used.","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2021-12-17T16:48:49Z","receivedAt":"2021-12-17T16:49:23Z","isPatch":false,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 16.12.21 um 20:42 schrieb Junio C Hamano:\n> Dotan Cohen <dotancohen@gmail.com> writes:\n>\n>>> I think --author and --grep uses the same internal pattern matching\n>>> engine, so with --invert-grep, I would not be surprised if the\n>>> command looks for commits that do not have Revert and (or is that\n>>> or?  I dunno) not authored by Shachar.\n>>\n>> Possibly, but the flag is called --invert-grep not --invert-matches so\n>> one would expect it to revert grep only.\n>\n> That is an actionable improvement idea to introduce a synonym ;-)\n\nDocumentation/rev-list-options.txt says about --invert-grep:\n\n        Limit the commits output to ones with log message that do not\n        match the pattern specified with `--grep=<pattern>`.\n\nBoth the option name and this sentence suggest that it only should\ninvert --grep, which makes sense to me.\n\n> But in general, the way the internal \"git grep\" machinery is exposed\n> to the commands in the \"git log\" family is very limited.  With \"git\n> grep\", it is quite straight-forward to say \"report hits for lines\n> that has this but not that\"\n>\n>     $ git grep -e this --and --not -e that\n>\n> but because that the commands in the \"log\" family already use\n> \"--not\" for a quite different purpose, \"git log --grep\" cannot even\n> express something similar, even to find hits on a single line, let\n> alone finding hits on two different lines (i.e. one on the \"author\"\n> header, the other in the message part, of the commit object).\n\nRight, but we can pass in the necessary bit via struct grep_opt.\n22dfa8a23d (log: teach --invert-grep option, 2015-01-12) even mentions\nthat this done in an earlier iteration of that feature.\n\nRepresenting buffer-level operations like --all-match and this one as\nexpression nodes would be nice.  At least I suspect that would make\nchanging the behavior easier, without having to touch as many places.\n\nAnyway, here's a patch that is intended to bring the code in line\nwith its documentation.  The multiple negations hurt my head, so I\nmay have snuck in some logic errors, though. :-/\n\n--- >8 ---\nSubject: [PATCH] log: let --invert-grep only invert --grep\n\nThe option --invert-grep is documented to filter out commits whose\nmessages match the --grep filters.  However, it also affects the\nheader matches (--author, --committer), which is not intended.\n\nMove the handling of that option to grep.c, as only the code there can\ndistinguish between matches in the header from those in the message\nbody.  If --invert-grep is given then enable extended expressions (not\nthe regex type, we just need git grep's --not to work), negate the body\npatterns and check if any of them match by piggy-backing on the\ncollect_hits mechanism of grep_source_1().\n\nCollecting the matches in struct grep_opt is a bit iffy, but with\n\"last_shown\" we have a precedent for writing state information to that\nstruct.\n\nReported-by: Dotan Cohen <dotancohen@gmail.com>\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\n grep.c         | 22 +++++++++++++++++++---\n grep.h         |  2 ++\n revision.c     |  4 ++--\n revision.h     |  2 --\n t/t4202-log.sh | 19 +++++++++++++++++++\n 5 files changed, 42 insertions(+), 7 deletions(-)\n\ndiff --git a/grep.c b/grep.c\nindex fe847a0111..beef5fe47e 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -699,6 +699,14 @@ static struct grep_expr *compile_pattern_expr(struct grep_pat **list)\n \treturn compile_pattern_or(list);\n }\n\n+static struct grep_expr *grep_not_expr(struct grep_expr *expr)\n+{\n+\tstruct grep_expr *z = xcalloc(1, sizeof(*z));\n+\tz->node = GREP_NODE_NOT;\n+\tz->u.unary = expr;\n+\treturn z;\n+}\n+\n static struct grep_expr *grep_true_expr(void)\n {\n \tstruct grep_expr *z = xcalloc(1, sizeof(*z));\n@@ -797,7 +805,7 @@ void compile_grep_patterns(struct grep_opt *opt)\n \t\t}\n \t}\n\n-\tif (opt->all_match || header_expr)\n+\tif (opt->all_match || opt->no_body_match || header_expr)\n \t\topt->extended = 1;\n \telse if (!opt->extended)\n \t\treturn;\n@@ -808,6 +816,9 @@ void compile_grep_patterns(struct grep_opt *opt)\n \tif (p)\n \t\tdie(\"incomplete pattern expression: %s\", p->pattern);\n\n+\tif (opt->no_body_match && opt->pattern_expression)\n+\t\topt->pattern_expression = grep_not_expr(opt->pattern_expression);\n+\n \tif (!header_expr)\n \t\treturn;\n\n@@ -1057,6 +1068,8 @@ static int match_expr_eval(struct grep_opt *opt, struct grep_expr *x,\n \t\t\tif (h && (*col < 0 || tmp.rm_so < *col))\n \t\t\t\t*col = tmp.rm_so;\n \t\t}\n+\t\tif (x->u.atom->token == GREP_PATTERN_BODY)\n+\t\t\topt->body_hit |= h;\n \t\tbreak;\n \tcase GREP_NODE_NOT:\n \t\t/*\n@@ -1825,16 +1838,19 @@ int grep_source(struct grep_opt *opt, struct grep_source *gs)\n \t * we do not have to do the two-pass grep when we do not check\n \t * buffer-wide \"all-match\".\n \t */\n-\tif (!opt->all_match)\n+\tif (!opt->all_match && !opt->no_body_match)\n \t\treturn grep_source_1(opt, gs, 0);\n\n \t/* Otherwise the toplevel \"or\" terms hit a bit differently.\n \t * We first clear hit markers from them.\n \t */\n \tclr_hit_marker(opt->pattern_expression);\n+\topt->body_hit = 0;\n \tgrep_source_1(opt, gs, 1);\n\n-\tif (!chk_hit_marker(opt->pattern_expression))\n+\tif (opt->all_match && !chk_hit_marker(opt->pattern_expression))\n+\t\treturn 0;\n+\tif (opt->no_body_match && opt->body_hit)\n \t\treturn 0;\n\n \treturn grep_source_1(opt, gs, 0);\ndiff --git a/grep.h b/grep.h\nindex 3e8815c347..6a1f0ab017 100644\n--- a/grep.h\n+++ b/grep.h\n@@ -148,6 +148,8 @@ struct grep_opt {\n \tint word_regexp;\n \tint fixed;\n \tint all_match;\n+\tint no_body_match;\n+\tint body_hit;\n #define GREP_BINARY_DEFAULT\t0\n #define GREP_BINARY_NOMATCH\t1\n #define GREP_BINARY_TEXT\t2\ndiff --git a/revision.c b/revision.c\nindex 1981a0859f..97a06bc8fe 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -2493,7 +2493,7 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg\n \t} else if (!strcmp(arg, \"--all-match\")) {\n \t\trevs->grep_filter.all_match = 1;\n \t} else if (!strcmp(arg, \"--invert-grep\")) {\n-\t\trevs->invert_grep = 1;\n+\t\trevs->grep_filter.no_body_match = 1;\n \t} else if ((argcount = parse_long_opt(\"encoding\", argv, &optarg))) {\n \t\tif (strcmp(optarg, \"none\"))\n \t\t\tgit_log_output_encoding = xstrdup(optarg);\n@@ -3778,7 +3778,7 @@ static int commit_match(struct commit *commit, struct rev_info *opt)\n \t\t\t\t     (char *)message, strlen(message));\n \tstrbuf_release(&buf);\n \tunuse_commit_buffer(commit, message);\n-\treturn opt->invert_grep ? !retval : retval;\n+\treturn retval;\n }\n\n static inline int want_ancestry(const struct rev_info *revs)\ndiff --git a/revision.h b/revision.h\nindex 5578bb4720..3f66147bfd 100644\n--- a/revision.h\n+++ b/revision.h\n@@ -246,8 +246,6 @@ struct rev_info {\n\n \t/* Filter by commit log message */\n \tstruct grep_opt\tgrep_filter;\n-\t/* Negate the match of grep_filter */\n-\tint invert_grep;\n\n \t/* Display history graph */\n \tstruct git_graph *graph;\ndiff --git a/t/t4202-log.sh b/t/t4202-log.sh\nindex 7884e3d46b..765742fdbc 100755\n--- a/t/t4202-log.sh\n+++ b/t/t4202-log.sh\n@@ -2010,4 +2010,23 @@ test_expect_success 'log --end-of-options' '\n        test_cmp expect actual\n '\n\n+test_expect_success 'set up commits with different authors' '\n+\tgit checkout --orphan authors &&\n+\ttest_commit --author \"Jim <jim@example.com>\" jim_1 &&\n+\ttest_commit --author \"Val <val@example.com>\" val_1 &&\n+\ttest_commit --author \"Val <val@example.com>\" val_2 &&\n+\ttest_commit --author \"Jim <jim@example.com>\" jim_2 &&\n+\ttest_commit --author \"Val <val@example.com>\" val_3 &&\n+\ttest_commit --author \"Jim <jim@example.com>\" jim_3\n+'\n+\n+test_expect_success 'log --invert-grep --grep --author' '\n+\tcat >expect <<-\\EOF &&\n+\tval_3\n+\tval_1\n+\tEOF\n+\tgit log --format=%s --author=Val --grep 2 --invert-grep >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n--\n2.34.0\n"},{"id":"444384","messageId":"xmqqbl1f5c7z.fsf@gitster.g","threadId":"57083","inReplyTo":"e2e7759e-aa97-1117-6df2-f93a12afb094@web.de","subject":"Re: Git bug: Filter ignored when \"--invert-grep\" option is used.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-12-17T18:16:00Z","receivedAt":"2021-12-17T18:16:07Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe <l.s.r@web.de> writes:\n\n> Subject: [PATCH] log: let --invert-grep only invert --grep\n>\n> The option --invert-grep is documented to filter out commits whose\n> messages match the --grep filters.  However, it also affects the\n> header matches (--author, --committer), which is not intended.\n\nI re-read the log message that introduced this feature, and I agree\nwith the \"not intended\" part.  I do not think the change itself was\neven done with awareness that the header matches may also be\naffected, and there is no test for it to see the interaction.\n\n> Move the handling of that option to grep.c, as only the code there can\n> distinguish between matches in the header from those in the message\n> body.  If --invert-grep is given then enable extended expressions (not\n> the regex type, we just need git grep's --not to work), negate the body\n> patterns and check if any of them match by piggy-backing on the\n> collect_hits mechanism of grep_source_1().\n\nNice.  The original says that --files-without-matches being a\nnegation of --files-with-matches was what triggered them to have the\nbit in the revisions, not in grep_opt, by the way.\n\n> Collecting the matches in struct grep_opt is a bit iffy, but with\n> \"last_shown\" we have a precedent for writing state information to that\n> struct.\n\nI think this is perfectly fine.  apply_state, grep_opt,\ndiff_options, and rev_info are used the same way within their\nsubsystems to carry in options that affect behaviour, carry around\nthe state of the machinery, and carry out the result.  The word\n\"option\" does make it sound it is an input-only thing, but others\nare not much better ;-).\n\n> diff --git a/grep.c b/grep.c\n> index fe847a0111..beef5fe47e 100644\n> --- a/grep.c\n> +++ b/grep.c\n> @@ -699,6 +699,14 @@ static struct grep_expr *compile_pattern_expr(struct grep_pat **list)\n>  \treturn compile_pattern_or(list);\n>  }\n>\n> +static struct grep_expr *grep_not_expr(struct grep_expr *expr)\n> +{\n> +\tstruct grep_expr *z = xcalloc(1, sizeof(*z));\n> +\tz->node = GREP_NODE_NOT;\n> +\tz->u.unary = expr;\n> +\treturn z;\n> +}\n\nA bit surprising to see that we already had GREP_NODE_NOT without a\nhelper to create a node.  Not updating compile_pattern_not() to use\nthis new helper does make this patch simpler to read by allowing\nreaders to focus on what matters, which is very much appreciaed.\n\nThe rest of the patch looks good to me, too.\n"}]}