Re: bug:git-check-ignore exit status is wrong for negative patterns when -v option used

[Elijah Newren <newren@gmail.com>]

On Fri, Apr 30, 2021 at 2:57 AM Jeremy Faith <jeremy.faith@jci.com> wrote:
>
> On 30 April 2021 01:22 Junio C Hamano <gitster@pobox.com> wrote:
>
> >Jeremy Faith <jeremy.faith@jci.com> writes:
> >
> >> man git-check-ignore states:-
> >> EXIT STATUS
> >> -----------
> >> 0::
> >>        One or more of the provided paths is ignored.
> >> 1::
> >>        None of the provided paths are ignored.
> >> 128::
> >>        A fatal error was encountered.
> >>
> >> So my change matches what the manual states.
>
> >That is part of what I meant by "understandable, given the history".
>
> >The above description is _wrong_ ever since it was introduced, along
> >with check-ignore, at 368aa529 (add git-check-ignore sub-command,
> >2013-01-06).  It should have said "has/have entry that affects it in
> >the gitignore/exclude files" instead of "is/are ignored".  After
> >all, that is what the tool was written to do, i.e. to help debugging
> >the gitignore/exclude files.
> >
> >    git-check-ignore(1)
> >    ===================
> >
> >    NAME
> >    ----
> >    git-check-ignore - Debug gitignore / exclude files
> >
> >Having said all that.
> >
> >It is a misunderstanding that check-ignore is a tool to learn if a
> >path is or is not ignored, but the misunderstanding is so widespread.
> >
> >I wonder if we can repurpose the command to "match" the
> >misunderstanding, without hurting existing users, by
> >
> > (1) updating the one-liner description of the command in the
> >     documentation;
>
> > (2) keep the EXIT STATUS section as-is; and
> >
> > (3) adjust the code to exit with status that reflects if there was
> >     at least one path that was ignored (not "that had an entry in
> >     the gitignore/exclude files that affected its fate").
>
> If I understand correctly:-
>   (2) requires no change
>   (3) I believe my one line change does this
>
> Which just leaves (1) where current line is
>    git-check-ignore - Debug gitignore / exclude files
> I think with the exit status operating as documented this description still
> works i.e. check-ignore can be used to test if the .gitignore/exclude files
> are working as desired.
>
> The longer Description is:
>   DESCRIPTION
>        For each pathname given via the command-line or from a file via
>        --stdin, check whether the file is excluded by .gitignore (or other
>        input files to the exclude mechanism) and output the path if it is
>        excluded.
> Which matches the exit status meaning as is.
>
> >That certainly is a backward compatible change, but I suspect that
> >we may be able to sell it as a bugfix, taking advantage of the
> >documentation bug you quoted above.  Of course, people do not read
> >documentation, so scripts that used to use the command in the way it
> >was intended to be used (as opposed to "the way it was documented")
> >will still get broken with such a change, though.
>
> I'm not sure how the old exit status could be used in a useful way but you are
> correct there is a chance that some existing scripts depend on it.
>
> I was originally confused by the exit status when using git versions 1.8.3.1
> and 2.25.1. With these versions check-ignore returned 0 when a matching
> pattern was found, it did not matter if it was a positive or negative pattern.
> This did not match the exit status documented in the man page so I thought my
> .gitignore patterns were not working when they were.
> Perhaps I should stop reading man pages...
>
> I then tried the same patterns on an up to date version of git built from a
> a git clone and found that without -v it returned the exit status as the man
> page states but with -v the older exit status was given.
> The commit that changed the exit status was 7ec8125 from 2020-02-18, first
> release that included this commit was 2.26.
>
> So in 2.26 the exit status without -v was changed, maybe accidentally, in a
> way that made it match the man page. But this commit broke scripts(that
> do not use -v) that depend on exit status. My change extends the breakage to
> scripts that do use -v. On the other hand scripts written that expect the
> exit status to match the man page will be fixed!
>
> I'm not sure what the best course of action is but at least with my change
> the exit status matches the man page(even with -v).

I'm inclined to agree with Jeremy here.  Commit  7ec8125fba96
(check-ignore: fix documentation and implementation to match,
2020-02-18) demonstrated pretty clearly that check-ignore was just
flat-out broken since the beginning in regards to negated patterns, in
multiple ways: wrong exit status, documentation that was
self-contradictory, and non-matching documentation and implementation.
Among other things, that commit from last year fixed the exit status
without -v.  Unfortunately, when I (or the users I was interacting
with) use -v, I ignore the exit status and am just looking for the
output.  So I missed the exit status inconsistency for -v when I
created that commit and only fixed the exit status without -v.