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

From: Junio C Hamano <gitster@pobox.com>
Date: May 3, 2021, 03:09 UTC
Message-ID: <xmqqo8dsa4y8.fsf@gitster.g>
In-Reply-To: <CY4P132MB00883DC8ABC72790A15C9630855E9@CY4P132MB0088.NAMP132.PROD.OUTLOOK.COM>

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

Show 21 quoted lines

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

Show 9 quoted lines

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

Show 5 quoted lines

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