git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Jeremy Faith <jeremy.faith@jci.com>
Cc: "git@vger.kernel.org" <git@vger.kernel.org>
Subject: Re: bug:git-check-ignore exit status is wrong for negative patterns when -v option used
Date: Mon, 03 May 2021 12:09:35 +0900	[thread overview]
Message-ID: <xmqqo8dsa4y8.fsf@gitster.g> (raw)
In-Reply-To: <CY4P132MB00883DC8ABC72790A15C9630855E9@CY4P132MB0088.NAMP132.PROD.OUTLOOK.COM> (Jeremy Faith's message of "Fri, 30 Apr 2021 09:56:11 +0000")

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.





      parent reply	other threads:[~2021-05-03  3:09 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-04-27 16:30 bug:git-check-ignore exit status is wrong for negative patterns when -v option used Jeremy Faith
2021-04-28  6:15 ` Bagas Sanjaya
2021-04-29 14:26   ` Jeremy Faith
2021-04-28  7:13 ` Junio C Hamano
2021-04-29 14:10   ` Jeremy Faith
2021-04-30  0:22     ` Junio C Hamano
2021-04-30  9:56       ` Jeremy Faith
2021-04-30 20:51         ` Elijah Newren
2021-05-03  3:09         ` Junio C Hamano [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqqo8dsa4y8.fsf@gitster.g \
    --to=gitster@pobox.com \
    --cc=git@vger.kernel.org \
    --cc=jeremy.faith@jci.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).