{"thread":{"id":"46596","subject":"[PATCH/RFC] git-grep: correct exit code with --quiet and -L","startedAt":"2017-08-15T20:49:02Z","lastAt":"2017-08-15T22:43:57Z","messageCount":5,"participants":["Anthony Sottile","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"326456","messageId":"20170815203503.12299-1-asottile@umich.edu","threadId":"46596","inReplyTo":null,"subject":"[PATCH/RFC] git-grep: correct exit code with --quiet and -L","fromName":"Anthony Sottile","fromEmail":"asottile@umich.edu","sentAt":"2017-08-15T20:35:03Z","receivedAt":"2017-08-15T20:49:02Z","isPatch":true,"sender":{"key":"asottile@umich.edu","avatar":"https://avatars.githubusercontent.com/u/1810591?v=4"},"body":"The handling of `status_only` no longer interferes with the handling of\n`unmatch_name_only`. `--quiet` no longer affects the exit code when using\n`-L`/`--files-without-match`.\n\nSigned-off-by: Anthony Sottile \n---\n grep.c | 9 +++++----\n t/t7810-grep.sh | 5 +++++\n 2 files changed, 10 insertions(+), 4 deletions(-)\n\ndiff --git a/grep.c b/grep.c\nindex 2efec0e..a893d09 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -1755,7 +1755,7 @@ static int grep_source_1(struct grep_opt *opt, struct grep_source *gs, int colle\n \t\t}\n \t\tif (hit) {\n \t\t\tcount++;\n-\t\t\tif (opt->status_only)\n+\t\t\tif (!opt->unmatch_name_only && opt->status_only)\n \t\t\t\treturn 1;\n \t\t\tif (opt->name_only) {\n \t\t\t\tshow_name(opt, gs->name);\n@@ -1820,13 +1820,14 @@ static int grep_source_1(struct grep_opt *opt, struct grep_source *gs, int colle\n \tif (collect_hits)\n \t\treturn 0;\n \n-\tif (opt->status_only)\n-\t\treturn 0;\n \tif (opt->unmatch_name_only) {\n \t\t/* We did not see any hit, so we want to show this */\n-\t\tshow_name(opt, gs->name);\n+\t\tif (!opt->status_only)\n+\t\t\tshow_name(opt, gs->name);\n \t\treturn 1;\n \t}\n+\tif (opt->status_only)\n+\t\treturn 0;\n \n \txdiff_clear_find_func(&xecfg);\n \topt->priv = NULL;\ndiff --git a/t/t7810-grep.sh b/t/t7810-grep.sh\nindex f106387..2a6679c 100755\n--- a/t/t7810-grep.sh\n+++ b/t/t7810-grep.sh\n@@ -374,6 +374,11 @@ test_expect_success 'grep -L -C' '\n \ttest_cmp expected actual\n '\n \n+test_expect_success 'grep --files-without-match --quiet' '\n+\tgit grep --files-without-match --quiet nonexistent_string >actual &&\n+\ttest_cmp /dev/null actual\n+'\n+\n cat >expected < writes:\n\n> The handling of `status_only` no longer interferes with the handling of\n> `unmatch_name_only`. `--quiet` no longer affects the exit code when using\n> `-L`/`--files-without-match`.\n\nI agree with the above statement of yours that --quiet should not\naffect what the exit status is.\n\nBut I am not sure what the exit code from these commands _should_\nbe:\n\n $ git grep -L qfwfq \\*.h ;# no file matches\n $ git grep -L \\# \\*.h ;# some but not all files match\n $ git grep -L . \\*.h ;# all files match\n\nwith or without --quiet. I seem to get 0, 0, 1, which I am not sure\nis correct. I do recall writing \"git grep\" _without_ thinking what\nthe exit code should be when we added --files-without-match, so the\nexit status the current code gives out may be just a random garbage.\n\nAsking GNU grep (because --files-without-match is not a POSIX thing):\n\n $ grep -L qfwfq *.h ;# no file matches\n $ grep -L \\# *.h ;# some but not all files match\n $ grep -L . *.h ;# all files match\n\nI seem to get 1, 0, 0. So the exit status should reflect if there\nwas _any_ hit from any file that were inspected.\n\n> @@ -1755,7 +1755,7 @@ static int grep_source_1(struct grep_opt *opt, struct grep_source *gs, int colle\n> \t\t}\n> \t\tif (hit) {\n> \t\t\tcount++;\n> -\t\t\tif (opt->status_only)\n> +\t\t\tif (!opt->unmatch_name_only && opt->status_only)\n> \t\t\t\treturn 1;\n> \t\t\tif (opt->name_only) {\n> \t\t\t\tshow_name(opt, gs->name);\n\nDoes the change in this hunk have any effect?\n\nJust before this hunk there is this code:\n\n\t\t/* \"grep -v -e foo -e bla\" should list lines\n\t\t * that do not have either, so inversion should\n\t\t * be done outside.\n\t\t */\n\t\tif (opt->invert)\n\t\t\thit = !hit;\n\t\tif (opt->unmatch_name_only) {\n\t\t\tif (hit)\n\t\t\t\treturn 0;\n\t\t\tgoto next_line;\n\nIf (opt->unmatch_name_only && hit) then the function would have\nalready returned and the control wouldn't have reached here.\n\nWhich would mean that when the control reaches the line this hunk\ntouches, either one of these must be false, and because we are\ninside \"if (hit)\", opt->unmatch_name_only must be false.\n\n> @@ -1820,13 +1820,14 @@ static int grep_source_1(struct grep_opt *opt, struct grep_source *gs, int colle\n> \tif (collect_hits)\n> \t\treturn 0;\n> \n> -\tif (opt->status_only)\n> -\t\treturn 0;\n> \tif (opt->unmatch_name_only) {\n> \t\t/* We did not see any hit, so we want to show this */\n> -\t\tshow_name(opt, gs->name);\n> +\t\tif (!opt->status_only)\n> +\t\t\tshow_name(opt, gs->name);\n> \t\treturn 1;\n> \t}\n> +\tif (opt->status_only)\n> +\t\treturn 0;\n\nThis hunk makes sense to me (provided if the semantics we want out\nof --files-without-match is sensible, which is dubious), even though\nI would have limited the change to just a single line, i.e.\n\n\tif (opt->status_only)\n-\t\treturn 0;\n+\t\treturn opt->unmatch_name_only;\n\tif (opt->unmatch_name_only) {\n\t\t/* We did not see any hit, ... */\n\nBut I suspect we want to fix the exit code not to be affected by\nthe \"--files-without-match\" option in the first place, so all the\ncode changes we see in this patch might be moot X-<.\n\n\n"},{"id":"326461","messageId":"CA+dzEBnMOoXYaP7QadQ+gzKKJ7OzJKAPwbW=zr=1fksCQNBVcw@mail.gmail.com","threadId":"46596","inReplyTo":"xmqqpobwplde.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH/RFC] git-grep: correct exit code with --quiet and -L","fromName":"Anthony Sottile","fromEmail":"asottile@umich.edu","sentAt":"2017-08-15T21:41:19Z","receivedAt":"2017-08-15T21:41:25Z","isPatch":true,"sender":{"key":"asottile@umich.edu","avatar":"https://avatars.githubusercontent.com/u/1810591?v=4"},"body":"Ah yes, I didn't intend to include the first hunk (forgot to amend it\nout when formatting the patch).\n\nI think git's exit codes for -L actually make more sense than the GNU\nexit codes (especially when comparing with `grep` vs `grep -v`) --\nthat is, produce `0` when the search is successful (producing\n*something* on stdout) and `1` when the search fails.\n\nShall I create a new mail with the adjusted patch as suggested above?\n(I'm not familiar with the expected workflow).\n\nAnthony\n\nOn Tue, Aug 15, 2017 at 2:33 PM, Junio C Hamano wrote:\n> Anthony Sottile writes:\n>\n>> The handling of `status_only` no longer interferes with the handling of\n>> `unmatch_name_only`. `--quiet` no longer affects the exit code when using\n>> `-L`/`--files-without-match`.\n>\n> I agree with the above statement of yours that --quiet should not\n> affect what the exit status is.\n>\n> But I am not sure what the exit code from these commands _should_\n> be:\n>\n> $ git grep -L qfwfq \\*.h ;# no file matches\n> $ git grep -L \\# \\*.h ;# some but not all files match\n> $ git grep -L . \\*.h ;# all files match\n>\n> with or without --quiet. I seem to get 0, 0, 1, which I am not sure\n> is correct. I do recall writing \"git grep\" _without_ thinking what\n> the exit code should be when we added --files-without-match, so the\n> exit status the current code gives out may be just a random garbage.\n>\n> Asking GNU grep (because --files-without-match is not a POSIX thing):\n>\n> $ grep -L qfwfq *.h ;# no file matches\n> $ grep -L \\# *.h ;# some but not all files match\n> $ grep -L . *.h ;# all files match\n>\n> I seem to get 1, 0, 0. So the exit status should reflect if there\n> was _any_ hit from any file that were inspected.\n>\n>> @@ -1755,7 +1755,7 @@ static int grep_source_1(struct grep_opt *opt, struct grep_source *gs, int colle\n>> }\n>> if (hit) {\n>> count++;\n>> - if (opt->status_only)\n>> + if (!opt->unmatch_name_only && opt->status_only)\n>> return 1;\n>> if (opt->name_only) {\n>> show_name(opt, gs->name);\n>\n> Does the change in this hunk have any effect?\n>\n> Just before this hunk there is this code:\n>\n> /* \"grep -v -e foo -e bla\" should list lines\n> * that do not have either, so inversion should\n> * be done outside.\n> */\n> if (opt->invert)\n> hit = !hit;\n> if (opt->unmatch_name_only) {\n> if (hit)\n> return 0;\n> goto next_line;\n>\n> If (opt->unmatch_name_only && hit) then the function would have\n> already returned and the control wouldn't have reached here.\n>\n> Which would mean that when the control reaches the line this hunk\n> touches, either one of these must be false, and because we are\n> inside \"if (hit)\", opt->unmatch_name_only must be false.\n>\n>> @@ -1820,13 +1820,14 @@ static int grep_source_1(struct grep_opt *opt, struct grep_source *gs, int colle\n>> if (collect_hits)\n>> return 0;\n>>\n>> - if (opt->status_only)\n>> - return 0;\n>> if (opt->unmatch_name_only) {\n>> /* We did not see any hit, so we want to show this */\n>> - show_name(opt, gs->name);\n>> + if (!opt->status_only)\n>> + show_name(opt, gs->name);\n>> return 1;\n>> }\n>> + if (opt->status_only)\n>> + return 0;\n>\n> This hunk makes sense to me (provided if the semantics we want out\n> of --files-without-match is sensible, which is dubious), even though\n> I would have limited the change to just a single line, i.e.\n>\n> if (opt->status_only)\n> - return 0;\n> + return opt->unmatch_name_only;\n> if (opt->unmatch_name_only) {\n> /* We did not see any hit, ... */\n>\n> But I suspect we want to fix the exit code not to be affected by\n> the \"--files-without-match\" option in the first place, so all the\n> code changes we see in this patch might be moot X-<.\n>\n>\n"},{"id":"326463","messageId":"xmqqlgmkpizm.fsf@gitster.mtv.corp.google.com","threadId":"46596","inReplyTo":"CA+dzEBnMOoXYaP7QadQ+gzKKJ7OzJKAPwbW=zr=1fksCQNBVcw@mail.gmail.com","subject":"Re: [PATCH/RFC] git-grep: correct exit code with --quiet and -L","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-08-15T22:24:29Z","receivedAt":"2017-08-15T22:24:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Anthony Sottile writes:\n\n> Ah yes, I didn't intend to include the first hunk (forgot to amend it\n> out when formatting the patch).\n>\n> I think git's exit codes for -L actually make more sense than the GNU\n> exit codes (especially when comparing with `grep` vs `grep -v`) --\n> that is, produce `0` when the search is successful (producing\n> *something* on stdout) and `1` when the search fails.\n>\n> Shall I create a new mail with the adjusted patch as suggested above?\n\nI do not mind seeing an updated patch that does not change the exit\nstatus (as you seem to like what we have currently that contradicts\nwhat GNU grep does) but makes it consistent between \"--quiet\" and\n\"--no-quiet\". But I would not be surprised if people seeing this\nexchange from the sideline are already working on fixing the exit\nstatus and also making sure that the fixed code would produce the\nsame corrected exit status with or without \"--quiet\", so an updated\npatch from you will likely conflict with their effort.\n\nSo if I were you, I'd wait to see what other people would say about\nthe actual exit codes we give when \"git grep -L\" is run without the\n\"--quiet\" option, and if they are also happy with the current exit\ncode, then send in an updated patch.\n\nThanks.\n\n\n"},{"id":"326465","messageId":"CA+dzEB=ggLQGaEL21TChLkeiAvCGf4LZ0QFevBZCAEMhxyJwqQ@mail.gmail.com","threadId":"46596","inReplyTo":"xmqqlgmkpizm.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH/RFC] git-grep: correct exit code with --quiet and -L","fromName":"Anthony Sottile","fromEmail":"asottile@umich.edu","sentAt":"2017-08-15T22:43:51Z","receivedAt":"2017-08-15T22:43:57Z","isPatch":true,"sender":{"key":"asottile@umich.edu","avatar":"https://avatars.githubusercontent.com/u/1810591?v=4"},"body":"Sounds good, I'll wait.\n\nI've also created a mailing list entry on the gnu grep mailing list as\nI believe the current exit status for --files-without-match is\ninconsistent:\n\nhttp://lists.gnu.org/archive/html/bug-grep/2017-08/msg00010.html\n\nAnthony\n\nOn Tue, Aug 15, 2017 at 3:24 PM, Junio C Hamano wrote:\n> Anthony Sottile writes:\n>\n>> Ah yes, I didn't intend to include the first hunk (forgot to amend it\n>> out when formatting the patch).\n>>\n>> I think git's exit codes for -L actually make more sense than the GNU\n>> exit codes (especially when comparing with `grep` vs `grep -v`) --\n>> that is, produce `0` when the search is successful (producing\n>> *something* on stdout) and `1` when the search fails.\n>>\n>> Shall I create a new mail with the adjusted patch as suggested above?\n>\n> I do not mind seeing an updated patch that does not change the exit\n> status (as you seem to like what we have currently that contradicts\n> what GNU grep does) but makes it consistent between \"--quiet\" and\n> \"--no-quiet\". But I would not be surprised if people seeing this\n> exchange from the sideline are already working on fixing the exit\n> status and also making sure that the fixed code would produce the\n> same corrected exit status with or without \"--quiet\", so an updated\n> patch from you will likely conflict with their effort.\n>\n> So if I were you, I'd wait to see what other people would say about\n> the actual exit codes we give when \"git grep -L\" is run without the\n> \"--quiet\" option, and if they are also happy with the current exit\n> code, then send in an updated patch.\n>\n> Thanks.\n>\n>\n"}]}