check-ignore: fix handling with negated patterns

Commit Message

Johannes Schindelin via GitGitGadget Feb. 17, 2020, 4:15 p.m. UTC

From: Elijah Newren <[e-mail]>

check-ignore was meant to check ignore rules the same way git status and
other commands would, and to report whether a path is excluded.  It
failed to do this (and generated a few bug reports), however, because it
did not account for negated patterns.

Commands other than check-ignore verify exclusion rules via calling

   ... -> treat_one_path() -> is_excluded() -> last_matching_pattern()

while check-ignore has a call path of the form:

   ... -> check_ignore()                    -> last_matching_pattern()

The fact that the latter does not include the call to is_excluded()
means that it is susceptible to to messing up negated patterns (since
that is the only significant thing is_excluded() adds over
last_matching_pattern()).  Unfortunately, we can't make it just call
is_excluded(), because is_excluded doesn't return the pattern in
question and part of check-ignore's functionality is not just checking
whether one of the patterns matches but returning which one does.

Further, check_ignore() is supposed to handle a --verbose mode, which
was ill-defined for the case of negated patterns: check-ignore was
documented to print just the excluded paths, whereas the --verbose mode
was there to document which patterns were matched by paths.  A path
which matches a negated exclude pattern is NOT excluded and thus
shouldn't be printed by the former logic, while it certainly does match
one of the explicit patterns and thus should be printed by the latter
logic.  Adjust the definition of --verbose to state that it is about
matching patterns INSTEAD of about showing which paths are excluded in
order to resolve this discrepancy.

Finally, also adjust a few tests in t0008 that were caught up by this
discrepancy in how negated paths were handled.

Signed-off-by: Elijah Newren <[e-mail]>
---
    check-ignore: fix handling with negated patterns
    
    Make check-ignore do what it claims to do: report whether the paths
    specified to it are ignored.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-711%2Fnewren%2Ffix-check-ignore-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-711/newren/fix-check-ignore-v1
Pull-Request: https://github.com/git/git/pull/711

 Documentation/git-check-ignore.txt | 12 ++++++---
 builtin/check-ignore.c             |  3 +++
 t/t0008-ignores.sh                 | 39 ++++++++++++++++++------------
 3 files changed, 35 insertions(+), 19 deletions(-)


base-commit: bfdd66e72fffd18235757bedbf355fd4176d6019

Comments

Junio C Hamano Feb. 17, 2020, 6:04 p.m. UTC | #1

"Elijah Newren via GitGitGadget" <[e-mail]> writes:

> From: Elijah Newren <[e-mail]>
>
> check-ignore was meant to check ignore rules the same way git status and
> other commands would, and to report whether a path is excluded.  It
> failed to do this (and generated a few bug reports), however, because it
> did not account for negated patterns.

I suspect that the above distorts history.  IIRC, it was meant as a
tool to see which exact pattern in the exclude sequence had the
final say for the given needle, written primarily as a debugging
aid.  In that context, "This rule has the final say", whether the
rule is a negative or positive, still means something.

It is just the behavior is _much_ less useful for those who want to
know what the final say is, and I tend to agree that we probably are
better off changing its output to reflect "so, are we ignoring the
path after all? yes/no?" because we are pretty much done with
debugging the exclude API implementation.

Elijah Newren Feb. 17, 2020, 6:41 p.m. UTC | #2

On Mon, Feb 17, 2020 at 10:05 AM Junio C Hamano <[e-mail]> wrote:
>
> "Elijah Newren via GitGitGadget" <[e-mail]> writes:
>
> > From: Elijah Newren <[e-mail]>
> >
> > check-ignore was meant to check ignore rules the same way git status and
> > other commands would, and to report whether a path is excluded.  It
> > failed to do this (and generated a few bug reports), however, because it
> > did not account for negated patterns.
>
> I suspect that the above distorts history.  IIRC, it was meant as a
> tool to see which exact pattern in the exclude sequence had the
> final say for the given needle, written primarily as a debugging
> aid.  In that context, "This rule has the final say", whether the
> rule is a negative or positive, still means something.

I can reword it; how does the following sound?

check-ignore claims that it reports whether each path it is given is
excluded.  However, it fails to do so because it did not account for
negated patterns.


Also, I think the "This rule has the final say" functionality of the
tool might still be useful, so I kept it -- see my updates to the
--verbose flag (mentioned later in the commit message).

> It is just the behavior is _much_ less useful for those who want to
> know what the final say is, and I tend to agree that we probably are
> better off changing its output to reflect "so, are we ignoring the
> path after all? yes/no?" because we are pretty much done with
> debugging the exclude API implementation.

Junio C Hamano Feb. 17, 2020, 8:41 p.m. UTC | #3

Elijah Newren <[e-mail]> writes:

>> I suspect that the above distorts history.  IIRC, it was meant as a
>> tool to see which exact pattern in the exclude sequence had the
>> final say for the given needle, written primarily as a debugging
>> aid.  In that context, "This rule has the final say", whether the
>> rule is a negative or positive, still means something.
>
> I can reword it; how does the following sound?
>
> check-ignore claims that it reports whether each path it is given is
> excluded.  However, it fails to do so because it did not account for
> negated patterns.

I am not sure about "claims" part.  

Isn't it more like "check-ignore has been the tool that reports the
rule that has final say on each of the paths it is given, but that
is not very useful when the user wants to see if the path is
excluded (e.g. the rule with the final say may be negative).  Let's
change the behaviour so that it reports if the path is excluded or
not"?  As I said, I tend to agree with the direction your patch
wants to go (iow, we probably are better off changing the
behaviour"); the question is if we want a transition plan and how
extensive it needs be if we do.

Elijah Newren Feb. 17, 2020, 9:07 p.m. UTC | #4

On Mon, Feb 17, 2020 at 12:41 PM Junio C Hamano <[e-mail]> wrote:
>
> Elijah Newren <[e-mail]> writes:
>
> >> I suspect that the above distorts history.  IIRC, it was meant as a
> >> tool to see which exact pattern in the exclude sequence had the
> >> final say for the given needle, written primarily as a debugging
> >> aid.  In that context, "This rule has the final say", whether the
> >> rule is a negative or positive, still means something.
> >
> > I can reword it; how does the following sound?
> >
> > check-ignore claims that it reports whether each path it is given is
> > excluded.  However, it fails to do so because it did not account for
> > negated patterns.
>
> I am not sure about "claims" part.
>
> Isn't it more like "check-ignore has been the tool that reports the
> rule that has final say on each of the paths it is given, but that
> is not very useful when the user wants to see if the path is
> excluded (e.g. the rule with the final say may be negative).

No, it is not more like that; the check-ignore manpage currently claims this:

       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.

Note also that this description at the beginning of the manpage says
nothing about reporting which rule has the final say.  And, in fact,
the command in default mode does not report which rule or rules were
involved.  All of that work falls to the --verbose flag, which was
documented as

           Also output details about the matching pattern (if any) for
each given pathname. For precedence rules within and between
           exclude sources, see gitignore(5).

Now, if you read both descriptions together, you find that these
claims are contradictory and that it cannot do both, so the "Also" bit
it leads with is a lie.  As such, my commit modified the definition of
verbose to make it instead read:

       Instead of printing the paths that are excluded, for each path
       that matches an exclude pattern print the exclude pattern
       together with the path.  (Matching an exclude pattern usually
       means the path is excluded, but if the pattern begins with '!'
       then it is a negated pattern and matching it means the path is
       NOT excluded.)

This was a change of description for the --verbose flag, not a change
of implementation.  Thus, in my opinion, no transition period is
needed: those who wanted to use check-ignore to see what rule would
have matched had to use --verbose before, and --verbose behaves the
same as before.

Those who wanted to use check-ignore without the --verbose flag to see
if a rule is excluded, get corrected behavior that will actually do
that.

Junio C Hamano Feb. 19, 2020, 9:36 p.m. UTC | #5

Elijah Newren <[e-mail]> writes:

> No, it is not more like that; the check-ignore manpage currently claims this:
>
>        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.

Thanks.  I wasn't paying attention to what happened to the manpage
(or to the command for that matter) after I wrote it as a debugging
aid X-<.

The updated version looked reasonable to me.  Thanks.

Patch

diff --git a/Documentation/git-check-ignore.txt b/Documentation/git-check-ignore.txt
index 8b2d49c79e1..85ef46e2eff 100644
--- a/Documentation/git-check-ignore.txt
+++ b/Documentation/git-check-ignore.txt
@@ -30,9 +30,15 @@  OPTIONS
 	valid with a single pathname.
 
 -v, --verbose::
-	Also output details about the matching pattern (if any)
-	for each given pathname. For precedence rules within and
-	between exclude sources, see linkgit:gitignore[5].
+	Instead of printing the paths that are excluded, for each path
+	that matches an exclude pattern print the exclude pattern
+	together with the path.  (Matching an exclude pattern usually
+	means the path is excluded, but if the pattern begins with '!'
+	then it is a negated pattern and matching it means the path is
+	NOT excluded.)
++
+For precedence rules within and between exclude sources, see
+linkgit:gitignore[5].
 
 --stdin::
 	Read pathnames from the standard input, one per line,
diff --git a/builtin/check-ignore.c b/builtin/check-ignore.c
index 5a4f92395f3..ea5d0ae3a6a 100644
--- a/builtin/check-ignore.c
+++ b/builtin/check-ignore.c
@@ -108,6 +108,9 @@  static int check_ignore(struct dir_struct *dir,
 			int dtype = DT_UNKNOWN;
 			pattern = last_matching_pattern(dir, &the_index,
 							full_path, &dtype);
+			if (!verbose && pattern &&
+			    pattern->flags & PATTERN_FLAG_NEGATIVE)
+				pattern = NULL;
 		}
 		if (!quiet && (pattern || show_non_matching))
 			output_pattern(pathspec.items[i].original, pattern);
diff --git a/t/t0008-ignores.sh b/t/t0008-ignores.sh
index 1744cee5e99..370a389e5c5 100755
--- a/t/t0008-ignores.sh
+++ b/t/t0008-ignores.sh
@@ -424,9 +424,24 @@  test_expect_success 'local ignore inside a sub-directory with --verbose' '
 	)
 '
 
-test_expect_success_multi 'nested include' \
-	'a/b/.gitignore:8:!on*	a/b/one' '
-	test_check_ignore "a/b/one"
+test_expect_success 'nested include of negated pattern' '
+	expect "" &&
+	test_check_ignore "a/b/one" 1
+'
+
+test_expect_success 'nested include of negated pattern with -q' '
+	expect "" &&
+	test_check_ignore "-q a/b/one" 1
+'
+
+test_expect_success 'nested include of negated pattern with -v' '
+	expect "a/b/.gitignore:8:!on*	a/b/one" &&
+	test_check_ignore "-v a/b/one" 0
+'
+
+test_expect_success 'nested include of negated pattern with -v -n' '
+	expect "a/b/.gitignore:8:!on*	a/b/one" &&
+	test_check_ignore "-v -n a/b/one" 0
 '
 
 ############################################################################
@@ -460,7 +475,6 @@  test_expect_success 'cd to ignored sub-directory' '
 	expect_from_stdin <<-\EOF &&
 		foo
 		twoooo
-		../one
 		seven
 		../../one
 	EOF
@@ -543,7 +557,6 @@  test_expect_success 'global ignore' '
 		globalthree
 		a/globalthree
 		a/per-repo
-		globaltwo
 	EOF
 	test_check_ignore "globalone per-repo globalthree a/globalthree a/per-repo not-ignored globaltwo"
 '
@@ -586,17 +599,7 @@  EOF
 cat <<-\EOF >expected-default
 	one
 	a/one
-	a/b/on
-	a/b/one
-	a/b/one one
-	a/b/one two
-	"a/b/one\"three"
-	a/b/two
 	a/b/twooo
-	globaltwo
-	a/globaltwo
-	a/b/globaltwo
-	b/globaltwo
 EOF
 cat <<-EOF >expected-verbose
 	.gitignore:1:one	one
@@ -696,8 +699,12 @@  cat <<-EOF >expected-all
 	$global_excludes:2:!globaltwo	../b/globaltwo
 	::	c/not-ignored
 EOF
+cat <<-EOF >expected-default
+../one
+one
+b/twooo
+EOF
 grep -v '^::	' expected-all >expected-verbose
-sed -e 's/.*	//' expected-verbose >expected-default
 
 broken_c_unquote stdin >stdin0