{"thread":{"id":"52827","subject":"[PATCH] check-ignore: fix handling with negated patterns","startedAt":"2020-02-17T16:15:11Z","lastAt":"2020-02-19T21:36:48Z","messageCount":9,"participants":["Elijah Newren via GitGitGadget","Junio C Hamano","Elijah Newren"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"391921","messageId":"pull.711.git.git.1581956106255.gitgitgadget@gmail.com","threadId":"52827","inReplyTo":null,"subject":"[PATCH] check-ignore: fix handling with negated patterns","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-02-17T16:15:06Z","receivedAt":"2020-02-17T16:15:11Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren <newren@gmail.com>\n\ncheck-ignore was meant to check ignore rules the same way git status and\nother commands would, and to report whether a path is excluded.  It\nfailed to do this (and generated a few bug reports), however, because it\ndid not account for negated patterns.\n\nCommands other than check-ignore verify exclusion rules via calling\n\n   ... -> treat_one_path() -> is_excluded() -> last_matching_pattern()\n\nwhile check-ignore has a call path of the form:\n\n   ... -> check_ignore()                    -> last_matching_pattern()\n\nThe fact that the latter does not include the call to is_excluded()\nmeans that it is susceptible to to messing up negated patterns (since\nthat is the only significant thing is_excluded() adds over\nlast_matching_pattern()).  Unfortunately, we can't make it just call\nis_excluded(), because is_excluded doesn't return the pattern in\nquestion and part of check-ignore's functionality is not just checking\nwhether one of the patterns matches but returning which one does.\n\nFurther, check_ignore() is supposed to handle a --verbose mode, which\nwas ill-defined for the case of negated patterns: check-ignore was\ndocumented to print just the excluded paths, whereas the --verbose mode\nwas there to document which patterns were matched by paths.  A path\nwhich matches a negated exclude pattern is NOT excluded and thus\nshouldn't be printed by the former logic, while it certainly does match\none of the explicit patterns and thus should be printed by the latter\nlogic.  Adjust the definition of --verbose to state that it is about\nmatching patterns INSTEAD of about showing which paths are excluded in\norder to resolve this discrepancy.\n\nFinally, also adjust a few tests in t0008 that were caught up by this\ndiscrepancy in how negated paths were handled.\n\nSigned-off-by: Elijah Newren <newren@gmail.com>\n---\n    check-ignore: fix handling with negated patterns\n    \n    Make check-ignore do what it claims to do: report whether the paths\n    specified to it are ignored.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-711%2Fnewren%2Ffix-check-ignore-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-711/newren/fix-check-ignore-v1\nPull-Request: https://github.com/git/git/pull/711\n\n Documentation/git-check-ignore.txt | 12 ++++++---\n builtin/check-ignore.c             |  3 +++\n t/t0008-ignores.sh                 | 39 ++++++++++++++++++------------\n 3 files changed, 35 insertions(+), 19 deletions(-)\n\ndiff --git a/Documentation/git-check-ignore.txt b/Documentation/git-check-ignore.txt\nindex 8b2d49c79e1..85ef46e2eff 100644\n--- a/Documentation/git-check-ignore.txt\n+++ b/Documentation/git-check-ignore.txt\n@@ -30,9 +30,15 @@ OPTIONS\n \tvalid with a single pathname.\n \n -v, --verbose::\n-\tAlso output details about the matching pattern (if any)\n-\tfor each given pathname. For precedence rules within and\n-\tbetween exclude sources, see linkgit:gitignore[5].\n+\tInstead of printing the paths that are excluded, for each path\n+\tthat matches an exclude pattern print the exclude pattern\n+\ttogether with the path.  (Matching an exclude pattern usually\n+\tmeans the path is excluded, but if the pattern begins with '!'\n+\tthen it is a negated pattern and matching it means the path is\n+\tNOT excluded.)\n++\n+For precedence rules within and between exclude sources, see\n+linkgit:gitignore[5].\n \n --stdin::\n \tRead pathnames from the standard input, one per line,\ndiff --git a/builtin/check-ignore.c b/builtin/check-ignore.c\nindex 5a4f92395f3..ea5d0ae3a6a 100644\n--- a/builtin/check-ignore.c\n+++ b/builtin/check-ignore.c\n@@ -108,6 +108,9 @@ static int check_ignore(struct dir_struct *dir,\n \t\t\tint dtype = DT_UNKNOWN;\n \t\t\tpattern = last_matching_pattern(dir, &the_index,\n \t\t\t\t\t\t\tfull_path, &dtype);\n+\t\t\tif (!verbose && pattern &&\n+\t\t\t    pattern->flags & PATTERN_FLAG_NEGATIVE)\n+\t\t\t\tpattern = NULL;\n \t\t}\n \t\tif (!quiet && (pattern || show_non_matching))\n \t\t\toutput_pattern(pathspec.items[i].original, pattern);\ndiff --git a/t/t0008-ignores.sh b/t/t0008-ignores.sh\nindex 1744cee5e99..370a389e5c5 100755\n--- a/t/t0008-ignores.sh\n+++ b/t/t0008-ignores.sh\n@@ -424,9 +424,24 @@ test_expect_success 'local ignore inside a sub-directory with --verbose' '\n \t)\n '\n \n-test_expect_success_multi 'nested include' \\\n-\t'a/b/.gitignore:8:!on*\ta/b/one' '\n-\ttest_check_ignore \"a/b/one\"\n+test_expect_success 'nested include of negated pattern' '\n+\texpect \"\" &&\n+\ttest_check_ignore \"a/b/one\" 1\n+'\n+\n+test_expect_success 'nested include of negated pattern with -q' '\n+\texpect \"\" &&\n+\ttest_check_ignore \"-q a/b/one\" 1\n+'\n+\n+test_expect_success 'nested include of negated pattern with -v' '\n+\texpect \"a/b/.gitignore:8:!on*\ta/b/one\" &&\n+\ttest_check_ignore \"-v a/b/one\" 0\n+'\n+\n+test_expect_success 'nested include of negated pattern with -v -n' '\n+\texpect \"a/b/.gitignore:8:!on*\ta/b/one\" &&\n+\ttest_check_ignore \"-v -n a/b/one\" 0\n '\n \n ############################################################################\n@@ -460,7 +475,6 @@ test_expect_success 'cd to ignored sub-directory' '\n \texpect_from_stdin <<-\\EOF &&\n \t\tfoo\n \t\ttwoooo\n-\t\t../one\n \t\tseven\n \t\t../../one\n \tEOF\n@@ -543,7 +557,6 @@ test_expect_success 'global ignore' '\n \t\tglobalthree\n \t\ta/globalthree\n \t\ta/per-repo\n-\t\tglobaltwo\n \tEOF\n \ttest_check_ignore \"globalone per-repo globalthree a/globalthree a/per-repo not-ignored globaltwo\"\n '\n@@ -586,17 +599,7 @@ EOF\n cat <<-\\EOF >expected-default\n \tone\n \ta/one\n-\ta/b/on\n-\ta/b/one\n-\ta/b/one one\n-\ta/b/one two\n-\t\"a/b/one\\\"three\"\n-\ta/b/two\n \ta/b/twooo\n-\tglobaltwo\n-\ta/globaltwo\n-\ta/b/globaltwo\n-\tb/globaltwo\n EOF\n cat <<-EOF >expected-verbose\n \t.gitignore:1:one\tone\n@@ -696,8 +699,12 @@ cat <<-EOF >expected-all\n \t$global_excludes:2:!globaltwo\t../b/globaltwo\n \t::\tc/not-ignored\n EOF\n+cat <<-EOF >expected-default\n+../one\n+one\n+b/twooo\n+EOF\n grep -v '^::\t' expected-all >expected-verbose\n-sed -e 's/.*\t//' expected-verbose >expected-default\n \n broken_c_unquote stdin >stdin0\n \n\nbase-commit: bfdd66e72fffd18235757bedbf355fd4176d6019\n-- \ngitgitgadget\n"},{"id":"391944","messageId":"xmqqimk5ks39.fsf@gitster-ct.c.googlers.com","threadId":"52827","inReplyTo":"pull.711.git.git.1581956106255.gitgitgadget@gmail.com","subject":"Re: [PATCH] check-ignore: fix handling with negated patterns","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-02-17T18:04:58Z","receivedAt":"2020-02-17T18:05:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Elijah Newren via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> From: Elijah Newren <newren@gmail.com>\n>\n> check-ignore was meant to check ignore rules the same way git status and\n> other commands would, and to report whether a path is excluded.  It\n> failed to do this (and generated a few bug reports), however, because it\n> did not account for negated patterns.\n\nI suspect that the above distorts history.  IIRC, it was meant as a\ntool to see which exact pattern in the exclude sequence had the\nfinal say for the given needle, written primarily as a debugging\naid.  In that context, \"This rule has the final say\", whether the\nrule is a negative or positive, still means something.\n\nIt is just the behavior is _much_ less useful for those who want to\nknow what the final say is, and I tend to agree that we probably are\nbetter off changing its output to reflect \"so, are we ignoring the\npath after all? yes/no?\" because we are pretty much done with\ndebugging the exclude API implementation.\n"},{"id":"391948","messageId":"CABPp-BEbojaeYkSMR7vntW0SkWf6dVOko5H=jqT-Yv2USRerxA@mail.gmail.com","threadId":"52827","inReplyTo":"xmqqimk5ks39.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] check-ignore: fix handling with negated patterns","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2020-02-17T18:41:47Z","receivedAt":"2020-02-17T18:42:00Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Mon, Feb 17, 2020 at 10:05 AM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> \"Elijah Newren via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n>\n> > From: Elijah Newren <newren@gmail.com>\n> >\n> > check-ignore was meant to check ignore rules the same way git status and\n> > other commands would, and to report whether a path is excluded.  It\n> > failed to do this (and generated a few bug reports), however, because it\n> > did not account for negated patterns.\n>\n> I suspect that the above distorts history.  IIRC, it was meant as a\n> tool to see which exact pattern in the exclude sequence had the\n> final say for the given needle, written primarily as a debugging\n> aid.  In that context, \"This rule has the final say\", whether the\n> rule is a negative or positive, still means something.\n\nI can reword it; how does the following sound?\n\ncheck-ignore claims that it reports whether each path it is given is\nexcluded.  However, it fails to do so because it did not account for\nnegated patterns.\n\n\nAlso, I think the \"This rule has the final say\" functionality of the\ntool might still be useful, so I kept it -- see my updates to the\n--verbose flag (mentioned later in the commit message).\n\n> It is just the behavior is _much_ less useful for those who want to\n> know what the final say is, and I tend to agree that we probably are\n> better off changing its output to reflect \"so, are we ignoring the\n> path after all? yes/no?\" because we are pretty much done with\n> debugging the exclude API implementation.\n"},{"id":"391951","messageId":"xmqqeeutkkur.fsf@gitster-ct.c.googlers.com","threadId":"52827","inReplyTo":"CABPp-BEbojaeYkSMR7vntW0SkWf6dVOko5H=jqT-Yv2USRerxA@mail.gmail.com","subject":"Re: [PATCH] check-ignore: fix handling with negated patterns","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-02-17T20:41:16Z","receivedAt":"2020-02-17T20:41:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren <newren@gmail.com> writes:\n\n>> I suspect that the above distorts history.  IIRC, it was meant as a\n>> tool to see which exact pattern in the exclude sequence had the\n>> final say for the given needle, written primarily as a debugging\n>> aid.  In that context, \"This rule has the final say\", whether the\n>> rule is a negative or positive, still means something.\n>\n> I can reword it; how does the following sound?\n>\n> check-ignore claims that it reports whether each path it is given is\n> excluded.  However, it fails to do so because it did not account for\n> negated patterns.\n\nI am not sure about \"claims\" part.  \n\nIsn't it more like \"check-ignore has been the tool that reports the\nrule that has final say on each of the paths it is given, but that\nis not very useful when the user wants to see if the path is\nexcluded (e.g. the rule with the final say may be negative).  Let's\nchange the behaviour so that it reports if the path is excluded or\nnot\"?  As I said, I tend to agree with the direction your patch\nwants to go (iow, we probably are better off changing the\nbehaviour\"); the question is if we want a transition plan and how\nextensive it needs be if we do.\n\n"},{"id":"391954","messageId":"CABPp-BHaTK62W4_rUaJXNUPSfu9cBD5MrmjgbJeMuA+7s3+rGg@mail.gmail.com","threadId":"52827","inReplyTo":"xmqqeeutkkur.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] check-ignore: fix handling with negated patterns","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2020-02-17T21:07:46Z","receivedAt":"2020-02-17T21:08:00Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Mon, Feb 17, 2020 at 12:41 PM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Elijah Newren <newren@gmail.com> writes:\n>\n> >> I suspect that the above distorts history.  IIRC, it was meant as a\n> >> tool to see which exact pattern in the exclude sequence had the\n> >> final say for the given needle, written primarily as a debugging\n> >> aid.  In that context, \"This rule has the final say\", whether the\n> >> rule is a negative or positive, still means something.\n> >\n> > I can reword it; how does the following sound?\n> >\n> > check-ignore claims that it reports whether each path it is given is\n> > excluded.  However, it fails to do so because it did not account for\n> > negated patterns.\n>\n> I am not sure about \"claims\" part.\n>\n> Isn't it more like \"check-ignore has been the tool that reports the\n> rule that has final say on each of the paths it is given, but that\n> is not very useful when the user wants to see if the path is\n> excluded (e.g. the rule with the final say may be negative).\n\nNo, it is not more like that; the check-ignore manpage currently claims this:\n\n       For each pathname given via the command-line or from a file via\n--stdin, check whether the file is excluded by .gitignore (or\n       other input files to the exclude mechanism) and output the path\nif it is excluded.\n\nNote also that this description at the beginning of the manpage says\nnothing about reporting which rule has the final say.  And, in fact,\nthe command in default mode does not report which rule or rules were\ninvolved.  All of that work falls to the --verbose flag, which was\ndocumented as\n\n           Also output details about the matching pattern (if any) for\neach given pathname. For precedence rules within and between\n           exclude sources, see gitignore(5).\n\nNow, if you read both descriptions together, you find that these\nclaims are contradictory and that it cannot do both, so the \"Also\" bit\nit leads with is a lie.  As such, my commit modified the definition of\nverbose to make it instead read:\n\n       Instead of printing the paths that are excluded, for each path\n       that matches an exclude pattern print the exclude pattern\n       together with the path.  (Matching an exclude pattern usually\n       means the path is excluded, but if the pattern begins with '!'\n       then it is a negated pattern and matching it means the path is\n       NOT excluded.)\n\nThis was a change of description for the --verbose flag, not a change\nof implementation.  Thus, in my opinion, no transition period is\nneeded: those who wanted to use check-ignore to see what rule would\nhave matched had to use --verbose before, and --verbose behaves the\nsame as before.\n\nThose who wanted to use check-ignore without the --verbose flag to see\nif a rule is excluded, get corrected behavior that will actually do\nthat.\n"},{"id":"392015","messageId":"pull.711.v2.git.git.1582058162095.gitgitgadget@gmail.com","threadId":"52827","inReplyTo":"pull.711.git.git.1581956106255.gitgitgadget@gmail.com","subject":"[PATCH v2] check-ignore: fix documentation and implementation to match","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-02-18T20:36:01Z","receivedAt":"2020-02-18T20:36:08Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren <newren@gmail.com>\n\ncheck-ignore has two different modes, and neither of these modes has an\nimplementation that matches the documentation.  These modes differ in\nwhether they just print paths or whether they also print the final\npattern matched by the path.  The fix is different for both modes, so\nI'll discuss both separately.\n\n=== First (default) mode ===\n\nThe first mode is documented as:\n\n    For each pathname given via the command-line or from a file via\n    --stdin, check whether the file is excluded by .gitignore (or other\n    input files to the exclude mechanism) and output the path if it is\n    excluded.\n\nHowever, it fails to do this because it did not account for negated\npatterns.  Commands other than check-ignore verify exclusion rules via\ncalling\n\n   ... -> treat_one_path() -> is_excluded() -> last_matching_pattern()\n\nwhile check-ignore has a call path of the form:\n\n   ... -> check_ignore()                    -> last_matching_pattern()\n\nThe fact that the latter does not include the call to is_excluded()\nmeans that it is susceptible to to messing up negated patterns (since\nthat is the only significant thing is_excluded() adds over\nlast_matching_pattern()).  Unfortunately, we can't make it just call\nis_excluded(), because the same codepath is used by the verbose mode\nwhich needs to know the matched pattern in question.  This brings us\nto...\n\n=== Second (verbose) mode ===\n\nThe second mode, known as verbose mode, references the first in the\ndocumentation and says:\n\n    Also output details about the matching pattern (if any) for each\n    given pathname. For precedence rules within and between exclude\n    sources, see gitignore(5).\n\nThe \"Also\" means it will print patterns that match the exclude rules as\nnoted for the first mode, and also print which pattern matches.  Unless\nmore information is printed than just pathname and pattern (which is not\ndone), this definition is somewhat ill-defined and perhaps even\nself-contradictory for negated patterns: A path which matches a negated\nexclude pattern is NOT excluded and thus shouldn't be printed by the\nformer logic, while it certainly does match one of the explicit patterns\nand thus should be printed by the latter logic.\n\n=== Resolution ==\n\nSince the second mode exists to find out which pattern matches given\npaths, and showing the user a pattern that begins with a '!' is\nsufficient for them to figure out whether the pattern is excluded, the\nexisting behavior is desirable -- we just need to update the\ndocumentation to match the implementation (i.e. it is about printing\nwhich pattern is matched by paths, not about showing which paths are\nexcluded).\n\nFor the first or default mode, users just want to know whether a pattern\nis excluded.  As such, the existing documentation is desirable; change\nthe implementation to match the documented behavior.\n\nFinally, also adjust a few tests in t0008 that were caught up by this\ndiscrepancy in how negated paths were handled.\n\nSigned-off-by: Elijah Newren <newren@gmail.com>\n---\n    check-ignore: fix handling with negated patterns\n    \n    Overhauled the commit message to clarify that there are multiple modes\n    for check-ignore and how they require different handling.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-711%2Fnewren%2Ffix-check-ignore-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-711/newren/fix-check-ignore-v2\nPull-Request: https://github.com/git/git/pull/711\n\nRange-diff vs v1:\n\n 1:  3fbcc93e8bc ! 1:  1c289cb4d9a check-ignore: fix handling with negated patterns\n     @@ -1,13 +1,25 @@\n      Author: Elijah Newren <newren@gmail.com>\n      \n     -    check-ignore: fix handling with negated patterns\n     +    check-ignore: fix documentation and implementation to match\n      \n     -    check-ignore was meant to check ignore rules the same way git status and\n     -    other commands would, and to report whether a path is excluded.  It\n     -    failed to do this (and generated a few bug reports), however, because it\n     -    did not account for negated patterns.\n     +    check-ignore has two different modes, and neither of these modes has an\n     +    implementation that matches the documentation.  These modes differ in\n     +    whether they just print paths or whether they also print the final\n     +    pattern matched by the path.  The fix is different for both modes, so\n     +    I'll discuss both separately.\n      \n     -    Commands other than check-ignore verify exclusion rules via calling\n     +    === First (default) mode ===\n     +\n     +    The first mode is documented as:\n     +\n     +        For each pathname given via the command-line or from a file via\n     +        --stdin, check whether the file is excluded by .gitignore (or other\n     +        input files to the exclude mechanism) and output the path if it is\n     +        excluded.\n     +\n     +    However, it fails to do this because it did not account for negated\n     +    patterns.  Commands other than check-ignore verify exclusion rules via\n     +    calling\n      \n             ... -> treat_one_path() -> is_excluded() -> last_matching_pattern()\n      \n     @@ -19,20 +31,41 @@\n          means that it is susceptible to to messing up negated patterns (since\n          that is the only significant thing is_excluded() adds over\n          last_matching_pattern()).  Unfortunately, we can't make it just call\n     -    is_excluded(), because is_excluded doesn't return the pattern in\n     -    question and part of check-ignore's functionality is not just checking\n     -    whether one of the patterns matches but returning which one does.\n     -\n     -    Further, check_ignore() is supposed to handle a --verbose mode, which\n     -    was ill-defined for the case of negated patterns: check-ignore was\n     -    documented to print just the excluded paths, whereas the --verbose mode\n     -    was there to document which patterns were matched by paths.  A path\n     -    which matches a negated exclude pattern is NOT excluded and thus\n     -    shouldn't be printed by the former logic, while it certainly does match\n     -    one of the explicit patterns and thus should be printed by the latter\n     -    logic.  Adjust the definition of --verbose to state that it is about\n     -    matching patterns INSTEAD of about showing which paths are excluded in\n     -    order to resolve this discrepancy.\n     +    is_excluded(), because the same codepath is used by the verbose mode\n     +    which needs to know the matched pattern in question.  This brings us\n     +    to...\n     +\n     +    === Second (verbose) mode ===\n     +\n     +    The second mode, known as verbose mode, references the first in the\n     +    documentation and says:\n     +\n     +        Also output details about the matching pattern (if any) for each\n     +        given pathname. For precedence rules within and between exclude\n     +        sources, see gitignore(5).\n     +\n     +    The \"Also\" means it will print patterns that match the exclude rules as\n     +    noted for the first mode, and also print which pattern matches.  Unless\n     +    more information is printed than just pathname and pattern (which is not\n     +    done), this definition is somewhat ill-defined and perhaps even\n     +    self-contradictory for negated patterns: A path which matches a negated\n     +    exclude pattern is NOT excluded and thus shouldn't be printed by the\n     +    former logic, while it certainly does match one of the explicit patterns\n     +    and thus should be printed by the latter logic.\n     +\n     +    === Resolution ==\n     +\n     +    Since the second mode exists to find out which pattern matches given\n     +    paths, and showing the user a pattern that begins with a '!' is\n     +    sufficient for them to figure out whether the pattern is excluded, the\n     +    existing behavior is desirable -- we just need to update the\n     +    documentation to match the implementation (i.e. it is about printing\n     +    which pattern is matched by paths, not about showing which paths are\n     +    excluded).\n     +\n     +    For the first or default mode, users just want to know whether a pattern\n     +    is excluded.  As such, the existing documentation is desirable; change\n     +    the implementation to match the documented behavior.\n      \n          Finally, also adjust a few tests in t0008 that were caught up by this\n          discrepancy in how negated paths were handled.\n\n\n Documentation/git-check-ignore.txt | 12 ++++++---\n builtin/check-ignore.c             |  3 +++\n t/t0008-ignores.sh                 | 39 ++++++++++++++++++------------\n 3 files changed, 35 insertions(+), 19 deletions(-)\n\ndiff --git a/Documentation/git-check-ignore.txt b/Documentation/git-check-ignore.txt\nindex 8b2d49c79e1..85ef46e2eff 100644\n--- a/Documentation/git-check-ignore.txt\n+++ b/Documentation/git-check-ignore.txt\n@@ -30,9 +30,15 @@ OPTIONS\n \tvalid with a single pathname.\n \n -v, --verbose::\n-\tAlso output details about the matching pattern (if any)\n-\tfor each given pathname. For precedence rules within and\n-\tbetween exclude sources, see linkgit:gitignore[5].\n+\tInstead of printing the paths that are excluded, for each path\n+\tthat matches an exclude pattern print the exclude pattern\n+\ttogether with the path.  (Matching an exclude pattern usually\n+\tmeans the path is excluded, but if the pattern begins with '!'\n+\tthen it is a negated pattern and matching it means the path is\n+\tNOT excluded.)\n++\n+For precedence rules within and between exclude sources, see\n+linkgit:gitignore[5].\n \n --stdin::\n \tRead pathnames from the standard input, one per line,\ndiff --git a/builtin/check-ignore.c b/builtin/check-ignore.c\nindex 5a4f92395f3..ea5d0ae3a6a 100644\n--- a/builtin/check-ignore.c\n+++ b/builtin/check-ignore.c\n@@ -108,6 +108,9 @@ static int check_ignore(struct dir_struct *dir,\n \t\t\tint dtype = DT_UNKNOWN;\n \t\t\tpattern = last_matching_pattern(dir, &the_index,\n \t\t\t\t\t\t\tfull_path, &dtype);\n+\t\t\tif (!verbose && pattern &&\n+\t\t\t    pattern->flags & PATTERN_FLAG_NEGATIVE)\n+\t\t\t\tpattern = NULL;\n \t\t}\n \t\tif (!quiet && (pattern || show_non_matching))\n \t\t\toutput_pattern(pathspec.items[i].original, pattern);\ndiff --git a/t/t0008-ignores.sh b/t/t0008-ignores.sh\nindex 1744cee5e99..370a389e5c5 100755\n--- a/t/t0008-ignores.sh\n+++ b/t/t0008-ignores.sh\n@@ -424,9 +424,24 @@ test_expect_success 'local ignore inside a sub-directory with --verbose' '\n \t)\n '\n \n-test_expect_success_multi 'nested include' \\\n-\t'a/b/.gitignore:8:!on*\ta/b/one' '\n-\ttest_check_ignore \"a/b/one\"\n+test_expect_success 'nested include of negated pattern' '\n+\texpect \"\" &&\n+\ttest_check_ignore \"a/b/one\" 1\n+'\n+\n+test_expect_success 'nested include of negated pattern with -q' '\n+\texpect \"\" &&\n+\ttest_check_ignore \"-q a/b/one\" 1\n+'\n+\n+test_expect_success 'nested include of negated pattern with -v' '\n+\texpect \"a/b/.gitignore:8:!on*\ta/b/one\" &&\n+\ttest_check_ignore \"-v a/b/one\" 0\n+'\n+\n+test_expect_success 'nested include of negated pattern with -v -n' '\n+\texpect \"a/b/.gitignore:8:!on*\ta/b/one\" &&\n+\ttest_check_ignore \"-v -n a/b/one\" 0\n '\n \n ############################################################################\n@@ -460,7 +475,6 @@ test_expect_success 'cd to ignored sub-directory' '\n \texpect_from_stdin <<-\\EOF &&\n \t\tfoo\n \t\ttwoooo\n-\t\t../one\n \t\tseven\n \t\t../../one\n \tEOF\n@@ -543,7 +557,6 @@ test_expect_success 'global ignore' '\n \t\tglobalthree\n \t\ta/globalthree\n \t\ta/per-repo\n-\t\tglobaltwo\n \tEOF\n \ttest_check_ignore \"globalone per-repo globalthree a/globalthree a/per-repo not-ignored globaltwo\"\n '\n@@ -586,17 +599,7 @@ EOF\n cat <<-\\EOF >expected-default\n \tone\n \ta/one\n-\ta/b/on\n-\ta/b/one\n-\ta/b/one one\n-\ta/b/one two\n-\t\"a/b/one\\\"three\"\n-\ta/b/two\n \ta/b/twooo\n-\tglobaltwo\n-\ta/globaltwo\n-\ta/b/globaltwo\n-\tb/globaltwo\n EOF\n cat <<-EOF >expected-verbose\n \t.gitignore:1:one\tone\n@@ -696,8 +699,12 @@ cat <<-EOF >expected-all\n \t$global_excludes:2:!globaltwo\t../b/globaltwo\n \t::\tc/not-ignored\n EOF\n+cat <<-EOF >expected-default\n+../one\n+one\n+b/twooo\n+EOF\n grep -v '^::\t' expected-all >expected-verbose\n-sed -e 's/.*\t//' expected-verbose >expected-default\n \n broken_c_unquote stdin >stdin0\n \n\nbase-commit: bfdd66e72fffd18235757bedbf355fd4176d6019\n-- \ngitgitgadget\n"},{"id":"392024","messageId":"xmqqlfozio1o.fsf@gitster-ct.c.googlers.com","threadId":"52827","inReplyTo":"pull.711.v2.git.git.1582058162095.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] check-ignore: fix documentation and implementation to match","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-02-18T21:27:31Z","receivedAt":"2020-02-18T21:27:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Elijah Newren via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> Since the second mode exists to find out which pattern matches given\n> paths, and showing the user a pattern that begins with a '!' is\n> sufficient for them to figure out whether the pattern is excluded, the\n> existing behavior is desirable -- we just need to update the\n> documentation to match the implementation (i.e. it is about printing\n> which pattern is matched by paths, not about showing which paths are\n> excluded).\n>\n> For the first or default mode, users just want to know whether a pattern\n> is excluded.  As such, the existing documentation is desirable; change\n> the implementation to match the documented behavior.\n>\n> Finally, also adjust a few tests in t0008 that were caught up by this\n> discrepancy in how negated paths were handled.\n>\n> Signed-off-by: Elijah Newren <newren@gmail.com>\n> ...\n> diff --git a/Documentation/git-check-ignore.txt b/Documentation/git-check-ignore.txt\n> index 8b2d49c79e1..85ef46e2eff 100644\n> --- a/Documentation/git-check-ignore.txt\n> +++ b/Documentation/git-check-ignore.txt\n> @@ -30,9 +30,15 @@ OPTIONS\n>  \tvalid with a single pathname.\n>  \n>  -v, --verbose::\n> -\tAlso output details about the matching pattern (if any)\n> -\tfor each given pathname. For precedence rules within and\n> -\tbetween exclude sources, see linkgit:gitignore[5].\n> +\tInstead of printing the paths that are excluded, for each path\n> +\tthat matches an exclude pattern print the exclude pattern\n\ns/pattern print/pattern, print/;\n\n> +\ttogether with the path.  (Matching an exclude pattern usually\n> +\tmeans the path is excluded, but if the pattern begins with '!'\n> +\tthen it is a negated pattern and matching it means the path is\n> +\tNOT excluded.)\n\nNicely and clearly written.\n\n> diff --git a/builtin/check-ignore.c b/builtin/check-ignore.c\n> index 5a4f92395f3..ea5d0ae3a6a 100644\n> --- a/builtin/check-ignore.c\n> +++ b/builtin/check-ignore.c\n> @@ -108,6 +108,9 @@ static int check_ignore(struct dir_struct *dir,\n>  \t\t\tint dtype = DT_UNKNOWN;\n>  \t\t\tpattern = last_matching_pattern(dir, &the_index,\n>  \t\t\t\t\t\t\tfull_path, &dtype);\n> +\t\t\tif (!verbose && pattern &&\n> +\t\t\t    pattern->flags & PATTERN_FLAG_NEGATIVE)\n> +\t\t\t\tpattern = NULL;\n\nOK.\n\nThanks.  Will queue.\n"},{"id":"392031","messageId":"pull.711.v3.git.git.1582067137362.gitgitgadget@gmail.com","threadId":"52827","inReplyTo":"pull.711.v2.git.git.1582058162095.gitgitgadget@gmail.com","subject":"[PATCH v3] check-ignore: fix documentation and implementation to match","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-02-18T23:05:37Z","receivedAt":"2020-02-18T23:05:44Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren <newren@gmail.com>\n\ncheck-ignore has two different modes, and neither of these modes has an\nimplementation that matches the documentation.  These modes differ in\nwhether they just print paths or whether they also print the final\npattern matched by the path.  The fix is different for both modes, so\nI'll discuss both separately.\n\n=== First (default) mode ===\n\nThe first mode is documented as:\n\n    For each pathname given via the command-line or from a file via\n    --stdin, check whether the file is excluded by .gitignore (or other\n    input files to the exclude mechanism) and output the path if it is\n    excluded.\n\nHowever, it fails to do this because it did not account for negated\npatterns.  Commands other than check-ignore verify exclusion rules via\ncalling\n\n   ... -> treat_one_path() -> is_excluded() -> last_matching_pattern()\n\nwhile check-ignore has a call path of the form:\n\n   ... -> check_ignore()                    -> last_matching_pattern()\n\nThe fact that the latter does not include the call to is_excluded()\nmeans that it is susceptible to to messing up negated patterns (since\nthat is the only significant thing is_excluded() adds over\nlast_matching_pattern()).  Unfortunately, we can't make it just call\nis_excluded(), because the same codepath is used by the verbose mode\nwhich needs to know the matched pattern in question.  This brings us\nto...\n\n=== Second (verbose) mode ===\n\nThe second mode, known as verbose mode, references the first in the\ndocumentation and says:\n\n    Also output details about the matching pattern (if any) for each\n    given pathname. For precedence rules within and between exclude\n    sources, see gitignore(5).\n\nThe \"Also\" means it will print patterns that match the exclude rules as\nnoted for the first mode, and also print which pattern matches.  Unless\nmore information is printed than just pathname and pattern (which is not\ndone), this definition is somewhat ill-defined and perhaps even\nself-contradictory for negated patterns: A path which matches a negated\nexclude pattern is NOT excluded and thus shouldn't be printed by the\nformer logic, while it certainly does match one of the explicit patterns\nand thus should be printed by the latter logic.\n\n=== Resolution ==\n\nSince the second mode exists to find out which pattern matches given\npaths, and showing the user a pattern that begins with a '!' is\nsufficient for them to figure out whether the pattern is excluded, the\nexisting behavior is desirable -- we just need to update the\ndocumentation to match the implementation (i.e. it is about printing\nwhich pattern is matched by paths, not about showing which paths are\nexcluded).\n\nFor the first or default mode, users just want to know whether a pattern\nis excluded.  As such, the existing documentation is desirable; change\nthe implementation to match the documented behavior.\n\nFinally, also adjust a few tests in t0008 that were caught up by this\ndiscrepancy in how negated paths were handled.\n\nSigned-off-by: Elijah Newren <newren@gmail.com>\n---\n    check-ignore: fix handling with negated patterns\n    \n    Changes since v2:\n    \n     * Add missing comma, as pointed out by Junio.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-711%2Fnewren%2Ffix-check-ignore-v3\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-711/newren/fix-check-ignore-v3\nPull-Request: https://github.com/git/git/pull/711\n\nRange-diff vs v2:\n\n 1:  1c289cb4d9a ! 1:  99d2e466844 check-ignore: fix documentation and implementation to match\n     @@ -83,7 +83,7 @@\n      -\tfor each given pathname. For precedence rules within and\n      -\tbetween exclude sources, see linkgit:gitignore[5].\n      +\tInstead of printing the paths that are excluded, for each path\n     -+\tthat matches an exclude pattern print the exclude pattern\n     ++\tthat matches an exclude pattern, print the exclude pattern\n      +\ttogether with the path.  (Matching an exclude pattern usually\n      +\tmeans the path is excluded, but if the pattern begins with '!'\n      +\tthen it is a negated pattern and matching it means the path is\n\n\n Documentation/git-check-ignore.txt | 12 ++++++---\n builtin/check-ignore.c             |  3 +++\n t/t0008-ignores.sh                 | 39 ++++++++++++++++++------------\n 3 files changed, 35 insertions(+), 19 deletions(-)\n\ndiff --git a/Documentation/git-check-ignore.txt b/Documentation/git-check-ignore.txt\nindex 8b2d49c79e1..0c3924a63d2 100644\n--- a/Documentation/git-check-ignore.txt\n+++ b/Documentation/git-check-ignore.txt\n@@ -30,9 +30,15 @@ OPTIONS\n \tvalid with a single pathname.\n \n -v, --verbose::\n-\tAlso output details about the matching pattern (if any)\n-\tfor each given pathname. For precedence rules within and\n-\tbetween exclude sources, see linkgit:gitignore[5].\n+\tInstead of printing the paths that are excluded, for each path\n+\tthat matches an exclude pattern, print the exclude pattern\n+\ttogether with the path.  (Matching an exclude pattern usually\n+\tmeans the path is excluded, but if the pattern begins with '!'\n+\tthen it is a negated pattern and matching it means the path is\n+\tNOT excluded.)\n++\n+For precedence rules within and between exclude sources, see\n+linkgit:gitignore[5].\n \n --stdin::\n \tRead pathnames from the standard input, one per line,\ndiff --git a/builtin/check-ignore.c b/builtin/check-ignore.c\nindex 5a4f92395f3..ea5d0ae3a6a 100644\n--- a/builtin/check-ignore.c\n+++ b/builtin/check-ignore.c\n@@ -108,6 +108,9 @@ static int check_ignore(struct dir_struct *dir,\n \t\t\tint dtype = DT_UNKNOWN;\n \t\t\tpattern = last_matching_pattern(dir, &the_index,\n \t\t\t\t\t\t\tfull_path, &dtype);\n+\t\t\tif (!verbose && pattern &&\n+\t\t\t    pattern->flags & PATTERN_FLAG_NEGATIVE)\n+\t\t\t\tpattern = NULL;\n \t\t}\n \t\tif (!quiet && (pattern || show_non_matching))\n \t\t\toutput_pattern(pathspec.items[i].original, pattern);\ndiff --git a/t/t0008-ignores.sh b/t/t0008-ignores.sh\nindex 1744cee5e99..370a389e5c5 100755\n--- a/t/t0008-ignores.sh\n+++ b/t/t0008-ignores.sh\n@@ -424,9 +424,24 @@ test_expect_success 'local ignore inside a sub-directory with --verbose' '\n \t)\n '\n \n-test_expect_success_multi 'nested include' \\\n-\t'a/b/.gitignore:8:!on*\ta/b/one' '\n-\ttest_check_ignore \"a/b/one\"\n+test_expect_success 'nested include of negated pattern' '\n+\texpect \"\" &&\n+\ttest_check_ignore \"a/b/one\" 1\n+'\n+\n+test_expect_success 'nested include of negated pattern with -q' '\n+\texpect \"\" &&\n+\ttest_check_ignore \"-q a/b/one\" 1\n+'\n+\n+test_expect_success 'nested include of negated pattern with -v' '\n+\texpect \"a/b/.gitignore:8:!on*\ta/b/one\" &&\n+\ttest_check_ignore \"-v a/b/one\" 0\n+'\n+\n+test_expect_success 'nested include of negated pattern with -v -n' '\n+\texpect \"a/b/.gitignore:8:!on*\ta/b/one\" &&\n+\ttest_check_ignore \"-v -n a/b/one\" 0\n '\n \n ############################################################################\n@@ -460,7 +475,6 @@ test_expect_success 'cd to ignored sub-directory' '\n \texpect_from_stdin <<-\\EOF &&\n \t\tfoo\n \t\ttwoooo\n-\t\t../one\n \t\tseven\n \t\t../../one\n \tEOF\n@@ -543,7 +557,6 @@ test_expect_success 'global ignore' '\n \t\tglobalthree\n \t\ta/globalthree\n \t\ta/per-repo\n-\t\tglobaltwo\n \tEOF\n \ttest_check_ignore \"globalone per-repo globalthree a/globalthree a/per-repo not-ignored globaltwo\"\n '\n@@ -586,17 +599,7 @@ EOF\n cat <<-\\EOF >expected-default\n \tone\n \ta/one\n-\ta/b/on\n-\ta/b/one\n-\ta/b/one one\n-\ta/b/one two\n-\t\"a/b/one\\\"three\"\n-\ta/b/two\n \ta/b/twooo\n-\tglobaltwo\n-\ta/globaltwo\n-\ta/b/globaltwo\n-\tb/globaltwo\n EOF\n cat <<-EOF >expected-verbose\n \t.gitignore:1:one\tone\n@@ -696,8 +699,12 @@ cat <<-EOF >expected-all\n \t$global_excludes:2:!globaltwo\t../b/globaltwo\n \t::\tc/not-ignored\n EOF\n+cat <<-EOF >expected-default\n+../one\n+one\n+b/twooo\n+EOF\n grep -v '^::\t' expected-all >expected-verbose\n-sed -e 's/.*\t//' expected-verbose >expected-default\n \n broken_c_unquote stdin >stdin0\n \n\nbase-commit: bfdd66e72fffd18235757bedbf355fd4176d6019\n-- \ngitgitgadget\n"},{"id":"392118","messageId":"xmqq7e0idztk.fsf@gitster-ct.c.googlers.com","threadId":"52827","inReplyTo":"CABPp-BHaTK62W4_rUaJXNUPSfu9cBD5MrmjgbJeMuA+7s3+rGg@mail.gmail.com","subject":"Re: [PATCH] check-ignore: fix handling with negated patterns","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-02-19T21:36:39Z","receivedAt":"2020-02-19T21:36:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren <newren@gmail.com> writes:\n\n> No, it is not more like that; the check-ignore manpage currently claims this:\n>\n>        For each pathname given via the command-line or from a file via\n> --stdin, check whether the file is excluded by .gitignore (or\n>        other input files to the exclude mechanism) and output the path\n> if it is excluded.\n\nThanks.  I wasn't paying attention to what happened to the manpage\n(or to the command for that matter) after I wrote it as a debugging\naid X-<.\n\nThe updated version looked reasonable to me.  Thanks.\n"}]}