Re: [PATCH/RFC] git-grep: correct exit code with --quiet and -L

> Anthony Sottile <asottile@umich.edu> writes:
>
>> The handling of `status_only` no longer interferes with the handling of
>> `unmatch_name_only`.  `--quiet` no longer affects the exit code when using
>> `-L`/`--files-without-match`.
>
> I agree with the above statement of yours that --quiet should not
> affect what the exit status is.
>
> But I am not sure what the exit code from these commands _should_
> be:
>
>     $ git grep -L qfwfq \*.h    ;# no file matches
>     $ git grep -L \# \*.h       ;# some but not all files match
>     $ git grep -L . \*.h        ;# all files match
>
> with or without --quiet.  I seem to get 0, 0, 1, which I am not sure
> is correct.  I do recall writing "git grep" _without_ thinking what
> the exit code should be when we added --files-without-match, so the
> exit status the current code gives out may be just a random garbage.
>
> Asking GNU grep (because --files-without-match is not a POSIX thing):
>
>     $ grep -L qfwfq *.h          ;# no file matches
>     $ grep -L \# *.h             ;# some but not all files match
>     $ grep -L . *.h              ;# all files match
>
> I seem to get 1, 0, 0.  So the exit status should reflect if there
> was _any_ hit from any file that were inspected.
>
>> @@ -1755,7 +1755,7 @@ static int grep_source_1(struct grep_opt *opt, struct grep_source *gs, int colle
>>               }
>>               if (hit) {
>>                       count++;
>> -                     if (opt->status_only)
>> +                     if (!opt->unmatch_name_only && opt->status_only)
>>                               return 1;
>>                       if (opt->name_only) {
>>                               show_name(opt, gs->name);
>
> Does the change in this hunk have any effect?
>
> Just before this hunk there is this code:
>
>                 /* "grep -v -e foo -e bla" should list lines
>                  * that do not have either, so inversion should
>                  * be done outside.
>                  */
>                 if (opt->invert)
>                         hit = !hit;
>                 if (opt->unmatch_name_only) {
>                         if (hit)
>                                 return 0;
>                         goto next_line;
>
> If (opt->unmatch_name_only && hit) then the function would have
> already returned and the control wouldn't have reached here.
>
> Which would mean that when the control reaches the line this hunk
> touches, either one of these must be false, and because we are
> inside "if (hit)", opt->unmatch_name_only must be false.
>
>> @@ -1820,13 +1820,14 @@ static int grep_source_1(struct grep_opt *opt, struct grep_source *gs, int colle
>>       if (collect_hits)
>>               return 0;
>>
>> -     if (opt->status_only)
>> -             return 0;
>>       if (opt->unmatch_name_only) {
>>               /* We did not see any hit, so we want to show this */
>> -             show_name(opt, gs->name);
>> +             if (!opt->status_only)
>> +                     show_name(opt, gs->name);
>>               return 1;
>>       }
>> +     if (opt->status_only)
>> +             return 0;
>
> This hunk makes sense to me (provided if the semantics we want out
> of --files-without-match is sensible, which is dubious), even though
> I would have limited the change to just a single line, i.e.
>
>         if (opt->status_only)
> -               return 0;
> +               return opt->unmatch_name_only;
>         if (opt->unmatch_name_only) {
>                 /* We did not see any hit, ... */
>
> But I suspect we want to fix the exit code not to be affected by
> the "--files-without-match" option in the first place, so all the
> code changes we see in this patch might be moot X-<.
>
>