Ah yes, I didn't intend to include the first hunk (forgot to amend it
out when formatting the patch).
I think git's exit codes for -L actually make more sense than the GNU
exit codes (especially when comparing with `grep` vs `grep -v`) --
that is, produce `0` when the search is successful (producing
*something* on stdout) and `1` when the search fails.
Shall I create a new mail with the adjusted patch as suggested above?
(I'm not familiar with the expected workflow).
Anthony
On Tue, Aug 15, 2017 at 2:33 PM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> Anthony Sottile <asottile@xxxxxxxxx> 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-<.
>
>
