{"thread":{"id":"53619","subject":"[PATCH] dir: fix treatment of negated pathspecs","startedAt":"2020-06-05T18:23:52Z","lastAt":"2020-06-05T18:23:52Z","messageCount":1,"participants":["Elijah Newren via GitGitGadget"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"399206","messageId":"pull.803.git.git.1591381428302.gitgitgadget@gmail.com","threadId":"53619","inReplyTo":null,"subject":"[PATCH] dir: fix treatment of negated pathspecs","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-06-05T18:23:48Z","receivedAt":"2020-06-05T18:23:52Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren <newren@gmail.com>\n\ndo_match_pathspec() started life as match_pathspec_depth_1() and for\ncorrectness was only supposed to be called from match_pathspec_depth().\nmatch_pathspec_depth() was later renamed to match_pathspec(), so the\ninvariant we expect today is that do_match_pathspec() has no direct\ncallers outside of match_pathspec().\n\nUnfortunately, this intention was lost with the renames of the two\nfunctions, and additional calls to do_match_pathspec() were added in\ncommits 75a6315f74 (\"ls-files: add pathspec matching for submodules\",\n2016-10-07) and 89a1f4aaf7 (\"dir: if our pathspec might match files\nunder a dir, recurse into it\", 2019-09-17).  Of course,\ndo_match_pathspec() had an important advantge over match_pathspec() --\nmatch_pathspec() would hardcode flags to one of two values, and these\nnew callers needed to pass some other value for flags.  Also, although\ncalling do_match_pathspec() directly was incorrect, there likely wasn't\nany difference in the observable end output, because the bug just meant\nthat fill_diretory() would recurse into unneeded directories.  Since\nsubsequent does-this-path-match checks on individual paths under the\ndirectory would cause those extra paths to be filtered out, the only\ndifference from using the wrong function was unnecessary computation.\n\nThe second of those bad calls to do_match_pathspec() was involved -- via\neither direct movement or via copying+editing -- into a number of later\nrefactors.  See commits 777b420347 (\"dir: synchronize\ntreat_leading_path() and read_directory_recursive()\", 2019-12-19),\n8d92fb2927 (\"dir: replace exponential algorithm with a linear one\",\n2020-04-01), and 95c11ecc73 (\"Fix error-prone fill_directory() API; make\nit only return matches\", 2020-04-01).  The last of those introduced the\nusage of do_match_pathspec() on an individual file, and thus resulted in\nindividual paths being returned that shouldn't be.\n\nThe problem with calling do_match_pathspec() instead of match_pathspec()\nis that any negated patterns such as ':!unwanted_path` will be ignored.\nAdd a new match_pathspec_with_flags() function to fulfill the needs of\nspecifying special flags while still correctly checking negated\npatterns, add a big comment above do_match_pathspec() to prevent others\nfrom misusing it, and correct current callers of do_match_pathspec() to\ninstead use either match_pathspec() or match_pathspec_with_flags().\n\nOne final note is that DO_MATCH_LEADING_PATHSPEC needs special\nconsideration when working with DO_MATCH_EXCLUDE.  The point of\nDO_MATCH_LEADING_PATHSPEC is that if we have a pathspec like\n   */Makefile\nand we are checking a directory path like\n   src/module/component\nthat we want to consider it a match so that we recurse into the\ndirectory because it _might_ have a file named Makefile somewhere below.\nHowever, when we are using an exclusion pattern, i.e. we have a pathspec\nlike\n   :(exclude)*/Makefile\nwe do NOT want to say that a directory path like\n   src/module/component\nis a (negative) match.  While there *might* be a file named 'Makefile'\nsomewhere below that directory, there could also be other files and we\ncannot pre-emptively rule all the files under that directory out; we\nneed to recurse and then check individual files.  Adjust the\nDO_MATCH_LEADING_PATHSPEC logic to only get activated for positive\npathspecs.\n\nReported-by: John Millikin <jmillikin@stripe.com>\nSigned-off-by: Elijah Newren <newren@gmail.com>\n---\n    dir: fix treatment of negated pathspecs\n    \n    The issue reported by John is fixed by just changing treat_path()'s\n    do_match_pathspec() call to match_pathspec(), but the report caused me\n    to dig up some history and audit all the other do_match_pathspec() calls\n    once I found that out. This patch tries to carefully explain the\n    problems, fixes all the loose ends I found, and also tries to prevent\n    similar mistakes from being made in the future.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-803%2Fnewren%2Fnegated-pathspecs-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-803/newren/negated-pathspecs-v1\nPull-Request: https://github.com/git/git/pull/803\n\n dir.c                       | 50 ++++++++++++++++++++++++-------------\n t/t6132-pathspec-exclude.sh | 33 ++++++++++++++++++++++++\n 2 files changed, 66 insertions(+), 17 deletions(-)\n\ndiff --git a/dir.c b/dir.c\nindex d97e9558489..9ecd349e27d 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -364,7 +364,8 @@ static int match_pathspec_item(const struct index_state *istate,\n \t\treturn MATCHED_FNMATCH;\n \n \t/* Perform checks to see if \"name\" is a leading string of the pathspec */\n-\tif (flags & DO_MATCH_LEADING_PATHSPEC) {\n+\tif ( (flags & DO_MATCH_LEADING_PATHSPEC) &&\n+\t    !(flags & DO_MATCH_EXCLUDE)) {\n \t\t/* name is a literal prefix of the pathspec */\n \t\tint offset = name[namelen-1] == '/' ? 1 : 0;\n \t\tif ((namelen < matchlen) &&\n@@ -401,6 +402,10 @@ static int match_pathspec_item(const struct index_state *istate,\n }\n \n /*\n+ * do_match_pathspec() is meant to ONLY be called by\n+ * match_pathspec_with_flags(); calling it directly risks pathspecs\n+ * like ':!unwanted_path' being ignored.\n+ *\n  * Given a name and a list of pathspecs, returns the nature of the\n  * closest (i.e. most specific) match of the name to any of the\n  * pathspecs.\n@@ -486,13 +491,12 @@ static int do_match_pathspec(const struct index_state *istate,\n \treturn retval;\n }\n \n-int match_pathspec(const struct index_state *istate,\n-\t\t   const struct pathspec *ps,\n-\t\t   const char *name, int namelen,\n-\t\t   int prefix, char *seen, int is_dir)\n+static int match_pathspec_with_flags(const struct index_state *istate,\n+\t\t\t\t     const struct pathspec *ps,\n+\t\t\t\t     const char *name, int namelen,\n+\t\t\t\t     int prefix, char *seen, unsigned flags)\n {\n \tint positive, negative;\n-\tunsigned flags = is_dir ? DO_MATCH_DIRECTORY : 0;\n \tpositive = do_match_pathspec(istate, ps, name, namelen,\n \t\t\t\t     prefix, seen, flags);\n \tif (!(ps->magic & PATHSPEC_EXCLUDE) || !positive)\n@@ -503,6 +507,16 @@ int match_pathspec(const struct index_state *istate,\n \treturn negative ? 0 : positive;\n }\n \n+int match_pathspec(const struct index_state *istate,\n+\t\t   const struct pathspec *ps,\n+\t\t   const char *name, int namelen,\n+\t\t   int prefix, char *seen, int is_dir)\n+{\n+\tunsigned flags = is_dir ? DO_MATCH_DIRECTORY : 0;\n+\treturn match_pathspec_with_flags(istate, ps, name, namelen,\n+\t\t\t\t\t prefix, seen, flags);\n+}\n+\n /**\n  * Check if a submodule is a superset of the pathspec\n  */\n@@ -511,11 +525,11 @@ int submodule_path_match(const struct index_state *istate,\n \t\t\t const char *submodule_name,\n \t\t\t char *seen)\n {\n-\tint matched = do_match_pathspec(istate, ps, submodule_name,\n-\t\t\t\t\tstrlen(submodule_name),\n-\t\t\t\t\t0, seen,\n-\t\t\t\t\tDO_MATCH_DIRECTORY |\n-\t\t\t\t\tDO_MATCH_LEADING_PATHSPEC);\n+\tint matched = match_pathspec_with_flags(istate, ps, submodule_name,\n+\t\t\t\t\t\tstrlen(submodule_name),\n+\t\t\t\t\t\t0, seen,\n+\t\t\t\t\t\tDO_MATCH_DIRECTORY |\n+\t\t\t\t\t\tDO_MATCH_LEADING_PATHSPEC);\n \treturn matched;\n }\n \n@@ -1757,9 +1771,11 @@ static enum path_treatment treat_directory(struct dir_struct *dir,\n \t * for matching patterns.\n \t */\n \tif (pathspec && !excluded) {\n-\t\tmatches_how = do_match_pathspec(istate, pathspec, dirname, len,\n-\t\t\t\t\t\t0 /* prefix */, NULL /* seen */,\n-\t\t\t\t\t\tDO_MATCH_LEADING_PATHSPEC);\n+\t\tmatches_how = match_pathspec_with_flags(istate, pathspec,\n+\t\t\t\t\t\t\tdirname, len,\n+\t\t\t\t\t\t\t0 /* prefix */,\n+\t\t\t\t\t\t\tNULL /* seen */,\n+\t\t\t\t\t\t\tDO_MATCH_LEADING_PATHSPEC);\n \t\tif (!matches_how)\n \t\t\treturn path_none;\n \t}\n@@ -2191,9 +2207,9 @@ static enum path_treatment treat_path(struct dir_struct *dir,\n \t\tif (excluded)\n \t\t\treturn path_excluded;\n \t\tif (pathspec &&\n-\t\t    !do_match_pathspec(istate, pathspec, path->buf, path->len,\n-\t\t\t\t       0 /* prefix */, NULL /* seen */,\n-\t\t\t\t       0 /* flags */))\n+\t\t    !match_pathspec(istate, pathspec, path->buf, path->len,\n+\t\t\t\t    0 /* prefix */, NULL /* seen */,\n+\t\t\t\t    0 /* is_dir */))\n \t\t\treturn path_none;\n \t\treturn path_untracked;\n \t}\ndiff --git a/t/t6132-pathspec-exclude.sh b/t/t6132-pathspec-exclude.sh\nindex 2462b19ddd3..30328b87f07 100755\n--- a/t/t6132-pathspec-exclude.sh\n+++ b/t/t6132-pathspec-exclude.sh\n@@ -211,4 +211,37 @@ test_expect_success 't_e_i() exclude case #8' '\n \t)\n '\n \n+test_expect_success 'grep --untracked PATTERN' '\n+\t# This test is not an actual test of exclude patterns, rather it\n+\t# is here solely to ensure that if any tests are inserted, deleted, or\n+\t# changed above, that we still have untracked files with the expected\n+\t# contents for the NEXT two tests.\n+\tcat <<-\\EOF >expect-grep &&\n+\tactual\n+\texpect\n+\tsub/actual\n+\tsub/expect\n+\tEOF\n+\tgit grep -l --untracked file -- >actual-grep &&\n+\ttest_cmp expect-grep actual-grep\n+'\n+\n+test_expect_success 'grep --untracked PATTERN :(exclude)DIR' '\n+\tcat <<-\\EOF >expect-grep &&\n+\tactual\n+\texpect\n+\tEOF\n+\tgit grep -l --untracked file -- \":(exclude)sub\" >actual-grep &&\n+\ttest_cmp expect-grep actual-grep\n+'\n+\n+test_expect_success 'grep --untracked PATTERN :(exclude)*FILE' '\n+\tcat <<-\\EOF >expect-grep &&\n+\tactual\n+\tsub/actual\n+\tEOF\n+\tgit grep -l --untracked file -- \":(exclude)*expect\" >actual-grep &&\n+\ttest_cmp expect-grep actual-grep\n+'\n+\n test_done\n\nbase-commit: 20514004ddf1a3528de8933bc32f284e175e1012\n-- \ngitgitgadget\n"}]}