{"thread":{"id":"57930","subject":"Excluding paths with wildcard not working with add -p","startedAt":"2022-05-29T17:27:49Z","lastAt":"2022-05-30T17:20:59Z","messageCount":7,"participants":["Robert Dailey","Junio C Hamano","rsbecker@nexbridge.com"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"456355","messageId":"CAHd499D81VN=aGsM6kaNLF2ZMg-Zg10U=qU-j7gQ7uXnqqfdqg@mail.gmail.com","threadId":"57930","inReplyTo":null,"subject":"Excluding paths with wildcard not working with add -p","fromName":"Robert Dailey","fromEmail":"rcdailey.lists@gmail.com","sentAt":"2022-05-29T17:27:33Z","receivedAt":"2022-05-29T17:27:49Z","isPatch":false,"sender":{"key":"rcdailey.lists@gmail.com","avatar":null},"body":"If I run the command:\n\n    git add -p -- ':^*.cs'\n\nI get an error:\n\nfatal: empty string is not a valid pathspec. please use . instead if\nyou meant to match all paths\nCannot close git diff-index --cached --numstat --summary HEAD --\n:(exclude,prefix:0)*.cs  () at C:/Program\nFiles/Git/mingw64/libexec/git-core\\git-add--interactive line 242.\n\nHowever, it works if I remove `-p`. Also observed this works too:\n\n    git add -p -- ':*.cs'\n\nSo it looks like we have a corner case where I can't do a patch add\nwhile excluding files. Is this intentional?\n"},{"id":"456356","messageId":"xmqqh758yz4u.fsf@gitster.g","threadId":"57930","inReplyTo":"CAHd499D81VN=aGsM6kaNLF2ZMg-Zg10U=qU-j7gQ7uXnqqfdqg@mail.gmail.com","subject":"Re: Excluding paths with wildcard not working with add -p","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-05-29T18:25:37Z","receivedAt":"2022-05-29T18:25:42Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Robert Dailey <rcdailey.lists@gmail.com> writes:\n\n> If I run the command:\n>\n>     git add -p -- ':^*.cs'\n>\n> I get an error:\n>\n> fatal: empty string is not a valid pathspec. please use . instead if\n> you meant to match all paths\n> Cannot close git diff-index --cached --numstat --summary HEAD --\n> :(exclude,prefix:0)*.cs  () at C:/Program\n> Files/Git/mingw64/libexec/git-core\\git-add--interactive line 242.\n>\n> However, it works if I remove `-p`. Also observed this works too:\n>\n>     git add -p -- ':*.cs'\n>\n> So it looks like we have a corner case where I can't do a patch add\n> while excluding files. Is this intentional?\n\nI do not think so.\n\nDoes this command\n\n    git add -p -- . ':^*.cs'\n\nwork as you expect?\n"},{"id":"456357","messageId":"xmqq8rqkyyc2.fsf@gitster.g","threadId":"57930","inReplyTo":"xmqqh758yz4u.fsf@gitster.g","subject":"Re: Excluding paths with wildcard not working with add -p","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-05-29T18:42:53Z","receivedAt":"2022-05-29T18:43:02Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Does this command\n>\n>     git add -p -- . ':^*.cs'\n>\n> work as you expect?\n\nWe used to error out when you give a pathspec with only negative\nelements in it, like the one you gave above.  Later, we tweaked this\nlogic at 859b7f1d (pathspec: don't error out on all-exclusionary\npathspec patterns, 2017-02-07) so that we add an empty string as an\nextra element when your pathspec has only negative elements.\n\nAt around the same time, we were migrating from \"an empty string is\na valid pathspec element that matches everything\" to \"either a dot\nor \":/\" is used for that purpose, and an empty string is rejected\",\nbetween d426430e (pathspec: warn on empty strings as pathspec,\n2016-06-22) and 9e4e8a64 (pathspec: die on empty strings as\npathspec, 2017-06-06).  I think 9e4e8a64 was not careful enough to\nturn the empty string 859b7f1d added to either a dot or \":/\"\n\nFor the purpose of \"add -p\", I _think_ adding a \"dot\" is correct,\nbut depending on the command, the code needs to add \":/\".\n\nHere is a quick trial patch, which seems to compile and pass all the\ntests we have, but the fact that this lingered with us for the past\n5 years is a strong sign that we lack coverage in this area, so it\nmay be breaking something else in a big way.\n\n pathspec.c | 6 ++++--\n 1 file changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git c/pathspec.c w/pathspec.c\nindex ddeeba7911..1b0ae51aa4 100644\n--- c/pathspec.c\n+++ w/pathspec.c\n@@ -628,8 +628,10 @@ void parse_pathspec(struct pathspec *pathspec,\n \t * that matches everything. We allocated an extra one for this.\n \t */\n \tif (nr_exclude == n) {\n-\t\tint plen = (!(flags & PATHSPEC_PREFER_CWD)) ? 0 : prefixlen;\n-\t\tinit_pathspec_item(item + n, 0, prefix, plen, \"\");\n+\t\tif (!(flags & PATHSPEC_PREFER_CWD))\n+\t\t\tinit_pathspec_item(item + n, 0, NULL, 0, \":/\");\n+\t\telse\n+\t\t\tinit_pathspec_item(item + n, 0, prefix, prefixlen, \".\");\n \t\tpathspec->nr++;\n \t}\n \n"},{"id":"456358","messageId":"CAHd499BX_8fP=BdJW8cuZnwJFoqxrsiLCZ45Ke12MOsaj7M-Dw@mail.gmail.com","threadId":"57930","inReplyTo":"xmqqh758yz4u.fsf@gitster.g","subject":"Re: Excluding paths with wildcard not working with add -p","fromName":"Robert Dailey","fromEmail":"rcdailey.lists@gmail.com","sentAt":"2022-05-29T21:15:16Z","receivedAt":"2022-05-29T21:15:31Z","isPatch":false,"sender":{"key":"rcdailey.lists@gmail.com","avatar":null},"body":"On Sun, May 29, 2022 at 1:25 PM Junio C Hamano <gitster@pobox.com> wrote:\n> Does this command\n>\n>     git add -p -- . ':^*.cs'\n>\n> work as you expect?\n\nyes I can confirm this works.\n"},{"id":"456359","messageId":"xmqqpmjwx8so.fsf_-_@gitster.g","threadId":"57930","inReplyTo":"xmqq8rqkyyc2.fsf@gitster.g","subject":"[PATCH] pathspec: correct an empty string used as a pathspec element","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-05-29T22:39:51Z","receivedAt":"2022-05-29T22:39:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Pathspecs with only negative elements did not work with some\ncommands that pass the pathspec along to a subprocess.  For\ninstance,\n\n    $ git add -p -- ':!*.txt'\n\nshould add everything except for paths ending in \".txt\", but it gets\ncomplaint from underlying \"diff-index\" and aborts.\n\nWe used to error out when a pathspec with only negative elements in\nit, like the one in the above example.  Later, 859b7f1d (pathspec:\ndon't error out on all-exclusionary pathspec patterns, 2017-02-07)\nupdated the logic to add an empty string as an extra element.  The\nintention was to let the extra element to match everything and let\nthe negative ones given by the user to subtract from it.\n\nAt around the same time, we were migrating from \"an empty string is\na valid pathspec element that matches everything\" to \"either a dot\nor \":/\" is used to match all, and an empty string is rejected\",\nbetween d426430e (pathspec: warn on empty strings as pathspec,\n2016-06-22) and 9e4e8a64 (pathspec: die on empty strings as\npathspec, 2017-06-06).  I think 9e4e8a64, which happened long after\n859b7f1d happened, was not careful enough to turn the empty string\n859b7f1d added to either a dot or \":/\".\n\nA care should be taken as the definition of \"everything\" depends on\nsubcommand.  For the purpose of \"add -p\", adding a \".\" to add\neverything in the current directory is the right thing to do.  But\nfor some other commands, \":/\" (i.e. really really everything, even\nthings outside the current subdirectory) is the right choice.\n\nWe would break commands in a big way if we get this wrong, so add a\nhandful of test pieces to make sure the resulting code still\nexcludes the paths that are expected and includes \"everything\" else.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n pathspec.c                  |   2 +-\n t/t6132-pathspec-exclude.sh | 181 ++++++++++++++++++++++++++++++++++++\n 2 files changed, 182 insertions(+), 1 deletion(-)\n\ndiff --git a/pathspec.c b/pathspec.c\nindex ddeeba7911..84ad9c73cf 100644\n--- a/pathspec.c\n+++ b/pathspec.c\n@@ -629,7 +629,7 @@ void parse_pathspec(struct pathspec *pathspec,\n \t */\n \tif (nr_exclude == n) {\n \t\tint plen = (!(flags & PATHSPEC_PREFER_CWD)) ? 0 : prefixlen;\n-\t\tinit_pathspec_item(item + n, 0, prefix, plen, \"\");\n+\t\tinit_pathspec_item(item + n, 0, prefix, plen, \".\");\n \t\tpathspec->nr++;\n \t}\n \ndiff --git a/t/t6132-pathspec-exclude.sh b/t/t6132-pathspec-exclude.sh\nindex 8ff1d76f79..9fdafeb1e9 100755\n--- a/t/t6132-pathspec-exclude.sh\n+++ b/t/t6132-pathspec-exclude.sh\n@@ -195,6 +195,7 @@ test_expect_success 'multiple exclusions' '\n '\n \n test_expect_success 't_e_i() exclude case #8' '\n+\ttest_when_finished \"rm -fr case8\" &&\n \tgit init case8 &&\n \t(\n \t\tcd case8 &&\n@@ -244,4 +245,184 @@ test_expect_success 'grep --untracked PATTERN :(exclude)*FILE' '\n \ttest_cmp expect-grep actual-grep\n '\n \n+# Depending on the command, all negative pathspec needs to subtract\n+# either from the full tree, or from the current directory.\n+#\n+# The sample tree checked out at this point has:\n+# file\n+# sub/file\n+# sub/file2\n+# sub/sub/file\n+# sub/sub/sub/file\n+# sub2/file\n+#\n+# but there may also be some cruft that interferes with \"git clean\"\n+# and \"git add\" tests.\n+\n+test_expect_success 'archive with all negative' '\n+\tgit reset --hard &&\n+\tgit clean -f &&\n+\tgit -C sub archive --format=tar HEAD -- \":!sub/\" >archive &&\n+\t\"$TAR\" tf archive >actual &&\n+\tcat >expect <<-\\EOF &&\n+\tfile\n+\tfile2\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'add with all negative' '\n+\tH=$(git rev-parse HEAD) &&\n+\tgit reset --hard $H &&\n+\tgit clean -f &&\n+\ttest_when_finished \"git reset --hard $H\" &&\n+\tfor path in file sub/file sub/sub/file sub2/file\n+\tdo\n+\t\techo smudge >>\"$path\" || return 1\n+\tdone &&\n+\tgit -C sub add -- \":!sub/\" &&\n+\tgit diff --name-only --no-renames --cached >actual &&\n+\tcat >expect <<-\\EOF &&\n+\tfile\n+\tsub/file\n+\tsub2/file\n+\tEOF\n+\ttest_cmp expect actual &&\n+\tgit diff --name-only --no-renames >actual &&\n+\techo sub/sub/file >expect &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'add -p with all negative' '\n+\tH=$(git rev-parse HEAD) &&\n+\tgit reset --hard $H &&\n+\tgit clean -f &&\n+\ttest_when_finished \"git reset --hard $H\" &&\n+\tfor path in file sub/file sub/sub/file sub2/file\n+\tdo\n+\t\techo smudge >>\"$path\" || return 1\n+\tdone &&\n+\tyes | git -C sub add -p -- \":!sub/\" &&\n+\tgit diff --name-only --no-renames --cached >actual &&\n+\tcat >expect <<-\\EOF &&\n+\tfile\n+\tsub/file\n+\tsub2/file\n+\tEOF\n+\ttest_cmp expect actual &&\n+\tgit diff --name-only --no-renames >actual &&\n+\techo sub/sub/file >expect &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'clean with all negative' '\n+\tH=$(git rev-parse HEAD) &&\n+\tgit reset --hard $H &&\n+\ttest_when_finished \"git reset --hard $H && git clean -f\" &&\n+\tgit clean -f &&\n+\tfor path in file9 sub/file9 sub/sub/file9 sub2/file9\n+\tdo\n+\t\techo cruft >\"$path\" || return 1\n+\tdone &&\n+\tgit -C sub clean -f -- \":!sub\" &&\n+\ttest_path_is_file file9 &&\n+\ttest_path_is_missing sub/file9 &&\n+\ttest_path_is_file sub/sub/file9 &&\n+\ttest_path_is_file sub2/file9\n+'\n+\n+test_expect_success 'commit with all negative' '\n+\tH=$(git rev-parse HEAD) &&\n+\tgit reset --hard $H &&\n+\ttest_when_finished \"git reset --hard $H\" &&\n+\tfor path in file sub/file sub/sub/file sub2/file\n+\tdo\n+\t\techo smudge >>\"$path\" || return 1\n+\tdone &&\n+\tgit -C sub commit -m sample -- \":!sub/\" &&\n+\tgit diff --name-only --no-renames HEAD^ HEAD >actual &&\n+\tcat >expect <<-\\EOF &&\n+\tfile\n+\tsub/file\n+\tsub2/file\n+\tEOF\n+\ttest_cmp expect actual &&\n+\tgit diff --name-only --no-renames HEAD >actual &&\n+\techo sub/sub/file >expect &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'reset with all negative' '\n+\tH=$(git rev-parse HEAD) &&\n+\tgit reset --hard $H &&\n+\ttest_when_finished \"git reset --hard $H\" &&\n+\tfor path in file sub/file sub/sub/file sub2/file\n+\tdo\n+\t\techo smudge >>\"$path\" &&\n+\t\tgit add \"$path\" || return 1\n+\tdone &&\n+\tgit -C sub reset --quiet -- \":!sub/\" &&\n+\tgit diff --name-only --no-renames --cached >actual &&\n+\techo sub/sub/file >expect &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'grep with all negative' '\n+\tH=$(git rev-parse HEAD) &&\n+\tgit reset --hard $H &&\n+\ttest_when_finished \"git reset --hard $H\" &&\n+\tfor path in file sub/file sub/sub/file sub2/file\n+\tdo\n+\t\techo \"needle $path\" >>\"$path\" || return 1\n+\tdone &&\n+\tgit -C sub grep -h needle -- \":!sub/\" >actual &&\n+\tcat >expect <<-\\EOF &&\n+\tneedle sub/file\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'ls-files with all negative' '\n+\tgit reset --hard &&\n+\tgit -C sub ls-files -- \":!sub/\" >actual &&\n+\tcat >expect <<-\\EOF &&\n+\tfile\n+\tfile2\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'rm with all negative' '\n+\tgit reset --hard &&\n+\ttest_when_finished \"git reset --hard\" &&\n+\tgit -C sub rm -r --cached -- \":!sub/\" >actual &&\n+\tgit diff --name-only --no-renames --diff-filter=D --cached >actual &&\n+\tcat >expect <<-\\EOF &&\n+\tsub/file\n+\tsub/file2\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'stash with all negative' '\n+\tH=$(git rev-parse HEAD) &&\n+\tgit reset --hard $H &&\n+\ttest_when_finished \"git reset --hard $H\" &&\n+\tfor path in file sub/file sub/sub/file sub2/file\n+\tdo\n+\t\techo smudge >>\"$path\" || return 1\n+\tdone &&\n+\tgit -C sub stash push -m sample -- \":!sub/\" &&\n+\tgit diff --name-only --no-renames HEAD >actual &&\n+\techo sub/sub/file >expect &&\n+\ttest_cmp expect actual &&\n+\tgit stash show --name-only >actual &&\n+\tcat >expect <<-\\EOF &&\n+\tfile\n+\tsub/file\n+\tsub2/file\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.36.1-385-g60203f3fdb\n\n"},{"id":"456360","messageId":"032f01d873b0$008c1b50$01a451f0$@nexbridge.com","threadId":"57930","inReplyTo":"xmqqpmjwx8so.fsf_-_@gitster.g","subject":"RE: [PATCH] pathspec: correct an empty string used as a pathspec element","fromName":"","fromEmail":"rsbecker@nexbridge.com","sentAt":"2022-05-29T23:01:11Z","receivedAt":"2022-05-29T23:04:07Z","isPatch":true,"sender":{"key":"randall.becker@nexbridge.ca","avatar":"https://avatars.githubusercontent.com/u/28956764?v=4"},"body":"On May 29, 2022 6:40 PM, Junio C Hamano wrote:\n>Pathspecs with only negative elements did not work with some commands that\n>pass the pathspec along to a subprocess.  For instance,\n>\n>    $ git add -p -- ':!*.txt'\n>\n>should add everything except for paths ending in \".txt\", but it gets\ncomplaint from\n>underlying \"diff-index\" and aborts.\n>\n>We used to error out when a pathspec with only negative elements in it,\nlike the\n>one in the above example.  Later, 859b7f1d (pathspec:\n>don't error out on all-exclusionary pathspec patterns, 2017-02-07) updated\nthe\n>logic to add an empty string as an extra element.  The intention was to let\nthe\n>extra element to match everything and let the negative ones given by the\nuser to\n>subtract from it.\n>\n>At around the same time, we were migrating from \"an empty string is a valid\n>pathspec element that matches everything\" to \"either a dot or \":/\" is used\nto\n>match all, and an empty string is rejected\", between d426430e (pathspec:\nwarn on\n>empty strings as pathspec,\n>2016-06-22) and 9e4e8a64 (pathspec: die on empty strings as pathspec,\n2017-06-\n>06).  I think 9e4e8a64, which happened long after 859b7f1d happened, was\nnot\n>careful enough to turn the empty string 859b7f1d added to either a dot or\n\":/\".\n>\n>A care should be taken as the definition of \"everything\" depends on\n>subcommand.  For the purpose of \"add -p\", adding a \".\" to add everything in\nthe\n>current directory is the right thing to do.  But for some other commands,\n\":/\" (i.e.\n>really really everything, even things outside the current subdirectory) is\nthe right\n>choice.\n>\n>We would break commands in a big way if we get this wrong, so add a handful\nof\n>test pieces to make sure the resulting code still excludes the paths that\nare\n>expected and includes \"everything\" else.\n\nThanks for the heads up. I have to check into this for some scripting. Not\nworried but glad to know.\nThanks,\n--Randall\n\n"},{"id":"456374","messageId":"xmqqee0bx7gt.fsf@gitster.g","threadId":"57930","inReplyTo":"CAHd499BX_8fP=BdJW8cuZnwJFoqxrsiLCZ45Ke12MOsaj7M-Dw@mail.gmail.com","subject":"Re: Excluding paths with wildcard not working with add -p","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-05-30T17:20:50Z","receivedAt":"2022-05-30T17:20:59Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Robert Dailey <rcdailey.lists@gmail.com> writes:\n\n> On Sun, May 29, 2022 at 1:25 PM Junio C Hamano <gitster@pobox.com> wrote:\n>> Does this command\n>>\n>>     git add -p -- . ':^*.cs'\n>>\n>> work as you expect?\n>\n> yes I can confirm this works.\n\nThanks for confirming; you helped a lot to come up with a fix that\nis found at\n\nhttps://lore.kernel.org/git/xmqqpmjwx8so.fsf_-_@gitster.g/\n\n\n"}]}