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

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

[Jeremy Faith]

Hi,

git version 2.31.1.362.g311531c9de
git-check-ignore 
When a negative pattern is the last .gitignore match the -v option causes the exit status to be 0 rather than the expected 1.
e.g say .gitignore contains  one line: !hello
git check-ignore hello #outputs nothing
echo $?  #shows correct exit status=1 i.e None of the provided paths are ignored.
but
git check-ignore -v hello #output is next line
.gitignore:4:!hello	hello
echo $?  #shows wrong exit status=0 i.e. One or more of the provided paths is ignored

Following change seems to fix it for me
--- a/builtin/check-ignore.c
+++ b/builtin/check-ignore.c
@@ -114,7 +114,7 @@ static int check_ignore(struct dir_struct *dir,
                }
                if (!quiet && (pattern || show_non_matching))
                        output_pattern(pathspec.items[i].original, pattern);
-               if (pattern)
+               if (pattern && !(pattern->flags & PATTERN_FLAG_NEGATIVE))
                        num_ignored++;
        }
        free(seen);

Regards,
Jeremy Faith

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

[Bagas Sanjaya]

On 27/04/21 23.30, Jeremy Faith wrote:
> Hi,
> 
> git version 2.31.1.362.g311531c9de
> git-check-ignore
> When a negative pattern is the last .gitignore match the -v option causes the exit status to be 0 rather than the expected 1.
> e.g say .gitignore contains  one line: !hello
> git check-ignore hello #outputs nothing
> echo $?  #shows correct exit status=1 i.e None of the provided paths are ignored.
> but
> git check-ignore -v hello #output is next line
> .gitignore:4:!hello	hello
> echo $?  #shows wrong exit status=0 i.e. One or more of the provided paths is ignored
> 
> Following change seems to fix it for me
> --- a/builtin/check-ignore.c
> +++ b/builtin/check-ignore.c
> @@ -114,7 +114,7 @@ static int check_ignore(struct dir_struct *dir,
>                  }
>                  if (!quiet && (pattern || show_non_matching))
>                          output_pattern(pathspec.items[i].original, pattern);
> -               if (pattern)
> +               if (pattern && !(pattern->flags & PATTERN_FLAG_NEGATIVE))
>                          num_ignored++;
>          }
>          free(seen);
> 
I tried to apply this patch from the mbox, but it was corrupted, so I had to
manually write the changes above. I tested that with your reproduction case and
it worked as expected.

But anyway, please send the proper patch and (preferentially) with the test
consisting of the reproduction case.

Thanks.

-- 
An old man doll... just what I always wanted! - Clara

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

[Jeremy Faith]

on 28 April 2021 07:15 Bagas Sanjaya <bagasdotme@gmail.com> wrote:
 
>On 27/04/21 23.30, Jeremy Faith wrote:
>> Hi,
>> 
>> git version 2.31.1.362.g311531c9de
>> git-check-ignore
>> When a negative pattern is the last .gitignore match the -v option causes the exit status to be 0 rather than the expected 1.
>> e.g say .gitignore contains  one line: !hello
>> git check-ignore hello #outputs nothing
>> echo $?  #shows correct exit status=1 i.e None of the provided paths are ignored.
>> but
>> git check-ignore -v hello #output is next line
>> .gitignore:4:!hello   hello
>> echo $?  #shows wrong exit status=0 i.e. One or more of the provided paths is ignored
>> 
>> Following change seems to fix it for me
>> --- a/builtin/check-ignore.c
>> +++ b/builtin/check-ignore.c
>> @@ -114,7 +114,7 @@ static int check_ignore(struct dir_struct *dir,
>>                  }
>>                  if (!quiet && (pattern || show_non_matching))
>>                          output_pattern(pathspec.items[i].original, pattern);
>> -               if (pattern)
>> +               if (pattern && !(pattern->flags & PATTERN_FLAG_NEGATIVE))
>>                          num_ignored++;
>>          }
>>          free(seen);
 
>I tried to apply this patch from the mbox, but it was corrupted, so I had to
>manually write the changes above. I tested that with your reproduction case and
>it worked as expected.

>But anyway, please send the proper patch and (preferentially) with the test
>consisting of the reproduction case.

>Thanks.

Sorry I just posted the output of 'git diff' previously but I cut off the first 2 lines by mistake.
I'm not sure what is required as a test script.

diff --git a/builtin/check-ignore.c b/builtin/check-ignore.c
index 3c652748d5..223cc6a1ef 100644
--- a/builtin/check-ignore.c
+++ b/builtin/check-ignore.c
@@ -114,7 +114,7 @@ static int check_ignore(struct dir_struct *dir,
                }
                if (!quiet && (pattern || show_non_matching))
                        output_pattern(pathspec.items[i].original, pattern);
-               if (pattern)
+               if (pattern && !(pattern->flags & PATTERN_FLAG_NEGATIVE))
                        num_ignored++;
        }
        free(seen);

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

[Junio C Hamano]

Jeremy Faith <jeremy.faith@jci.com> writes:

> git version 2.31.1.362.g311531c9de
> git-check-ignore 
> When a negative pattern is the last .gitignore match the -v option causes the exit status to be 0 rather than the expected 1.
> e.g say .gitignore contains  one line: !hello
> git check-ignore hello #outputs nothing
> echo $?  #shows correct exit status=1 i.e None of the provided paths are ignored.
> but
> git check-ignore -v hello #output is next line
> .gitignore:4:!hello	hello
> echo $?  #shows wrong exit status=0 i.e. One or more of the provided paths is ignored

Hmph.  This is kind of understandable given the history of the
command, which was *not* about programatically ask "is this path
ignored?" question at all.  Instead, it was invented to answer this
question: I am puzzled by the fact that Git considers this path is
to be ignored (or "not to be ignored").  Show me which entry in what
exclude file made the final decision to ignore (or "not to ignore")
it to help me debug my ignore file(s).

And the exit code was to signal "yes, I found a relevant entry",
which made sense for the tool's nature as a debugging aid.

So, I suspect that this is working as designed/intended.  I agree
that it is debatable that the way it was designed to work is a good
one, though.

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

[Jeremy Faith]

Junio C Hamano <gitster@pobox.com> wrote:
 
>Jeremy Faith <jeremy.faith@jci.com> writes:

>> git version 2.31.1.362.g311531c9de
>> git-check-ignore 
>> When a negative pattern is the last .gitignore match the -v option causes the exit status to be 0 rather than the expected 1.
>> e.g say .gitignore contains  one line: !hello
>> git check-ignore hello #outputs nothing
>> echo $?  #shows correct exit status=1 i.e None of the provided paths are ignored.
>> but
>> git check-ignore -v hello #output is next line
>> .gitignore:4:!hello   hello
>> echo $?  #shows wrong exit status=0 i.e. One or more of the provided paths is ignored

>Hmph.  This is kind of understandable given the history of the
>command, which was *not* about programatically ask "is this path
>ignored?" question at all.  Instead, it was invented to answer this
>question: I am puzzled by the fact that Git considers this path is
>to be ignored (or "not to be ignored").  Show me which entry in what
>exclude file made the final decision to ignore (or "not to ignore")
>it to help me debug my ignore file(s).

>And the exit code was to signal "yes, I found a relevant entry",
>which made sense for the tool's nature as a debugging aid.

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.

>So, I suspect that this is working as designed/intended.  I agree
>that it is debatable that the way it was designed to work is a good
>one, though.

I doubt that changing the exit status when -v is added is intended behaviour.

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

[Junio C Hamano]

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").

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.

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

[Jeremy Faith]

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).

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

[Elijah Newren]

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.

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

[Junio C Hamano]

Jeremy Faith <jeremy.faith@jci.com> writes:

>>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.

As an end-user facing command, we'd probably want to align this more
with the description of check-attr.  I think that the description as
you quoted is OK 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 have a suspicion that the existing tests may depend on the current
"we found a match" semantics---they may not count as "existing
scripts".  We'd need to update them if that is the case.

> 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.

Yup, that is understandable as the manpage was utterly wrong, and
its description was so plausible that nobody noticed.

As I already said, I think it is OK to sell this change as a bugfix
to align implementation with the documentation wrt negative results;
in addition to the 3 item list quoted at the top, we might need to
adjust tests (or add tests if we are not covering the "not excluded
because we explicitly have a negative entry" case).

Thanks.