{"thread":{"id":"51732","subject":"[PATCH] t7300-clean: demonstrate deleting nested repo with an ignored file breakage","startedAt":"2019-08-25T18:59:27Z","lastAt":"2019-10-07T18:04:18Z","messageCount":73,"participants":["SZEDER Gábor","Philip Oakley","Elijah Newren","Junio C Hamano","Denton Liu"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"381122","messageId":"20190825185918.3909-1-szeder.dev@gmail.com","threadId":"51732","inReplyTo":null,"subject":"[PATCH] t7300-clean: demonstrate deleting nested repo with an ignored file breakage","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-08-25T18:59:18Z","receivedAt":"2019-08-25T18:59:27Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"'git clean -fd' must not delete an untracked directory if it belongs\nto a different Git repository or worktree. Unfortunately, if a\n'.gitignore' rule in the outer repository happens to match a file in a\nnested repository or worktree, then something goes awry and 'git clean\n-fd' does delete the content of the nested repository's worktree\nexcept that ignored file, potentially leading to data loss.\n\nAdd a test to 't7300-clean.sh' to demonstrate this breakage.\n\nThis issue is a regression introduced in 6b1db43109 (clean: teach\nclean -d to preserve ignored paths, 2017-05-23).\n\nSigned-off-by: SZEDER Gábor \n---\n\nBEWARE: Our toplevel '.gitignore' currently contains the '*.manifest'\nrule [1], which ignores the file 'compat/win32/git.manifest' [2], so\nif you use nested worktrees in your git repo, then a 'git clean -fd'\nwill delete them.\n\n[1] 516dfb8416 (.gitignore: touch up the entries regarding Visual\n Studio, 2019-07-29)\n[2] fe90397604 (mingw: embed a manifest to trick UAC into Doing The\n Right Thing, 2019-06-27)\n\n\n t/t7300-clean.sh | 22 ++++++++++++++++++++++\n 1 file changed, 22 insertions(+)\n\ndiff --git a/t/t7300-clean.sh b/t/t7300-clean.sh\nindex a2c45d1902..d01fd120ab 100755\n--- a/t/t7300-clean.sh\n+++ b/t/t7300-clean.sh\n@@ -669,6 +669,28 @@ test_expect_success 'git clean -d skips untracked dirs containing ignored files'\n \ttest_path_is_missing foo/b/bb\n '\n \n+test_expect_failure 'git clean -d skips nested repo containing ignored files' '\n+\ttest_when_finished \"rm -rf nested-repo-with-ignored-file\" &&\n+\n+\tgit init nested-repo-with-ignored-file &&\n+\t(\n+\t\tcd nested-repo-with-ignored-file &&\n+\t\t>file &&\n+\t\tgit add file &&\n+\t\tgit commit -m Initial &&\n+\n+\t\t# This file is ignored by a .gitignore rule in the outer repo\n+\t\t# added in the previous test.\n+\t\t>ignoreme\n+\t) &&\n+\n+\tgit clean -fd &&\n+\n+\ttest_path_is_file nested-repo-with-ignored-file/.git/index &&\n+\ttest_path_is_file nested-repo-with-ignored-file/ignoreme &&\n+\ttest_path_is_file nested-repo-with-ignored-file/file\n+'\n+\n test_expect_success MINGW 'handle clean & core.longpaths = false nicely' '\n \ttest_config core.longpaths false &&\n \ta50=aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa &&\n-- \n2.23.0.331.g4e51dcdf11\n\n"},{"id":"381126","messageId":"20190825203452.GO20404@szeder.dev","threadId":"51732","inReplyTo":"20190825185918.3909-1-szeder.dev@gmail.com","subject":"Re: [PATCH] t7300-clean: demonstrate deleting nested repo with an ignored file breakage","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-08-25T20:34:52Z","receivedAt":"2019-08-25T20:34:59Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Sun, Aug 25, 2019 at 08:59:18PM +0200, SZEDER Gábor wrote:\n> 'git clean -fd' must not delete an untracked directory if it belongs\n> to a different Git repository or worktree. Unfortunately, if a\n> '.gitignore' rule in the outer repository happens to match a file in a\n> nested repository or worktree, then something goes awry and 'git clean\n> -fd' does delete the content of the nested repository's worktree\n> except that ignored file, potentially leading to data loss.\n> \n> Add a test to 't7300-clean.sh' to demonstrate this breakage.\n> \n> This issue is a regression introduced in 6b1db43109 (clean: teach\n> clean -d to preserve ignored paths, 2017-05-23).\n> \n> Signed-off-by: SZEDER Gábor \n> ---\n> \n> BEWARE: Our toplevel '.gitignore' currently contains the '*.manifest'\n> rule [1], which ignores the file 'compat/win32/git.manifest' [2], so\n> if you use nested worktrees in your git repo, then a 'git clean -fd'\n> will delete them.\n\nOK, singling out that manifest file is just nonsense, any object file,\netc... in the nested worktree/repo can trigger the same issue just as\nwell.\n\n(It just so happened that when I ran 'git clean -fd' I had a nested\nworktree where I haven't build anything yet, and besides the .git file\nonly that 'git.manifest' file survived in the nested worktree, and then\nI got misled by it.)\n \n"},{"id":"381129","messageId":"20f71f02-4895-fe57-8704-a44494b971b9@iee.email","threadId":"51732","inReplyTo":"20190825185918.3909-1-szeder.dev@gmail.com","subject":"Re: [PATCH] t7300-clean: demonstrate deleting nested repo with an ignored file breakage","fromName":"Philip Oakley","fromEmail":"philipoakley@iee.email","sentAt":"2019-08-25T22:32:28Z","receivedAt":"2019-08-25T22:35:41Z","isPatch":true,"sender":{"key":"philipoakley@iee.email","avatar":"https://avatars.githubusercontent.com/u/914343?v=4"},"body":"Hi Szeder,\n\nOn 25/08/2019 19:59, SZEDER Gábor wrote:\n> 'git clean -fd' must not delete an untracked directory if it belongs\ns/untracked//\nI don't believe it should matter either way for a sub-module \n(sub-directory).\n> to a different Git repository or worktree.\nmsybr split the assertion from the fault explanation.\n> Unfortunately, if a\n> '.gitignore' rule in the outer repository happens to match a file in a\n> nested repository or worktree, then something goes awry and 'git clean\n> -fd' does delete the content of the nested repository's worktree\ngood so far.\n> except that ignored file, potentially leading to data loss.\nthis appears at cross purposes as the description has changed from \n'ignored/untracked directory' to 'ignored file'. I'm not sure which part \nthe data loss is meant to refer to.\n>\n> Add a test to 't7300-clean.sh' to demonstrate this breakage.\n>\n> This issue is a regression introduced in 6b1db43109 (clean: teach\n> clean -d to preserve ignored paths, 2017-05-23).\n>\n> Signed-off-by: SZEDER Gábor \n> ---\n>\n> BEWARE: Our toplevel '.gitignore' currently contains the '*.manifest'\n> rule [1], which ignores the file 'compat/win32/git.manifest' [2], so\n> if you use nested worktrees in your git repo, then a 'git clean -fd'\n> will delete them.\n>\n> [1] 516dfb8416 (.gitignore: touch up the entries regarding Visual\n> Studio, 2019-07-29)\n> [2] fe90397604 (mingw: embed a manifest to trick UAC into Doing The\n> Right Thing, 2019-06-27)\n>\n>\n> t/t7300-clean.sh | 22 ++++++++++++++++++++++\n> 1 file changed, 22 insertions(+)\n>\n> diff --git a/t/t7300-clean.sh b/t/t7300-clean.sh\n> index a2c45d1902..d01fd120ab 100755\n> --- a/t/t7300-clean.sh\n> +++ b/t/t7300-clean.sh\n> @@ -669,6 +669,28 @@ test_expect_success 'git clean -d skips untracked dirs containing ignored files'\n> \ttest_path_is_missing foo/b/bb\n> '\n> \n> +test_expect_failure 'git clean -d skips nested repo containing ignored files' '\n> +\ttest_when_finished \"rm -rf nested-repo-with-ignored-file\" &&\n> +\n> +\tgit init nested-repo-with-ignored-file &&\n> +\t(\n> +\t\tcd nested-repo-with-ignored-file &&\n> +\t\t>file &&\n> +\t\tgit add file &&\n> +\t\tgit commit -m Initial &&\n> +\n> +\t\t# This file is ignored by a .gitignore rule in the outer repo\n> +\t\t# added in the previous test.\n> +\t\t>ignoreme\n> +\t) &&\n> +\n> +\tgit clean -fd &&\n> +\n> +\ttest_path_is_file nested-repo-with-ignored-file/.git/index &&\n> +\ttest_path_is_file nested-repo-with-ignored-file/ignoreme &&\n> +\ttest_path_is_file nested-repo-with-ignored-file/file\n> +'\n> +\n> test_expect_success MINGW 'handle clean & core.longpaths = false nicely' '\n> \ttest_config core.longpaths false &&\n> \ta50=aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa &&\n\n"},{"id":"381166","messageId":"20190826074841.GP20404@szeder.dev","threadId":"51732","inReplyTo":"20f71f02-4895-fe57-8704-a44494b971b9@iee.email","subject":"Re: [PATCH] t7300-clean: demonstrate deleting nested repo with an ignored file breakage","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-08-26T07:48:41Z","receivedAt":"2019-08-26T07:48:47Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Sun, Aug 25, 2019 at 11:32:28PM +0100, Philip Oakley wrote:\n> Hi Szeder,\n> \n> On 25/08/2019 19:59, SZEDER Gábor wrote:\n> >'git clean -fd' must not delete an untracked directory if it belongs\n> s/untracked//\n> I don't believe it should matter either way for a sub-module\n> (sub-directory).\n\nI just paraphrased the documentation of the '-d' option for a bit of\ncontext.\n\n Remove untracked directories in addition to untracked files. If an\n untracked directory is managed by a different Git repository, it is\n not removed by default. Use -f option twice if you really want to\n remove such a directory.\n\n> >to a different Git repository or worktree.\n> msybr split the assertion from the fault explanation.\n> > Unfortunately, if a\n> >'.gitignore' rule in the outer repository happens to match a file in a\n> >nested repository or worktree, then something goes awry and 'git clean\n> >-fd' does delete the content of the nested repository's worktree\n> good so far.\n> >except that ignored file, potentially leading to data loss.\n> this appears at cross purposes as the description has changed from\n> 'ignored/untracked directory' to 'ignored file'.\n\nThe description does not mention any ignored directories.\n\n> I'm not sure which part the\n> data loss is meant to refer to.\n\nWell, there is only one part where the description talks about stuff\ngetting deleted... and that's what it refers to :)\n\n> >Add a test to 't7300-clean.sh' to demonstrate this breakage.\n> >\n> >This issue is a regression introduced in 6b1db43109 (clean: teach\n> >clean -d to preserve ignored paths, 2017-05-23).\n> >\n> >Signed-off-by: SZEDER Gábor \n> >---\n\nOn a related note, 'git clean -fdx' does leave the nested repository\nor worktree intact in the same situation, as it should.\n\n> > t/t7300-clean.sh | 22 ++++++++++++++++++++++\n> > 1 file changed, 22 insertions(+)\n> >\n> >diff --git a/t/t7300-clean.sh b/t/t7300-clean.sh\n> >index a2c45d1902..d01fd120ab 100755\n> >--- a/t/t7300-clean.sh\n> >+++ b/t/t7300-clean.sh\n> >@@ -669,6 +669,28 @@ test_expect_success 'git clean -d skips untracked dirs containing ignored files'\n> > \ttest_path_is_missing foo/b/bb\n> > '\n> >+test_expect_failure 'git clean -d skips nested repo containing ignored files' '\n> >+\ttest_when_finished \"rm -rf nested-repo-with-ignored-file\" &&\n> >+\n> >+\tgit init nested-repo-with-ignored-file &&\n> >+\t(\n> >+\t\tcd nested-repo-with-ignored-file &&\n> >+\t\t>file &&\n> >+\t\tgit add file &&\n> >+\t\tgit commit -m Initial &&\n> >+\n> >+\t\t# This file is ignored by a .gitignore rule in the outer repo\n> >+\t\t# added in the previous test.\n> >+\t\t>ignoreme\n> >+\t) &&\n> >+\n> >+\tgit clean -fd &&\n> >+\n> >+\ttest_path_is_file nested-repo-with-ignored-file/.git/index &&\n> >+\ttest_path_is_file nested-repo-with-ignored-file/ignoreme &&\n> >+\ttest_path_is_file nested-repo-with-ignored-file/file\n> >+'\n> >+\n> > test_expect_success MINGW 'handle clean & core.longpaths = false nicely' '\n> > \ttest_config core.longpaths false &&\n> > \ta50=aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa &&\n> \n"},{"id":"381867","messageId":"20190905154735.29784-2-newren@gmail.com","threadId":"51732","inReplyTo":"20190905154735.29784-1-newren@gmail.com","subject":"[RFC PATCH v2 01/12] t7300: Add some testcases showing failure to clean specified pathspecs","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2019-09-05T15:47:24Z","receivedAt":"2019-09-05T15:47:43Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Someone brought me a testcase where multiple git-clean invocations were\nrequired to clean out unwanted files:\n mkdir d{1,2}\n touch d{1,2}/ut\n touch d1/t && git add d1/t\nWith this setup, the user would need to run\n git clean -ffd */ut\ntwice to delete both ut files.\n\nA little testing showed some interesting variants:\n * If only one of those two ut files existed (either one), then only one\n clean command would be necessary.\n * If both directories had tracked files, then only one git clean would\n be necessary to clean both files.\n * If both directories had no tracked files then the clean command above\n would never clean either of the untracked files despite the pathspec\n explicitly calling both of them out.\n\nA bisect showed that the failure to clean out the files started with\ncommit cf424f5fd89b (\"clean: respect pathspecs with \"-d\", 2014-03-10).\nHowever, that pointed to a separate issue: while the \"-d\" flag was used\nby the original user who showed me this problem, that flag should have\nbeen irrelevant to this problem. Testing again without the \"-d\" flag\nshowed that the same buggy behavior exists without using that flag, and\nhas in fact existed since before cf424f5fd89b.\n\nAdd testcases showing that multiple untracked files within entirely\nuntracked directories cannot be cleaned when specifying these files to\ngit clean via pathspecs.\n\nSigned-off-by: Elijah Newren \n---\n t/t7300-clean.sh | 32 ++++++++++++++++++++++++++++++++\n 1 file changed, 32 insertions(+)\n\ndiff --git a/t/t7300-clean.sh b/t/t7300-clean.sh\nindex d01fd120ab..2c254c773c 100755\n--- a/t/t7300-clean.sh\n+++ b/t/t7300-clean.sh\n@@ -691,6 +691,38 @@ test_expect_failure 'git clean -d skips nested repo containing ignored files' '\n \ttest_path_is_file nested-repo-with-ignored-file/file\n '\n \n+test_expect_failure 'git clean handles being told what to clean' '\n+\tmkdir -p d1 d2 &&\n+\ttouch d1/ut d2/ut &&\n+\tgit clean -f */ut &&\n+\ttest_path_is_missing d1/ut &&\n+\ttest_path_is_missing d2/ut\n+'\n+\n+test_expect_failure 'git clean handles being told what to clean, with -d' '\n+\tmkdir -p d1 d2 &&\n+\ttouch d1/ut d2/ut &&\n+\tgit clean -ffd */ut &&\n+\ttest_path_is_missing d1/ut &&\n+\ttest_path_is_missing d2/ut\n+'\n+\n+test_expect_failure 'git clean works if a glob is passed without -d' '\n+\tmkdir -p d1 d2 &&\n+\ttouch d1/ut d2/ut &&\n+\tgit clean -f \"*ut\" &&\n+\ttest_path_is_missing d1/ut &&\n+\ttest_path_is_missing d2/ut\n+'\n+\n+test_expect_failure 'git clean works if a glob is passed with -d' '\n+\tmkdir -p d1 d2 &&\n+\ttouch d1/ut d2/ut &&\n+\tgit clean -ffd \"*ut\" &&\n+\ttest_path_is_missing d1/ut &&\n+\ttest_path_is_missing d2/ut\n+'\n+\n test_expect_success MINGW 'handle clean & core.longpaths = false nicely' '\n \ttest_config core.longpaths false &&\n \ta50=aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa &&\n-- \n2.22.1.11.g45a39ee867\n\n"},{"id":"381868","messageId":"20190905154735.29784-1-newren@gmail.com","threadId":"51732","inReplyTo":"20190825185918.3909-1-szeder.dev@gmail.com","subject":"[RFC PATCH v2 00/12] Fix some git clean issues","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2019-09-05T15:47:23Z","receivedAt":"2019-09-05T15:47:44Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"NOTE: This series builds on sg/clean-nested-repo-with-ignored, as it\n (among other things) modifies his testcase from expect_failure\n to expect_success. Also, Peff is probably the only one who\n remembers v1 (and even he may have forgotten it): v1 was posted\n a year and a half ago.\n\nThis patch series fixes a few issues with git-clean:\n * Failure to clean when multiple pathspecs are specified, reported both\n in April 2018[1] and again in May 2019[2].\n * Failure to preserve both tracked and untracked files within a nested\n Git repository reported a few weeks ago by SZEDER[3].\n\n[1] https://public-inbox.org/git/20180405173446.32372-4-newren@gmail.com/\n[2] https://public-inbox.org/git/20190531183651.10067-1-rafa.almas@gmail.com/\n[3] https://public-inbox.org/git/20190825185918.3909-1-szeder.dev@gmail.com/\n\nI still never got answers to some questions in v1 of my RFC, so after\nconsiderable thought I eventually decided to:\n\n * Declare the existing documentation to be ambiguous and hard to\n interpret correctly; modified the documentation to clearly\n document 'correct behavior' with how different pieces interact.\n \n * Overrule four regression tests as having the wrong *expectation*,\n and modify them to have a correct one. That sounds like a\n backward compatibility issue BUT: The tests were written to check\n for issues that were orthogonal to the pieces that mattered in this\n series and thus couldn't be viewed as actually having an opinion\n on correct behavior on my issues; rather, they were simply\n reinforcing existing (buggy) implementation results.\n\n * Add a few tests which actually check relevant interactions of\n parameters and setup, to make this area less ambiguous. (Though\n one of them was added by SZEDER before my patches, and I should\n probably add a couple more tests...)\n\nHelp from reviewers:\n\nThe biggest area I need help from reviewers is to look at the commit\nmessages for patches 9 and 10, to see if folks agree with my\ndeclaration of 'correct behavior' and my changes to the regression\ntests. If those are good, this series can proceed. If they aren't,\nand someone else can't provide an alternate easy-to-explain 'correct\nbehavior' that we should implement and which is devoid of ugly edge\ncases for users, then this patch series may languish for another few\nyears.\n\nOther notes:\n * Patches 1-6 were included in v1 and have almost no changes (just one\n fix pointed out by Peff).\n * Patch 6's commit message has some additional RFC-related comments\n and questions, one of which ties in with Patch 9.\n * Patch 7 was added as per (old) conversation with Peff.\n * Patch 9 & 10 are in most need of review (see above); each has\n lengthy commit messages.\n * It would be nice if someone knows whether the codepath edited in\n Patch 12 is dead code. If so, we could change that patch to just\n drop that if-check block. If it's not dead code, that patch fixes\n what is probably a rare but ugly bug.\n\nElijah Newren (12):\n t7300: Add some testcases showing failure to clean specified pathspecs\n dir: fix typo in comment\n dir: fix off-by-one error in match_pathspec_item\n dir: Directories should be checked for matching pathspecs too\n dir: Make the DO_MATCH_SUBMODULE code reusable for a non-submodule\n case\n dir: If our pathspec might match files under a dir, recurse into it\n dir: add commentary explaining match_pathspec_item's return value\n git-clean.txt: do not claim we will delete files with -n/--dry-run\n clean: disambiguate the definition of -d\n clean: avoid removing untracked files in a nested git repository\n clean: rewrap overly long line\n clean: fix theoretical path corruption\n\n Documentation/git-clean.txt | 16 +++++-----\n builtin/clean.c | 17 ++++++++--\n dir.c | 63 +++++++++++++++++++++++++++----------\n dir.h | 8 +++--\n t/t7300-clean.sh | 44 +++++++++++++++++++++++---\n 5 files changed, 114 insertions(+), 34 deletions(-)\n\n-- \n2.22.1.11.g45a39ee867\n\n"},{"id":"381869","messageId":"20190905154735.29784-3-newren@gmail.com","threadId":"51732","inReplyTo":"20190905154735.29784-1-newren@gmail.com","subject":"[RFC PATCH v2 02/12] dir: fix typo in comment","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2019-09-05T15:47:25Z","receivedAt":"2019-09-05T15:47:46Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Signed-off-by: Elijah Newren \n---\n dir.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/dir.c b/dir.c\nindex d021c908e5..a9168bed96 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -139,7 +139,7 @@ static size_t common_prefix_len(const struct pathspec *pathspec)\n \t * \":(icase)path\" is treated as a pathspec full of\n \t * wildcard. In other words, only prefix is considered common\n \t * prefix. If the pathspec is abc/foo abc/bar, running in\n-\t * subdir xyz, the common prefix is still xyz, not xuz/abc as\n+\t * subdir xyz, the common prefix is still xyz, not xyz/abc as\n \t * in non-:(icase).\n \t */\n \tGUARD_PATHSPEC(pathspec,\n-- \n2.22.1.11.g45a39ee867\n\n"},{"id":"381870","messageId":"20190905154735.29784-5-newren@gmail.com","threadId":"51732","inReplyTo":"20190905154735.29784-1-newren@gmail.com","subject":"[RFC PATCH v2 04/12] dir: Directories should be checked for matching pathspecs too","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2019-09-05T15:47:27Z","receivedAt":"2019-09-05T15:47:47Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Even if a directory doesn't match a pathspec, it is possible, depending\non the precise pathspecs, that some file underneath it might. So we\nspecial case and recurse into the directory for such situations. However,\nwe previously always added any untracked directory that we recursed into\nto the list of untracked paths, regardless of whether the directory\nitself matched the pathspec.\n\nFor the case of git-clean and a set of pathspecs of \"dir/file\" and \"more\",\nthis caused a problem because we'd end up with dir entries for both of\n \"dir\"\n \"dir/file\"\nThen correct_untracked_entries() would try to helpfully prune duplicates\nfor us by removing \"dir/file\" since it's under \"dir\", leaving us with\n \"dir\"\nSince the original pathspec only had \"dir/file\", the only entry left\ndoesn't match and leaves nothing to be removed. (Note that if only one\npathspec was specified, e.g. only \"dir/file\", then the common_prefix_len\noptimizations in fill_directory would cause us to bypass this problem,\nmaking it appear in simple tests that we could correctly remove manually\nspecified pathspecs.)\n\nFix this by actually checking whether the directory we are about to add\nto the list of dir entries actually matches the pathspec; only do this\nmatching check after we have already returned from recursing into the\ndirectory.\n\nSigned-off-by: Elijah Newren \n---\n dir.c | 5 +++++\n t/t7300-clean.sh | 4 ++--\n 2 files changed, 7 insertions(+), 2 deletions(-)\n\ndiff --git a/dir.c b/dir.c\nindex bf1a74799e..76a3c3894b 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -1951,6 +1951,11 @@ static enum path_treatment read_directory_recursive(struct dir_struct *dir,\n \t\t\t\t\t\t\t check_only, stop_at_first_file, pathspec);\n \t\t\tif (subdir_state > dir_state)\n \t\t\t\tdir_state = subdir_state;\n+\n+\t\t\tif (!match_pathspec(istate, pathspec, path.buf, path.len,\n+\t\t\t\t\t 0 /* prefix */, NULL,\n+\t\t\t\t\t 0 /* do NOT special case dirs */))\n+\t\t\t\tstate = path_none;\n \t\t}\n \n \t\tif (check_only) {\ndiff --git a/t/t7300-clean.sh b/t/t7300-clean.sh\nindex 2c254c773c..12617158db 100755\n--- a/t/t7300-clean.sh\n+++ b/t/t7300-clean.sh\n@@ -699,7 +699,7 @@ test_expect_failure 'git clean handles being told what to clean' '\n \ttest_path_is_missing d2/ut\n '\n \n-test_expect_failure 'git clean handles being told what to clean, with -d' '\n+test_expect_success 'git clean handles being told what to clean, with -d' '\n \tmkdir -p d1 d2 &&\n \ttouch d1/ut d2/ut &&\n \tgit clean -ffd */ut &&\n@@ -715,7 +715,7 @@ test_expect_failure 'git clean works if a glob is passed without -d' '\n \ttest_path_is_missing d2/ut\n '\n \n-test_expect_failure 'git clean works if a glob is passed with -d' '\n+test_expect_success 'git clean works if a glob is passed with -d' '\n \tmkdir -p d1 d2 &&\n \ttouch d1/ut d2/ut &&\n \tgit clean -ffd \"*ut\" &&\n-- \n2.22.1.11.g45a39ee867\n\n"},{"id":"381871","messageId":"20190905154735.29784-4-newren@gmail.com","threadId":"51732","inReplyTo":"20190905154735.29784-1-newren@gmail.com","subject":"[RFC PATCH v2 03/12] dir: fix off-by-one error in match_pathspec_item","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2019-09-05T15:47:26Z","receivedAt":"2019-09-05T15:47:48Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"For a pathspec like 'foo/bar' comparing against a path named \"foo/\",\nnamelen will be 4, and match[namelen] will be 'b'. The correct location\nof the directory separator is namelen-1.\n\nThe reason the code worked anyway was that the following code immediately\nchecked whether the first matchlen characters matched (which they do) and\nthen bailed and return MATCHED_RECURSIVELY anyway since wildmatch doesn't\nhave the ability to check if \"name\" can be matched as a directory (or\nprefix) against the pathspec.\n\nSigned-off-by: Elijah Newren \n---\n dir.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/dir.c b/dir.c\nindex a9168bed96..bf1a74799e 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -356,8 +356,9 @@ static int match_pathspec_item(const struct index_state *istate,\n \t/* Perform checks to see if \"name\" is a super set of the pathspec */\n \tif (flags & DO_MATCH_SUBMODULE) {\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-\t\t (match[namelen] == '/') &&\n+\t\t (match[namelen-offset] == '/') &&\n \t\t !ps_strncmp(item, match, name, namelen))\n \t\t\treturn MATCHED_RECURSIVELY;\n \n-- \n2.22.1.11.g45a39ee867\n\n"},{"id":"381872","messageId":"20190905154735.29784-6-newren@gmail.com","threadId":"51732","inReplyTo":"20190905154735.29784-1-newren@gmail.com","subject":"[RFC PATCH v2 05/12] dir: Make the DO_MATCH_SUBMODULE code reusable for a non-submodule case","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2019-09-05T15:47:28Z","receivedAt":"2019-09-05T15:47:48Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"The specific checks done in match_pathspec_item for the DO_MATCH_SUBMODULE\ncase are useful for other cases which have nothing to do with submodules.\nRename this constant; a subsequent commit will make use of this change.\n\nSigned-off-by: Elijah Newren \n---\n dir.c | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/dir.c b/dir.c\nindex 76a3c3894b..b4d656192e 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -273,7 +273,7 @@ static int do_read_blob(const struct object_id *oid, struct oid_stat *oid_stat,\n \n #define DO_MATCH_EXCLUDE (1<<0)\n #define DO_MATCH_DIRECTORY (1<<1)\n-#define DO_MATCH_SUBMODULE (1<<2)\n+#define DO_MATCH_LEADING_PATHSPEC (1<<2)\n \n /*\n * Does 'match' match the given name?\n@@ -354,7 +354,7 @@ 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 super set of the pathspec */\n-\tif (flags & DO_MATCH_SUBMODULE) {\n+\tif (flags & DO_MATCH_LEADING_PATHSPEC) {\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@@ -498,7 +498,7 @@ int submodule_path_match(const struct index_state *istate,\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_SUBMODULE);\n+\t\t\t\t\tDO_MATCH_LEADING_PATHSPEC);\n \treturn matched;\n }\n \n-- \n2.22.1.11.g45a39ee867\n\n"},{"id":"381874","messageId":"20190905154735.29784-8-newren@gmail.com","threadId":"51732","inReplyTo":"20190905154735.29784-1-newren@gmail.com","subject":"[RFC PATCH v2 07/12] dir: add commentary explaining match_pathspec_item's return value","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2019-09-05T15:47:30Z","receivedAt":"2019-09-05T15:47:51Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"The way match_pathspec_item() handles names and pathspecs with trailing\nslash characters, in conjunction with special options like\nDO_MATCH_DIRECTORY and DO_MATCH_LEADING_PATHSPEC were non-obvious, and\nbroken until this patch series. Add a table in a comment explaining the\nintent of how these work.\n\nSigned-off-by: Elijah Newren \n---\n dir.c | 27 +++++++++++++++++++--------\n 1 file changed, 19 insertions(+), 8 deletions(-)\n\ndiff --git a/dir.c b/dir.c\nindex 47c0a99cb5..3b2fe1701c 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -276,16 +276,27 @@ static int do_read_blob(const struct object_id *oid, struct oid_stat *oid_stat,\n #define DO_MATCH_LEADING_PATHSPEC (1<<2)\n \n /*\n- * Does 'match' match the given name?\n- * A match is found if\n+ * Does the given pathspec match the given name? A match is found if\n *\n- * (1) the 'match' string is leading directory of 'name', or\n- * (2) the 'match' string is a wildcard and matches 'name', or\n- * (3) the 'match' string is exactly the same as 'name'.\n+ * (1) the pathspec string is leading directory of 'name' (\"RECURSIVELY\"), or\n+ * (2) the pathspec string has a leading part matching 'name' (\"LEADING\"), or\n+ * (3) the pathspec string is a wildcard and matches 'name' (\"WILDCARD\"), or\n+ * (4) the pathspec string is exactly the same as 'name' (\"EXACT\").\n *\n- * and the return value tells which case it was.\n+ * Return value tells which case it was (1-4), or 0 when there is no match.\n *\n- * It returns 0 when there is no match.\n+ * It may be instructive to look at a small table of concrete examples\n+ * to understand the differences between 1, 2, and 4:\n+ *\n+ * Pathspecs\n+ * | a/b | a/b/ | a/b/c\n+ * ------+-----------+-----------+------------\n+ * a/b | EXACT | EXACT[1] | LEADING[2]\n+ * Names a/b/ | RECURSIVE | EXACT | LEADING[2]\n+ * a/b/c | RECURSIVE | RECURSIVE | EXACT\n+ *\n+ * [1] Only if DO_MATCH_DIRECTORY is passed; otherwise, this is NOT a match.\n+ * [2] Only if DO_MATCH_LEADING_PATHSPEC is passed; otherwise, not a match.\n */\n static int match_pathspec_item(const struct index_state *istate,\n \t\t\t const struct pathspec_item *item, int prefix,\n@@ -353,7 +364,7 @@ static int match_pathspec_item(const struct index_state *istate,\n \t\t\t item->nowildcard_len - prefix))\n \t\treturn MATCHED_FNMATCH;\n \n-\t/* Perform checks to see if \"name\" is a super set of the pathspec */\n+\t/* Perform checks to see if \"name\" is a leading string of the pathspec */\n \tif (flags & DO_MATCH_LEADING_PATHSPEC) {\n \t\t/* name is a literal prefix of the pathspec */\n \t\tint offset = name[namelen-1] == '/' ? 1 : 0;\n-- \n2.22.1.11.g45a39ee867\n\n"},{"id":"381873","messageId":"20190905154735.29784-7-newren@gmail.com","threadId":"51732","inReplyTo":"20190905154735.29784-1-newren@gmail.com","subject":"[RFC PATCH v2 06/12] dir: If our pathspec might match files under a dir, recurse into it","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2019-09-05T15:47:29Z","receivedAt":"2019-09-05T15:47:52Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"For git clean, if a directory is entirely untracked and the user did not\nspecify -d (corresponding to DIR_SHOW_IGNORED_TOO), then we usually do\nnot want to remove that directory and thus do not recurse into it.\nHowever, if the user manually specified specific (or even globbed) paths\nsomewhere under that directory to remove, then we need to recurse into\nthe directory to make sure we remove the relevant paths under that\ndirectory as the user requested.\n\nNote that this does not mean that the recursed-into directory will be\nadded to dir->entries for later removal; as of a few commits earlier in\nthis series, there is another more strict match check that is run after\nreturning from a recursed-into directory before deciding to add it to the\nlist of entries. Therefore, this will only result in files underneath\nthe given directory which match one of the pathspecs being added to the\nentries list.\n\nTwo particular considerations for this patch:\n\n * If we want to only recurse into a directory when it is specifically\n matched rather than matched-via-glob (e.g. '*.c'), then we could do\n so via making the final non-zero return in match_pathspec_item be\n MATCHED_RECURSIVELY instead of MATCHED_RECURSIVELY_LEADING_PATHSPEC.\n (See final patch of this RFC series for details; note that the\n relative order of MATCHED_RECURSIVELY_LEADING_PATHSPEC and\n MATCHED_RECURSIVELY are important for such a change.))\n\n * There is a growing amount of logic in read_directory_recursive() for\n deciding whether to recurse into a subdirectory. However, there is\n a comment immediately preceding this logic that says to recurse if\n instructed by treat_path(). It may be better for the logic in\n read_directory_recursive() to be moved to treat_path() (or another\n function it calls, such as treat_directory()), but I did not feel\n strongly about this and just left the logic where it was while\n adding to it. Do others have strong opinions on this?\n\nSigned-off-by: Elijah Newren \n---\n dir.c | 10 ++++++----\n dir.h | 5 +++--\n t/t7300-clean.sh | 4 ++--\n 3 files changed, 11 insertions(+), 8 deletions(-)\n\ndiff --git a/dir.c b/dir.c\nindex b4d656192e..47c0a99cb5 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -360,7 +360,7 @@ static int match_pathspec_item(const struct index_state *istate,\n \t\tif ((namelen < matchlen) &&\n \t\t (match[namelen-offset] == '/') &&\n \t\t !ps_strncmp(item, match, name, namelen))\n-\t\t\treturn MATCHED_RECURSIVELY;\n+\t\t\treturn MATCHED_RECURSIVELY_LEADING_PATHSPEC;\n \n \t\t/* name\" doesn't match up to the first wild character */\n \t\tif (item->nowildcard_len < item->len &&\n@@ -377,7 +377,7 @@ static int match_pathspec_item(const struct index_state *istate,\n \t\t * The submodules themselves will be able to perform more\n \t\t * accurate matching to determine if the pathspec matches.\n \t\t */\n-\t\treturn MATCHED_RECURSIVELY;\n+\t\treturn MATCHED_RECURSIVELY_LEADING_PATHSPEC;\n \t}\n \n \treturn 0;\n@@ -1939,8 +1939,10 @@ static enum path_treatment read_directory_recursive(struct dir_struct *dir,\n \t\t/* recurse into subdir if instructed by treat_path */\n \t\tif ((state == path_recurse) ||\n \t\t\t((state == path_untracked) &&\n-\t\t\t (dir->flags & DIR_SHOW_IGNORED_TOO) &&\n-\t\t\t (get_dtype(cdir.de, istate, path.buf, path.len) == DT_DIR))) {\n+\t\t\t (get_dtype(cdir.de, istate, path.buf, path.len) == DT_DIR) &&\n+\t\t\t ((dir->flags & DIR_SHOW_IGNORED_TOO) ||\n+\t\t\t do_match_pathspec(istate, pathspec, path.buf, path.len,\n+\t\t\t\t\t baselen, NULL, DO_MATCH_LEADING_PATHSPEC) == MATCHED_RECURSIVELY_LEADING_PATHSPEC))) {\n \t\t\tstruct untracked_cache_dir *ud;\n \t\t\tud = lookup_untracked(dir->untracked, untracked,\n \t\t\t\t\t path.buf + baselen,\ndiff --git a/dir.h b/dir.h\nindex 680079bbe3..46c238ab49 100644\n--- a/dir.h\n+++ b/dir.h\n@@ -211,8 +211,9 @@ int count_slashes(const char *s);\n * when populating the seen[] array.\n */\n #define MATCHED_RECURSIVELY 1\n-#define MATCHED_FNMATCH 2\n-#define MATCHED_EXACTLY 3\n+#define MATCHED_RECURSIVELY_LEADING_PATHSPEC 2\n+#define MATCHED_FNMATCH 3\n+#define MATCHED_EXACTLY 4\n int simple_length(const char *match);\n int no_wildcard(const char *string);\n char *common_prefix(const struct pathspec *pathspec);\ndiff --git a/t/t7300-clean.sh b/t/t7300-clean.sh\nindex 12617158db..d83aeb7dc2 100755\n--- a/t/t7300-clean.sh\n+++ b/t/t7300-clean.sh\n@@ -691,7 +691,7 @@ test_expect_failure 'git clean -d skips nested repo containing ignored files' '\n \ttest_path_is_file nested-repo-with-ignored-file/file\n '\n \n-test_expect_failure 'git clean handles being told what to clean' '\n+test_expect_success 'git clean handles being told what to clean' '\n \tmkdir -p d1 d2 &&\n \ttouch d1/ut d2/ut &&\n \tgit clean -f */ut &&\n@@ -707,7 +707,7 @@ test_expect_success 'git clean handles being told what to clean, with -d' '\n \ttest_path_is_missing d2/ut\n '\n \n-test_expect_failure 'git clean works if a glob is passed without -d' '\n+test_expect_success 'git clean works if a glob is passed without -d' '\n \tmkdir -p d1 d2 &&\n \ttouch d1/ut d2/ut &&\n \tgit clean -f \"*ut\" &&\n-- \n2.22.1.11.g45a39ee867\n\n"},{"id":"381875","messageId":"20190905154735.29784-11-newren@gmail.com","threadId":"51732","inReplyTo":"20190905154735.29784-1-newren@gmail.com","subject":"[RFC PATCH v2 10/12] clean: avoid removing untracked files in a nested git repository","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2019-09-05T15:47:33Z","receivedAt":"2019-09-05T15:47:53Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Users expect files in a nested git repository to be left alone unless\nsufficiently forced (with two -f's). Unfortunately, in certain\ncircumstances, git would delete both tracked (and possibly dirty) files\nand untracked files within a nested repository. To explain how this\nhappens, let's contrast a couple cases. First, take the following\nexample setup (which assumes we are already within a git repo):\n\n git init nested\n cd nested\n >tracked\n git add tracked\n git commit -m init\n >untracked\n cd ..\n\nIn this setup, everything works as expected; running 'git clean -fd'\nwill result in fill_directory() returning the following paths:\n nested/\n nested/tracked\n nested/untracked\nand then correct_untracked_entries() would notice this can be compressed\nto\n nested/\nand then since \"nested/\" is a directory, we would call\nremove_dirs(\"nested/\", ...), which would\ncheck is_nonbare_repository_dir() and then decide to skip it.\n\nHowever, if someone also creates an ignored file:\n >nested/ignored\nthen running 'git clean -fd' would result in fill_directory() returning\nthe same paths:\n nested/\n nested/tracked\n nested/untracked\nbut correct_untracked_entries() will notice that we had ignored entries\nunder nested/ and thus simplify this list to\n nested/tracked\n nested/untracked\nSince these are not directories, we do not call remove_dirs() which was\nthe only place that had the is_nonbare_repository_dir() safety check --\nresulting in us deleting both the untracked file and the tracked (and\npossibly dirty) file.\n\nOne possible fix for this issue would be walking the parent directories\nof each path and checking if they represent nonbare repositories, but\nthat would be wasteful. Even if we added caching of some sort, it's\nstill a waste because we should have been able to check that \"nested/\"\nrepresented a nonbare repository before even descending into it in the\nfirst place. Add a DIR_SKIP_NESTED_GIT flag to dir_struct.flags and use\nit to prevent fill_directory() and friends from descending into nested\ngit repos.\n\nWith this change, we also modify two regression tests added in commit\n91479b9c72f1 (\"t7300: add tests to document behavior of clean and nested\ngit\", 2015-06-15). That commit, nor its series, nor the six previous\niterations of that series on the mailing list discussed why those tests\ncoded the expectation they did. In fact, it appears their purpose was\nsimply to test _existing_ behavior to make sure that the performance\nchanges didn't change the behavior. However, these two tests directly\ncontradicted the manpage's claims that two -f's were required to delete\nfiles/directories under a nested git repository. While one could argue\nthat the user gave an explicit path which matched files/directories that\nwere within a nested repository, there's a slippery slope that becomes\nvery difficult for users to understand once you go down that route (e.g.\nwhat if they specified \"git clean -f -d '*.c'\"?) It would also be hard\nto explain what the exact behavior was; avoid such problems by making it\nreally simple.\n\nAlso, clean up some grammar errors describing this functionality in the\ngit-clean manpage.\n\nFinally, there is one somewhat related bug which this patch does not\nfix, coming from the opposite angle. If the user runs\n git clean -ffd\nto force deletion of untracked nested repositories, and within an\nuntracked nested repo the user has ignored files (according to the inner\nOR outer repositories' .gitignore), then not only will those ignored\nfiles be left alone but the .git/ subdirectory of the nested repo will\nbe left alone too. I am not completely sure if this should be\nconsidered a bug (though it seems like it since the lack of the\nuntracked file would result in the .git/ subdirectory being deleted),\nbut in any event it is very minor compared to accidentally deleting user\ndata and I did not dive into it.\n\nSigned-off-by: Elijah Newren \n---\n Documentation/git-clean.txt | 6 +++---\n builtin/clean.c | 2 ++\n dir.c | 10 ++++++++++\n dir.h | 3 ++-\n t/t7300-clean.sh | 10 +++++-----\n 5 files changed, 22 insertions(+), 9 deletions(-)\n\ndiff --git a/Documentation/git-clean.txt b/Documentation/git-clean.txt\nindex 3ab749b921..ba31d8d166 100644\n--- a/Documentation/git-clean.txt\n+++ b/Documentation/git-clean.txt\n@@ -37,9 +37,9 @@ OPTIONS\n --force::\n \tIf the Git configuration variable clean.requireForce is not set\n \tto false, 'git clean' will refuse to delete files or directories\n-\tunless given -f or -i. Git will refuse to delete directories\n-\twith .git sub directory or file unless a second -f\n-\tis given.\n+\tunless given -f or -i. Git will refuse to modify untracked\n+\tnested git repositories (directories with a .git subdirectory)\n+\tunless a second -f is given.\n \n -i::\n --interactive::\ndiff --git a/builtin/clean.c b/builtin/clean.c\nindex 68d70e41c0..3a7a63ae71 100644\n--- a/builtin/clean.c\n+++ b/builtin/clean.c\n@@ -946,6 +946,8 @@ int cmd_clean(int argc, const char **argv, const char *prefix)\n \n \tif (force > 1)\n \t\trm_flags = 0;\n+\telse\n+\t\tdir.flags |= DIR_SKIP_NESTED_GIT;\n \n \tdir.flags |= DIR_SHOW_OTHER_DIRECTORIES;\n \ndiff --git a/dir.c b/dir.c\nindex 3b2fe1701c..7ff79170fc 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -1451,6 +1451,16 @@ static enum path_treatment treat_directory(struct dir_struct *dir,\n \t\treturn path_none;\n \n \tcase index_nonexistent:\n+\t\tif (dir->flags & DIR_SKIP_NESTED_GIT) {\n+\t\t\tint nested_repo;\n+\t\t\tstruct strbuf sb = STRBUF_INIT;\n+\t\t\tstrbuf_addstr(&sb, dirname);\n+\t\t\tnested_repo = is_nonbare_repository_dir(&sb);\n+\t\t\tstrbuf_release(&sb);\n+\t\t\tif (nested_repo)\n+\t\t\t\treturn path_none;\n+\t\t}\n+\n \t\tif (dir->flags & DIR_SHOW_OTHER_DIRECTORIES)\n \t\t\tbreak;\n \t\tif (exclude &&\ndiff --git a/dir.h b/dir.h\nindex 46c238ab49..739aea7c96 100644\n--- a/dir.h\n+++ b/dir.h\n@@ -156,7 +156,8 @@ struct dir_struct {\n \t\tDIR_SHOW_IGNORED_TOO = 1<<5,\n \t\tDIR_COLLECT_KILLED_ONLY = 1<<6,\n \t\tDIR_KEEP_UNTRACKED_CONTENTS = 1<<7,\n-\t\tDIR_SHOW_IGNORED_TOO_MODE_MATCHING = 1<<8\n+\t\tDIR_SHOW_IGNORED_TOO_MODE_MATCHING = 1<<8,\n+\t\tDIR_SKIP_NESTED_GIT = 1<<9\n \t} flags;\n \tstruct dir_entry **entries;\n \tstruct dir_entry **ignored;\ndiff --git a/t/t7300-clean.sh b/t/t7300-clean.sh\nindex 530dfdab34..6e6d24c1c3 100755\n--- a/t/t7300-clean.sh\n+++ b/t/t7300-clean.sh\n@@ -549,7 +549,7 @@ test_expect_failure 'nested (non-empty) bare repositories should be cleaned even\n \ttest_path_is_missing strange_bare\n '\n \n-test_expect_success 'giving path in nested git work tree will remove it' '\n+test_expect_success 'giving path in nested git work tree will NOT remove it' '\n \trm -fr repo &&\n \tmkdir repo &&\n \t(\n@@ -561,7 +561,7 @@ test_expect_success 'giving path in nested git work tree will remove it' '\n \tgit clean -f -d repo/bar/baz &&\n \ttest_path_is_file repo/.git/HEAD &&\n \ttest_path_is_dir repo/bar/ &&\n-\ttest_path_is_missing repo/bar/baz\n+\ttest_path_is_file repo/bar/baz/hello.world\n '\n \n test_expect_success 'giving path to nested .git will not remove it' '\n@@ -579,7 +579,7 @@ test_expect_success 'giving path to nested .git will not remove it' '\n \ttest_path_is_dir untracked/\n '\n \n-test_expect_success 'giving path to nested .git/ will remove contents' '\n+test_expect_success 'giving path to nested .git/ will NOT remove contents' '\n \trm -fr repo untracked &&\n \tmkdir repo untracked &&\n \t(\n@@ -589,7 +589,7 @@ test_expect_success 'giving path to nested .git/ will remove contents' '\n \t) &&\n \tgit clean -f -d repo/.git/ &&\n \ttest_path_is_dir repo/.git &&\n-\ttest_dir_is_empty repo/.git &&\n+\ttest_path_is_file repo/.git/HEAD &&\n \ttest_path_is_dir untracked/\n '\n \n@@ -671,7 +671,7 @@ test_expect_success 'git clean -d skips untracked dirs containing ignored files'\n \ttest_path_is_missing foo/b/bb\n '\n \n-test_expect_failure 'git clean -d skips nested repo containing ignored files' '\n+test_expect_success 'git clean -d skips nested repo containing ignored files' '\n \ttest_when_finished \"rm -rf nested-repo-with-ignored-file\" &&\n \n \tgit init nested-repo-with-ignored-file &&\n-- \n2.22.1.11.g45a39ee867\n\n"},{"id":"381876","messageId":"20190905154735.29784-10-newren@gmail.com","threadId":"51732","inReplyTo":"20190905154735.29784-1-newren@gmail.com","subject":"[RFC PATCH v2 09/12] clean: disambiguate the definition of -d","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2019-09-05T15:47:32Z","receivedAt":"2019-09-05T15:47:54Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"The -d flag pre-dated git-clean's ability to have paths specified. As\nsuch, the default for git-clean was to only remove untracked files in\nthe current directory, and -d existed to allow it to recurse into\nsubdirectories.\n\nThe interaction of paths and the -d option appears to not have been\ncarefully considered, as evidenced by numerous bugs and a dearth of\ntests covering such pairings in the testsuite. The definition turns out\nto be important, so let's look at some of the various ways one could\ninterpret the -d option:\n\n A) Without -d, only look in subdirectories which contain tracked\n files under them; with -d, also look in subdirectories which\n are untracked for files to clean.\n\n B) Without specified paths from the user for us to delete, we need to\n have some kind of default, so...without -d, only look in\n subdirectories which contain tracked files under them; with -d,\n also look in subdirectories which are untracked for files to clean.\n\nThe important distinction here is that choice B says that the presence\nor absence of '-d' is irrelevant if paths are specified. The logic\nbehind option B is that if a user explicitly asked us to clean a\nspecified pathspec, then we should clean anything that matches that\npathspec. Some examples may clarify. Should\n\n git clean -f untracked_dir/file\n\nremove untracked_dir/file or not? It seems crazy not to, but a strict\nreading of option A says it shouldn't be removed. How about\n\n git clean -f untracked_dir/file1 tracked_dir/file2\n\nor\n\n git clean -f untracked_dir_1/file1 untracked_dir_2/file2\n\n? Should it remove either or both of these files? Should it require\nmultiple runs to remove both the files listed? (If this sounds like a\ncrazy question to even ask, see the commit message of \"t7300: Add some\ntestcases showing failure to clean specified pathspecs\" added earlier in\nthis patch series.) What if -ffd were used instead of -f -- should that\nallow these to be removed? Should it take multiple invocations with\n-ffd? What if a glob (such as '*tracked*') were used instead of\nspelling out the directory names? What if the filenames involved globs,\nsuch as\n\n git clean -f '*.o'\n\nor\n\n git clean -f '*/*.o'\n\n?\n\nThe current documentation actually suggests a definition that is\nslightly different than choice A, and the implementation prior to this\nseries provided something radically different than either choices A or\nB. (The implementation, though, was clearly just buggy). There may be\nother choices as well. However, for almost any given choice of\ndefinition for -d that I can think of, some of the examples above will\nappear buggy to the user. The only case that doesn't have negative\nsurprises is choice B: treat a user-specified path as a request to clean\nall untracked files which match that path specification, including\nrecursing into any untracked directories.\n\nChange the documentation and basic implementation to use this\ndefinition.\n\nThere were two regression tests that indirectly depended on the current\nimplementation, but neither was about subdirectory handling. These two\ntests were introduced in commit 5b7570cfb41c (\"git-clean: add tests for\nrelative path\", 2008-03-07) which was solely created to add coverage for\nthe changes in commit fb328947c8e (\"git-clean: correct printing relative\npath\", 2008-03-07). Both tests specified a directory that happened to\nhave an untracked subdirectory, but both were only checking that the\nresulting printout of a file that was removed was shown with a relative\npath. Update these tests appropriately.\n\nSigned-off-by: Elijah Newren \n---\n Documentation/git-clean.txt | 10 ++++++----\n builtin/clean.c | 8 ++++++++\n t/t7300-clean.sh | 2 ++\n 3 files changed, 16 insertions(+), 4 deletions(-)\n\ndiff --git a/Documentation/git-clean.txt b/Documentation/git-clean.txt\nindex e84ffc9396..3ab749b921 100644\n--- a/Documentation/git-clean.txt\n+++ b/Documentation/git-clean.txt\n@@ -26,10 +26,12 @@ are affected.\n OPTIONS\n -------\n -d::\n-\tRemove untracked directories in addition to untracked files.\n-\tIf an untracked directory is managed by a different Git\n-\trepository, it is not removed by default. Use -f option twice\n-\tif you really want to remove such a directory.\n+\tNormally, when no is specified, git clean will not\n+\trecurse into untracked directories to avoid removing too much.\n+\tSpecify -d to have it recurse into such directories as well.\n+\tIf any paths are specified, -d is irrelevant; all untracked\n+\tfiles matching the specified paths (with exceptions for nested\n+\tgit directories mentioned under `--force`) will be removed.\n \n -f::\n --force::\ndiff --git a/builtin/clean.c b/builtin/clean.c\nindex d5579da716..68d70e41c0 100644\n--- a/builtin/clean.c\n+++ b/builtin/clean.c\n@@ -949,6 +949,14 @@ int cmd_clean(int argc, const char **argv, const char *prefix)\n \n \tdir.flags |= DIR_SHOW_OTHER_DIRECTORIES;\n \n+\tif (argc) {\n+\t\t/*\n+\t\t * Remaining args implies pathspecs specified, and we should\n+\t\t * recurse within those.\n+\t\t */\n+\t\tremove_directories = 1;\n+\t}\n+\n \tif (remove_directories)\n \t\tdir.flags |= DIR_SHOW_IGNORED_TOO | DIR_KEEP_UNTRACKED_CONTENTS;\n \ndiff --git a/t/t7300-clean.sh b/t/t7300-clean.sh\nindex d83aeb7dc2..530dfdab34 100755\n--- a/t/t7300-clean.sh\n+++ b/t/t7300-clean.sh\n@@ -117,6 +117,7 @@ test_expect_success C_LOCALE_OUTPUT 'git clean with relative prefix' '\n \twould_clean=$(\n \t\tcd docs &&\n \t\tgit clean -n ../src |\n+\t\tgrep part3 |\n \t\tsed -n -e \"s|^Would remove ||p\"\n \t) &&\n \tverbose test \"$would_clean\" = ../src/part3.c\n@@ -129,6 +130,7 @@ test_expect_success C_LOCALE_OUTPUT 'git clean with absolute path' '\n \twould_clean=$(\n \t\tcd docs &&\n \t\tgit clean -n \"$(pwd)/../src\" |\n+\t\tgrep part3 |\n \t\tsed -n -e \"s|^Would remove ||p\"\n \t) &&\n \tverbose test \"$would_clean\" = ../src/part3.c\n-- \n2.22.1.11.g45a39ee867\n\n"},{"id":"381877","messageId":"20190905154735.29784-12-newren@gmail.com","threadId":"51732","inReplyTo":"20190905154735.29784-1-newren@gmail.com","subject":"[RFC PATCH v2 11/12] clean: rewrap overly long line","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2019-09-05T15:47:34Z","receivedAt":"2019-09-05T15:47:55Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Signed-off-by: Elijah Newren \n---\n builtin/clean.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/clean.c b/builtin/clean.c\nindex 3a7a63ae71..6030842f3a 100644\n--- a/builtin/clean.c\n+++ b/builtin/clean.c\n@@ -158,7 +158,8 @@ static int remove_dirs(struct strbuf *path, const char *prefix, int force_flag,\n \n \t*dir_gone = 1;\n \n-\tif ((force_flag & REMOVE_DIR_KEEP_NESTED_GIT) && is_nonbare_repository_dir(path)) {\n+\tif ((force_flag & REMOVE_DIR_KEEP_NESTED_GIT) &&\n+\t is_nonbare_repository_dir(path)) {\n \t\tif (!quiet) {\n \t\t\tquote_path_relative(path->buf, prefix, "ed);\n \t\t\tprintf(dry_run ? _(msg_would_skip_git_dir) : _(msg_skip_git_dir),\n-- \n2.22.1.11.g45a39ee867\n\n"},{"id":"381878","messageId":"20190905154735.29784-13-newren@gmail.com","threadId":"51732","inReplyTo":"20190905154735.29784-1-newren@gmail.com","subject":"[RFC PATCH v2 12/12] clean: fix theoretical path corruption","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2019-09-05T15:47:35Z","receivedAt":"2019-09-05T15:47:56Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"cmd_clean() had the following code structure:\n\n struct strbuf abs_path = STRBUF_INIT;\n for_each_string_list_item(item, &del_list) {\n strbuf_addstr(&abs_path, prefix);\n strbuf_addstr(&abs_path, item->string);\n PROCESS(&abs_path);\n strbuf_reset(&abs_path);\n }\n\nwhere I've elided a bunch of unnecessary details and PROCESS(&abs_path)\nrepresents a big chunk of code rather than an actual function call. One\npiece of PROCESS was:\n\n if (lstat(abs_path.buf, &st))\n continue;\n\nwhich would cause the strbuf_reset() to be missed -- meaning that the\nnext path to be handled would have two paths concatenated. This path\nused to use die_errno() instead of continue prior to commit 396049e5fb62\n(\"git-clean: refactor git-clean into two phases\", 2013-06-25), but my\nunderstanding of how correct_untracked_entries() works is that it will\nprevent both dir/ and dir/file from being in the list to clean so this\nshould be dead code and the die_errno() should be safe. But I hesitate\nto remove it since I am not certain. Instead, just fix it to avoid path\ncorruption in case it is possible to reach this continue statement.\n\nSigned-off-by: Elijah Newren \n---\n builtin/clean.c | 4 +++-\n 1 file changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/clean.c b/builtin/clean.c\nindex 6030842f3a..ccb6e23f0b 100644\n--- a/builtin/clean.c\n+++ b/builtin/clean.c\n@@ -1028,8 +1028,10 @@ int cmd_clean(int argc, const char **argv, const char *prefix)\n \t\t * recursive directory removal, so lstat() here could\n \t\t * fail with ENOENT.\n \t\t */\n-\t\tif (lstat(abs_path.buf, &st))\n+\t\tif (lstat(abs_path.buf, &st)) {\n+\t\t\tstrbuf_reset(&abs_path);\n \t\t\tcontinue;\n+\t\t}\n \n \t\tif (S_ISDIR(st.st_mode)) {\n \t\t\tif (remove_dirs(&abs_path, prefix, rm_flags, dry_run, quiet, &gone))\n-- \n2.22.1.11.g45a39ee867\n\n"},{"id":"381879","messageId":"20190905154735.29784-9-newren@gmail.com","threadId":"51732","inReplyTo":"20190905154735.29784-1-newren@gmail.com","subject":"[RFC PATCH v2 08/12] git-clean.txt: do not claim we will delete files with -n/--dry-run","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2019-09-05T15:47:31Z","receivedAt":"2019-09-05T15:47:57Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"It appears that the wrong option got included in the list of what will\ncause git-clean to actually take action. Correct the list.\n\nSigned-off-by: Elijah Newren \n---\n Documentation/git-clean.txt | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/Documentation/git-clean.txt b/Documentation/git-clean.txt\nindex db876f7dde..e84ffc9396 100644\n--- a/Documentation/git-clean.txt\n+++ b/Documentation/git-clean.txt\n@@ -35,7 +35,7 @@ OPTIONS\n --force::\n \tIf the Git configuration variable clean.requireForce is not set\n \tto false, 'git clean' will refuse to delete files or directories\n-\tunless given -f, -n or -i. Git will refuse to delete directories\n+\tunless given -f or -i. Git will refuse to delete directories\n \twith .git sub directory or file unless a second -f\n \tis given.\n \n-- \n2.22.1.11.g45a39ee867\n\n"},{"id":"381898","messageId":"20190905190126.GA32087@szeder.dev","threadId":"51732","inReplyTo":"20190905154735.29784-1-newren@gmail.com","subject":"Re: [RFC PATCH v2 00/12] Fix some git clean issues","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-09-05T19:01:26Z","receivedAt":"2019-09-05T19:01:36Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Thu, Sep 05, 2019 at 08:47:23AM -0700, Elijah Newren wrote:\n> This patch series fixes a few issues with git-clean:\n\n> * Failure to preserve both tracked and untracked files within a nested\n> Git repository reported a few weeks ago by SZEDER[3].\n\nWow, I didn't expect a 12 patch series to fix that issue...\nThanks.\n\n> Elijah Newren (12):\n> t7300: Add some testcases showing failure to clean specified pathspecs\n> dir: fix typo in comment\n> dir: fix off-by-one error in match_pathspec_item\n> dir: Directories should be checked for matching pathspecs too\n> dir: Make the DO_MATCH_SUBMODULE code reusable for a non-submodule\n> case\n> dir: If our pathspec might match files under a dir, recurse into it\n\nNit: no capital letters after the ':' prefix.\n\n> dir: add commentary explaining match_pathspec_item's return value\n> git-clean.txt: do not claim we will delete files with -n/--dry-run\n> clean: disambiguate the definition of -d\n> clean: avoid removing untracked files in a nested git repository\n> clean: rewrap overly long line\n> clean: fix theoretical path corruption\n> \n> Documentation/git-clean.txt | 16 +++++-----\n> builtin/clean.c | 17 ++++++++--\n> dir.c | 63 +++++++++++++++++++++++++++----------\n> dir.h | 8 +++--\n> t/t7300-clean.sh | 44 +++++++++++++++++++++++---\n> 5 files changed, 114 insertions(+), 34 deletions(-)\n> \n> -- \n> 2.22.1.11.g45a39ee867\n> \n"},{"id":"381901","messageId":"20190905192744.GB32087@szeder.dev","threadId":"51732","inReplyTo":"20190905154735.29784-13-newren@gmail.com","subject":"Re: [RFC PATCH v2 12/12] clean: fix theoretical path corruption","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-09-05T19:27:44Z","receivedAt":"2019-09-05T19:27:50Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Thu, Sep 05, 2019 at 08:47:35AM -0700, Elijah Newren wrote:\n> cmd_clean() had the following code structure:\n> \n> struct strbuf abs_path = STRBUF_INIT;\n> for_each_string_list_item(item, &del_list) {\n> strbuf_addstr(&abs_path, prefix);\n> strbuf_addstr(&abs_path, item->string);\n> PROCESS(&abs_path);\n> strbuf_reset(&abs_path);\n> }\n> \n> where I've elided a bunch of unnecessary details and PROCESS(&abs_path)\n> represents a big chunk of code rather than an actual function call. One\n> piece of PROCESS was:\n> \n> if (lstat(abs_path.buf, &st))\n> continue;\n> \n> which would cause the strbuf_reset() to be missed -- meaning that the\n> next path to be handled would have two paths concatenated. This path\n> used to use die_errno() instead of continue prior to commit 396049e5fb62\n> (\"git-clean: refactor git-clean into two phases\", 2013-06-25), but my\n> understanding of how correct_untracked_entries() works is that it will\n> prevent both dir/ and dir/file from being in the list to clean so this\n> should be dead code and the die_errno() should be safe. But I hesitate\n> to remove it since I am not certain. Instead, just fix it to avoid path\n> corruption in case it is possible to reach this continue statement.\n> \n> Signed-off-by: Elijah Newren \n> ---\n> builtin/clean.c | 4 +++-\n> 1 file changed, 3 insertions(+), 1 deletion(-)\n> \n> diff --git a/builtin/clean.c b/builtin/clean.c\n> index 6030842f3a..ccb6e23f0b 100644\n> --- a/builtin/clean.c\n> +++ b/builtin/clean.c\n> @@ -1028,8 +1028,10 @@ int cmd_clean(int argc, const char **argv, const char *prefix)\n> \t\t * recursive directory removal, so lstat() here could\n> \t\t * fail with ENOENT.\n> \t\t */\n> -\t\tif (lstat(abs_path.buf, &st))\n> +\t\tif (lstat(abs_path.buf, &st)) {\n> +\t\t\tstrbuf_reset(&abs_path);\n> \t\t\tcontinue;\n> +\t\t}\n\nI wonder whether it would be safer to call strbuf_reset() at the start\nof each loop iteration instead of before 'continue'. That way we\nwouldn't have to worry about another 'continue' statements forgetting\nabout it.\n\nIt probably doesn't really matter in this particular case (considering\nthat it's potentially dead code to begin with), but have a look at\ne.g. diff.c:show_stats() and its several strbuf_reset(&out) calls\npreceeding continue statements.\n\n> \t\tif (S_ISDIR(st.st_mode)) {\n> \t\t\tif (remove_dirs(&abs_path, prefix, rm_flags, dry_run, quiet, &gone))\n> -- \n> 2.22.1.11.g45a39ee867\n> \n"},{"id":"381917","messageId":"20190905212043.GC32087@szeder.dev","threadId":"51732","inReplyTo":"20190905154735.29784-11-newren@gmail.com","subject":"Re: [RFC PATCH v2 10/12] clean: avoid removing untracked files in a nested git repository","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-09-05T21:20:43Z","receivedAt":"2019-09-05T21:20:50Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Thu, Sep 05, 2019 at 08:47:33AM -0700, Elijah Newren wrote:\n> Users expect files in a nested git repository to be left alone unless\n> sufficiently forced (with two -f's). Unfortunately, in certain\n> circumstances, git would delete both tracked (and possibly dirty) files\n> and untracked files within a nested repository. To explain how this\n> happens, let's contrast a couple cases. First, take the following\n> example setup (which assumes we are already within a git repo):\n> \n> git init nested\n> cd nested\n> >tracked\n> git add tracked\n> git commit -m init\n> >untracked\n> cd ..\n> \n> In this setup, everything works as expected; running 'git clean -fd'\n> will result in fill_directory() returning the following paths:\n> nested/\n> nested/tracked\n> nested/untracked\n> and then correct_untracked_entries() would notice this can be compressed\n> to\n> nested/\n> and then since \"nested/\" is a directory, we would call\n> remove_dirs(\"nested/\", ...), which would\n> check is_nonbare_repository_dir() and then decide to skip it.\n> \n> However, if someone also creates an ignored file:\n> >nested/ignored\n> then running 'git clean -fd' would result in fill_directory() returning\n> the same paths:\n> nested/\n> nested/tracked\n> nested/untracked\n> but correct_untracked_entries() will notice that we had ignored entries\n> under nested/ and thus simplify this list to\n> nested/tracked\n> nested/untracked\n> Since these are not directories, we do not call remove_dirs() which was\n> the only place that had the is_nonbare_repository_dir() safety check --\n> resulting in us deleting both the untracked file and the tracked (and\n> possibly dirty) file.\n> \n> One possible fix for this issue would be walking the parent directories\n> of each path and checking if they represent nonbare repositories, but\n> that would be wasteful. Even if we added caching of some sort, it's\n> still a waste because we should have been able to check that \"nested/\"\n> represented a nonbare repository before even descending into it in the\n> first place. Add a DIR_SKIP_NESTED_GIT flag to dir_struct.flags and use\n> it to prevent fill_directory() and friends from descending into nested\n> git repos.\n\n> Finally, there is one somewhat related bug which this patch does not\n> fix, coming from the opposite angle. If the user runs\n> git clean -ffd\n> to force deletion of untracked nested repositories, and within an\n> untracked nested repo the user has ignored files (according to the inner\n> OR outer repositories' .gitignore), then not only will those ignored\n> files be left alone but the .git/ subdirectory of the nested repo will\n> be left alone too. I am not completely sure if this should be\n> considered a bug (though it seems like it since the lack of the\n> untracked file would result in the .git/ subdirectory being deleted),\n> but in any event it is very minor compared to accidentally deleting user\n> data and I did not dive into it.\n\nWe briefly mentioned this \"ignored file in a nested repo fools 'git\nclean -d'\" issue in an unrelated thread as well, where Philip\nsuggested that the gitignore of the outer repository should not have\nany effect on the nested repository. I'm inclined to agree.\n\n https://public-inbox.org/git/e221aaf8-7d0b-6feb-3f58-1e9e4382939b@iee.email/\n\nNow, 'git clean -X' is supposed to \"Remove only files ignored by\nGit.\". I'm not entirely sure what 'git clean -ffdX' is supposed to do\n(or whether it makes any sense in the first place), but it does delete\nfiles in the nested repository that are ignored only in the outer\nrepository, both tracked (and possibly dirty) and untracked, even with\nthis patch series. Without this series '-fdX' is just as bad, but\nwith this patch (i.e. by not descending into nested repositories)\n'-fdX' becomes sensible and leaves the nested repository alone.\n\n> diff --git a/Documentation/git-clean.txt b/Documentation/git-clean.txt\n> index 3ab749b921..ba31d8d166 100644\n> --- a/Documentation/git-clean.txt\n> +++ b/Documentation/git-clean.txt\n> @@ -37,9 +37,9 @@ OPTIONS\n> --force::\n> \tIf the Git configuration variable clean.requireForce is not set\n> \tto false, 'git clean' will refuse to delete files or directories\n> -\tunless given -f or -i. Git will refuse to delete directories\n> -\twith .git sub directory or file unless a second -f\n> -\tis given.\n> +\tunless given -f or -i. Git will refuse to modify untracked\n> +\tnested git repositories (directories with a .git subdirectory)\n> +\tunless a second -f is given.\n\nI like this wording.\n\n"},{"id":"381990","messageId":"CABPp-BE51REGkEWSO1Dm-TS=vsY2BpoDCtOh1B2CNAaQBFk+ug@mail.gmail.com","threadId":"51732","inReplyTo":"20190905190126.GA32087@szeder.dev","subject":"Re: [RFC PATCH v2 00/12] Fix some git clean issues","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2019-09-07T00:33:21Z","receivedAt":"2019-09-07T00:44:04Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Thu, Sep 5, 2019 at 12:01 PM SZEDER Gábor wrote:\n>\n> On Thu, Sep 05, 2019 at 08:47:23AM -0700, Elijah Newren wrote:\n> > This patch series fixes a few issues with git-clean:\n>\n> > * Failure to preserve both tracked and untracked files within a nested\n> > Git repository reported a few weeks ago by SZEDER[3].\n>\n> Wow, I didn't expect a 12 patch series to fix that issue...\n> Thanks.\n\nWell, to be fair, only the last three patches were about that issue.\nThe first 9 were about the other issues. It's just that your testcase\nreminded me of my old series and gave me another nudge to dig it out\nand see if it helped with your problem. I had to rebase it and look\nback over it, and then found it didn't help with your problem, but by\nthen I had refamiliarized myself with the code so...\n\n> > Elijah Newren (12):\n> > t7300: Add some testcases showing failure to clean specified pathspecs\n> > dir: fix typo in comment\n> > dir: fix off-by-one error in match_pathspec_item\n> > dir: Directories should be checked for matching pathspecs too\n> > dir: Make the DO_MATCH_SUBMODULE code reusable for a non-submodule\n> > case\n> > dir: If our pathspec might match files under a dir, recurse into it\n>\n> Nit: no capital letters after the ':' prefix.\n\nGah, I should know that any patch series I submitted from a year and a\nhalf ago probably made that mistake. I've mostly trained myself out\nof it now, but I certainly hadn't back then.\n\nThanks for pointing it out; will fix.\n"},{"id":"381991","messageId":"CABPp-BFOyVaK=hvWZtXfBmiSvf2DXf3c-GvPBt7kBu9wtCZSGg@mail.gmail.com","threadId":"51732","inReplyTo":"20190905192744.GB32087@szeder.dev","subject":"Re: [RFC PATCH v2 12/12] clean: fix theoretical path corruption","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2019-09-07T00:34:55Z","receivedAt":"2019-09-07T00:44:04Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Thu, Sep 5, 2019 at 12:27 PM SZEDER Gábor wrote:\n>\n> On Thu, Sep 05, 2019 at 08:47:35AM -0700, Elijah Newren wrote:\n> > cmd_clean() had the following code structure:\n> >\n> > struct strbuf abs_path = STRBUF_INIT;\n> > for_each_string_list_item(item, &del_list) {\n> > strbuf_addstr(&abs_path, prefix);\n> > strbuf_addstr(&abs_path, item->string);\n> > PROCESS(&abs_path);\n> > strbuf_reset(&abs_path);\n> > }\n> >\n> > where I've elided a bunch of unnecessary details and PROCESS(&abs_path)\n> > represents a big chunk of code rather than an actual function call. One\n> > piece of PROCESS was:\n> >\n> > if (lstat(abs_path.buf, &st))\n> > continue;\n> >\n> > which would cause the strbuf_reset() to be missed -- meaning that the\n> > next path to be handled would have two paths concatenated. This path\n> > used to use die_errno() instead of continue prior to commit 396049e5fb62\n> > (\"git-clean: refactor git-clean into two phases\", 2013-06-25), but my\n> > understanding of how correct_untracked_entries() works is that it will\n> > prevent both dir/ and dir/file from being in the list to clean so this\n> > should be dead code and the die_errno() should be safe. But I hesitate\n> > to remove it since I am not certain. Instead, just fix it to avoid path\n> > corruption in case it is possible to reach this continue statement.\n> >\n> > Signed-off-by: Elijah Newren \n> > ---\n> > builtin/clean.c | 4 +++-\n> > 1 file changed, 3 insertions(+), 1 deletion(-)\n> >\n> > diff --git a/builtin/clean.c b/builtin/clean.c\n> > index 6030842f3a..ccb6e23f0b 100644\n> > --- a/builtin/clean.c\n> > +++ b/builtin/clean.c\n> > @@ -1028,8 +1028,10 @@ int cmd_clean(int argc, const char **argv, const char *prefix)\n> > * recursive directory removal, so lstat() here could\n> > * fail with ENOENT.\n> > */\n> > - if (lstat(abs_path.buf, &st))\n> > + if (lstat(abs_path.buf, &st)) {\n> > + strbuf_reset(&abs_path);\n> > continue;\n> > + }\n>\n> I wonder whether it would be safer to call strbuf_reset() at the start\n> of each loop iteration instead of before 'continue'. That way we\n> wouldn't have to worry about another 'continue' statements forgetting\n> about it.\n>\n> It probably doesn't really matter in this particular case (considering\n> that it's potentially dead code to begin with), but have a look at\n> e.g. diff.c:show_stats() and its several strbuf_reset(&out) calls\n> preceeding continue statements.\n\nOoh, I like that idea. I think I'll apply that here. I'll probably\nleave diff.c:show_stats() as #leftoverbits for someone else, though I\nreally like the idea of fixing up other issues like this as you\nsuggest.\n"},{"id":"382261","messageId":"20190912221240.18057-2-newren@gmail.com","threadId":"51732","inReplyTo":"20190912221240.18057-1-newren@gmail.com","subject":"[PATCH v3 01/12] t7300: add testcases showing failure to clean specified pathspecs","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2019-09-12T22:12:29Z","receivedAt":"2019-09-12T22:12:49Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Someone brought me a testcase where multiple git-clean invocations were\nrequired to clean out unwanted files:\n mkdir d{1,2}\n touch d{1,2}/ut\n touch d1/t && git add d1/t\nWith this setup, the user would need to run\n git clean -ffd */ut\ntwice to delete both ut files.\n\nA little testing showed some interesting variants:\n * If only one of those two ut files existed (either one), then only one\n clean command would be necessary.\n * If both directories had tracked files, then only one git clean would\n be necessary to clean both files.\n * If both directories had no tracked files then the clean command above\n would never clean either of the untracked files despite the pathspec\n explicitly calling both of them out.\n\nA bisect showed that the failure to clean out the files started with\ncommit cf424f5fd89b (\"clean: respect pathspecs with \"-d\", 2014-03-10).\nHowever, that pointed to a separate issue: while the \"-d\" flag was used\nby the original user who showed me this problem, that flag should have\nbeen irrelevant to this problem. Testing again without the \"-d\" flag\nshowed that the same buggy behavior exists without using that flag, and\nhas in fact existed since before cf424f5fd89b.\n\nAdd testcases showing that multiple untracked files within entirely\nuntracked directories cannot be cleaned when specifying these files to\ngit clean via pathspecs.\n\nSigned-off-by: Elijah Newren \n---\n t/t7300-clean.sh | 32 ++++++++++++++++++++++++++++++++\n 1 file changed, 32 insertions(+)\n\ndiff --git a/t/t7300-clean.sh b/t/t7300-clean.sh\nindex d01fd120ab..2c254c773c 100755\n--- a/t/t7300-clean.sh\n+++ b/t/t7300-clean.sh\n@@ -691,6 +691,38 @@ test_expect_failure 'git clean -d skips nested repo containing ignored files' '\n \ttest_path_is_file nested-repo-with-ignored-file/file\n '\n \n+test_expect_failure 'git clean handles being told what to clean' '\n+\tmkdir -p d1 d2 &&\n+\ttouch d1/ut d2/ut &&\n+\tgit clean -f */ut &&\n+\ttest_path_is_missing d1/ut &&\n+\ttest_path_is_missing d2/ut\n+'\n+\n+test_expect_failure 'git clean handles being told what to clean, with -d' '\n+\tmkdir -p d1 d2 &&\n+\ttouch d1/ut d2/ut &&\n+\tgit clean -ffd */ut &&\n+\ttest_path_is_missing d1/ut &&\n+\ttest_path_is_missing d2/ut\n+'\n+\n+test_expect_failure 'git clean works if a glob is passed without -d' '\n+\tmkdir -p d1 d2 &&\n+\ttouch d1/ut d2/ut &&\n+\tgit clean -f \"*ut\" &&\n+\ttest_path_is_missing d1/ut &&\n+\ttest_path_is_missing d2/ut\n+'\n+\n+test_expect_failure 'git clean works if a glob is passed with -d' '\n+\tmkdir -p d1 d2 &&\n+\ttouch d1/ut d2/ut &&\n+\tgit clean -ffd \"*ut\" &&\n+\ttest_path_is_missing d1/ut &&\n+\ttest_path_is_missing d2/ut\n+'\n+\n test_expect_success MINGW 'handle clean & core.longpaths = false nicely' '\n \ttest_config core.longpaths false &&\n \ta50=aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa &&\n-- \n2.23.0.173.gad11b3a635.dirty\n\n"},{"id":"382262","messageId":"20190912221240.18057-1-newren@gmail.com","threadId":"51732","inReplyTo":"20190905154735.29784-1-newren@gmail.com","subject":"[PATCH v3 00/12] Fix some git clean issues","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2019-09-12T22:12:28Z","receivedAt":"2019-09-12T22:12:50Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"NOTE: This series builds on sg/clean-nested-repo-with-ignored, as it\n (among other things) modifies his testcase from expect_failure\n to expect_success. Also, Peff is probably the only one who\n remembers v1 (and even he may have forgotten it): v1 was posted\n a year and a half ago.\n\nThis patch series fixes a few issues with git-clean:\n * Failure to clean when multiple pathspecs are specified, reported both\n in April 2018[1] and again in May 2019[2].\n * Failure to preserve both tracked and untracked files within a nested\n Git repository reported a few weeks ago by SZEDER[3].\n\n[1] https://public-inbox.org/git/20180405173446.32372-4-newren@gmail.com/\n[2] https://public-inbox.org/git/20190531183651.10067-1-rafa.almas@gmail.com/\n[3] https://public-inbox.org/git/20190825185918.3909-1-szeder.dev@gmail.com/\n\nChanges since v2:\n * Removed the RFC label\n * Fixed up a few things SZEDER pointed out in v2 -- some commit\n message improvements, and an extra safety precaution for the\n final patch.\n\nStuff I'd like reviewer to focus on:\n * Read the (long) commit messages for patches 9 & 10; do folks agree\n with my declaration of \"correct\" behavior and my changes to the\n few regression tests in those patches?\n\nElijah Newren (12):\n t7300: add testcases showing failure to clean specified pathspecs\n dir: fix typo in comment\n dir: fix off-by-one error in match_pathspec_item\n dir: also check directories for matching pathspecs\n dir: make the DO_MATCH_SUBMODULE code reusable for a non-submodule\n case\n dir: if our pathspec might match files under a dir, recurse into it\n dir: add commentary explaining match_pathspec_item's return value\n git-clean.txt: do not claim we will delete files with -n/--dry-run\n clean: disambiguate the definition of -d\n clean: avoid removing untracked files in a nested git repository\n clean: rewrap overly long line\n clean: fix theoretical path corruption\n\n Documentation/git-clean.txt | 16 +++++-----\n builtin/clean.c | 15 +++++++--\n dir.c | 63 +++++++++++++++++++++++++++----------\n dir.h | 8 +++--\n t/t7300-clean.sh | 44 +++++++++++++++++++++++---\n 5 files changed, 112 insertions(+), 34 deletions(-)\n\nRange-diff:\n 1: 82328e2033 ! 1: fe35ab8cc3 t7300: Add some testcases showing failure to clean specified pathspecs\n @@ Metadata\n Author: Elijah Newren \n \n ## Commit message ##\n - t7300: Add some testcases showing failure to clean specified pathspecs\n + t7300: add testcases showing failure to clean specified pathspecs\n \n Someone brought me a testcase where multiple git-clean invocations were\n required to clean out unwanted files:\n 2: 5c1f58fd9d = 2: 707d287d79 dir: fix typo in comment\n 3: 0e8b415af3 = 3: bb316e82b2 dir: fix off-by-one error in match_pathspec_item\n 4: 30b3ede443 ! 4: 56319f934a dir: Directories should be checked for matching pathspecs too\n @@ Metadata\n Author: Elijah Newren \n \n ## Commit message ##\n - dir: Directories should be checked for matching pathspecs too\n + dir: also check directories for matching pathspecs\n \n Even if a directory doesn't match a pathspec, it is possible, depending\n on the precise pathspecs, that some file underneath it might. So we\n 5: bab01f4cda ! 5: 81593a565c dir: Make the DO_MATCH_SUBMODULE code reusable for a non-submodule case\n @@ Metadata\n Author: Elijah Newren \n \n ## Commit message ##\n - dir: Make the DO_MATCH_SUBMODULE code reusable for a non-submodule case\n + dir: make the DO_MATCH_SUBMODULE code reusable for a non-submodule case\n \n The specific checks done in match_pathspec_item for the DO_MATCH_SUBMODULE\n case are useful for other cases which have nothing to do with submodules.\n 6: c619ab4b3e ! 6: 9566823a0f dir: If our pathspec might match files under a dir, recurse into it\n @@ Metadata\n Author: Elijah Newren \n \n ## Commit message ##\n - dir: If our pathspec might match files under a dir, recurse into it\n + dir: if our pathspec might match files under a dir, recurse into it\n \n For git clean, if a directory is entirely untracked and the user did not\n specify -d (corresponding to DIR_SHOW_IGNORED_TOO), then we usually do\n @@ Commit message\n the given directory which match one of the pathspecs being added to the\n entries list.\n \n - Two particular considerations for this patch:\n + Two notes of potential interest to future readers:\n \n - * If we want to only recurse into a directory when it is specifically\n + * If we wanted to only recurse into a directory when it is specifically\n matched rather than matched-via-glob (e.g. '*.c'), then we could do\n so via making the final non-zero return in match_pathspec_item be\n MATCHED_RECURSIVELY instead of MATCHED_RECURSIVELY_LEADING_PATHSPEC.\n - (See final patch of this RFC series for details; note that the\n - relative order of MATCHED_RECURSIVELY_LEADING_PATHSPEC and\n - MATCHED_RECURSIVELY are important for such a change.))\n + (Note that the relative order of MATCHED_RECURSIVELY_LEADING_PATHSPEC\n + and MATCHED_RECURSIVELY are important for such a change.) I was\n + leaving open that possibility while writing an RFC asking for the\n + behavior we want, but even though we don't want it, that knowledge\n + might help you understand the code flow better.\n \n * There is a growing amount of logic in read_directory_recursive() for\n - deciding whether to recurse into a subdirectory. However, there is\n - a comment immediately preceding this logic that says to recurse if\n + deciding whether to recurse into a subdirectory. However, there is a\n + comment immediately preceding this logic that says to recurse if\n instructed by treat_path(). It may be better for the logic in\n - read_directory_recursive() to be moved to treat_path() (or another\n - function it calls, such as treat_directory()), but I did not feel\n - strongly about this and just left the logic where it was while\n - adding to it. Do others have strong opinions on this?\n + read_directory_recursive() to ultimately be moved to treat_path() (or\n + another function it calls, such as treat_directory()), but I have\n + left that for someone else to tackle in the future.\n \n Signed-off-by: Elijah Newren \n \n 7: 5e1686a30e = 7: 7821898ba7 dir: add commentary explaining match_pathspec_item's return value\n 8: d92637f961 = 8: 13def5df57 git-clean.txt: do not claim we will delete files with -n/--dry-run\n 9: 4550a30df8 = 9: e6b274abf7 clean: disambiguate the definition of -d\n10: 0985be2793 ! 10: 5f4ef14765 clean: avoid removing untracked files in a nested git repository\n @@ Commit message\n Also, clean up some grammar errors describing this functionality in the\n git-clean manpage.\n \n - Finally, there is one somewhat related bug which this patch does not\n - fix, coming from the opposite angle. If the user runs\n - git clean -ffd\n - to force deletion of untracked nested repositories, and within an\n - untracked nested repo the user has ignored files (according to the inner\n - OR outer repositories' .gitignore), then not only will those ignored\n - files be left alone but the .git/ subdirectory of the nested repo will\n - be left alone too. I am not completely sure if this should be\n - considered a bug (though it seems like it since the lack of the\n - untracked file would result in the .git/ subdirectory being deleted),\n - but in any event it is very minor compared to accidentally deleting user\n - data and I did not dive into it.\n + Finally, there are still a couple bugs with -ffd not cleaning out enough\n + (e.g. missing the nested .git) and with -ffdX possibly cleaning out the\n + wrong files (paying attention to outer .gitignore instead of inner).\n + This patch does not address these cases at all (and does not change the\n + behavior relative to those flags), it only fixes the handling when given\n + a single -f. See\n + https://public-inbox.org/git/20190905212043.GC32087@szeder.dev/ for more\n + discussion of the -ffd[X?] bugs.\n \n Signed-off-by: Elijah Newren \n \n11: 2d36e3f7cb = 11: 4e30e62eb1 clean: rewrap overly long line\n12: 3c5d1ff16a < -: ---------- clean: fix theoretical path corruption\n -: ---------- > 12: de2444f7cb clean: fix theoretical path corruption\n-- \n2.23.0.173.gad11b3a635.dirty\n\n"},{"id":"382263","messageId":"20190912221240.18057-4-newren@gmail.com","threadId":"51732","inReplyTo":"20190912221240.18057-1-newren@gmail.com","subject":"[PATCH v3 03/12] dir: fix off-by-one error in match_pathspec_item","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2019-09-12T22:12:31Z","receivedAt":"2019-09-12T22:12:52Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"For a pathspec like 'foo/bar' comparing against a path named \"foo/\",\nnamelen will be 4, and match[namelen] will be 'b'. The correct location\nof the directory separator is namelen-1.\n\nThe reason the code worked anyway was that the following code immediately\nchecked whether the first matchlen characters matched (which they do) and\nthen bailed and return MATCHED_RECURSIVELY anyway since wildmatch doesn't\nhave the ability to check if \"name\" can be matched as a directory (or\nprefix) against the pathspec.\n\nSigned-off-by: Elijah Newren \n---\n dir.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/dir.c b/dir.c\nindex a9168bed96..bf1a74799e 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -356,8 +356,9 @@ static int match_pathspec_item(const struct index_state *istate,\n \t/* Perform checks to see if \"name\" is a super set of the pathspec */\n \tif (flags & DO_MATCH_SUBMODULE) {\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-\t\t (match[namelen] == '/') &&\n+\t\t (match[namelen-offset] == '/') &&\n \t\t !ps_strncmp(item, match, name, namelen))\n \t\t\treturn MATCHED_RECURSIVELY;\n \n-- \n2.23.0.173.gad11b3a635.dirty\n\n"},{"id":"382264","messageId":"20190912221240.18057-3-newren@gmail.com","threadId":"51732","inReplyTo":"20190912221240.18057-1-newren@gmail.com","subject":"[PATCH v3 02/12] dir: fix typo in comment","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2019-09-12T22:12:30Z","receivedAt":"2019-09-12T22:12:52Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Signed-off-by: Elijah Newren \n---\n dir.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/dir.c b/dir.c\nindex d021c908e5..a9168bed96 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -139,7 +139,7 @@ static size_t common_prefix_len(const struct pathspec *pathspec)\n \t * \":(icase)path\" is treated as a pathspec full of\n \t * wildcard. In other words, only prefix is considered common\n \t * prefix. If the pathspec is abc/foo abc/bar, running in\n-\t * subdir xyz, the common prefix is still xyz, not xuz/abc as\n+\t * subdir xyz, the common prefix is still xyz, not xyz/abc as\n \t * in non-:(icase).\n \t */\n \tGUARD_PATHSPEC(pathspec,\n-- \n2.23.0.173.gad11b3a635.dirty\n\n"},{"id":"382265","messageId":"20190912221240.18057-6-newren@gmail.com","threadId":"51732","inReplyTo":"20190912221240.18057-1-newren@gmail.com","subject":"[PATCH v3 05/12] dir: make the DO_MATCH_SUBMODULE code reusable for a non-submodule case","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2019-09-12T22:12:33Z","receivedAt":"2019-09-12T22:12:53Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"The specific checks done in match_pathspec_item for the DO_MATCH_SUBMODULE\ncase are useful for other cases which have nothing to do with submodules.\nRename this constant; a subsequent commit will make use of this change.\n\nSigned-off-by: Elijah Newren \n---\n dir.c | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/dir.c b/dir.c\nindex 76a3c3894b..b4d656192e 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -273,7 +273,7 @@ static int do_read_blob(const struct object_id *oid, struct oid_stat *oid_stat,\n \n #define DO_MATCH_EXCLUDE (1<<0)\n #define DO_MATCH_DIRECTORY (1<<1)\n-#define DO_MATCH_SUBMODULE (1<<2)\n+#define DO_MATCH_LEADING_PATHSPEC (1<<2)\n \n /*\n * Does 'match' match the given name?\n@@ -354,7 +354,7 @@ 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 super set of the pathspec */\n-\tif (flags & DO_MATCH_SUBMODULE) {\n+\tif (flags & DO_MATCH_LEADING_PATHSPEC) {\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@@ -498,7 +498,7 @@ int submodule_path_match(const struct index_state *istate,\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_SUBMODULE);\n+\t\t\t\t\tDO_MATCH_LEADING_PATHSPEC);\n \treturn matched;\n }\n \n-- \n2.23.0.173.gad11b3a635.dirty\n\n"},{"id":"382267","messageId":"20190912221240.18057-5-newren@gmail.com","threadId":"51732","inReplyTo":"20190912221240.18057-1-newren@gmail.com","subject":"[PATCH v3 04/12] dir: also check directories for matching pathspecs","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2019-09-12T22:12:32Z","receivedAt":"2019-09-12T22:12:54Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Even if a directory doesn't match a pathspec, it is possible, depending\non the precise pathspecs, that some file underneath it might. So we\nspecial case and recurse into the directory for such situations. However,\nwe previously always added any untracked directory that we recursed into\nto the list of untracked paths, regardless of whether the directory\nitself matched the pathspec.\n\nFor the case of git-clean and a set of pathspecs of \"dir/file\" and \"more\",\nthis caused a problem because we'd end up with dir entries for both of\n \"dir\"\n \"dir/file\"\nThen correct_untracked_entries() would try to helpfully prune duplicates\nfor us by removing \"dir/file\" since it's under \"dir\", leaving us with\n \"dir\"\nSince the original pathspec only had \"dir/file\", the only entry left\ndoesn't match and leaves nothing to be removed. (Note that if only one\npathspec was specified, e.g. only \"dir/file\", then the common_prefix_len\noptimizations in fill_directory would cause us to bypass this problem,\nmaking it appear in simple tests that we could correctly remove manually\nspecified pathspecs.)\n\nFix this by actually checking whether the directory we are about to add\nto the list of dir entries actually matches the pathspec; only do this\nmatching check after we have already returned from recursing into the\ndirectory.\n\nSigned-off-by: Elijah Newren \n---\n dir.c | 5 +++++\n t/t7300-clean.sh | 4 ++--\n 2 files changed, 7 insertions(+), 2 deletions(-)\n\ndiff --git a/dir.c b/dir.c\nindex bf1a74799e..76a3c3894b 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -1951,6 +1951,11 @@ static enum path_treatment read_directory_recursive(struct dir_struct *dir,\n \t\t\t\t\t\t\t check_only, stop_at_first_file, pathspec);\n \t\t\tif (subdir_state > dir_state)\n \t\t\t\tdir_state = subdir_state;\n+\n+\t\t\tif (!match_pathspec(istate, pathspec, path.buf, path.len,\n+\t\t\t\t\t 0 /* prefix */, NULL,\n+\t\t\t\t\t 0 /* do NOT special case dirs */))\n+\t\t\t\tstate = path_none;\n \t\t}\n \n \t\tif (check_only) {\ndiff --git a/t/t7300-clean.sh b/t/t7300-clean.sh\nindex 2c254c773c..12617158db 100755\n--- a/t/t7300-clean.sh\n+++ b/t/t7300-clean.sh\n@@ -699,7 +699,7 @@ test_expect_failure 'git clean handles being told what to clean' '\n \ttest_path_is_missing d2/ut\n '\n \n-test_expect_failure 'git clean handles being told what to clean, with -d' '\n+test_expect_success 'git clean handles being told what to clean, with -d' '\n \tmkdir -p d1 d2 &&\n \ttouch d1/ut d2/ut &&\n \tgit clean -ffd */ut &&\n@@ -715,7 +715,7 @@ test_expect_failure 'git clean works if a glob is passed without -d' '\n \ttest_path_is_missing d2/ut\n '\n \n-test_expect_failure 'git clean works if a glob is passed with -d' '\n+test_expect_success 'git clean works if a glob is passed with -d' '\n \tmkdir -p d1 d2 &&\n \ttouch d1/ut d2/ut &&\n \tgit clean -ffd \"*ut\" &&\n-- \n2.23.0.173.gad11b3a635.dirty\n\n"},{"id":"382266","messageId":"20190912221240.18057-7-newren@gmail.com","threadId":"51732","inReplyTo":"20190912221240.18057-1-newren@gmail.com","subject":"[PATCH v3 06/12] dir: if our pathspec might match files under a dir, recurse into it","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2019-09-12T22:12:34Z","receivedAt":"2019-09-12T22:12:56Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"For git clean, if a directory is entirely untracked and the user did not\nspecify -d (corresponding to DIR_SHOW_IGNORED_TOO), then we usually do\nnot want to remove that directory and thus do not recurse into it.\nHowever, if the user manually specified specific (or even globbed) paths\nsomewhere under that directory to remove, then we need to recurse into\nthe directory to make sure we remove the relevant paths under that\ndirectory as the user requested.\n\nNote that this does not mean that the recursed-into directory will be\nadded to dir->entries for later removal; as of a few commits earlier in\nthis series, there is another more strict match check that is run after\nreturning from a recursed-into directory before deciding to add it to the\nlist of entries. Therefore, this will only result in files underneath\nthe given directory which match one of the pathspecs being added to the\nentries list.\n\nTwo notes of potential interest to future readers:\n\n * If we wanted to only recurse into a directory when it is specifically\n matched rather than matched-via-glob (e.g. '*.c'), then we could do\n so via making the final non-zero return in match_pathspec_item be\n MATCHED_RECURSIVELY instead of MATCHED_RECURSIVELY_LEADING_PATHSPEC.\n (Note that the relative order of MATCHED_RECURSIVELY_LEADING_PATHSPEC\n and MATCHED_RECURSIVELY are important for such a change.) I was\n leaving open that possibility while writing an RFC asking for the\n behavior we want, but even though we don't want it, that knowledge\n might help you understand the code flow better.\n\n * There is a growing amount of logic in read_directory_recursive() for\n deciding whether to recurse into a subdirectory. However, there is a\n comment immediately preceding this logic that says to recurse if\n instructed by treat_path(). It may be better for the logic in\n read_directory_recursive() to ultimately be moved to treat_path() (or\n another function it calls, such as treat_directory()), but I have\n left that for someone else to tackle in the future.\n\nSigned-off-by: Elijah Newren \n---\n dir.c | 10 ++++++----\n dir.h | 5 +++--\n t/t7300-clean.sh | 4 ++--\n 3 files changed, 11 insertions(+), 8 deletions(-)\n\ndiff --git a/dir.c b/dir.c\nindex b4d656192e..47c0a99cb5 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -360,7 +360,7 @@ static int match_pathspec_item(const struct index_state *istate,\n \t\tif ((namelen < matchlen) &&\n \t\t (match[namelen-offset] == '/') &&\n \t\t !ps_strncmp(item, match, name, namelen))\n-\t\t\treturn MATCHED_RECURSIVELY;\n+\t\t\treturn MATCHED_RECURSIVELY_LEADING_PATHSPEC;\n \n \t\t/* name\" doesn't match up to the first wild character */\n \t\tif (item->nowildcard_len < item->len &&\n@@ -377,7 +377,7 @@ static int match_pathspec_item(const struct index_state *istate,\n \t\t * The submodules themselves will be able to perform more\n \t\t * accurate matching to determine if the pathspec matches.\n \t\t */\n-\t\treturn MATCHED_RECURSIVELY;\n+\t\treturn MATCHED_RECURSIVELY_LEADING_PATHSPEC;\n \t}\n \n \treturn 0;\n@@ -1939,8 +1939,10 @@ static enum path_treatment read_directory_recursive(struct dir_struct *dir,\n \t\t/* recurse into subdir if instructed by treat_path */\n \t\tif ((state == path_recurse) ||\n \t\t\t((state == path_untracked) &&\n-\t\t\t (dir->flags & DIR_SHOW_IGNORED_TOO) &&\n-\t\t\t (get_dtype(cdir.de, istate, path.buf, path.len) == DT_DIR))) {\n+\t\t\t (get_dtype(cdir.de, istate, path.buf, path.len) == DT_DIR) &&\n+\t\t\t ((dir->flags & DIR_SHOW_IGNORED_TOO) ||\n+\t\t\t do_match_pathspec(istate, pathspec, path.buf, path.len,\n+\t\t\t\t\t baselen, NULL, DO_MATCH_LEADING_PATHSPEC) == MATCHED_RECURSIVELY_LEADING_PATHSPEC))) {\n \t\t\tstruct untracked_cache_dir *ud;\n \t\t\tud = lookup_untracked(dir->untracked, untracked,\n \t\t\t\t\t path.buf + baselen,\ndiff --git a/dir.h b/dir.h\nindex 680079bbe3..46c238ab49 100644\n--- a/dir.h\n+++ b/dir.h\n@@ -211,8 +211,9 @@ int count_slashes(const char *s);\n * when populating the seen[] array.\n */\n #define MATCHED_RECURSIVELY 1\n-#define MATCHED_FNMATCH 2\n-#define MATCHED_EXACTLY 3\n+#define MATCHED_RECURSIVELY_LEADING_PATHSPEC 2\n+#define MATCHED_FNMATCH 3\n+#define MATCHED_EXACTLY 4\n int simple_length(const char *match);\n int no_wildcard(const char *string);\n char *common_prefix(const struct pathspec *pathspec);\ndiff --git a/t/t7300-clean.sh b/t/t7300-clean.sh\nindex 12617158db..d83aeb7dc2 100755\n--- a/t/t7300-clean.sh\n+++ b/t/t7300-clean.sh\n@@ -691,7 +691,7 @@ test_expect_failure 'git clean -d skips nested repo containing ignored files' '\n \ttest_path_is_file nested-repo-with-ignored-file/file\n '\n \n-test_expect_failure 'git clean handles being told what to clean' '\n+test_expect_success 'git clean handles being told what to clean' '\n \tmkdir -p d1 d2 &&\n \ttouch d1/ut d2/ut &&\n \tgit clean -f */ut &&\n@@ -707,7 +707,7 @@ test_expect_success 'git clean handles being told what to clean, with -d' '\n \ttest_path_is_missing d2/ut\n '\n \n-test_expect_failure 'git clean works if a glob is passed without -d' '\n+test_expect_success 'git clean works if a glob is passed without -d' '\n \tmkdir -p d1 d2 &&\n \ttouch d1/ut d2/ut &&\n \tgit clean -f \"*ut\" &&\n-- \n2.23.0.173.gad11b3a635.dirty\n\n"},{"id":"382268","messageId":"20190912221240.18057-8-newren@gmail.com","threadId":"51732","inReplyTo":"20190912221240.18057-1-newren@gmail.com","subject":"[PATCH v3 07/12] dir: add commentary explaining match_pathspec_item's return value","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2019-09-12T22:12:35Z","receivedAt":"2019-09-12T22:12:59Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"The way match_pathspec_item() handles names and pathspecs with trailing\nslash characters, in conjunction with special options like\nDO_MATCH_DIRECTORY and DO_MATCH_LEADING_PATHSPEC were non-obvious, and\nbroken until this patch series. Add a table in a comment explaining the\nintent of how these work.\n\nSigned-off-by: Elijah Newren \n---\n dir.c | 27 +++++++++++++++++++--------\n 1 file changed, 19 insertions(+), 8 deletions(-)\n\ndiff --git a/dir.c b/dir.c\nindex 47c0a99cb5..3b2fe1701c 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -276,16 +276,27 @@ static int do_read_blob(const struct object_id *oid, struct oid_stat *oid_stat,\n #define DO_MATCH_LEADING_PATHSPEC (1<<2)\n \n /*\n- * Does 'match' match the given name?\n- * A match is found if\n+ * Does the given pathspec match the given name? A match is found if\n *\n- * (1) the 'match' string is leading directory of 'name', or\n- * (2) the 'match' string is a wildcard and matches 'name', or\n- * (3) the 'match' string is exactly the same as 'name'.\n+ * (1) the pathspec string is leading directory of 'name' (\"RECURSIVELY\"), or\n+ * (2) the pathspec string has a leading part matching 'name' (\"LEADING\"), or\n+ * (3) the pathspec string is a wildcard and matches 'name' (\"WILDCARD\"), or\n+ * (4) the pathspec string is exactly the same as 'name' (\"EXACT\").\n *\n- * and the return value tells which case it was.\n+ * Return value tells which case it was (1-4), or 0 when there is no match.\n *\n- * It returns 0 when there is no match.\n+ * It may be instructive to look at a small table of concrete examples\n+ * to understand the differences between 1, 2, and 4:\n+ *\n+ * Pathspecs\n+ * | a/b | a/b/ | a/b/c\n+ * ------+-----------+-----------+------------\n+ * a/b | EXACT | EXACT[1] | LEADING[2]\n+ * Names a/b/ | RECURSIVE | EXACT | LEADING[2]\n+ * a/b/c | RECURSIVE | RECURSIVE | EXACT\n+ *\n+ * [1] Only if DO_MATCH_DIRECTORY is passed; otherwise, this is NOT a match.\n+ * [2] Only if DO_MATCH_LEADING_PATHSPEC is passed; otherwise, not a match.\n */\n static int match_pathspec_item(const struct index_state *istate,\n \t\t\t const struct pathspec_item *item, int prefix,\n@@ -353,7 +364,7 @@ static int match_pathspec_item(const struct index_state *istate,\n \t\t\t item->nowildcard_len - prefix))\n \t\treturn MATCHED_FNMATCH;\n \n-\t/* Perform checks to see if \"name\" is a super set of the pathspec */\n+\t/* Perform checks to see if \"name\" is a leading string of the pathspec */\n \tif (flags & DO_MATCH_LEADING_PATHSPEC) {\n \t\t/* name is a literal prefix of the pathspec */\n \t\tint offset = name[namelen-1] == '/' ? 1 : 0;\n-- \n2.23.0.173.gad11b3a635.dirty\n\n"},{"id":"382269","messageId":"20190912221240.18057-10-newren@gmail.com","threadId":"51732","inReplyTo":"20190912221240.18057-1-newren@gmail.com","subject":"[PATCH v3 09/12] clean: disambiguate the definition of -d","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2019-09-12T22:12:37Z","receivedAt":"2019-09-12T22:12:59Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"The -d flag pre-dated git-clean's ability to have paths specified. As\nsuch, the default for git-clean was to only remove untracked files in\nthe current directory, and -d existed to allow it to recurse into\nsubdirectories.\n\nThe interaction of paths and the -d option appears to not have been\ncarefully considered, as evidenced by numerous bugs and a dearth of\ntests covering such pairings in the testsuite. The definition turns out\nto be important, so let's look at some of the various ways one could\ninterpret the -d option:\n\n A) Without -d, only look in subdirectories which contain tracked\n files under them; with -d, also look in subdirectories which\n are untracked for files to clean.\n\n B) Without specified paths from the user for us to delete, we need to\n have some kind of default, so...without -d, only look in\n subdirectories which contain tracked files under them; with -d,\n also look in subdirectories which are untracked for files to clean.\n\nThe important distinction here is that choice B says that the presence\nor absence of '-d' is irrelevant if paths are specified. The logic\nbehind option B is that if a user explicitly asked us to clean a\nspecified pathspec, then we should clean anything that matches that\npathspec. Some examples may clarify. Should\n\n git clean -f untracked_dir/file\n\nremove untracked_dir/file or not? It seems crazy not to, but a strict\nreading of option A says it shouldn't be removed. How about\n\n git clean -f untracked_dir/file1 tracked_dir/file2\n\nor\n\n git clean -f untracked_dir_1/file1 untracked_dir_2/file2\n\n? Should it remove either or both of these files? Should it require\nmultiple runs to remove both the files listed? (If this sounds like a\ncrazy question to even ask, see the commit message of \"t7300: Add some\ntestcases showing failure to clean specified pathspecs\" added earlier in\nthis patch series.) What if -ffd were used instead of -f -- should that\nallow these to be removed? Should it take multiple invocations with\n-ffd? What if a glob (such as '*tracked*') were used instead of\nspelling out the directory names? What if the filenames involved globs,\nsuch as\n\n git clean -f '*.o'\n\nor\n\n git clean -f '*/*.o'\n\n?\n\nThe current documentation actually suggests a definition that is\nslightly different than choice A, and the implementation prior to this\nseries provided something radically different than either choices A or\nB. (The implementation, though, was clearly just buggy). There may be\nother choices as well. However, for almost any given choice of\ndefinition for -d that I can think of, some of the examples above will\nappear buggy to the user. The only case that doesn't have negative\nsurprises is choice B: treat a user-specified path as a request to clean\nall untracked files which match that path specification, including\nrecursing into any untracked directories.\n\nChange the documentation and basic implementation to use this\ndefinition.\n\nThere were two regression tests that indirectly depended on the current\nimplementation, but neither was about subdirectory handling. These two\ntests were introduced in commit 5b7570cfb41c (\"git-clean: add tests for\nrelative path\", 2008-03-07) which was solely created to add coverage for\nthe changes in commit fb328947c8e (\"git-clean: correct printing relative\npath\", 2008-03-07). Both tests specified a directory that happened to\nhave an untracked subdirectory, but both were only checking that the\nresulting printout of a file that was removed was shown with a relative\npath. Update these tests appropriately.\n\nSigned-off-by: Elijah Newren \n---\n Documentation/git-clean.txt | 10 ++++++----\n builtin/clean.c | 8 ++++++++\n t/t7300-clean.sh | 2 ++\n 3 files changed, 16 insertions(+), 4 deletions(-)\n\ndiff --git a/Documentation/git-clean.txt b/Documentation/git-clean.txt\nindex e84ffc9396..3ab749b921 100644\n--- a/Documentation/git-clean.txt\n+++ b/Documentation/git-clean.txt\n@@ -26,10 +26,12 @@ are affected.\n OPTIONS\n -------\n -d::\n-\tRemove untracked directories in addition to untracked files.\n-\tIf an untracked directory is managed by a different Git\n-\trepository, it is not removed by default. Use -f option twice\n-\tif you really want to remove such a directory.\n+\tNormally, when no is specified, git clean will not\n+\trecurse into untracked directories to avoid removing too much.\n+\tSpecify -d to have it recurse into such directories as well.\n+\tIf any paths are specified, -d is irrelevant; all untracked\n+\tfiles matching the specified paths (with exceptions for nested\n+\tgit directories mentioned under `--force`) will be removed.\n \n -f::\n --force::\ndiff --git a/builtin/clean.c b/builtin/clean.c\nindex d5579da716..68d70e41c0 100644\n--- a/builtin/clean.c\n+++ b/builtin/clean.c\n@@ -949,6 +949,14 @@ int cmd_clean(int argc, const char **argv, const char *prefix)\n \n \tdir.flags |= DIR_SHOW_OTHER_DIRECTORIES;\n \n+\tif (argc) {\n+\t\t/*\n+\t\t * Remaining args implies pathspecs specified, and we should\n+\t\t * recurse within those.\n+\t\t */\n+\t\tremove_directories = 1;\n+\t}\n+\n \tif (remove_directories)\n \t\tdir.flags |= DIR_SHOW_IGNORED_TOO | DIR_KEEP_UNTRACKED_CONTENTS;\n \ndiff --git a/t/t7300-clean.sh b/t/t7300-clean.sh\nindex d83aeb7dc2..530dfdab34 100755\n--- a/t/t7300-clean.sh\n+++ b/t/t7300-clean.sh\n@@ -117,6 +117,7 @@ test_expect_success C_LOCALE_OUTPUT 'git clean with relative prefix' '\n \twould_clean=$(\n \t\tcd docs &&\n \t\tgit clean -n ../src |\n+\t\tgrep part3 |\n \t\tsed -n -e \"s|^Would remove ||p\"\n \t) &&\n \tverbose test \"$would_clean\" = ../src/part3.c\n@@ -129,6 +130,7 @@ test_expect_success C_LOCALE_OUTPUT 'git clean with absolute path' '\n \twould_clean=$(\n \t\tcd docs &&\n \t\tgit clean -n \"$(pwd)/../src\" |\n+\t\tgrep part3 |\n \t\tsed -n -e \"s|^Would remove ||p\"\n \t) &&\n \tverbose test \"$would_clean\" = ../src/part3.c\n-- \n2.23.0.173.gad11b3a635.dirty\n\n"},{"id":"382270","messageId":"20190912221240.18057-11-newren@gmail.com","threadId":"51732","inReplyTo":"20190912221240.18057-1-newren@gmail.com","subject":"[PATCH v3 10/12] clean: avoid removing untracked files in a nested git repository","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2019-09-12T22:12:38Z","receivedAt":"2019-09-12T22:13:00Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Users expect files in a nested git repository to be left alone unless\nsufficiently forced (with two -f's). Unfortunately, in certain\ncircumstances, git would delete both tracked (and possibly dirty) files\nand untracked files within a nested repository. To explain how this\nhappens, let's contrast a couple cases. First, take the following\nexample setup (which assumes we are already within a git repo):\n\n git init nested\n cd nested\n >tracked\n git add tracked\n git commit -m init\n >untracked\n cd ..\n\nIn this setup, everything works as expected; running 'git clean -fd'\nwill result in fill_directory() returning the following paths:\n nested/\n nested/tracked\n nested/untracked\nand then correct_untracked_entries() would notice this can be compressed\nto\n nested/\nand then since \"nested/\" is a directory, we would call\nremove_dirs(\"nested/\", ...), which would\ncheck is_nonbare_repository_dir() and then decide to skip it.\n\nHowever, if someone also creates an ignored file:\n >nested/ignored\nthen running 'git clean -fd' would result in fill_directory() returning\nthe same paths:\n nested/\n nested/tracked\n nested/untracked\nbut correct_untracked_entries() will notice that we had ignored entries\nunder nested/ and thus simplify this list to\n nested/tracked\n nested/untracked\nSince these are not directories, we do not call remove_dirs() which was\nthe only place that had the is_nonbare_repository_dir() safety check --\nresulting in us deleting both the untracked file and the tracked (and\npossibly dirty) file.\n\nOne possible fix for this issue would be walking the parent directories\nof each path and checking if they represent nonbare repositories, but\nthat would be wasteful. Even if we added caching of some sort, it's\nstill a waste because we should have been able to check that \"nested/\"\nrepresented a nonbare repository before even descending into it in the\nfirst place. Add a DIR_SKIP_NESTED_GIT flag to dir_struct.flags and use\nit to prevent fill_directory() and friends from descending into nested\ngit repos.\n\nWith this change, we also modify two regression tests added in commit\n91479b9c72f1 (\"t7300: add tests to document behavior of clean and nested\ngit\", 2015-06-15). That commit, nor its series, nor the six previous\niterations of that series on the mailing list discussed why those tests\ncoded the expectation they did. In fact, it appears their purpose was\nsimply to test _existing_ behavior to make sure that the performance\nchanges didn't change the behavior. However, these two tests directly\ncontradicted the manpage's claims that two -f's were required to delete\nfiles/directories under a nested git repository. While one could argue\nthat the user gave an explicit path which matched files/directories that\nwere within a nested repository, there's a slippery slope that becomes\nvery difficult for users to understand once you go down that route (e.g.\nwhat if they specified \"git clean -f -d '*.c'\"?) It would also be hard\nto explain what the exact behavior was; avoid such problems by making it\nreally simple.\n\nAlso, clean up some grammar errors describing this functionality in the\ngit-clean manpage.\n\nFinally, there are still a couple bugs with -ffd not cleaning out enough\n(e.g. missing the nested .git) and with -ffdX possibly cleaning out the\nwrong files (paying attention to outer .gitignore instead of inner).\nThis patch does not address these cases at all (and does not change the\nbehavior relative to those flags), it only fixes the handling when given\na single -f. See\nhttps://public-inbox.org/git/20190905212043.GC32087@szeder.dev/ for more\ndiscussion of the -ffd[X?] bugs.\n\nSigned-off-by: Elijah Newren \n---\n Documentation/git-clean.txt | 6 +++---\n builtin/clean.c | 2 ++\n dir.c | 10 ++++++++++\n dir.h | 3 ++-\n t/t7300-clean.sh | 10 +++++-----\n 5 files changed, 22 insertions(+), 9 deletions(-)\n\ndiff --git a/Documentation/git-clean.txt b/Documentation/git-clean.txt\nindex 3ab749b921..ba31d8d166 100644\n--- a/Documentation/git-clean.txt\n+++ b/Documentation/git-clean.txt\n@@ -37,9 +37,9 @@ OPTIONS\n --force::\n \tIf the Git configuration variable clean.requireForce is not set\n \tto false, 'git clean' will refuse to delete files or directories\n-\tunless given -f or -i. Git will refuse to delete directories\n-\twith .git sub directory or file unless a second -f\n-\tis given.\n+\tunless given -f or -i. Git will refuse to modify untracked\n+\tnested git repositories (directories with a .git subdirectory)\n+\tunless a second -f is given.\n \n -i::\n --interactive::\ndiff --git a/builtin/clean.c b/builtin/clean.c\nindex 68d70e41c0..3a7a63ae71 100644\n--- a/builtin/clean.c\n+++ b/builtin/clean.c\n@@ -946,6 +946,8 @@ int cmd_clean(int argc, const char **argv, const char *prefix)\n \n \tif (force > 1)\n \t\trm_flags = 0;\n+\telse\n+\t\tdir.flags |= DIR_SKIP_NESTED_GIT;\n \n \tdir.flags |= DIR_SHOW_OTHER_DIRECTORIES;\n \ndiff --git a/dir.c b/dir.c\nindex 3b2fe1701c..7ff79170fc 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -1451,6 +1451,16 @@ static enum path_treatment treat_directory(struct dir_struct *dir,\n \t\treturn path_none;\n \n \tcase index_nonexistent:\n+\t\tif (dir->flags & DIR_SKIP_NESTED_GIT) {\n+\t\t\tint nested_repo;\n+\t\t\tstruct strbuf sb = STRBUF_INIT;\n+\t\t\tstrbuf_addstr(&sb, dirname);\n+\t\t\tnested_repo = is_nonbare_repository_dir(&sb);\n+\t\t\tstrbuf_release(&sb);\n+\t\t\tif (nested_repo)\n+\t\t\t\treturn path_none;\n+\t\t}\n+\n \t\tif (dir->flags & DIR_SHOW_OTHER_DIRECTORIES)\n \t\t\tbreak;\n \t\tif (exclude &&\ndiff --git a/dir.h b/dir.h\nindex 46c238ab49..739aea7c96 100644\n--- a/dir.h\n+++ b/dir.h\n@@ -156,7 +156,8 @@ struct dir_struct {\n \t\tDIR_SHOW_IGNORED_TOO = 1<<5,\n \t\tDIR_COLLECT_KILLED_ONLY = 1<<6,\n \t\tDIR_KEEP_UNTRACKED_CONTENTS = 1<<7,\n-\t\tDIR_SHOW_IGNORED_TOO_MODE_MATCHING = 1<<8\n+\t\tDIR_SHOW_IGNORED_TOO_MODE_MATCHING = 1<<8,\n+\t\tDIR_SKIP_NESTED_GIT = 1<<9\n \t} flags;\n \tstruct dir_entry **entries;\n \tstruct dir_entry **ignored;\ndiff --git a/t/t7300-clean.sh b/t/t7300-clean.sh\nindex 530dfdab34..6e6d24c1c3 100755\n--- a/t/t7300-clean.sh\n+++ b/t/t7300-clean.sh\n@@ -549,7 +549,7 @@ test_expect_failure 'nested (non-empty) bare repositories should be cleaned even\n \ttest_path_is_missing strange_bare\n '\n \n-test_expect_success 'giving path in nested git work tree will remove it' '\n+test_expect_success 'giving path in nested git work tree will NOT remove it' '\n \trm -fr repo &&\n \tmkdir repo &&\n \t(\n@@ -561,7 +561,7 @@ test_expect_success 'giving path in nested git work tree will remove it' '\n \tgit clean -f -d repo/bar/baz &&\n \ttest_path_is_file repo/.git/HEAD &&\n \ttest_path_is_dir repo/bar/ &&\n-\ttest_path_is_missing repo/bar/baz\n+\ttest_path_is_file repo/bar/baz/hello.world\n '\n \n test_expect_success 'giving path to nested .git will not remove it' '\n@@ -579,7 +579,7 @@ test_expect_success 'giving path to nested .git will not remove it' '\n \ttest_path_is_dir untracked/\n '\n \n-test_expect_success 'giving path to nested .git/ will remove contents' '\n+test_expect_success 'giving path to nested .git/ will NOT remove contents' '\n \trm -fr repo untracked &&\n \tmkdir repo untracked &&\n \t(\n@@ -589,7 +589,7 @@ test_expect_success 'giving path to nested .git/ will remove contents' '\n \t) &&\n \tgit clean -f -d repo/.git/ &&\n \ttest_path_is_dir repo/.git &&\n-\ttest_dir_is_empty repo/.git &&\n+\ttest_path_is_file repo/.git/HEAD &&\n \ttest_path_is_dir untracked/\n '\n \n@@ -671,7 +671,7 @@ test_expect_success 'git clean -d skips untracked dirs containing ignored files'\n \ttest_path_is_missing foo/b/bb\n '\n \n-test_expect_failure 'git clean -d skips nested repo containing ignored files' '\n+test_expect_success 'git clean -d skips nested repo containing ignored files' '\n \ttest_when_finished \"rm -rf nested-repo-with-ignored-file\" &&\n \n \tgit init nested-repo-with-ignored-file &&\n-- \n2.23.0.173.gad11b3a635.dirty\n\n"},{"id":"382271","messageId":"20190912221240.18057-12-newren@gmail.com","threadId":"51732","inReplyTo":"20190912221240.18057-1-newren@gmail.com","subject":"[PATCH v3 11/12] clean: rewrap overly long line","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2019-09-12T22:12:39Z","receivedAt":"2019-09-12T22:13:02Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Signed-off-by: Elijah Newren \n---\n builtin/clean.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/clean.c b/builtin/clean.c\nindex 3a7a63ae71..6030842f3a 100644\n--- a/builtin/clean.c\n+++ b/builtin/clean.c\n@@ -158,7 +158,8 @@ static int remove_dirs(struct strbuf *path, const char *prefix, int force_flag,\n \n \t*dir_gone = 1;\n \n-\tif ((force_flag & REMOVE_DIR_KEEP_NESTED_GIT) && is_nonbare_repository_dir(path)) {\n+\tif ((force_flag & REMOVE_DIR_KEEP_NESTED_GIT) &&\n+\t is_nonbare_repository_dir(path)) {\n \t\tif (!quiet) {\n \t\t\tquote_path_relative(path->buf, prefix, "ed);\n \t\t\tprintf(dry_run ? _(msg_would_skip_git_dir) : _(msg_skip_git_dir),\n-- \n2.23.0.173.gad11b3a635.dirty\n\n"},{"id":"382272","messageId":"20190912221240.18057-9-newren@gmail.com","threadId":"51732","inReplyTo":"20190912221240.18057-1-newren@gmail.com","subject":"[PATCH v3 08/12] git-clean.txt: do not claim we will delete files with -n/--dry-run","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2019-09-12T22:12:36Z","receivedAt":"2019-09-12T22:13:03Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"It appears that the wrong option got included in the list of what will\ncause git-clean to actually take action. Correct the list.\n\nSigned-off-by: Elijah Newren \n---\n Documentation/git-clean.txt | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/Documentation/git-clean.txt b/Documentation/git-clean.txt\nindex db876f7dde..e84ffc9396 100644\n--- a/Documentation/git-clean.txt\n+++ b/Documentation/git-clean.txt\n@@ -35,7 +35,7 @@ OPTIONS\n --force::\n \tIf the Git configuration variable clean.requireForce is not set\n \tto false, 'git clean' will refuse to delete files or directories\n-\tunless given -f, -n or -i. Git will refuse to delete directories\n+\tunless given -f or -i. Git will refuse to delete directories\n \twith .git sub directory or file unless a second -f\n \tis given.\n \n-- \n2.23.0.173.gad11b3a635.dirty\n\n"},{"id":"382273","messageId":"20190912221240.18057-13-newren@gmail.com","threadId":"51732","inReplyTo":"20190912221240.18057-1-newren@gmail.com","subject":"[PATCH v3 12/12] clean: fix theoretical path corruption","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2019-09-12T22:12:40Z","receivedAt":"2019-09-12T22:13:05Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"cmd_clean() had the following code structure:\n\n struct strbuf abs_path = STRBUF_INIT;\n for_each_string_list_item(item, &del_list) {\n strbuf_addstr(&abs_path, prefix);\n strbuf_addstr(&abs_path, item->string);\n PROCESS(&abs_path);\n strbuf_reset(&abs_path);\n }\n\nwhere I've elided a bunch of unnecessary details and PROCESS(&abs_path)\nrepresents a big chunk of code rather than an actual function call. One\npiece of PROCESS was:\n\n if (lstat(abs_path.buf, &st))\n continue;\n\nwhich would cause the strbuf_reset() to be missed -- meaning that the\nnext path to be handled would have two paths concatenated. This path\nused to use die_errno() instead of continue prior to commit 396049e5fb62\n(\"git-clean: refactor git-clean into two phases\", 2013-06-25), but my\nunderstanding of how correct_untracked_entries() works is that it will\nprevent both dir/ and dir/file from being in the list to clean so this\nshould be dead code and the die_errno() should be safe. But I hesitate\nto remove it since I am not certain.\n\nHowever, we can fix both this bug and possible similar future bugs by\nsimply moving the strbuf_reset(&abs_path) to the beginning of the loop.\nIt'll result in N calls to strbuf_reset() instead of N-1, but that's a\nsmall price to pay to avoid sneaky bugs like this.\n\nSigned-off-by: Elijah Newren \n---\n builtin/clean.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/clean.c b/builtin/clean.c\nindex 6030842f3a..4cf2399f59 100644\n--- a/builtin/clean.c\n+++ b/builtin/clean.c\n@@ -1018,6 +1018,7 @@ int cmd_clean(int argc, const char **argv, const char *prefix)\n \tfor_each_string_list_item(item, &del_list) {\n \t\tstruct stat st;\n \n+\t\tstrbuf_reset(&abs_path);\n \t\tif (prefix)\n \t\t\tstrbuf_addstr(&abs_path, prefix);\n \n@@ -1051,7 +1052,6 @@ int cmd_clean(int argc, const char **argv, const char *prefix)\n \t\t\t\tprintf(dry_run ? _(msg_would_remove) : _(msg_remove), qname);\n \t\t\t}\n \t\t}\n-\t\tstrbuf_reset(&abs_path);\n \t}\n \n \tstrbuf_release(&abs_path);\n-- \n2.23.0.173.gad11b3a635.dirty\n\n"},{"id":"382320","messageId":"xmqqblvodo1v.fsf@gitster-ct.c.googlers.com","threadId":"51732","inReplyTo":"20190912221240.18057-2-newren@gmail.com","subject":"Re: [PATCH v3 01/12] t7300: add testcases showing failure to clean specified pathspecs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-09-13T18:54:52Z","receivedAt":"2019-09-13T18:54:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren writes:\n\n> +test_expect_failure 'git clean handles being told what to clean' '\n> +\tmkdir -p d1 d2 &&\n> +\ttouch d1/ut d2/ut &&\n> +\tgit clean -f */ut &&\n> +\ttest_path_is_missing d1/ut &&\n> +\ttest_path_is_missing d2/ut\n> +'\n\nLooks like d1 and d2 are new directories and the paths we see in the\ntest are the only ones that are involved (i.e. we do not rely on any\nleftover cruft in d[12]/ from previous tests). If so, perhaps it is\neasier to follow by starting the tests with \"rm -fr d1 d2 &&\" or\nsomething to assure the readers of the script (not this patch, but\nthe resulting file down the road) about the isolation? The same\ncomment applies to the remainder.\n\nAlso, you talked about tracked paths in the proposed log message; do\nthey not participate in reproducing the issue(s)?\n\nThanks.\n\n\n> +test_expect_failure 'git clean handles being told what to clean, with -d' '\n> +\tmkdir -p d1 d2 &&\n> +\ttouch d1/ut d2/ut &&\n> +\tgit clean -ffd */ut &&\n> +\ttest_path_is_missing d1/ut &&\n> +\ttest_path_is_missing d2/ut\n> +'\n> +\n> +test_expect_failure 'git clean works if a glob is passed without -d' '\n> +\tmkdir -p d1 d2 &&\n> +\ttouch d1/ut d2/ut &&\n> +\tgit clean -f \"*ut\" &&\n> +\ttest_path_is_missing d1/ut &&\n> +\ttest_path_is_missing d2/ut\n> +'\n> +\n> +test_expect_failure 'git clean works if a glob is passed with -d' '\n> +\tmkdir -p d1 d2 &&\n> +\ttouch d1/ut d2/ut &&\n> +\tgit clean -ffd \"*ut\" &&\n> +\ttest_path_is_missing d1/ut &&\n> +\ttest_path_is_missing d2/ut\n> +'\n> +\n> test_expect_success MINGW 'handle clean & core.longpaths = false nicely' '\n> \ttest_config core.longpaths false &&\n> \ta50=aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa &&\n"},{"id":"382321","messageId":"xmqq7e6cdnkr.fsf@gitster-ct.c.googlers.com","threadId":"51732","inReplyTo":"20190912221240.18057-4-newren@gmail.com","subject":"Re: [PATCH v3 03/12] dir: fix off-by-one error in match_pathspec_item","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-09-13T19:05:08Z","receivedAt":"2019-09-13T19:05:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren writes:\n\n> For a pathspec like 'foo/bar' comparing against a path named \"foo/\",\n> namelen will be 4, and match[namelen] will be 'b'. The correct location\n> of the directory separator is namelen-1.\n\nAnd the reason why name[namelen-1] may not be slash, in which case\nyour new code makes offset 0, is because we need to handle what\ncase? When path is \"foo\" (not \"foo/\")? Just makes me wonder why\nthis callee allows the caller(s) to be inconsistent, sometimes\nincluding the trailing slash in tuple, sometimes\nnot.\n\n> The reason the code worked anyway was that the following code immediately\n> checked whether the first matchlen characters matched (which they do) and\n> then bailed and return MATCHED_RECURSIVELY anyway since wildmatch doesn't\n> have the ability to check if \"name\" can be matched as a directory (or\n> prefix) against the pathspec.\n\nNicely spotted and explained.\n\n>\n> Signed-off-by: Elijah Newren \n> ---\n> dir.c | 3 ++-\n> 1 file changed, 2 insertions(+), 1 deletion(-)\n>\n> diff --git a/dir.c b/dir.c\n> index a9168bed96..bf1a74799e 100644\n> --- a/dir.c\n> +++ b/dir.c\n> @@ -356,8 +356,9 @@ static int match_pathspec_item(const struct index_state *istate,\n> \t/* Perform checks to see if \"name\" is a super set of the pathspec */\n> \tif (flags & DO_MATCH_SUBMODULE) {\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> -\t\t (match[namelen] == '/') &&\n> +\t\t (match[namelen-offset] == '/') &&\n> \t\t !ps_strncmp(item, match, name, namelen))\n> \t\t\treturn MATCHED_RECURSIVELY;\n"},{"id":"382322","messageId":"CABPp-BG1Hvr6joYh-qT6B-8iEetGtj2pE5Mo-gVjdDRre=yrag@mail.gmail.com","threadId":"51732","inReplyTo":"xmqqblvodo1v.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v3 01/12] t7300: add testcases showing failure to clean specified pathspecs","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2019-09-13T19:10:54Z","receivedAt":"2019-09-13T19:11:07Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Fri, Sep 13, 2019 at 11:54 AM Junio C Hamano wrote:\n>\n> Elijah Newren writes:\n>\n> > +test_expect_failure 'git clean handles being told what to clean' '\n> > + mkdir -p d1 d2 &&\n> > + touch d1/ut d2/ut &&\n> > + git clean -f */ut &&\n> > + test_path_is_missing d1/ut &&\n> > + test_path_is_missing d2/ut\n> > +'\n>\n> Looks like d1 and d2 are new directories and the paths we see in the\n> test are the only ones that are involved (i.e. we do not rely on any\n> leftover cruft in d[12]/ from previous tests). If so, perhaps it is\n> easier to follow by starting the tests with \"rm -fr d1 d2 &&\" or\n> something to assure the readers of the script (not this patch, but\n> the resulting file down the road) about the isolation? The same\n> comment applies to the remainder.\n\nMakes sense.\n\n> Also, you talked about tracked paths in the proposed log message; do\n> they not participate in reproducing the issue(s)?\n\nIf there is only one directory which has no tracked files, then the\nuser can clean up the files -- but confusingly, they have to issue the\nsame git-clean command multiple times. If multiple directories have\nno untracked files, git-clean will never clean them out. I probably\ndidn't do a very good job explaining that although I started with the\ncase with one tracked, that I view the case without any as the more\ngeneral case -- and that solving it solves both problems. I could\nprobably make that clearer in the commit message. (Or maybe just add\nmore testcases even if slightly duplicative, I guess.)\n"},{"id":"382323","messageId":"xmqqwoecc75e.fsf@gitster-ct.c.googlers.com","threadId":"51732","inReplyTo":"20190912221240.18057-7-newren@gmail.com","subject":"Re: [PATCH v3 06/12] dir: if our pathspec might match files under a dir, recurse into it","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-09-13T19:45:17Z","receivedAt":"2019-09-13T19:45:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren writes:\n\n> For git clean, if a directory is entirely untracked and the user did not\n> specify -d (corresponding to DIR_SHOW_IGNORED_TOO), then we usually do\n> not want to remove that directory and thus do not recurse into it.\n\nMakes sense. To clean named paths in such a directory, we'd need an\noption to recurse into it to find them, yet make sure the directory\nitself does not get removed.\n\n> However, if the user manually specified specific (or even globbed) paths\n> somewhere under that directory to remove, then we need to recurse into\n> the directory to make sure we remove the relevant paths under that\n> directory as the user requested.\n\nSurely.\n\n> @@ -1939,8 +1939,10 @@ static enum path_treatment read_directory_recursive(struct dir_struct *dir,\n> \t\t/* recurse into subdir if instructed by treat_path */\n> \t\tif ((state == path_recurse) ||\n> \t\t\t((state == path_untracked) &&\n> -\t\t\t (dir->flags & DIR_SHOW_IGNORED_TOO) &&\n> -\t\t\t (get_dtype(cdir.de, istate, path.buf, path.len) == DT_DIR))) {\n> +\t\t\t (get_dtype(cdir.de, istate, path.buf, path.len) == DT_DIR) &&\n> +\t\t\t ((dir->flags & DIR_SHOW_IGNORED_TOO) ||\n> +\t\t\t do_match_pathspec(istate, pathspec, path.buf, path.len,\n> +\t\t\t\t\t baselen, NULL, DO_MATCH_LEADING_PATHSPEC) == MATCHED_RECURSIVELY_LEADING_PATHSPEC))) {\n> \t\t\tstruct untracked_cache_dir *ud;\n> \t\t\tud = lookup_untracked(dir->untracked, untracked,\n> \t\t\t\t\t path.buf + baselen,\n\nOK.\n\n> diff --git a/t/t7300-clean.sh b/t/t7300-clean.sh\n> index 12617158db..d83aeb7dc2 100755\n> --- a/t/t7300-clean.sh\n> +++ b/t/t7300-clean.sh\n> @@ -691,7 +691,7 @@ test_expect_failure 'git clean -d skips nested repo containing ignored files' '\n> \ttest_path_is_file nested-repo-with-ignored-file/file\n> '\n> \n> -test_expect_failure 'git clean handles being told what to clean' '\n> +test_expect_success 'git clean handles being told what to clean' '\n> \tmkdir -p d1 d2 &&\n> \ttouch d1/ut d2/ut &&\n> \tgit clean -f */ut &&\n> @@ -707,7 +707,7 @@ test_expect_success 'git clean handles being told what to clean, with -d' '\n> \ttest_path_is_missing d2/ut\n> '\n> \n> -test_expect_failure 'git clean works if a glob is passed without -d' '\n> +test_expect_success 'git clean works if a glob is passed without -d' '\n> \tmkdir -p d1 d2 &&\n> \ttouch d1/ut d2/ut &&\n> \tgit clean -f \"*ut\" &&\n\nNice.\n\nThanks.\n"},{"id":"382326","messageId":"xmqqsgp0c68k.fsf@gitster-ct.c.googlers.com","threadId":"51732","inReplyTo":"20190912221240.18057-8-newren@gmail.com","subject":"Re: [PATCH v3 07/12] dir: add commentary explaining match_pathspec_item's return value","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-09-13T20:04:59Z","receivedAt":"2019-09-13T20:05:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren writes:\n\n> The way match_pathspec_item() handles names and pathspecs with trailing\n> slash characters, in conjunction with special options like\n> DO_MATCH_DIRECTORY and DO_MATCH_LEADING_PATHSPEC were non-obvious, and\n> broken until this patch series. Add a table in a comment explaining the\n> intent of how these work.\n\nThanks.\n\n>\n> Signed-off-by: Elijah Newren \n> ---\n> dir.c | 27 +++++++++++++++++++--------\n> 1 file changed, 19 insertions(+), 8 deletions(-)\n>\n> diff --git a/dir.c b/dir.c\n> index 47c0a99cb5..3b2fe1701c 100644\n> --- a/dir.c\n> +++ b/dir.c\n> @@ -276,16 +276,27 @@ static int do_read_blob(const struct object_id *oid, struct oid_stat *oid_stat,\n> #define DO_MATCH_LEADING_PATHSPEC (1<<2)\n> \n> /*\n> - * Does 'match' match the given name?\n> - * A match is found if\n> + * Does the given pathspec match the given name? A match is found if\n> *\n> - * (1) the 'match' string is leading directory of 'name', or\n> - * (2) the 'match' string is a wildcard and matches 'name', or\n> - * (3) the 'match' string is exactly the same as 'name'.\n> + * (1) the pathspec string is leading directory of 'name' (\"RECURSIVELY\"), or\n> + * (2) the pathspec string has a leading part matching 'name' (\"LEADING\"), or\n> + * (3) the pathspec string is a wildcard and matches 'name' (\"WILDCARD\"), or\n> + * (4) the pathspec string is exactly the same as 'name' (\"EXACT\").\n> *\n> - * and the return value tells which case it was.\n> + * Return value tells which case it was (1-4), or 0 when there is no match.\n> *\n> - * It returns 0 when there is no match.\n> + * It may be instructive to look at a small table of concrete examples\n> + * to understand the differences between 1, 2, and 4:\n> + *\n> + * Pathspecs\n> + * | a/b | a/b/ | a/b/c\n> + * ------+-----------+-----------+------------\n> + * a/b | EXACT | EXACT[1] | LEADING[2]\n> + * Names a/b/ | RECURSIVE | EXACT | LEADING[2]\n> + * a/b/c | RECURSIVE | RECURSIVE | EXACT\n> + *\n> + * [1] Only if DO_MATCH_DIRECTORY is passed; otherwise, this is NOT a match.\n> + * [2] Only if DO_MATCH_LEADING_PATHSPEC is passed; otherwise, not a match.\n> */\n> static int match_pathspec_item(const struct index_state *istate,\n> \t\t\t const struct pathspec_item *item, int prefix,\n> @@ -353,7 +364,7 @@ static int match_pathspec_item(const struct index_state *istate,\n> \t\t\t item->nowildcard_len - prefix))\n> \t\treturn MATCHED_FNMATCH;\n> \n> -\t/* Perform checks to see if \"name\" is a super set of the pathspec */\n> +\t/* Perform checks to see if \"name\" is a leading string of the pathspec */\n> \tif (flags & DO_MATCH_LEADING_PATHSPEC) {\n> \t\t/* name is a literal prefix of the pathspec */\n> \t\tint offset = name[namelen-1] == '/' ? 1 : 0;\n"},{"id":"382334","messageId":"xmqqk1acc54j.fsf@gitster-ct.c.googlers.com","threadId":"51732","inReplyTo":"CABPp-BG1Hvr6joYh-qT6B-8iEetGtj2pE5Mo-gVjdDRre=yrag@mail.gmail.com","subject":"Re: [PATCH v3 01/12] t7300: add testcases showing failure to clean specified pathspecs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-09-13T20:29:00Z","receivedAt":"2019-09-13T20:29:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren writes:\n\n>> Also, you talked about tracked paths in the proposed log message; do\n>> they not participate in reproducing the issue(s)?\n>\n> If there is only one directory which has no tracked files, then the\n> user can clean up the files -- but confusingly, they have to issue the\n> same git-clean command multiple times. If multiple directories have\n> no untracked files, git-clean will never clean them out. I probably\n> didn't do a very good job explaining that although I started with the\n> case with one tracked, that I view the case without any as the more\n> general case -- and that solving it solves both problems. I could\n> probably make that clearer in the commit message. (Or maybe just add\n> more testcases even if slightly duplicative, I guess.)\n\nMy comment/puzzlement indeed was the lack of any tracked file in\nyour tests, even though the log message did talk about one's\npresence making a difference in the outcome.\n\nThanks.\n"},{"id":"382497","messageId":"20190917163504.14566-1-newren@gmail.com","threadId":"51732","inReplyTo":"20190912221240.18057-1-newren@gmail.com","subject":"[PATCH v4 00/12] Fix some git clean issues","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2019-09-17T16:34:52Z","receivedAt":"2019-09-17T16:35:13Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"This patch series fixes a few issues with git-clean:\n * Failure to clean when multiple pathspecs are specified, reported both\n in April 2018[1] and again in May 2019[2].\n * Failure to preserve both tracked and untracked files within a nested\n Git repository reported a few weeks ago by SZEDER[3].\nIt builds on sg/clean-nested-repo-with-ignored.\n\n[1] https://public-inbox.org/git/20180405173446.32372-4-newren@gmail.com/\n[2] https://public-inbox.org/git/20190531183651.10067-1-rafa.almas@gmail.com/\n[3] https://public-inbox.org/git/20190825185918.3909-1-szeder.dev@gmail.com/\n\nChanges since v3:\n * Clarified a couple commit messages highlighted by Junio.\n\nElijah Newren (12):\n t7300: add testcases showing failure to clean specified pathspecs\n dir: fix typo in comment\n dir: fix off-by-one error in match_pathspec_item\n dir: also check directories for matching pathspecs\n dir: make the DO_MATCH_SUBMODULE code reusable for a non-submodule\n case\n dir: if our pathspec might match files under a dir, recurse into it\n dir: add commentary explaining match_pathspec_item's return value\n git-clean.txt: do not claim we will delete files with -n/--dry-run\n clean: disambiguate the definition of -d\n clean: avoid removing untracked files in a nested git repository\n clean: rewrap overly long line\n clean: fix theoretical path corruption\n\n Documentation/git-clean.txt | 16 +++++-----\n builtin/clean.c | 15 +++++++--\n dir.c | 63 +++++++++++++++++++++++++++----------\n dir.h | 8 +++--\n t/t7300-clean.sh | 44 +++++++++++++++++++++++---\n 5 files changed, 112 insertions(+), 34 deletions(-)\n\nRange-diff:\n 1: fe35ab8cc3 ! 1: a48d4e7faf t7300: add testcases showing failure to clean specified pathspecs\n @@ -28,9 +28,15 @@\n showed that the same buggy behavior exists without using that flag, and\n has in fact existed since before cf424f5fd89b.\n \n - Add testcases showing that multiple untracked files within entirely\n - untracked directories cannot be cleaned when specifying these files to\n - git clean via pathspecs.\n + Although these problems at first are perceived to be different (e.g.\n + never clearing out the requested files vs. taking multiple invocations\n + to get everything cleared out), they are actually just different\n + manifestations of the same problem. The case with multiple directories\n + that have no tracked files is the more general case; solving it will\n + solve all the others. So, I concentrate on it. Add testcases showing\n + that multiple untracked files within entirely untracked directories\n + cannot be cleaned when specifying these files to git clean via\n + pathspecs.\n \n Signed-off-by: Elijah Newren \n \n 2: 707d287d79 = 2: eb00b46822 dir: fix typo in comment\n 3: bb316e82b2 ! 3: c0e5b820a9 dir: fix off-by-one error in match_pathspec_item\n @@ -6,11 +6,22 @@\n namelen will be 4, and match[namelen] will be 'b'. The correct location\n of the directory separator is namelen-1.\n \n - The reason the code worked anyway was that the following code immediately\n - checked whether the first matchlen characters matched (which they do) and\n - then bailed and return MATCHED_RECURSIVELY anyway since wildmatch doesn't\n - have the ability to check if \"name\" can be matched as a directory (or\n - prefix) against the pathspec.\n + However, other callers of match_pathspec_item() such as builtin/grep.c's\n + submodule_path_match() will compare against a path named \"foo\" instead of\n + \"foo/\". It might be better to change all the callers to be consistent,\n + as discussed at\n + https://public-inbox.org/git/xmqq7e6cdnkr.fsf@gitster-ct.c.googlers.com/\n + and\n + https://public-inbox.org/git/CABPp-BERWUPCPq-9fVW1LNocqkrfsoF4BPj3gJd9+En43vEkTQ@mail.gmail.com/\n + but there are many cases to audit, so for now just make sure we handle\n + both cases with and without a trailing slash.\n +\n + The reason the code worked despite this sometimes-off-by-one error was\n + that the subsequent code immediately checked whether the first matchlen\n + characters matched (which they do) and then bailed and return\n + MATCHED_RECURSIVELY anyway since wildmatch doesn't have the ability to\n + check if \"name\" can be matched as a directory (or prefix) against the\n + pathspec.\n \n Signed-off-by: Elijah Newren \n \n 4: 56319f934a = 4: 397775ec35 dir: also check directories for matching pathspecs\n 5: 81593a565c = 5: b836de82c0 dir: make the DO_MATCH_SUBMODULE code reusable for a non-submodule case\n 6: 9566823a0f = 6: feb317d090 dir: if our pathspec might match files under a dir, recurse into it\n 7: 7821898ba7 = 7: 0a574d6779 dir: add commentary explaining match_pathspec_item's return value\n 8: 13def5df57 = 8: 0eaa08537c git-clean.txt: do not claim we will delete files with -n/--dry-run\n 9: e6b274abf7 = 9: a1438301bb clean: disambiguate the definition of -d\n10: 5f4ef14765 = 10: 8dc21923ee clean: avoid removing untracked files in a nested git repository\n11: 4e30e62eb1 = 11: 707b6a5509 clean: rewrap overly long line\n12: de2444f7cb = 12: 84a90010ed clean: fix theoretical path corruption\n-- \n2.22.1.17.g6e632477f7.dirty\n\n"},{"id":"382498","messageId":"20190917163504.14566-3-newren@gmail.com","threadId":"51732","inReplyTo":"20190917163504.14566-1-newren@gmail.com","subject":"[PATCH v4 02/12] dir: fix typo in comment","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2019-09-17T16:34:54Z","receivedAt":"2019-09-17T16:35:15Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Signed-off-by: Elijah Newren \n---\n dir.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/dir.c b/dir.c\nindex d021c908e5..a9168bed96 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -139,7 +139,7 @@ static size_t common_prefix_len(const struct pathspec *pathspec)\n \t * \":(icase)path\" is treated as a pathspec full of\n \t * wildcard. In other words, only prefix is considered common\n \t * prefix. If the pathspec is abc/foo abc/bar, running in\n-\t * subdir xyz, the common prefix is still xyz, not xuz/abc as\n+\t * subdir xyz, the common prefix is still xyz, not xyz/abc as\n \t * in non-:(icase).\n \t */\n \tGUARD_PATHSPEC(pathspec,\n-- \n2.22.1.17.g6e632477f7.dirty\n\n"},{"id":"382499","messageId":"20190917163504.14566-2-newren@gmail.com","threadId":"51732","inReplyTo":"20190917163504.14566-1-newren@gmail.com","subject":"[PATCH v4 01/12] t7300: add testcases showing failure to clean specified pathspecs","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2019-09-17T16:34:53Z","receivedAt":"2019-09-17T16:35:15Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Someone brought me a testcase where multiple git-clean invocations were\nrequired to clean out unwanted files:\n mkdir d{1,2}\n touch d{1,2}/ut\n touch d1/t && git add d1/t\nWith this setup, the user would need to run\n git clean -ffd */ut\ntwice to delete both ut files.\n\nA little testing showed some interesting variants:\n * If only one of those two ut files existed (either one), then only one\n clean command would be necessary.\n * If both directories had tracked files, then only one git clean would\n be necessary to clean both files.\n * If both directories had no tracked files then the clean command above\n would never clean either of the untracked files despite the pathspec\n explicitly calling both of them out.\n\nA bisect showed that the failure to clean out the files started with\ncommit cf424f5fd89b (\"clean: respect pathspecs with \"-d\", 2014-03-10).\nHowever, that pointed to a separate issue: while the \"-d\" flag was used\nby the original user who showed me this problem, that flag should have\nbeen irrelevant to this problem. Testing again without the \"-d\" flag\nshowed that the same buggy behavior exists without using that flag, and\nhas in fact existed since before cf424f5fd89b.\n\nAlthough these problems at first are perceived to be different (e.g.\nnever clearing out the requested files vs. taking multiple invocations\nto get everything cleared out), they are actually just different\nmanifestations of the same problem. The case with multiple directories\nthat have no tracked files is the more general case; solving it will\nsolve all the others. So, I concentrate on it. Add testcases showing\nthat multiple untracked files within entirely untracked directories\ncannot be cleaned when specifying these files to git clean via\npathspecs.\n\nSigned-off-by: Elijah Newren \n---\n t/t7300-clean.sh | 32 ++++++++++++++++++++++++++++++++\n 1 file changed, 32 insertions(+)\n\ndiff --git a/t/t7300-clean.sh b/t/t7300-clean.sh\nindex d01fd120ab..2c254c773c 100755\n--- a/t/t7300-clean.sh\n+++ b/t/t7300-clean.sh\n@@ -691,6 +691,38 @@ test_expect_failure 'git clean -d skips nested repo containing ignored files' '\n \ttest_path_is_file nested-repo-with-ignored-file/file\n '\n \n+test_expect_failure 'git clean handles being told what to clean' '\n+\tmkdir -p d1 d2 &&\n+\ttouch d1/ut d2/ut &&\n+\tgit clean -f */ut &&\n+\ttest_path_is_missing d1/ut &&\n+\ttest_path_is_missing d2/ut\n+'\n+\n+test_expect_failure 'git clean handles being told what to clean, with -d' '\n+\tmkdir -p d1 d2 &&\n+\ttouch d1/ut d2/ut &&\n+\tgit clean -ffd */ut &&\n+\ttest_path_is_missing d1/ut &&\n+\ttest_path_is_missing d2/ut\n+'\n+\n+test_expect_failure 'git clean works if a glob is passed without -d' '\n+\tmkdir -p d1 d2 &&\n+\ttouch d1/ut d2/ut &&\n+\tgit clean -f \"*ut\" &&\n+\ttest_path_is_missing d1/ut &&\n+\ttest_path_is_missing d2/ut\n+'\n+\n+test_expect_failure 'git clean works if a glob is passed with -d' '\n+\tmkdir -p d1 d2 &&\n+\ttouch d1/ut d2/ut &&\n+\tgit clean -ffd \"*ut\" &&\n+\ttest_path_is_missing d1/ut &&\n+\ttest_path_is_missing d2/ut\n+'\n+\n test_expect_success MINGW 'handle clean & core.longpaths = false nicely' '\n \ttest_config core.longpaths false &&\n \ta50=aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa &&\n-- \n2.22.1.17.g6e632477f7.dirty\n\n"},{"id":"382500","messageId":"20190917163504.14566-4-newren@gmail.com","threadId":"51732","inReplyTo":"20190917163504.14566-1-newren@gmail.com","subject":"[PATCH v4 03/12] dir: fix off-by-one error in match_pathspec_item","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2019-09-17T16:34:55Z","receivedAt":"2019-09-17T16:35:17Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"For a pathspec like 'foo/bar' comparing against a path named \"foo/\",\nnamelen will be 4, and match[namelen] will be 'b'. The correct location\nof the directory separator is namelen-1.\n\nHowever, other callers of match_pathspec_item() such as builtin/grep.c's\nsubmodule_path_match() will compare against a path named \"foo\" instead of\n\"foo/\". It might be better to change all the callers to be consistent,\nas discussed at\n https://public-inbox.org/git/xmqq7e6cdnkr.fsf@gitster-ct.c.googlers.com/\nand\n https://public-inbox.org/git/CABPp-BERWUPCPq-9fVW1LNocqkrfsoF4BPj3gJd9+En43vEkTQ@mail.gmail.com/\nbut there are many cases to audit, so for now just make sure we handle\nboth cases with and without a trailing slash.\n\nThe reason the code worked despite this sometimes-off-by-one error was\nthat the subsequent code immediately checked whether the first matchlen\ncharacters matched (which they do) and then bailed and return\nMATCHED_RECURSIVELY anyway since wildmatch doesn't have the ability to\ncheck if \"name\" can be matched as a directory (or prefix) against the\npathspec.\n\nSigned-off-by: Elijah Newren \n---\n dir.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/dir.c b/dir.c\nindex a9168bed96..bf1a74799e 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -356,8 +356,9 @@ static int match_pathspec_item(const struct index_state *istate,\n \t/* Perform checks to see if \"name\" is a super set of the pathspec */\n \tif (flags & DO_MATCH_SUBMODULE) {\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-\t\t (match[namelen] == '/') &&\n+\t\t (match[namelen-offset] == '/') &&\n \t\t !ps_strncmp(item, match, name, namelen))\n \t\t\treturn MATCHED_RECURSIVELY;\n \n-- \n2.22.1.17.g6e632477f7.dirty\n\n"},{"id":"382501","messageId":"20190917163504.14566-5-newren@gmail.com","threadId":"51732","inReplyTo":"20190917163504.14566-1-newren@gmail.com","subject":"[PATCH v4 04/12] dir: also check directories for matching pathspecs","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2019-09-17T16:34:56Z","receivedAt":"2019-09-17T16:35:18Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Even if a directory doesn't match a pathspec, it is possible, depending\non the precise pathspecs, that some file underneath it might. So we\nspecial case and recurse into the directory for such situations. However,\nwe previously always added any untracked directory that we recursed into\nto the list of untracked paths, regardless of whether the directory\nitself matched the pathspec.\n\nFor the case of git-clean and a set of pathspecs of \"dir/file\" and \"more\",\nthis caused a problem because we'd end up with dir entries for both of\n \"dir\"\n \"dir/file\"\nThen correct_untracked_entries() would try to helpfully prune duplicates\nfor us by removing \"dir/file\" since it's under \"dir\", leaving us with\n \"dir\"\nSince the original pathspec only had \"dir/file\", the only entry left\ndoesn't match and leaves nothing to be removed. (Note that if only one\npathspec was specified, e.g. only \"dir/file\", then the common_prefix_len\noptimizations in fill_directory would cause us to bypass this problem,\nmaking it appear in simple tests that we could correctly remove manually\nspecified pathspecs.)\n\nFix this by actually checking whether the directory we are about to add\nto the list of dir entries actually matches the pathspec; only do this\nmatching check after we have already returned from recursing into the\ndirectory.\n\nSigned-off-by: Elijah Newren \n---\n dir.c | 5 +++++\n t/t7300-clean.sh | 4 ++--\n 2 files changed, 7 insertions(+), 2 deletions(-)\n\ndiff --git a/dir.c b/dir.c\nindex bf1a74799e..76a3c3894b 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -1951,6 +1951,11 @@ static enum path_treatment read_directory_recursive(struct dir_struct *dir,\n \t\t\t\t\t\t\t check_only, stop_at_first_file, pathspec);\n \t\t\tif (subdir_state > dir_state)\n \t\t\t\tdir_state = subdir_state;\n+\n+\t\t\tif (!match_pathspec(istate, pathspec, path.buf, path.len,\n+\t\t\t\t\t 0 /* prefix */, NULL,\n+\t\t\t\t\t 0 /* do NOT special case dirs */))\n+\t\t\t\tstate = path_none;\n \t\t}\n \n \t\tif (check_only) {\ndiff --git a/t/t7300-clean.sh b/t/t7300-clean.sh\nindex 2c254c773c..12617158db 100755\n--- a/t/t7300-clean.sh\n+++ b/t/t7300-clean.sh\n@@ -699,7 +699,7 @@ test_expect_failure 'git clean handles being told what to clean' '\n \ttest_path_is_missing d2/ut\n '\n \n-test_expect_failure 'git clean handles being told what to clean, with -d' '\n+test_expect_success 'git clean handles being told what to clean, with -d' '\n \tmkdir -p d1 d2 &&\n \ttouch d1/ut d2/ut &&\n \tgit clean -ffd */ut &&\n@@ -715,7 +715,7 @@ test_expect_failure 'git clean works if a glob is passed without -d' '\n \ttest_path_is_missing d2/ut\n '\n \n-test_expect_failure 'git clean works if a glob is passed with -d' '\n+test_expect_success 'git clean works if a glob is passed with -d' '\n \tmkdir -p d1 d2 &&\n \ttouch d1/ut d2/ut &&\n \tgit clean -ffd \"*ut\" &&\n-- \n2.22.1.17.g6e632477f7.dirty\n\n"},{"id":"382502","messageId":"20190917163504.14566-9-newren@gmail.com","threadId":"51732","inReplyTo":"20190917163504.14566-1-newren@gmail.com","subject":"[PATCH v4 08/12] git-clean.txt: do not claim we will delete files with -n/--dry-run","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2019-09-17T16:35:00Z","receivedAt":"2019-09-17T16:35:25Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"It appears that the wrong option got included in the list of what will\ncause git-clean to actually take action. Correct the list.\n\nSigned-off-by: Elijah Newren \n---\n Documentation/git-clean.txt | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/Documentation/git-clean.txt b/Documentation/git-clean.txt\nindex db876f7dde..e84ffc9396 100644\n--- a/Documentation/git-clean.txt\n+++ b/Documentation/git-clean.txt\n@@ -35,7 +35,7 @@ OPTIONS\n --force::\n \tIf the Git configuration variable clean.requireForce is not set\n \tto false, 'git clean' will refuse to delete files or directories\n-\tunless given -f, -n or -i. Git will refuse to delete directories\n+\tunless given -f or -i. Git will refuse to delete directories\n \twith .git sub directory or file unless a second -f\n \tis given.\n \n-- \n2.22.1.17.g6e632477f7.dirty\n\n"},{"id":"382503","messageId":"20190917163504.14566-12-newren@gmail.com","threadId":"51732","inReplyTo":"20190917163504.14566-1-newren@gmail.com","subject":"[PATCH v4 11/12] clean: rewrap overly long line","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2019-09-17T16:35:03Z","receivedAt":"2019-09-17T16:35:28Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Signed-off-by: Elijah Newren \n---\n builtin/clean.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/clean.c b/builtin/clean.c\nindex 3a7a63ae71..6030842f3a 100644\n--- a/builtin/clean.c\n+++ b/builtin/clean.c\n@@ -158,7 +158,8 @@ static int remove_dirs(struct strbuf *path, const char *prefix, int force_flag,\n \n \t*dir_gone = 1;\n \n-\tif ((force_flag & REMOVE_DIR_KEEP_NESTED_GIT) && is_nonbare_repository_dir(path)) {\n+\tif ((force_flag & REMOVE_DIR_KEEP_NESTED_GIT) &&\n+\t is_nonbare_repository_dir(path)) {\n \t\tif (!quiet) {\n \t\t\tquote_path_relative(path->buf, prefix, "ed);\n \t\t\tprintf(dry_run ? _(msg_would_skip_git_dir) : _(msg_skip_git_dir),\n-- \n2.22.1.17.g6e632477f7.dirty\n\n"},{"id":"382504","messageId":"20190917163504.14566-13-newren@gmail.com","threadId":"51732","inReplyTo":"20190917163504.14566-1-newren@gmail.com","subject":"[PATCH v4 12/12] clean: fix theoretical path corruption","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2019-09-17T16:35:04Z","receivedAt":"2019-09-17T16:35:31Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"cmd_clean() had the following code structure:\n\n struct strbuf abs_path = STRBUF_INIT;\n for_each_string_list_item(item, &del_list) {\n strbuf_addstr(&abs_path, prefix);\n strbuf_addstr(&abs_path, item->string);\n PROCESS(&abs_path);\n strbuf_reset(&abs_path);\n }\n\nwhere I've elided a bunch of unnecessary details and PROCESS(&abs_path)\nrepresents a big chunk of code rather than an actual function call. One\npiece of PROCESS was:\n\n if (lstat(abs_path.buf, &st))\n continue;\n\nwhich would cause the strbuf_reset() to be missed -- meaning that the\nnext path to be handled would have two paths concatenated. This path\nused to use die_errno() instead of continue prior to commit 396049e5fb62\n(\"git-clean: refactor git-clean into two phases\", 2013-06-25), but my\nunderstanding of how correct_untracked_entries() works is that it will\nprevent both dir/ and dir/file from being in the list to clean so this\nshould be dead code and the die_errno() should be safe. But I hesitate\nto remove it since I am not certain.\n\nHowever, we can fix both this bug and possible similar future bugs by\nsimply moving the strbuf_reset(&abs_path) to the beginning of the loop.\nIt'll result in N calls to strbuf_reset() instead of N-1, but that's a\nsmall price to pay to avoid sneaky bugs like this.\n\nSigned-off-by: Elijah Newren \n---\n builtin/clean.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/clean.c b/builtin/clean.c\nindex 6030842f3a..4cf2399f59 100644\n--- a/builtin/clean.c\n+++ b/builtin/clean.c\n@@ -1018,6 +1018,7 @@ int cmd_clean(int argc, const char **argv, const char *prefix)\n \tfor_each_string_list_item(item, &del_list) {\n \t\tstruct stat st;\n \n+\t\tstrbuf_reset(&abs_path);\n \t\tif (prefix)\n \t\t\tstrbuf_addstr(&abs_path, prefix);\n \n@@ -1051,7 +1052,6 @@ int cmd_clean(int argc, const char **argv, const char *prefix)\n \t\t\t\tprintf(dry_run ? _(msg_would_remove) : _(msg_remove), qname);\n \t\t\t}\n \t\t}\n-\t\tstrbuf_reset(&abs_path);\n \t}\n \n \tstrbuf_release(&abs_path);\n-- \n2.22.1.17.g6e632477f7.dirty\n\n"},{"id":"382505","messageId":"20190917163504.14566-10-newren@gmail.com","threadId":"51732","inReplyTo":"20190917163504.14566-1-newren@gmail.com","subject":"[PATCH v4 09/12] clean: disambiguate the definition of -d","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2019-09-17T16:35:01Z","receivedAt":"2019-09-17T16:35:32Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"The -d flag pre-dated git-clean's ability to have paths specified. As\nsuch, the default for git-clean was to only remove untracked files in\nthe current directory, and -d existed to allow it to recurse into\nsubdirectories.\n\nThe interaction of paths and the -d option appears to not have been\ncarefully considered, as evidenced by numerous bugs and a dearth of\ntests covering such pairings in the testsuite. The definition turns out\nto be important, so let's look at some of the various ways one could\ninterpret the -d option:\n\n A) Without -d, only look in subdirectories which contain tracked\n files under them; with -d, also look in subdirectories which\n are untracked for files to clean.\n\n B) Without specified paths from the user for us to delete, we need to\n have some kind of default, so...without -d, only look in\n subdirectories which contain tracked files under them; with -d,\n also look in subdirectories which are untracked for files to clean.\n\nThe important distinction here is that choice B says that the presence\nor absence of '-d' is irrelevant if paths are specified. The logic\nbehind option B is that if a user explicitly asked us to clean a\nspecified pathspec, then we should clean anything that matches that\npathspec. Some examples may clarify. Should\n\n git clean -f untracked_dir/file\n\nremove untracked_dir/file or not? It seems crazy not to, but a strict\nreading of option A says it shouldn't be removed. How about\n\n git clean -f untracked_dir/file1 tracked_dir/file2\n\nor\n\n git clean -f untracked_dir_1/file1 untracked_dir_2/file2\n\n? Should it remove either or both of these files? Should it require\nmultiple runs to remove both the files listed? (If this sounds like a\ncrazy question to even ask, see the commit message of \"t7300: Add some\ntestcases showing failure to clean specified pathspecs\" added earlier in\nthis patch series.) What if -ffd were used instead of -f -- should that\nallow these to be removed? Should it take multiple invocations with\n-ffd? What if a glob (such as '*tracked*') were used instead of\nspelling out the directory names? What if the filenames involved globs,\nsuch as\n\n git clean -f '*.o'\n\nor\n\n git clean -f '*/*.o'\n\n?\n\nThe current documentation actually suggests a definition that is\nslightly different than choice A, and the implementation prior to this\nseries provided something radically different than either choices A or\nB. (The implementation, though, was clearly just buggy). There may be\nother choices as well. However, for almost any given choice of\ndefinition for -d that I can think of, some of the examples above will\nappear buggy to the user. The only case that doesn't have negative\nsurprises is choice B: treat a user-specified path as a request to clean\nall untracked files which match that path specification, including\nrecursing into any untracked directories.\n\nChange the documentation and basic implementation to use this\ndefinition.\n\nThere were two regression tests that indirectly depended on the current\nimplementation, but neither was about subdirectory handling. These two\ntests were introduced in commit 5b7570cfb41c (\"git-clean: add tests for\nrelative path\", 2008-03-07) which was solely created to add coverage for\nthe changes in commit fb328947c8e (\"git-clean: correct printing relative\npath\", 2008-03-07). Both tests specified a directory that happened to\nhave an untracked subdirectory, but both were only checking that the\nresulting printout of a file that was removed was shown with a relative\npath. Update these tests appropriately.\n\nSigned-off-by: Elijah Newren \n---\n Documentation/git-clean.txt | 10 ++++++----\n builtin/clean.c | 8 ++++++++\n t/t7300-clean.sh | 2 ++\n 3 files changed, 16 insertions(+), 4 deletions(-)\n\ndiff --git a/Documentation/git-clean.txt b/Documentation/git-clean.txt\nindex e84ffc9396..3ab749b921 100644\n--- a/Documentation/git-clean.txt\n+++ b/Documentation/git-clean.txt\n@@ -26,10 +26,12 @@ are affected.\n OPTIONS\n -------\n -d::\n-\tRemove untracked directories in addition to untracked files.\n-\tIf an untracked directory is managed by a different Git\n-\trepository, it is not removed by default. Use -f option twice\n-\tif you really want to remove such a directory.\n+\tNormally, when no is specified, git clean will not\n+\trecurse into untracked directories to avoid removing too much.\n+\tSpecify -d to have it recurse into such directories as well.\n+\tIf any paths are specified, -d is irrelevant; all untracked\n+\tfiles matching the specified paths (with exceptions for nested\n+\tgit directories mentioned under `--force`) will be removed.\n \n -f::\n --force::\ndiff --git a/builtin/clean.c b/builtin/clean.c\nindex d5579da716..68d70e41c0 100644\n--- a/builtin/clean.c\n+++ b/builtin/clean.c\n@@ -949,6 +949,14 @@ int cmd_clean(int argc, const char **argv, const char *prefix)\n \n \tdir.flags |= DIR_SHOW_OTHER_DIRECTORIES;\n \n+\tif (argc) {\n+\t\t/*\n+\t\t * Remaining args implies pathspecs specified, and we should\n+\t\t * recurse within those.\n+\t\t */\n+\t\tremove_directories = 1;\n+\t}\n+\n \tif (remove_directories)\n \t\tdir.flags |= DIR_SHOW_IGNORED_TOO | DIR_KEEP_UNTRACKED_CONTENTS;\n \ndiff --git a/t/t7300-clean.sh b/t/t7300-clean.sh\nindex d83aeb7dc2..530dfdab34 100755\n--- a/t/t7300-clean.sh\n+++ b/t/t7300-clean.sh\n@@ -117,6 +117,7 @@ test_expect_success C_LOCALE_OUTPUT 'git clean with relative prefix' '\n \twould_clean=$(\n \t\tcd docs &&\n \t\tgit clean -n ../src |\n+\t\tgrep part3 |\n \t\tsed -n -e \"s|^Would remove ||p\"\n \t) &&\n \tverbose test \"$would_clean\" = ../src/part3.c\n@@ -129,6 +130,7 @@ test_expect_success C_LOCALE_OUTPUT 'git clean with absolute path' '\n \twould_clean=$(\n \t\tcd docs &&\n \t\tgit clean -n \"$(pwd)/../src\" |\n+\t\tgrep part3 |\n \t\tsed -n -e \"s|^Would remove ||p\"\n \t) &&\n \tverbose test \"$would_clean\" = ../src/part3.c\n-- \n2.22.1.17.g6e632477f7.dirty\n\n"},{"id":"382506","messageId":"20190917163504.14566-11-newren@gmail.com","threadId":"51732","inReplyTo":"20190917163504.14566-1-newren@gmail.com","subject":"[PATCH v4 10/12] clean: avoid removing untracked files in a nested git repository","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2019-09-17T16:35:02Z","receivedAt":"2019-09-17T16:35:33Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Users expect files in a nested git repository to be left alone unless\nsufficiently forced (with two -f's). Unfortunately, in certain\ncircumstances, git would delete both tracked (and possibly dirty) files\nand untracked files within a nested repository. To explain how this\nhappens, let's contrast a couple cases. First, take the following\nexample setup (which assumes we are already within a git repo):\n\n git init nested\n cd nested\n >tracked\n git add tracked\n git commit -m init\n >untracked\n cd ..\n\nIn this setup, everything works as expected; running 'git clean -fd'\nwill result in fill_directory() returning the following paths:\n nested/\n nested/tracked\n nested/untracked\nand then correct_untracked_entries() would notice this can be compressed\nto\n nested/\nand then since \"nested/\" is a directory, we would call\nremove_dirs(\"nested/\", ...), which would\ncheck is_nonbare_repository_dir() and then decide to skip it.\n\nHowever, if someone also creates an ignored file:\n >nested/ignored\nthen running 'git clean -fd' would result in fill_directory() returning\nthe same paths:\n nested/\n nested/tracked\n nested/untracked\nbut correct_untracked_entries() will notice that we had ignored entries\nunder nested/ and thus simplify this list to\n nested/tracked\n nested/untracked\nSince these are not directories, we do not call remove_dirs() which was\nthe only place that had the is_nonbare_repository_dir() safety check --\nresulting in us deleting both the untracked file and the tracked (and\npossibly dirty) file.\n\nOne possible fix for this issue would be walking the parent directories\nof each path and checking if they represent nonbare repositories, but\nthat would be wasteful. Even if we added caching of some sort, it's\nstill a waste because we should have been able to check that \"nested/\"\nrepresented a nonbare repository before even descending into it in the\nfirst place. Add a DIR_SKIP_NESTED_GIT flag to dir_struct.flags and use\nit to prevent fill_directory() and friends from descending into nested\ngit repos.\n\nWith this change, we also modify two regression tests added in commit\n91479b9c72f1 (\"t7300: add tests to document behavior of clean and nested\ngit\", 2015-06-15). That commit, nor its series, nor the six previous\niterations of that series on the mailing list discussed why those tests\ncoded the expectation they did. In fact, it appears their purpose was\nsimply to test _existing_ behavior to make sure that the performance\nchanges didn't change the behavior. However, these two tests directly\ncontradicted the manpage's claims that two -f's were required to delete\nfiles/directories under a nested git repository. While one could argue\nthat the user gave an explicit path which matched files/directories that\nwere within a nested repository, there's a slippery slope that becomes\nvery difficult for users to understand once you go down that route (e.g.\nwhat if they specified \"git clean -f -d '*.c'\"?) It would also be hard\nto explain what the exact behavior was; avoid such problems by making it\nreally simple.\n\nAlso, clean up some grammar errors describing this functionality in the\ngit-clean manpage.\n\nFinally, there are still a couple bugs with -ffd not cleaning out enough\n(e.g. missing the nested .git) and with -ffdX possibly cleaning out the\nwrong files (paying attention to outer .gitignore instead of inner).\nThis patch does not address these cases at all (and does not change the\nbehavior relative to those flags), it only fixes the handling when given\na single -f. See\nhttps://public-inbox.org/git/20190905212043.GC32087@szeder.dev/ for more\ndiscussion of the -ffd[X?] bugs.\n\nSigned-off-by: Elijah Newren \n---\n Documentation/git-clean.txt | 6 +++---\n builtin/clean.c | 2 ++\n dir.c | 10 ++++++++++\n dir.h | 3 ++-\n t/t7300-clean.sh | 10 +++++-----\n 5 files changed, 22 insertions(+), 9 deletions(-)\n\ndiff --git a/Documentation/git-clean.txt b/Documentation/git-clean.txt\nindex 3ab749b921..ba31d8d166 100644\n--- a/Documentation/git-clean.txt\n+++ b/Documentation/git-clean.txt\n@@ -37,9 +37,9 @@ OPTIONS\n --force::\n \tIf the Git configuration variable clean.requireForce is not set\n \tto false, 'git clean' will refuse to delete files or directories\n-\tunless given -f or -i. Git will refuse to delete directories\n-\twith .git sub directory or file unless a second -f\n-\tis given.\n+\tunless given -f or -i. Git will refuse to modify untracked\n+\tnested git repositories (directories with a .git subdirectory)\n+\tunless a second -f is given.\n \n -i::\n --interactive::\ndiff --git a/builtin/clean.c b/builtin/clean.c\nindex 68d70e41c0..3a7a63ae71 100644\n--- a/builtin/clean.c\n+++ b/builtin/clean.c\n@@ -946,6 +946,8 @@ int cmd_clean(int argc, const char **argv, const char *prefix)\n \n \tif (force > 1)\n \t\trm_flags = 0;\n+\telse\n+\t\tdir.flags |= DIR_SKIP_NESTED_GIT;\n \n \tdir.flags |= DIR_SHOW_OTHER_DIRECTORIES;\n \ndiff --git a/dir.c b/dir.c\nindex 3b2fe1701c..7ff79170fc 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -1451,6 +1451,16 @@ static enum path_treatment treat_directory(struct dir_struct *dir,\n \t\treturn path_none;\n \n \tcase index_nonexistent:\n+\t\tif (dir->flags & DIR_SKIP_NESTED_GIT) {\n+\t\t\tint nested_repo;\n+\t\t\tstruct strbuf sb = STRBUF_INIT;\n+\t\t\tstrbuf_addstr(&sb, dirname);\n+\t\t\tnested_repo = is_nonbare_repository_dir(&sb);\n+\t\t\tstrbuf_release(&sb);\n+\t\t\tif (nested_repo)\n+\t\t\t\treturn path_none;\n+\t\t}\n+\n \t\tif (dir->flags & DIR_SHOW_OTHER_DIRECTORIES)\n \t\t\tbreak;\n \t\tif (exclude &&\ndiff --git a/dir.h b/dir.h\nindex 46c238ab49..739aea7c96 100644\n--- a/dir.h\n+++ b/dir.h\n@@ -156,7 +156,8 @@ struct dir_struct {\n \t\tDIR_SHOW_IGNORED_TOO = 1<<5,\n \t\tDIR_COLLECT_KILLED_ONLY = 1<<6,\n \t\tDIR_KEEP_UNTRACKED_CONTENTS = 1<<7,\n-\t\tDIR_SHOW_IGNORED_TOO_MODE_MATCHING = 1<<8\n+\t\tDIR_SHOW_IGNORED_TOO_MODE_MATCHING = 1<<8,\n+\t\tDIR_SKIP_NESTED_GIT = 1<<9\n \t} flags;\n \tstruct dir_entry **entries;\n \tstruct dir_entry **ignored;\ndiff --git a/t/t7300-clean.sh b/t/t7300-clean.sh\nindex 530dfdab34..6e6d24c1c3 100755\n--- a/t/t7300-clean.sh\n+++ b/t/t7300-clean.sh\n@@ -549,7 +549,7 @@ test_expect_failure 'nested (non-empty) bare repositories should be cleaned even\n \ttest_path_is_missing strange_bare\n '\n \n-test_expect_success 'giving path in nested git work tree will remove it' '\n+test_expect_success 'giving path in nested git work tree will NOT remove it' '\n \trm -fr repo &&\n \tmkdir repo &&\n \t(\n@@ -561,7 +561,7 @@ test_expect_success 'giving path in nested git work tree will remove it' '\n \tgit clean -f -d repo/bar/baz &&\n \ttest_path_is_file repo/.git/HEAD &&\n \ttest_path_is_dir repo/bar/ &&\n-\ttest_path_is_missing repo/bar/baz\n+\ttest_path_is_file repo/bar/baz/hello.world\n '\n \n test_expect_success 'giving path to nested .git will not remove it' '\n@@ -579,7 +579,7 @@ test_expect_success 'giving path to nested .git will not remove it' '\n \ttest_path_is_dir untracked/\n '\n \n-test_expect_success 'giving path to nested .git/ will remove contents' '\n+test_expect_success 'giving path to nested .git/ will NOT remove contents' '\n \trm -fr repo untracked &&\n \tmkdir repo untracked &&\n \t(\n@@ -589,7 +589,7 @@ test_expect_success 'giving path to nested .git/ will remove contents' '\n \t) &&\n \tgit clean -f -d repo/.git/ &&\n \ttest_path_is_dir repo/.git &&\n-\ttest_dir_is_empty repo/.git &&\n+\ttest_path_is_file repo/.git/HEAD &&\n \ttest_path_is_dir untracked/\n '\n \n@@ -671,7 +671,7 @@ test_expect_success 'git clean -d skips untracked dirs containing ignored files'\n \ttest_path_is_missing foo/b/bb\n '\n \n-test_expect_failure 'git clean -d skips nested repo containing ignored files' '\n+test_expect_success 'git clean -d skips nested repo containing ignored files' '\n \ttest_when_finished \"rm -rf nested-repo-with-ignored-file\" &&\n \n \tgit init nested-repo-with-ignored-file &&\n-- \n2.22.1.17.g6e632477f7.dirty\n\n"},{"id":"382507","messageId":"20190917163504.14566-8-newren@gmail.com","threadId":"51732","inReplyTo":"20190917163504.14566-1-newren@gmail.com","subject":"[PATCH v4 07/12] dir: add commentary explaining match_pathspec_item's return value","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2019-09-17T16:34:59Z","receivedAt":"2019-09-17T16:35:40Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"The way match_pathspec_item() handles names and pathspecs with trailing\nslash characters, in conjunction with special options like\nDO_MATCH_DIRECTORY and DO_MATCH_LEADING_PATHSPEC were non-obvious, and\nbroken until this patch series. Add a table in a comment explaining the\nintent of how these work.\n\nSigned-off-by: Elijah Newren \n---\n dir.c | 27 +++++++++++++++++++--------\n 1 file changed, 19 insertions(+), 8 deletions(-)\n\ndiff --git a/dir.c b/dir.c\nindex 47c0a99cb5..3b2fe1701c 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -276,16 +276,27 @@ static int do_read_blob(const struct object_id *oid, struct oid_stat *oid_stat,\n #define DO_MATCH_LEADING_PATHSPEC (1<<2)\n \n /*\n- * Does 'match' match the given name?\n- * A match is found if\n+ * Does the given pathspec match the given name? A match is found if\n *\n- * (1) the 'match' string is leading directory of 'name', or\n- * (2) the 'match' string is a wildcard and matches 'name', or\n- * (3) the 'match' string is exactly the same as 'name'.\n+ * (1) the pathspec string is leading directory of 'name' (\"RECURSIVELY\"), or\n+ * (2) the pathspec string has a leading part matching 'name' (\"LEADING\"), or\n+ * (3) the pathspec string is a wildcard and matches 'name' (\"WILDCARD\"), or\n+ * (4) the pathspec string is exactly the same as 'name' (\"EXACT\").\n *\n- * and the return value tells which case it was.\n+ * Return value tells which case it was (1-4), or 0 when there is no match.\n *\n- * It returns 0 when there is no match.\n+ * It may be instructive to look at a small table of concrete examples\n+ * to understand the differences between 1, 2, and 4:\n+ *\n+ * Pathspecs\n+ * | a/b | a/b/ | a/b/c\n+ * ------+-----------+-----------+------------\n+ * a/b | EXACT | EXACT[1] | LEADING[2]\n+ * Names a/b/ | RECURSIVE | EXACT | LEADING[2]\n+ * a/b/c | RECURSIVE | RECURSIVE | EXACT\n+ *\n+ * [1] Only if DO_MATCH_DIRECTORY is passed; otherwise, this is NOT a match.\n+ * [2] Only if DO_MATCH_LEADING_PATHSPEC is passed; otherwise, not a match.\n */\n static int match_pathspec_item(const struct index_state *istate,\n \t\t\t const struct pathspec_item *item, int prefix,\n@@ -353,7 +364,7 @@ static int match_pathspec_item(const struct index_state *istate,\n \t\t\t item->nowildcard_len - prefix))\n \t\treturn MATCHED_FNMATCH;\n \n-\t/* Perform checks to see if \"name\" is a super set of the pathspec */\n+\t/* Perform checks to see if \"name\" is a leading string of the pathspec */\n \tif (flags & DO_MATCH_LEADING_PATHSPEC) {\n \t\t/* name is a literal prefix of the pathspec */\n \t\tint offset = name[namelen-1] == '/' ? 1 : 0;\n-- \n2.22.1.17.g6e632477f7.dirty\n\n"},{"id":"382508","messageId":"20190917163504.14566-7-newren@gmail.com","threadId":"51732","inReplyTo":"20190917163504.14566-1-newren@gmail.com","subject":"[PATCH v4 06/12] dir: if our pathspec might match files under a dir, recurse into it","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2019-09-17T16:34:58Z","receivedAt":"2019-09-17T16:35:43Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"For git clean, if a directory is entirely untracked and the user did not\nspecify -d (corresponding to DIR_SHOW_IGNORED_TOO), then we usually do\nnot want to remove that directory and thus do not recurse into it.\nHowever, if the user manually specified specific (or even globbed) paths\nsomewhere under that directory to remove, then we need to recurse into\nthe directory to make sure we remove the relevant paths under that\ndirectory as the user requested.\n\nNote that this does not mean that the recursed-into directory will be\nadded to dir->entries for later removal; as of a few commits earlier in\nthis series, there is another more strict match check that is run after\nreturning from a recursed-into directory before deciding to add it to the\nlist of entries. Therefore, this will only result in files underneath\nthe given directory which match one of the pathspecs being added to the\nentries list.\n\nTwo notes of potential interest to future readers:\n\n * If we wanted to only recurse into a directory when it is specifically\n matched rather than matched-via-glob (e.g. '*.c'), then we could do\n so via making the final non-zero return in match_pathspec_item be\n MATCHED_RECURSIVELY instead of MATCHED_RECURSIVELY_LEADING_PATHSPEC.\n (Note that the relative order of MATCHED_RECURSIVELY_LEADING_PATHSPEC\n and MATCHED_RECURSIVELY are important for such a change.) I was\n leaving open that possibility while writing an RFC asking for the\n behavior we want, but even though we don't want it, that knowledge\n might help you understand the code flow better.\n\n * There is a growing amount of logic in read_directory_recursive() for\n deciding whether to recurse into a subdirectory. However, there is a\n comment immediately preceding this logic that says to recurse if\n instructed by treat_path(). It may be better for the logic in\n read_directory_recursive() to ultimately be moved to treat_path() (or\n another function it calls, such as treat_directory()), but I have\n left that for someone else to tackle in the future.\n\nSigned-off-by: Elijah Newren \n---\n dir.c | 10 ++++++----\n dir.h | 5 +++--\n t/t7300-clean.sh | 4 ++--\n 3 files changed, 11 insertions(+), 8 deletions(-)\n\ndiff --git a/dir.c b/dir.c\nindex b4d656192e..47c0a99cb5 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -360,7 +360,7 @@ static int match_pathspec_item(const struct index_state *istate,\n \t\tif ((namelen < matchlen) &&\n \t\t (match[namelen-offset] == '/') &&\n \t\t !ps_strncmp(item, match, name, namelen))\n-\t\t\treturn MATCHED_RECURSIVELY;\n+\t\t\treturn MATCHED_RECURSIVELY_LEADING_PATHSPEC;\n \n \t\t/* name\" doesn't match up to the first wild character */\n \t\tif (item->nowildcard_len < item->len &&\n@@ -377,7 +377,7 @@ static int match_pathspec_item(const struct index_state *istate,\n \t\t * The submodules themselves will be able to perform more\n \t\t * accurate matching to determine if the pathspec matches.\n \t\t */\n-\t\treturn MATCHED_RECURSIVELY;\n+\t\treturn MATCHED_RECURSIVELY_LEADING_PATHSPEC;\n \t}\n \n \treturn 0;\n@@ -1939,8 +1939,10 @@ static enum path_treatment read_directory_recursive(struct dir_struct *dir,\n \t\t/* recurse into subdir if instructed by treat_path */\n \t\tif ((state == path_recurse) ||\n \t\t\t((state == path_untracked) &&\n-\t\t\t (dir->flags & DIR_SHOW_IGNORED_TOO) &&\n-\t\t\t (get_dtype(cdir.de, istate, path.buf, path.len) == DT_DIR))) {\n+\t\t\t (get_dtype(cdir.de, istate, path.buf, path.len) == DT_DIR) &&\n+\t\t\t ((dir->flags & DIR_SHOW_IGNORED_TOO) ||\n+\t\t\t do_match_pathspec(istate, pathspec, path.buf, path.len,\n+\t\t\t\t\t baselen, NULL, DO_MATCH_LEADING_PATHSPEC) == MATCHED_RECURSIVELY_LEADING_PATHSPEC))) {\n \t\t\tstruct untracked_cache_dir *ud;\n \t\t\tud = lookup_untracked(dir->untracked, untracked,\n \t\t\t\t\t path.buf + baselen,\ndiff --git a/dir.h b/dir.h\nindex 680079bbe3..46c238ab49 100644\n--- a/dir.h\n+++ b/dir.h\n@@ -211,8 +211,9 @@ int count_slashes(const char *s);\n * when populating the seen[] array.\n */\n #define MATCHED_RECURSIVELY 1\n-#define MATCHED_FNMATCH 2\n-#define MATCHED_EXACTLY 3\n+#define MATCHED_RECURSIVELY_LEADING_PATHSPEC 2\n+#define MATCHED_FNMATCH 3\n+#define MATCHED_EXACTLY 4\n int simple_length(const char *match);\n int no_wildcard(const char *string);\n char *common_prefix(const struct pathspec *pathspec);\ndiff --git a/t/t7300-clean.sh b/t/t7300-clean.sh\nindex 12617158db..d83aeb7dc2 100755\n--- a/t/t7300-clean.sh\n+++ b/t/t7300-clean.sh\n@@ -691,7 +691,7 @@ test_expect_failure 'git clean -d skips nested repo containing ignored files' '\n \ttest_path_is_file nested-repo-with-ignored-file/file\n '\n \n-test_expect_failure 'git clean handles being told what to clean' '\n+test_expect_success 'git clean handles being told what to clean' '\n \tmkdir -p d1 d2 &&\n \ttouch d1/ut d2/ut &&\n \tgit clean -f */ut &&\n@@ -707,7 +707,7 @@ test_expect_success 'git clean handles being told what to clean, with -d' '\n \ttest_path_is_missing d2/ut\n '\n \n-test_expect_failure 'git clean works if a glob is passed without -d' '\n+test_expect_success 'git clean works if a glob is passed without -d' '\n \tmkdir -p d1 d2 &&\n \ttouch d1/ut d2/ut &&\n \tgit clean -f \"*ut\" &&\n-- \n2.22.1.17.g6e632477f7.dirty\n\n"},{"id":"382509","messageId":"20190917163504.14566-6-newren@gmail.com","threadId":"51732","inReplyTo":"20190917163504.14566-1-newren@gmail.com","subject":"[PATCH v4 05/12] dir: make the DO_MATCH_SUBMODULE code reusable for a non-submodule case","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2019-09-17T16:34:57Z","receivedAt":"2019-09-17T16:35:52Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"The specific checks done in match_pathspec_item for the DO_MATCH_SUBMODULE\ncase are useful for other cases which have nothing to do with submodules.\nRename this constant; a subsequent commit will make use of this change.\n\nSigned-off-by: Elijah Newren \n---\n dir.c | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/dir.c b/dir.c\nindex 76a3c3894b..b4d656192e 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -273,7 +273,7 @@ static int do_read_blob(const struct object_id *oid, struct oid_stat *oid_stat,\n \n #define DO_MATCH_EXCLUDE (1<<0)\n #define DO_MATCH_DIRECTORY (1<<1)\n-#define DO_MATCH_SUBMODULE (1<<2)\n+#define DO_MATCH_LEADING_PATHSPEC (1<<2)\n \n /*\n * Does 'match' match the given name?\n@@ -354,7 +354,7 @@ 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 super set of the pathspec */\n-\tif (flags & DO_MATCH_SUBMODULE) {\n+\tif (flags & DO_MATCH_LEADING_PATHSPEC) {\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@@ -498,7 +498,7 @@ int submodule_path_match(const struct index_state *istate,\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_SUBMODULE);\n+\t\t\t\t\tDO_MATCH_LEADING_PATHSPEC);\n \treturn matched;\n }\n \n-- \n2.22.1.17.g6e632477f7.dirty\n\n"},{"id":"382944","messageId":"20190925203919.GA89135@generichostname","threadId":"51732","inReplyTo":"20190917163504.14566-5-newren@gmail.com","subject":"[BUG] git is segfaulting, was [PATCH v4 04/12] dir: also check directories for matching pathspecs","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-09-25T20:39:19Z","receivedAt":"2019-09-25T20:39:26Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"Hi Elijah,\n\nI ran into a segfault on MacOS. I managed to bisect it down to\n404ebceda0 (dir: also check directories for matching pathspecs,\n2019-09-17), which should be the patch in the parent thread. The test\ncase below works fine without this patch applied but segfaults once it\nis applied.\n\n\t#!/bin/sh\n\n\tgit worktree add testdir\n\tgit -C testdir checkout master\n\tgit -C testdir fetch https://github.com/git/git.git todo\n\tbin-wrappers/git -C testdir checkout FETCH_HEAD # segfault here\n\nNote that the worktree part isn't necessary to reproduce the problem but\nI didn't want my files to be constantly refreshed, triggering a rebuild\neach time.\n\nI also managed to get this backtrace from running lldb at the segfault\nbut it is based on the latest \"jch\" commit, 1cc52d20df (Merge branch\n'jt/merge-recursive-symlink-is-not-a-dir-in-way' into jch, 2019-09-20).\n\n\t* thread #1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x8)\n\t * frame #0: 0x00000001000f63a0 git`do_match_pathspec(istate=0x0000000100299940, ps=0x000000010200aa80, name=\"Gitweb/static/js/lib/\", namelen=21, prefix=0, seen=0x0000000000000000, flags=0) at dir.c:420:2 [opt]\n\t\tframe #1: 0x00000001000f632c git`match_pathspec(istate=0x0000000100299940, ps=0x0000000000000000, name=\"Gitweb/static/js/lib/\", namelen=21, prefix=0, seen=0x0000000000000000, is_dir=0) at dir.c:490:13 [opt]\n\t\tframe #2: 0x00000001000f8315 git`read_directory_recursive(dir=0x00007ffeefbfe278, istate=0x0000000100299940, base=, baselen=17, untracked=, check_only=0, stop_at_first_file=0, pathspec=0x0000000000000000) at dir.c:1990:9 [opt]\n\t\tframe #3: 0x00000001000f82e9 git`read_directory_recursive(dir=0x00007ffeefbfe278, istate=0x0000000100299940, base=, baselen=14, untracked=, check_only=0, stop_at_first_file=0, pathspec=0x0000000000000000) at dir.c:1984:5 [opt]\n\t\tframe #4: 0x00000001000f82e9 git`read_directory_recursive(dir=0x00007ffeefbfe278, istate=0x0000000100299940, base=, baselen=7, untracked=, check_only=0, stop_at_first_file=0, pathspec=0x0000000000000000) at dir.c:1984:5 [opt]\n\t\tframe #5: 0x00000001000f60d1 git`read_directory(dir=0x00007ffeefbfe278, istate=0x0000000100299940, path=\"Gitweb/\", len=7, pathspec=0x0000000000000000) at dir.c:2298:3 [opt]\n\t\tframe #6: 0x00000001001bded1 git`verify_clean_subdirectory(ce=, o=0x00007ffeefbfe8c0) at unpack-trees.c:1846:6 [opt]\n\t\tframe #7: 0x00000001001bdc1d git`check_ok_to_remove(name=\"Gitweb\", len=6, dtype=4, ce=0x0000000103e70de0, st=0x00007ffeefbfe438, error_type=ERROR_WOULD_LOSE_UNTRACKED_OVERWRITTEN, o=0x00007ffeefbfe8c0) at unpack-trees.c:1901:7 [opt]\n\t\tframe #8: 0x00000001001bdb01 git`verify_absent_1(ce=, error_type=, o=) at unpack-trees.c:1964:10 [opt]\n\t\tframe #9: 0x00000001001bafc0 git`verify_absent(ce=, error_type=, o=) at unpack-trees.c:1052:11 [opt] [artificial]\n\t\tframe #10: 0x00000001001bbc3c git`merged_entry(ce=0x0000000100605fb0, old=0x0000000000000000, o=0x00007ffeefbfe8c0) at unpack-trees.c:2013:7 [opt]\n\t\tframe #11: 0x00000001001bd2b7 git`call_unpack_fn(src=, o=) at unpack-trees.c:522:12 [opt]\n\t\tframe #12: 0x00000001001bca16 git`unpack_nondirectories(n=2, mask=2, dirmask=, src=0x00007ffeefbfe5d0, names=, info=0x00007ffeefbfe718) at unpack-trees.c:1029:12 [opt]\n\t\tframe #13: 0x00000001001bad1a git`unpack_callback(n=2, mask=2, dirmask=0, names=0x0000000102007390, info=0x00007ffeefbfe718) at unpack-trees.c:1229:6 [opt]\n\t\tframe #14: 0x00000001001b8be2 git`traverse_trees(istate=0x0000000100299940, n=2, t=, info=) at tree-walk.c:497:17 [opt]\n\t\tframe #15: 0x00000001001ba80f git`unpack_trees(len=2, t=0x00007ffeefbfebe0, o=0x00007ffeefbfe8c0) at unpack-trees.c:1546:9 [opt]\n\t\tframe #16: 0x000000010001a443 git`merge_working_tree(opts=0x00007ffeefbfee38, old_branch_info=0x00007ffeefbfeca0, new_branch_info=0x00007ffeefbfeda0, writeout_error=0x00007ffeefbfeccc) at checkout.c:704:9 [opt]\n\t\tframe #17: 0x000000010001a08c git`switch_branches(opts=0x00007ffeefbfee38, new_branch_info=0x00007ffeefbfeda0) at checkout.c:1057:9 [opt]\n\t\tframe #18: 0x0000000100018df0 git`checkout_branch(opts=, new_branch_info=) at checkout.c:1426:9 [opt]\n\t\tframe #19: 0x0000000100017b90 git`checkout_main(argc=0, argv=0x00007ffeefbff570, prefix=0x0000000000000000, opts=0x00007ffeefbfee38, options=, usagestr=) at checkout.c:1682:10 [opt]\n\t\tframe #20: 0x0000000100016f2d git`cmd_checkout(argc=2, argv=0x00007ffeefbff568, prefix=0x0000000000000000) at checkout.c:1731:8 [opt]\n\t\tframe #21: 0x00000001000026f6 git`run_builtin(p=0x000000010024c710, argc=2, argv=0x00007ffeefbff568) at git.c:444:11 [opt]\n\t\tframe #22: 0x0000000100001a36 git`handle_builtin(argc=2, argv=0x00007ffeefbff568) at git.c:673:3 [opt]\n\t\tframe #23: 0x000000010000235c git`run_argv(argcp=0x00007ffeefbff4ec, argv=0x00007ffeefbff4d8) at git.c:740:4 [opt]\n\t\tframe #24: 0x0000000100001794 git`cmd_main(argc=2, argv=0x00007ffeefbff568) at git.c:871:19 [opt]\n\t\tframe #25: 0x00000001000a4405 git`main(argc=, argv=0x00007ffeefbff560) at common-main.c:52:11 [opt]\n\t\tframe #26: 0x00007fff783053d5 libdyld.dylib`start + 1\n\nSorry for the information dump, I haven't had the time to properly look\ninto the issue but I just wanted to make sure that you're aware.\n\nThanks and hope this helps,\n\nDenton\n"},{"id":"382946","messageId":"CABPp-BHyjVSD6_S_dYgPiXM=GVEpT97FqJJearzg4hvKHXfNVA@mail.gmail.com","threadId":"51732","inReplyTo":"20190925203919.GA89135@generichostname","subject":"Re: [BUG] git is segfaulting, was [PATCH v4 04/12] dir: also check directories for matching pathspecs","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2019-09-25T21:28:15Z","receivedAt":"2019-09-25T21:28:31Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Hi Denton,\n\nOn Wed, Sep 25, 2019 at 1:39 PM Denton Liu wrote:\n>\n> Hi Elijah,\n>\n> I ran into a segfault on MacOS. I managed to bisect it down to\n> 404ebceda0 (dir: also check directories for matching pathspecs,\n> 2019-09-17), which should be the patch in the parent thread. The test\n> case below works fine without this patch applied but segfaults once it\n> is applied.\n>\n> #!/bin/sh\n>\n> git worktree add testdir\n> git -C testdir checkout master\n> git -C testdir fetch https://github.com/git/git.git todo\n> bin-wrappers/git -C testdir checkout FETCH_HEAD # segfault here\n>\n> Note that the worktree part isn't necessary to reproduce the problem but\n> I didn't want my files to be constantly refreshed, triggering a rebuild\n> each time.\n>\n> I also managed to get this backtrace from running lldb at the segfault\n> but it is based on the latest \"jch\" commit, 1cc52d20df (Merge branch\n> 'jt/merge-recursive-symlink-is-not-a-dir-in-way' into jch, 2019-09-20).\n>\n> * thread #1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x8)\n> * frame #0: 0x00000001000f63a0 git`do_match_pathspec(istate=0x0000000100299940, ps=0x000000010200aa80, name=\"Gitweb/static/js/lib/\", namelen=21, prefix=0, seen=0x0000000000000000, flags=0) at dir.c:420:2 [opt]\n> frame #1: 0x00000001000f632c git`match_pathspec(istate=0x0000000100299940, ps=0x0000000000000000, name=\"Gitweb/static/js/lib/\", namelen=21, prefix=0, seen=0x0000000000000000, is_dir=0) at dir.c:490:13 [opt]\n> frame #2: 0x00000001000f8315 git`read_directory_recursive(dir=0x00007ffeefbfe278, istate=0x0000000100299940, base=, baselen=17, untracked=, check_only=0, stop_at_first_file=0, pathspec=0x0000000000000000) at dir.c:1990:9 [opt]\n> frame #3: 0x00000001000f82e9 git`read_directory_recursive(dir=0x00007ffeefbfe278, istate=0x0000000100299940, base=, baselen=14, untracked=, check_only=0, stop_at_first_file=0, pathspec=0x0000000000000000) at dir.c:1984:5 [opt]\n> frame #4: 0x00000001000f82e9 git`read_directory_recursive(dir=0x00007ffeefbfe278, istate=0x0000000100299940, base=, baselen=7, untracked=, check_only=0, stop_at_first_file=0, pathspec=0x0000000000000000) at dir.c:1984:5 [opt]\n> frame #5: 0x00000001000f60d1 git`read_directory(dir=0x00007ffeefbfe278, istate=0x0000000100299940, path=\"Gitweb/\", len=7, pathspec=0x0000000000000000) at dir.c:2298:3 [opt]\n> frame #6: 0x00000001001bded1 git`verify_clean_subdirectory(ce=, o=0x00007ffeefbfe8c0) at unpack-trees.c:1846:6 [opt]\n> frame #7: 0x00000001001bdc1d git`check_ok_to_remove(name=\"Gitweb\", len=6, dtype=4, ce=0x0000000103e70de0, st=0x00007ffeefbfe438, error_type=ERROR_WOULD_LOSE_UNTRACKED_OVERWRITTEN, o=0x00007ffeefbfe8c0) at unpack-trees.c:1901:7 [opt]\n> frame #8: 0x00000001001bdb01 git`verify_absent_1(ce=, error_type=, o=) at unpack-trees.c:1964:10 [opt]\n> frame #9: 0x00000001001bafc0 git`verify_absent(ce=, error_type=, o=) at unpack-trees.c:1052:11 [opt] [artificial]\n> frame #10: 0x00000001001bbc3c git`merged_entry(ce=0x0000000100605fb0, old=0x0000000000000000, o=0x00007ffeefbfe8c0) at unpack-trees.c:2013:7 [opt]\n> frame #11: 0x00000001001bd2b7 git`call_unpack_fn(src=, o=) at unpack-trees.c:522:12 [opt]\n> frame #12: 0x00000001001bca16 git`unpack_nondirectories(n=2, mask=2, dirmask=, src=0x00007ffeefbfe5d0, names=, info=0x00007ffeefbfe718) at unpack-trees.c:1029:12 [opt]\n> frame #13: 0x00000001001bad1a git`unpack_callback(n=2, mask=2, dirmask=0, names=0x0000000102007390, info=0x00007ffeefbfe718) at unpack-trees.c:1229:6 [opt]\n> frame #14: 0x00000001001b8be2 git`traverse_trees(istate=0x0000000100299940, n=2, t=, info=) at tree-walk.c:497:17 [opt]\n> frame #15: 0x00000001001ba80f git`unpack_trees(len=2, t=0x00007ffeefbfebe0, o=0x00007ffeefbfe8c0) at unpack-trees.c:1546:9 [opt]\n> frame #16: 0x000000010001a443 git`merge_working_tree(opts=0x00007ffeefbfee38, old_branch_info=0x00007ffeefbfeca0, new_branch_info=0x00007ffeefbfeda0, writeout_error=0x00007ffeefbfeccc) at checkout.c:704:9 [opt]\n> frame #17: 0x000000010001a08c git`switch_branches(opts=0x00007ffeefbfee38, new_branch_info=0x00007ffeefbfeda0) at checkout.c:1057:9 [opt]\n> frame #18: 0x0000000100018df0 git`checkout_branch(opts=, new_branch_info=) at checkout.c:1426:9 [opt]\n> frame #19: 0x0000000100017b90 git`checkout_main(argc=0, argv=0x00007ffeefbff570, prefix=0x0000000000000000, opts=0x00007ffeefbfee38, options=, usagestr=) at checkout.c:1682:10 [opt]\n> frame #20: 0x0000000100016f2d git`cmd_checkout(argc=2, argv=0x00007ffeefbff568, prefix=0x0000000000000000) at checkout.c:1731:8 [opt]\n> frame #21: 0x00000001000026f6 git`run_builtin(p=0x000000010024c710, argc=2, argv=0x00007ffeefbff568) at git.c:444:11 [opt]\n> frame #22: 0x0000000100001a36 git`handle_builtin(argc=2, argv=0x00007ffeefbff568) at git.c:673:3 [opt]\n> frame #23: 0x000000010000235c git`run_argv(argcp=0x00007ffeefbff4ec, argv=0x00007ffeefbff4d8) at git.c:740:4 [opt]\n> frame #24: 0x0000000100001794 git`cmd_main(argc=2, argv=0x00007ffeefbff568) at git.c:871:19 [opt]\n> frame #25: 0x00000001000a4405 git`main(argc=, argv=0x00007ffeefbff560) at common-main.c:52:11 [opt]\n> frame #26: 0x00007fff783053d5 libdyld.dylib`start + 1\n>\n> Sorry for the information dump, I haven't had the time to properly look\n> into the issue but I just wanted to make sure that you're aware.\n\nThanks for testing and sending the heads up. Unfortunately, I cannot\nreproduce on either Linux or Mac. Do you have some special ignore\nfiles or sparse-checkout paths that are important to triggering?\nWhat's in your config.mak? What compiler and version?\n\nHere's what I did, just to verify:\n\ncd ~/floss/git\ngit checkout 404ebceda0\nNO_GETTEXT=1 make DEVELOPER=1 -j8 # I leave off the NO_GETTEXT=1 on linux\ngit worktree add testdir\ngit -C testdir checkout master\ngit -C testdir fetch https://github.com/git/git.git todo\nbin-wrappers/git -C testdir checkout FETCH_HEAD\n\nDid I get any of those steps wrong?\n\n\nThanks,\nElijah\n"},{"id":"382948","messageId":"20190925215530.GA9013@generichostname","threadId":"51732","inReplyTo":"CABPp-BHyjVSD6_S_dYgPiXM=GVEpT97FqJJearzg4hvKHXfNVA@mail.gmail.com","subject":"Re: [BUG] git is segfaulting, was [PATCH v4 04/12] dir: also check directories for matching pathspecs","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-09-25T21:55:30Z","receivedAt":"2019-09-25T22:02:13Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"On Wed, Sep 25, 2019 at 02:28:15PM -0700, Elijah Newren wrote:\n\n[...]\n\n> > Sorry for the information dump, I haven't had the time to properly look\n> > into the issue but I just wanted to make sure that you're aware.\n> \n> Thanks for testing and sending the heads up. Unfortunately, I cannot\n> reproduce on either Linux or Mac. Do you have some special ignore\n> files or sparse-checkout paths that are important to triggering?\n> What's in your config.mak? \n\nBefore, I had an empty config.mak and I also had the following\n.git/info/exclude (these are two worktrees I have checked out):\n\n\t/jch\n\t/patches\n\naside from that, I don't think I've changed anything else. Anyway, to\ndouble-check that it wasn't my setup that was broken, I ran\n\n\tcd ..\n\tgit clone git git2\n\tcd git2\n\tmake configure\n\t./configure\n\nand then followed the rest the steps and I could still reproduce it.\n\n> What compiler and version?\n\n\t$ cc --version\n\tApple LLVM version 10.0.1 (clang-1001.0.46.4)\n\tTarget: x86_64-apple-darwin18.7.0\n\tThread model: posix\n\tInstalledDir: /Library/Developer/CommandLineTools/usr/bin\n\n> \n> Here's what I did, just to verify:\n> \n> cd ~/floss/git\n> git checkout 404ebceda0\n> NO_GETTEXT=1 make DEVELOPER=1 -j8 # I leave off the NO_GETTEXT=1 on linux\n\nI don't have NO_GETTEXT on Mac but I don't think it affects anything.\n\n> git worktree add testdir\n> git -C testdir checkout master\n> git -C testdir fetch https://github.com/git/git.git todo\n> bin-wrappers/git -C testdir checkout FETCH_HEAD\n> \n> Did I get any of those steps wrong?\n\nLooks correct to me. I don't see why this wouldn't reproduce. I'll send\nyou more information if I figure anything else out.\n\nThanks,\n\nDenton\n\n> \n> \n> Thanks,\n> Elijah\n"},{"id":"383025","messageId":"20190926203509.GA61514@generichostname","threadId":"51732","inReplyTo":"20190925215530.GA9013@generichostname","subject":"Re: [BUG] git is segfaulting, was [PATCH v4 04/12] dir: also check directories for matching pathspecs","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-09-26T20:35:09Z","receivedAt":"2019-09-26T20:35:14Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"On Wed, Sep 25, 2019 at 02:55:30PM -0700, Denton Liu wrote:\n> Looks correct to me. I don't see why this wouldn't reproduce. I'll send\n> you more information if I figure anything else out.\n\nI looked into it a little more and I think I know why it's being\ntriggered.\n\nWhen we checkout 'todo' from 'master', since they're completely\ndifferent trees, all of git's source files need to be removed. As a\nresult, the checkout process at some point invokes check_ok_to_remove().\n\nThis kicks off the following call chain:\n\n\tcheck_ok_to_remove()\n\tverify_clean_subdirectory()\n\tread_directory()\n\tread_directory_recursive() (this is called recursively, of course)\n\tmatch_pathspec()\n\tdo_match_pathspec()\n\nWhere we segfault in do_match_pathspec() because ps is NULL:\n\n\tGUARD_PATHSPEC(ps,\n\t\t\t PATHSPEC_FROMTOP |\n\t\t\t PATHSPEC_MAXDEPTH |\n\t\t\t PATHSPEC_LITERAL |\n\t\t\t PATHSPEC_GLOB |\n\t\t\t PATHSPEC_ICASE |\n\t\t\t PATHSPEC_EXCLUDE |\n\t\t\t PATHSPEC_ATTR);\n\nSo why is ps == NULL? In verify_clean_subdirectory(), we call\nread_directory() like this:\n\n\ti = read_directory(&d, o->src_index, pathbuf, namelen+1, NULL);\n\nwhere we explictly pass in a NULL and it is handed down the callstack. I\nguess this means that we should be expecting that pathspecs can be NULL\nin this path. So I've applied the patch at the bottom and it fixes the\nproblem.\n\nI was wondering if we should stick a\n\n\tif (!ps)\n\t\tBUG(\"ps is NULL\");\n\ninto do_match_pathspec(), though, so we can avoid these situations in\nthe future.\n\nAlso, I'm still not sure why the issue wasn't reproducible on your\nside... I'm not too familiar with this area of the code, though.\n\n-- >8 --\ndiff --git a/dir.c b/dir.c\nindex 76a3c3894b..b7a6de58c6 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -1952,7 +1952,7 @@ static enum path_treatment read_directory_recursive(struct dir_struct *dir,\n if (subdir_state > dir_state)\n dir_state = subdir_state;\n \n- if (!match_pathspec(istate, pathspec, path.buf, path.len,\n+ if (pathspec && !match_pathspec(istate, pathspec, path.buf, path.len,\n 0 /* prefix */, NULL,\n 0 /* do NOT special case dirs */))\n state = path_none;\n"},{"id":"383045","messageId":"CABPp-BGWG-HExZSwzOL75-9Oi0Cs1c8dc6Hq9TotgpQa0XmNvg@mail.gmail.com","threadId":"51732","inReplyTo":"20190926203509.GA61514@generichostname","subject":"Re: [BUG] git is segfaulting, was [PATCH v4 04/12] dir: also check directories for matching pathspecs","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2019-09-27T00:12:23Z","receivedAt":"2019-09-27T00:12:37Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Hi Denton,\n\nOn Thu, Sep 26, 2019 at 1:35 PM Denton Liu wrote:\n>\n> On Wed, Sep 25, 2019 at 02:55:30PM -0700, Denton Liu wrote:\n> > Looks correct to me. I don't see why this wouldn't reproduce. I'll send\n> > you more information if I figure anything else out.\n>\n> I looked into it a little more and I think I know why it's being\n> triggered.\n>\n> When we checkout 'todo' from 'master', since they're completely\n> different trees, all of git's source files need to be removed. As a\n> result, the checkout process at some point invokes check_ok_to_remove().\n>\n> This kicks off the following call chain:\n>\n> check_ok_to_remove()\n> verify_clean_subdirectory()\n> read_directory()\n> read_directory_recursive() (this is called recursively, of course)\n> match_pathspec()\n> do_match_pathspec()\n>\n> Where we segfault in do_match_pathspec() because ps is NULL:\n>\n> GUARD_PATHSPEC(ps,\n> PATHSPEC_FROMTOP |\n> PATHSPEC_MAXDEPTH |\n> PATHSPEC_LITERAL |\n> PATHSPEC_GLOB |\n> PATHSPEC_ICASE |\n> PATHSPEC_EXCLUDE |\n> PATHSPEC_ATTR);\n>\n> So why is ps == NULL? In verify_clean_subdirectory(), we call\n> read_directory() like this:\n>\n> i = read_directory(&d, o->src_index, pathbuf, namelen+1, NULL);\n>\n> where we explictly pass in a NULL and it is handed down the callstack. I\n> guess this means that we should be expecting that pathspecs can be NULL\n> in this path. So I've applied the patch at the bottom and it fixes the\n> problem.\n>\n> I was wondering if we should stick a\n>\n> if (!ps)\n> BUG(\"ps is NULL\");\n>\n> into do_match_pathspec(), though, so we can avoid these situations in\n> the future.\n>\n> Also, I'm still not sure why the issue wasn't reproducible on your\n> side... I'm not too familiar with this area of the code, though.\n>\n> -- >8 --\n> diff --git a/dir.c b/dir.c\n> index 76a3c3894b..b7a6de58c6 100644\n> --- a/dir.c\n> +++ b/dir.c\n> @@ -1952,7 +1952,7 @@ static enum path_treatment read_directory_recursive(struct dir_struct *dir,\n> if (subdir_state > dir_state)\n> dir_state = subdir_state;\n>\n> - if (!match_pathspec(istate, pathspec, path.buf, path.len,\n> + if (pathspec && !match_pathspec(istate, pathspec, path.buf, path.len,\n> 0 /* prefix */, NULL,\n> 0 /* do NOT special case dirs */))\n> state = path_none;\n\nThe patch makes sense...but I'd really like to add a test, and\nunderstand it better so I can check to see if there are any other bad\ncodepaths. Sadly, I still have no idea how to reproduce the bug. I\ncan put\n\n char *oopsies = NULL;\n printf(\"oopsies = %s\\n\", oopsies);\n\nat the beginning of check_ok_to_remove() to verify that function is\nnever called and run the steps you gave with no problem. However, I\ndo notice that your reproduction steps involve 'master' which may have\nlocal changes for you that I don't have. Is there any chance you can\nreproduce this using a commit id that is already upstream instead of\n'master'? I've been poking around unpack-trees.c for a bit but I'm\nhaving a hard time reversing out of it what's different about our\nsetups and how to trigger.\n"},{"id":"383047","messageId":"20190927010930.GK2637@szeder.dev","threadId":"51732","inReplyTo":"20190925203919.GA89135@generichostname","subject":"Re: [BUG] git is segfaulting, was [PATCH v4 04/12] dir: also check directories for matching pathspecs","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-09-27T01:09:30Z","receivedAt":"2019-09-27T01:09:37Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Wed, Sep 25, 2019 at 01:39:19PM -0700, Denton Liu wrote:\n> Hi Elijah,\n> \n> I ran into a segfault on MacOS. I managed to bisect it down to\n> 404ebceda0 (dir: also check directories for matching pathspecs,\n> 2019-09-17), which should be the patch in the parent thread. The test\n> case below works fine without this patch applied but segfaults once it\n> is applied.\n> \n> \t#!/bin/sh\n> \n> \tgit worktree add testdir\n> \tgit -C testdir checkout master\n> \tgit -C testdir fetch https://github.com/git/git.git todo\n> \tbin-wrappers/git -C testdir checkout FETCH_HEAD # segfault here\n> \n> Note that the worktree part isn't necessary to reproduce the problem but\n> I didn't want my files to be constantly refreshed, triggering a rebuild\n> each time.\n> \n> I also managed to get this backtrace from running lldb at the segfault\n> but it is based on the latest \"jch\" commit, 1cc52d20df (Merge branch\n> 'jt/merge-recursive-symlink-is-not-a-dir-in-way' into jch, 2019-09-20).\n> \n> \t* thread #1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x8)\n> \t * frame #0: 0x00000001000f63a0 git`do_match_pathspec(istate=0x0000000100299940, ps=0x000000010200aa80, name=\"Gitweb/static/js/lib/\", namelen=21, prefix=0, seen=0x0000000000000000, flags=0) at dir.c:420:2 [opt]\n> \t\tframe #1: 0x00000001000f632c git`match_pathspec(istate=0x0000000100299940, ps=0x0000000000000000, name=\"Gitweb/static/js/lib/\", namelen=21, prefix=0, seen=0x0000000000000000, is_dir=0) at dir.c:490:13 [opt]\n> \t\tframe #2: 0x00000001000f8315 git`read_directory_recursive(dir=0x00007ffeefbfe278, istate=0x0000000100299940, base=, baselen=17, untracked=, check_only=0, stop_at_first_file=0, pathspec=0x0000000000000000) at dir.c:1990:9 [opt]\n> \t\tframe #3: 0x00000001000f82e9 git`read_directory_recursive(dir=0x00007ffeefbfe278, istate=0x0000000100299940, base=, baselen=14, untracked=, check_only=0, stop_at_first_file=0, pathspec=0x0000000000000000) at dir.c:1984:5 [opt]\n> \t\tframe #4: 0x00000001000f82e9 git`read_directory_recursive(dir=0x00007ffeefbfe278, istate=0x0000000100299940, base=, baselen=7, untracked=, check_only=0, stop_at_first_file=0, pathspec=0x0000000000000000) at dir.c:1984:5 [opt]\n> \t\tframe #5: 0x00000001000f60d1 git`read_directory(dir=0x00007ffeefbfe278, istate=0x0000000100299940, path=\"Gitweb/\", len=7, pathspec=0x0000000000000000) at dir.c:2298:3 [opt]\n> \t\tframe #6: 0x00000001001bded1 git`verify_clean_subdirectory(ce=, o=0x00007ffeefbfe8c0) at unpack-trees.c:1846:6 [opt]\n> \t\tframe #7: 0x00000001001bdc1d git`check_ok_to_remove(name=\"Gitweb\", len=6, dtype=4, ce=0x0000000103e70de0, st=0x00007ffeefbfe438, error_type=ERROR_WOULD_LOSE_UNTRACKED_OVERWRITTEN, o=0x00007ffeefbfe8c0) at unpack-trees.c:1901:7 [opt]\n\nThat 'name=\"Gitweb\" parameter caught my eye. origin/todo contains a\n'Gitweb' file, with upper case 'G', while master contains a 'gitweb'\ndirectory, with lower case 'g'. \n\nCould it be that case (in)sensitivity plays a crucial rule in\ntriggering the segfault? FWIW I could reproduce it following Denton's\ndescription on Travis CI's macOS VM with the debug shell access, and\nit uses case insensitive file system.\n\n"},{"id":"383048","messageId":"20190927021746.GL2637@szeder.dev","threadId":"51732","inReplyTo":"20190927010930.GK2637@szeder.dev","subject":"Re: [BUG] git is segfaulting, was [PATCH v4 04/12] dir: also check directories for matching pathspecs","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-09-27T02:17:46Z","receivedAt":"2019-09-27T02:17:54Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Fri, Sep 27, 2019 at 03:09:30AM +0200, SZEDER Gábor wrote:\n> On Wed, Sep 25, 2019 at 01:39:19PM -0700, Denton Liu wrote:\n> > Hi Elijah,\n> > \n> > I ran into a segfault on MacOS. I managed to bisect it down to\n> > 404ebceda0 (dir: also check directories for matching pathspecs,\n> > 2019-09-17), which should be the patch in the parent thread. The test\n> > case below works fine without this patch applied but segfaults once it\n> > is applied.\n> > \n> > \t#!/bin/sh\n> > \n> > \tgit worktree add testdir\n> > \tgit -C testdir checkout master\n> > \tgit -C testdir fetch https://github.com/git/git.git todo\n> > \tbin-wrappers/git -C testdir checkout FETCH_HEAD # segfault here\n> > \n> > Note that the worktree part isn't necessary to reproduce the problem but\n> > I didn't want my files to be constantly refreshed, triggering a rebuild\n> > each time.\n> > \n> > I also managed to get this backtrace from running lldb at the segfault\n> > but it is based on the latest \"jch\" commit, 1cc52d20df (Merge branch\n> > 'jt/merge-recursive-symlink-is-not-a-dir-in-way' into jch, 2019-09-20).\n> > \n> > \t* thread #1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x8)\n> > \t * frame #0: 0x00000001000f63a0 git`do_match_pathspec(istate=0x0000000100299940, ps=0x000000010200aa80, name=\"Gitweb/static/js/lib/\", namelen=21, prefix=0, seen=0x0000000000000000, flags=0) at dir.c:420:2 [opt]\n> > \t\tframe #1: 0x00000001000f632c git`match_pathspec(istate=0x0000000100299940, ps=0x0000000000000000, name=\"Gitweb/static/js/lib/\", namelen=21, prefix=0, seen=0x0000000000000000, is_dir=0) at dir.c:490:13 [opt]\n> > \t\tframe #2: 0x00000001000f8315 git`read_directory_recursive(dir=0x00007ffeefbfe278, istate=0x0000000100299940, base=, baselen=17, untracked=, check_only=0, stop_at_first_file=0, pathspec=0x0000000000000000) at dir.c:1990:9 [opt]\n> > \t\tframe #3: 0x00000001000f82e9 git`read_directory_recursive(dir=0x00007ffeefbfe278, istate=0x0000000100299940, base=, baselen=14, untracked=, check_only=0, stop_at_first_file=0, pathspec=0x0000000000000000) at dir.c:1984:5 [opt]\n> > \t\tframe #4: 0x00000001000f82e9 git`read_directory_recursive(dir=0x00007ffeefbfe278, istate=0x0000000100299940, base=, baselen=7, untracked=, check_only=0, stop_at_first_file=0, pathspec=0x0000000000000000) at dir.c:1984:5 [opt]\n> > \t\tframe #5: 0x00000001000f60d1 git`read_directory(dir=0x00007ffeefbfe278, istate=0x0000000100299940, path=\"Gitweb/\", len=7, pathspec=0x0000000000000000) at dir.c:2298:3 [opt]\n> > \t\tframe #6: 0x00000001001bded1 git`verify_clean_subdirectory(ce=, o=0x00007ffeefbfe8c0) at unpack-trees.c:1846:6 [opt]\n> > \t\tframe #7: 0x00000001001bdc1d git`check_ok_to_remove(name=\"Gitweb\", len=6, dtype=4, ce=0x0000000103e70de0, st=0x00007ffeefbfe438, error_type=ERROR_WOULD_LOSE_UNTRACKED_OVERWRITTEN, o=0x00007ffeefbfe8c0) at unpack-trees.c:1901:7 [opt]\n> \n> That 'name=\"Gitweb\" parameter caught my eye. origin/todo contains a\n> 'Gitweb' file, with upper case 'G', while master contains a 'gitweb'\n> directory, with lower case 'g'. \n> \n> Could it be that case (in)sensitivity plays a crucial rule in\n> triggering the segfault? FWIW I could reproduce it following Denton's\n> description on Travis CI's macOS VM with the debug shell access, and\n> it uses case insensitive file system.\n\nIndeed, with 404ebceda0 the test below segfaults on case insensitive\nfs, but not on a case sensitive one.\n\n\ndiff --git a/t/t0050-filesystem.sh b/t/t0050-filesystem.sh\nindex 192c94eccd..5b405c97d7 100755\n--- a/t/t0050-filesystem.sh\n+++ b/t/t0050-filesystem.sh\n@@ -131,4 +131,27 @@ $test_unicode 'merge (silent unicode normalization)' '\n \tgit merge topic\n '\n \n+test_expect_success CASE_INSENSITIVE_FS \"Denton's segfault\" '\n+\tgit init repo &&\n+\t(\n+\t\tcd repo &&\n+\n+\t\techo foo >Gitweb &&\n+\t\tgit add Gitweb &&\n+\t\tgit commit -m \"add Gitweb\" &&\n+\n+\t\tgit checkout --orphan todo &&\n+\t\tgit reset --hard &&\n+\t\t# the subdir is crucial, without it there is no segfault\n+\t\tmkdir -p gitweb/subdir &&\n+\t\techo bar >gitweb/subdir/file &&\n+\t\t# it is not strictly necessary to add and commit the\n+\t\t# gitweb directory, its presence is sufficient\n+\t\tgit add gitweb &&\n+\t\tgit commit -m \"add gitweb/subdir/file\" &&\n+\n+\t\tgit checkout master\n+\t)\n+'\n+\n test_done\n\n\n\nThe end of its trace:\n\n++git checkout master\n./test-lib.sh: line 910: 11220 Segmentation fault: 11 git checkout master\nerror: last command exited with $?=139\n\nCase insensitivity is important because check_ok_to_remove() is\ninvoked from verify_absent_1(), which looks like this:\n\n if (...)\n ....\n else if (...)\n ....\n else if (lstat(ce->name, &st))\n // That lstat() checked whether 'Gitweb' is absent. On a case\n // sensitive fs it's absent, so it returns. On a case\n // insensitive fs it finds 'master's 'gitweb' directory, so it\n // goes on to the else below, and eventually segfaults.\n return;\n else\n check_ok_to_remove()\n\n\nGood night :)\n"},{"id":"383070","messageId":"20190927171034.GA6542@generichostname","threadId":"51732","inReplyTo":"20190927021746.GL2637@szeder.dev","subject":"Re: [BUG] git is segfaulting, was [PATCH v4 04/12] dir: also check directories for matching pathspecs","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-09-27T17:10:34Z","receivedAt":"2019-09-27T17:10:40Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"On Fri, Sep 27, 2019 at 04:17:46AM +0200, SZEDER Gábor wrote:\n> On Fri, Sep 27, 2019 at 03:09:30AM +0200, SZEDER Gábor wrote:\n> > On Wed, Sep 25, 2019 at 01:39:19PM -0700, Denton Liu wrote:\n> > > Hi Elijah,\n> > > \n> > > I ran into a segfault on MacOS. I managed to bisect it down to\n> > > 404ebceda0 (dir: also check directories for matching pathspecs,\n> > > 2019-09-17), which should be the patch in the parent thread. The test\n> > > case below works fine without this patch applied but segfaults once it\n> > > is applied.\n> > > \n> > > \t#!/bin/sh\n> > > \n> > > \tgit worktree add testdir\n> > > \tgit -C testdir checkout master\n> > > \tgit -C testdir fetch https://github.com/git/git.git todo\n> > > \tbin-wrappers/git -C testdir checkout FETCH_HEAD # segfault here\n> > > \n> > > Note that the worktree part isn't necessary to reproduce the problem but\n> > > I didn't want my files to be constantly refreshed, triggering a rebuild\n> > > each time.\n> > > \n> > > I also managed to get this backtrace from running lldb at the segfault\n> > > but it is based on the latest \"jch\" commit, 1cc52d20df (Merge branch\n> > > 'jt/merge-recursive-symlink-is-not-a-dir-in-way' into jch, 2019-09-20).\n> > > \n> > > \t* thread #1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x8)\n> > > \t * frame #0: 0x00000001000f63a0 git`do_match_pathspec(istate=0x0000000100299940, ps=0x000000010200aa80, name=\"Gitweb/static/js/lib/\", namelen=21, prefix=0, seen=0x0000000000000000, flags=0) at dir.c:420:2 [opt]\n> > > \t\tframe #1: 0x00000001000f632c git`match_pathspec(istate=0x0000000100299940, ps=0x0000000000000000, name=\"Gitweb/static/js/lib/\", namelen=21, prefix=0, seen=0x0000000000000000, is_dir=0) at dir.c:490:13 [opt]\n> > > \t\tframe #2: 0x00000001000f8315 git`read_directory_recursive(dir=0x00007ffeefbfe278, istate=0x0000000100299940, base=, baselen=17, untracked=, check_only=0, stop_at_first_file=0, pathspec=0x0000000000000000) at dir.c:1990:9 [opt]\n> > > \t\tframe #3: 0x00000001000f82e9 git`read_directory_recursive(dir=0x00007ffeefbfe278, istate=0x0000000100299940, base=, baselen=14, untracked=, check_only=0, stop_at_first_file=0, pathspec=0x0000000000000000) at dir.c:1984:5 [opt]\n> > > \t\tframe #4: 0x00000001000f82e9 git`read_directory_recursive(dir=0x00007ffeefbfe278, istate=0x0000000100299940, base=, baselen=7, untracked=, check_only=0, stop_at_first_file=0, pathspec=0x0000000000000000) at dir.c:1984:5 [opt]\n> > > \t\tframe #5: 0x00000001000f60d1 git`read_directory(dir=0x00007ffeefbfe278, istate=0x0000000100299940, path=\"Gitweb/\", len=7, pathspec=0x0000000000000000) at dir.c:2298:3 [opt]\n> > > \t\tframe #6: 0x00000001001bded1 git`verify_clean_subdirectory(ce=, o=0x00007ffeefbfe8c0) at unpack-trees.c:1846:6 [opt]\n> > > \t\tframe #7: 0x00000001001bdc1d git`check_ok_to_remove(name=\"Gitweb\", len=6, dtype=4, ce=0x0000000103e70de0, st=0x00007ffeefbfe438, error_type=ERROR_WOULD_LOSE_UNTRACKED_OVERWRITTEN, o=0x00007ffeefbfe8c0) at unpack-trees.c:1901:7 [opt]\n> > \n> > That 'name=\"Gitweb\" parameter caught my eye. origin/todo contains a\n> > 'Gitweb' file, with upper case 'G', while master contains a 'gitweb'\n> > directory, with lower case 'g'. \n> > \n> > Could it be that case (in)sensitivity plays a crucial rule in\n> > triggering the segfault? FWIW I could reproduce it following Denton's\n> > description on Travis CI's macOS VM with the debug shell access, and\n> > it uses case insensitive file system.\n> \n> Indeed, with 404ebceda0 the test below segfaults on case insensitive\n> fs, but not on a case sensitive one.\n\nWow, good catch. I didn't even notice that in the backtrace.\n\n> \n> \n> diff --git a/t/t0050-filesystem.sh b/t/t0050-filesystem.sh\n> index 192c94eccd..5b405c97d7 100755\n> --- a/t/t0050-filesystem.sh\n> +++ b/t/t0050-filesystem.sh\n> @@ -131,4 +131,27 @@ $test_unicode 'merge (silent unicode normalization)' '\n> \tgit merge topic\n> '\n> \n> +test_expect_success CASE_INSENSITIVE_FS \"Denton's segfault\" '\n> +\tgit init repo &&\n> +\t(\n> +\t\tcd repo &&\n> +\n> +\t\techo foo >Gitweb &&\n> +\t\tgit add Gitweb &&\n> +\t\tgit commit -m \"add Gitweb\" &&\n> +\n> +\t\tgit checkout --orphan todo &&\n> +\t\tgit reset --hard &&\n> +\t\t# the subdir is crucial, without it there is no segfault\n> +\t\tmkdir -p gitweb/subdir &&\n> +\t\techo bar >gitweb/subdir/file &&\n> +\t\t# it is not strictly necessary to add and commit the\n> +\t\t# gitweb directory, its presence is sufficient\n> +\t\tgit add gitweb &&\n> +\t\tgit commit -m \"add gitweb/subdir/file\" &&\n> +\n> +\t\tgit checkout master\n> +\t)\n> +'\n> +\n> test_done\n\nI can confirm that this test case reproduces for me. Thanks for writing\nthis.\n\n> \n> \n> \n> The end of its trace:\n> \n> ++git checkout master\n> ./test-lib.sh: line 910: 11220 Segmentation fault: 11 git checkout master\n> error: last command exited with $?=139\n> \n> Case insensitivity is important because check_ok_to_remove() is\n> invoked from verify_absent_1(), which looks like this:\n> \n> if (...)\n> ....\n> else if (...)\n> ....\n> else if (lstat(ce->name, &st))\n> // That lstat() checked whether 'Gitweb' is absent. On a case\n> // sensitive fs it's absent, so it returns. On a case\n> // insensitive fs it finds 'master's 'gitweb' directory, so it\n> // goes on to the else below, and eventually segfaults.\n> return;\n> else\n> check_ok_to_remove()\n> \n> \n> Good night :)\n\nThanks for your help!\n"},{"id":"383164","messageId":"20190930191106.16128-1-newren@gmail.com","threadId":"51732","inReplyTo":"20190927171034.GA6542@generichostname","subject":"[PATCH] dir: special case check for the possibility that pathspec is NULL","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2019-09-30T19:11:06Z","receivedAt":"2019-09-30T21:01:54Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Commits 404ebceda01c (\"dir: also check directories for matching\npathspecs\", 2019-09-17) and 89a1f4aaf765 (\"dir: if our pathspec might\nmatch files under a dir, recurse into it\", 2019-09-17) added calls to\nmatch_pathspec() and do_match_pathspec() passing along their pathspec\nparameter. Both match_pathspec() and do_match_pathspec() assume the\npathspec argument they are given is non-NULL. It turns out that\nunpack-tree.c's verify_clean_subdirectory() calls read_directory() with\npathspec == NULL, and it is possible on case insensitive filesystems for\nthat NULL to make it to these new calls to match_pathspec() and\ndo_match_pathspec(). Add appropriate checks on the NULLness of pathspec\nto avoid a segfault.\n\nIn case the negation throws anyone off (one of the calls was to\ndo_match_pathspec() while the other was to !match_pathspec(), yet no\nnegation of the NULLness of pathspec is used), there are two ways to\nunderstand the differences:\n * The code already handled the pathspec == NULL cases before this\n series, and this series only tried to change behavior when there was\n a pathspec, thus we only want to go into the if-block if pathspec is\n non-NULL.\n * One of the calls is for whether to recurse into a subdirectory, the\n other is for after we've recursed into it for whether we want to\n remove the subdirectory itself (i.e. the subdirectory didn't match\n but something under it could have). That difference in situation\n leads to the slight differences in logic used (well, that and the\n slightly unusual fact that we don't want empty pathspecs to remove\n untracked directories by default).\n\nHelped-by: Denton Liu \nHelped-by: SZEDER Gábor \nSigned-off-by: Elijah Newren \n---\nThis patch applies on top of en/clean-nested-with-ignored, which is now\nin next.\n\nDenton found and analyzed one issue and provided the patch for the\nmatch_pathspec() call, SZEDER figured out why the issue only reproduced\nfor some folks and not others and provided the testcase, and I looked\nthrough the remainder of the series and noted the do_match_pathspec()\ncall that should have the same check.\n\nSo, I'm not sure who should be author and who should be helped-by; I\nfeel like their contributions are possibly bigger than mine. While I\ntried to reproduce and debug, they ended up doing the work, and I just\nlooked through the rest of the series for similar issues and wrote up\na commit message. *shrug*\n\n dir.c | 8 +++++---\n t/t0050-filesystem.sh | 23 +++++++++++++++++++++++\n 2 files changed, 28 insertions(+), 3 deletions(-)\n\ndiff --git a/dir.c b/dir.c\nindex 7ff79170fc..bd39b86be4 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -1962,8 +1962,9 @@ static enum path_treatment read_directory_recursive(struct dir_struct *dir,\n \t\t\t((state == path_untracked) &&\n \t\t\t (get_dtype(cdir.de, istate, path.buf, path.len) == DT_DIR) &&\n \t\t\t ((dir->flags & DIR_SHOW_IGNORED_TOO) ||\n-\t\t\t do_match_pathspec(istate, pathspec, path.buf, path.len,\n-\t\t\t\t\t baselen, NULL, DO_MATCH_LEADING_PATHSPEC) == MATCHED_RECURSIVELY_LEADING_PATHSPEC))) {\n+\t\t\t (pathspec &&\n+\t\t\t do_match_pathspec(istate, pathspec, path.buf, path.len,\n+\t\t\t\t\t baselen, NULL, DO_MATCH_LEADING_PATHSPEC) == MATCHED_RECURSIVELY_LEADING_PATHSPEC)))) {\n \t\t\tstruct untracked_cache_dir *ud;\n \t\t\tud = lookup_untracked(dir->untracked, untracked,\n \t\t\t\t\t path.buf + baselen,\n@@ -1975,7 +1976,8 @@ static enum path_treatment read_directory_recursive(struct dir_struct *dir,\n \t\t\tif (subdir_state > dir_state)\n \t\t\t\tdir_state = subdir_state;\n \n-\t\t\tif (!match_pathspec(istate, pathspec, path.buf, path.len,\n+\t\t\tif (pathspec &&\n+\t\t\t !match_pathspec(istate, pathspec, path.buf, path.len,\n \t\t\t\t\t 0 /* prefix */, NULL,\n \t\t\t\t\t 0 /* do NOT special case dirs */))\n \t\t\t\tstate = path_none;\ndiff --git a/t/t0050-filesystem.sh b/t/t0050-filesystem.sh\nindex 192c94eccd..edb30f9eb2 100755\n--- a/t/t0050-filesystem.sh\n+++ b/t/t0050-filesystem.sh\n@@ -131,4 +131,27 @@ $test_unicode 'merge (silent unicode normalization)' '\n \tgit merge topic\n '\n \n+test_expect_success CASE_INSENSITIVE_FS 'checkout with no pathspec and a case insensitive fs' '\n+\tgit init repo &&\n+\t(\n+\t\tcd repo &&\n+\n+\t\t>Gitweb &&\n+\t\tgit add Gitweb &&\n+\t\tgit commit -m \"add Gitweb\" &&\n+\n+\t\tgit checkout --orphan todo &&\n+\t\tgit reset --hard &&\n+\t\t# the subdir is crucial, without it there is no segfault\n+\t\tmkdir -p gitweb/subdir &&\n+\t\t>gitweb/subdir/file &&\n+\t\t# it is not strictly necessary to add and commit the\n+\t\t# gitweb directory, its presence is sufficient\n+\t\tgit add gitweb &&\n+\t\tgit commit -m \"add gitweb/subdir/file\" &&\n+\n+\t\tgit checkout master\n+\t)\n+'\n+\n test_done\n-- \n2.22.1.14.g885c22d24b\n\n"},{"id":"383184","messageId":"20190930223105.GA86525@generichostname","threadId":"51732","inReplyTo":"20190930191106.16128-1-newren@gmail.com","subject":"Re: [PATCH] dir: special case check for the possibility that pathspec is NULL","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-09-30T22:31:05Z","receivedAt":"2019-09-30T22:31:12Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"Hi Elijah,\n\nOn Mon, Sep 30, 2019 at 12:11:06PM -0700, Elijah Newren wrote:\n> Commits 404ebceda01c (\"dir: also check directories for matching\n> pathspecs\", 2019-09-17) and 89a1f4aaf765 (\"dir: if our pathspec might\n> match files under a dir, recurse into it\", 2019-09-17) added calls to\n> match_pathspec() and do_match_pathspec() passing along their pathspec\n> parameter. Both match_pathspec() and do_match_pathspec() assume the\n> pathspec argument they are given is non-NULL. It turns out that\n> unpack-tree.c's verify_clean_subdirectory() calls read_directory() with\n> pathspec == NULL, and it is possible on case insensitive filesystems for\n> that NULL to make it to these new calls to match_pathspec() and\n> do_match_pathspec(). Add appropriate checks on the NULLness of pathspec\n> to avoid a segfault.\n> \n> In case the negation throws anyone off (one of the calls was to\n> do_match_pathspec() while the other was to !match_pathspec(), yet no\n> negation of the NULLness of pathspec is used), there are two ways to\n> understand the differences:\n> * The code already handled the pathspec == NULL cases before this\n> series, and this series only tried to change behavior when there was\n> a pathspec, thus we only want to go into the if-block if pathspec is\n> non-NULL.\n> * One of the calls is for whether to recurse into a subdirectory, the\n> other is for after we've recursed into it for whether we want to\n> remove the subdirectory itself (i.e. the subdirectory didn't match\n> but something under it could have). That difference in situation\n> leads to the slight differences in logic used (well, that and the\n> slightly unusual fact that we don't want empty pathspecs to remove\n> untracked directories by default).\n> \n> Helped-by: Denton Liu \n> Helped-by: SZEDER Gábor \n> Signed-off-by: Elijah Newren \n> ---\n> This patch applies on top of en/clean-nested-with-ignored, which is now\n> in next.\n> \n> Denton found and analyzed one issue and provided the patch for the\n> match_pathspec() call, SZEDER figured out why the issue only reproduced\n> for some folks and not others and provided the testcase, and I looked\n> through the remainder of the series and noted the do_match_pathspec()\n> call that should have the same check.\n\nThanks for catching what I missed.\n\n> \n> So, I'm not sure who should be author and who should be helped-by; I\n> feel like their contributions are possibly bigger than mine. While I\n> tried to reproduce and debug, they ended up doing the work, and I just\n> looked through the rest of the series for similar issues and wrote up\n> a commit message. *shrug*\n\nEh, it doesn't really matter to me. GitHub appears to have de facto\nstandardised the Co-authored-by: trailer to allow credit to be split\namonst multiple authors so _maybe_ we could use that, but I'm pretty\nimpartial.\n\n> \n> dir.c | 8 +++++---\n> t/t0050-filesystem.sh | 23 +++++++++++++++++++++++\n> 2 files changed, 28 insertions(+), 3 deletions(-)\n> \n> diff --git a/dir.c b/dir.c\n> index 7ff79170fc..bd39b86be4 100644\n> --- a/dir.c\n> +++ b/dir.c\n> @@ -1962,8 +1962,9 @@ static enum path_treatment read_directory_recursive(struct dir_struct *dir,\n> \t\t\t((state == path_untracked) &&\n> \t\t\t (get_dtype(cdir.de, istate, path.buf, path.len) == DT_DIR) &&\n> \t\t\t ((dir->flags & DIR_SHOW_IGNORED_TOO) ||\n> -\t\t\t do_match_pathspec(istate, pathspec, path.buf, path.len,\n> -\t\t\t\t\t baselen, NULL, DO_MATCH_LEADING_PATHSPEC) == MATCHED_RECURSIVELY_LEADING_PATHSPEC))) {\n> +\t\t\t (pathspec &&\n> +\t\t\t do_match_pathspec(istate, pathspec, path.buf, path.len,\n> +\t\t\t\t\t baselen, NULL, DO_MATCH_LEADING_PATHSPEC) == MATCHED_RECURSIVELY_LEADING_PATHSPEC)))) {\n> \t\t\tstruct untracked_cache_dir *ud;\n> \t\t\tud = lookup_untracked(dir->untracked, untracked,\n> \t\t\t\t\t path.buf + baselen,\n> @@ -1975,7 +1976,8 @@ static enum path_treatment read_directory_recursive(struct dir_struct *dir,\n> \t\t\tif (subdir_state > dir_state)\n> \t\t\t\tdir_state = subdir_state;\n> \n> -\t\t\tif (!match_pathspec(istate, pathspec, path.buf, path.len,\n> +\t\t\tif (pathspec &&\n> +\t\t\t !match_pathspec(istate, pathspec, path.buf, path.len,\n> \t\t\t\t\t 0 /* prefix */, NULL,\n> \t\t\t\t\t 0 /* do NOT special case dirs */))\n> \t\t\t\tstate = path_none;\n> diff --git a/t/t0050-filesystem.sh b/t/t0050-filesystem.sh\n> index 192c94eccd..edb30f9eb2 100755\n> --- a/t/t0050-filesystem.sh\n> +++ b/t/t0050-filesystem.sh\n> @@ -131,4 +131,27 @@ $test_unicode 'merge (silent unicode normalization)' '\n> \tgit merge topic\n> '\n> \n> +test_expect_success CASE_INSENSITIVE_FS 'checkout with no pathspec and a case insensitive fs' '\n> +\tgit init repo &&\n> +\t(\n> +\t\tcd repo &&\n> +\n> +\t\t>Gitweb &&\n> +\t\tgit add Gitweb &&\n> +\t\tgit commit -m \"add Gitweb\" &&\n> +\n> +\t\tgit checkout --orphan todo &&\n> +\t\tgit reset --hard &&\n> +\t\t# the subdir is crucial, without it there is no segfault\n\nWe should either remove this comment or change the justification. A\nfuture reader may be confused at what particular segfault this refers\nto.\n\n> +\t\tmkdir -p gitweb/subdir &&\n> +\t\t>gitweb/subdir/file &&\n> +\t\t# it is not strictly necessary to add and commit the\n> +\t\t# gitweb directory, its presence is sufficient\n\nSame here, its presence is sufficient to... what?\n\nThanks,\n\nDenton\n\n> +\t\tgit add gitweb &&\n> +\t\tgit commit -m \"add gitweb/subdir/file\" &&\n> +\n> +\t\tgit checkout master\n> +\t)\n> +'\n> +\n> test_done\n> -- \n> 2.22.1.14.g885c22d24b\n> \n"},{"id":"383194","messageId":"CABPp-BGv0zRbvSkXSvF=Q55Z9gJ1od4dsDpTOSDLVv9tppkKnw@mail.gmail.com","threadId":"51732","inReplyTo":"20190930223105.GA86525@generichostname","subject":"Re: [PATCH] dir: special case check for the possibility that pathspec is NULL","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2019-10-01T07:01:44Z","receivedAt":"2019-10-01T07:01:58Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Mon, Sep 30, 2019 at 3:31 PM Denton Liu wrote:\n>\n> Hi Elijah,\n>\n> On Mon, Sep 30, 2019 at 12:11:06PM -0700, Elijah Newren wrote:\n> > Commits 404ebceda01c (\"dir: also check directories for matching\n> > pathspecs\", 2019-09-17) and 89a1f4aaf765 (\"dir: if our pathspec might\n> > match files under a dir, recurse into it\", 2019-09-17) added calls to\n> > match_pathspec() and do_match_pathspec() passing along their pathspec\n> > parameter. Both match_pathspec() and do_match_pathspec() assume the\n> > pathspec argument they are given is non-NULL. It turns out that\n> > unpack-tree.c's verify_clean_subdirectory() calls read_directory() with\n> > pathspec == NULL, and it is possible on case insensitive filesystems for\n> > that NULL to make it to these new calls to match_pathspec() and\n> > do_match_pathspec(). Add appropriate checks on the NULLness of pathspec\n> > to avoid a segfault.\n> >\n> > In case the negation throws anyone off (one of the calls was to\n> > do_match_pathspec() while the other was to !match_pathspec(), yet no\n> > negation of the NULLness of pathspec is used), there are two ways to\n> > understand the differences:\n> > * The code already handled the pathspec == NULL cases before this\n> > series, and this series only tried to change behavior when there was\n> > a pathspec, thus we only want to go into the if-block if pathspec is\n> > non-NULL.\n> > * One of the calls is for whether to recurse into a subdirectory, the\n> > other is for after we've recursed into it for whether we want to\n> > remove the subdirectory itself (i.e. the subdirectory didn't match\n> > but something under it could have). That difference in situation\n> > leads to the slight differences in logic used (well, that and the\n> > slightly unusual fact that we don't want empty pathspecs to remove\n> > untracked directories by default).\n> >\n> > Helped-by: Denton Liu \n> > Helped-by: SZEDER Gábor \n> > Signed-off-by: Elijah Newren \n> > ---\n> > This patch applies on top of en/clean-nested-with-ignored, which is now\n> > in next.\n> >\n> > Denton found and analyzed one issue and provided the patch for the\n> > match_pathspec() call, SZEDER figured out why the issue only reproduced\n> > for some folks and not others and provided the testcase, and I looked\n> > through the remainder of the series and noted the do_match_pathspec()\n> > call that should have the same check.\n>\n> Thanks for catching what I missed.\n>\n> >\n> > So, I'm not sure who should be author and who should be helped-by; I\n> > feel like their contributions are possibly bigger than mine. While I\n> > tried to reproduce and debug, they ended up doing the work, and I just\n> > looked through the rest of the series for similar issues and wrote up\n> > a commit message. *shrug*\n>\n> Eh, it doesn't really matter to me. GitHub appears to have de facto\n> standardised the Co-authored-by: trailer to allow credit to be split\n> amonst multiple authors so _maybe_ we could use that, but I'm pretty\n> impartial.\n>\n> >\n> > dir.c | 8 +++++---\n> > t/t0050-filesystem.sh | 23 +++++++++++++++++++++++\n> > 2 files changed, 28 insertions(+), 3 deletions(-)\n> >\n> > diff --git a/dir.c b/dir.c\n> > index 7ff79170fc..bd39b86be4 100644\n> > --- a/dir.c\n> > +++ b/dir.c\n> > @@ -1962,8 +1962,9 @@ static enum path_treatment read_directory_recursive(struct dir_struct *dir,\n> > ((state == path_untracked) &&\n> > (get_dtype(cdir.de, istate, path.buf, path.len) == DT_DIR) &&\n> > ((dir->flags & DIR_SHOW_IGNORED_TOO) ||\n> > - do_match_pathspec(istate, pathspec, path.buf, path.len,\n> > - baselen, NULL, DO_MATCH_LEADING_PATHSPEC) == MATCHED_RECURSIVELY_LEADING_PATHSPEC))) {\n> > + (pathspec &&\n> > + do_match_pathspec(istate, pathspec, path.buf, path.len,\n> > + baselen, NULL, DO_MATCH_LEADING_PATHSPEC) == MATCHED_RECURSIVELY_LEADING_PATHSPEC)))) {\n> > struct untracked_cache_dir *ud;\n> > ud = lookup_untracked(dir->untracked, untracked,\n> > path.buf + baselen,\n> > @@ -1975,7 +1976,8 @@ static enum path_treatment read_directory_recursive(struct dir_struct *dir,\n> > if (subdir_state > dir_state)\n> > dir_state = subdir_state;\n> >\n> > - if (!match_pathspec(istate, pathspec, path.buf, path.len,\n> > + if (pathspec &&\n> > + !match_pathspec(istate, pathspec, path.buf, path.len,\n> > 0 /* prefix */, NULL,\n> > 0 /* do NOT special case dirs */))\n> > state = path_none;\n> > diff --git a/t/t0050-filesystem.sh b/t/t0050-filesystem.sh\n> > index 192c94eccd..edb30f9eb2 100755\n> > --- a/t/t0050-filesystem.sh\n> > +++ b/t/t0050-filesystem.sh\n> > @@ -131,4 +131,27 @@ $test_unicode 'merge (silent unicode normalization)' '\n> > git merge topic\n> > '\n> >\n> > +test_expect_success CASE_INSENSITIVE_FS 'checkout with no pathspec and a case insensitive fs' '\n> > + git init repo &&\n> > + (\n> > + cd repo &&\n> > +\n> > + >Gitweb &&\n> > + git add Gitweb &&\n> > + git commit -m \"add Gitweb\" &&\n> > +\n> > + git checkout --orphan todo &&\n> > + git reset --hard &&\n> > + # the subdir is crucial, without it there is no segfault\n>\n> We should either remove this comment or change the justification. A\n> future reader may be confused at what particular segfault this refers\n> to.\n\nYep, good point, I'll just go ahead and remove it.\n\n> > + mkdir -p gitweb/subdir &&\n> > + >gitweb/subdir/file &&\n> > + # it is not strictly necessary to add and commit the\n> > + # gitweb directory, its presence is sufficient\n>\n> Same here, its presence is sufficient to... what?\n\nI will clean this one too and send a v2 tomorrow; it's getting late.\n\nThanks for all the digging you did on this bug to get it sorted out,\nDenton; I really appreciate it.\n\nElijah\n"},{"id":"383226","messageId":"20191001183005.16977-1-newren@gmail.com","threadId":"51732","inReplyTo":"20190930191106.16128-1-newren@gmail.com","subject":"[PATCH v2] dir: special case check for the possibility that pathspec is NULL","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2019-10-01T18:30:05Z","receivedAt":"2019-10-01T18:30:13Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Commits 404ebceda01c (\"dir: also check directories for matching\npathspecs\", 2019-09-17) and 89a1f4aaf765 (\"dir: if our pathspec might\nmatch files under a dir, recurse into it\", 2019-09-17) added calls to\nmatch_pathspec() and do_match_pathspec() passing along their pathspec\nparameter. Both match_pathspec() and do_match_pathspec() assume the\npathspec argument they are given is non-NULL. It turns out that\nunpack-tree.c's verify_clean_subdirectory() calls read_directory() with\npathspec == NULL, and it is possible on case insensitive filesystems for\nthat NULL to make it to these new calls to match_pathspec() and\ndo_match_pathspec(). Add appropriate checks on the NULLness of pathspec\nto avoid a segfault.\n\nIn case the negation throws anyone off (one of the calls was to\ndo_match_pathspec() while the other was to !match_pathspec(), yet no\nnegation of the NULLness of pathspec is used), there are two ways to\nunderstand the differences:\n * The code already handled the pathspec == NULL cases before this\n series, and this series only tried to change behavior when there was\n a pathspec, thus we only want to go into the if-block if pathspec is\n non-NULL.\n * One of the calls is for whether to recurse into a subdirectory, the\n other is for after we've recursed into it for whether we want to\n remove the subdirectory itself (i.e. the subdirectory didn't match\n but something under it could have). That difference in situation\n leads to the slight differences in logic used (well, that and the\n slightly unusual fact that we don't want empty pathspecs to remove\n untracked directories by default).\n\nDenton found and analyzed one issue and provided the patch for the\nmatch_pathspec() call, SZEDER figured out why the issue only reproduced\nfor some folks and not others and provided the testcase, and I looked\nthrough the remainder of the series and noted the do_match_pathspec()\ncall that should have the same check.\n\nCo-authored-by: Denton Liu \nCo-authored-by: SZEDER Gábor \nSigned-off-by: Elijah Newren \n---\nNote: Applies on top of en/clean-nested-with-ignored, in next.\n\nAs with v1, the authorship is really mixed, so I don't know if I\nshould use Co-authored-by (highlighted as a possibility by Denton), or\nthe far more common Helped-by (as suggested by Junio but based on a\nmore limited summary of the different contributions), or if perhaps\nDenton or SZEDER should be marked as the author and I be marked as\nHelped-by or Co-authored-by. Since Denton commented on round 1, I\nused his suggestion for attribution in this round, but I'm open to\nchanging it to whatever works best.\n\nChanges since v1:\n - Removed comments that made sense in context of the original thread\n but wouldn't be helpful to future readers.\n - s/Helped-by/Co-authored-by/\n\nRange-diff:\n1: 885c22d24b ! 1: c495b9303c dir: special case check for the possibility that pathspec is NULL\n @@ t/t0050-filesystem.sh: $test_unicode 'merge (silent unicode normalization)' '\n +\n +\t\tgit checkout --orphan todo &&\n +\t\tgit reset --hard &&\n -+\t\t# the subdir is crucial, without it there is no segfault\n +\t\tmkdir -p gitweb/subdir &&\n +\t\t>gitweb/subdir/file &&\n +\t\t# it is not strictly necessary to add and commit the\n -+\t\t# gitweb directory, its presence is sufficient\n +\t\tgit add gitweb &&\n +\t\tgit commit -m \"add gitweb/subdir/file\" &&\n +\n\n dir.c | 8 +++++---\n t/t0050-filesystem.sh | 21 +++++++++++++++++++++\n 2 files changed, 26 insertions(+), 3 deletions(-)\n\ndiff --git a/dir.c b/dir.c\nindex 7ff79170fc..bd39b86be4 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -1962,8 +1962,9 @@ static enum path_treatment read_directory_recursive(struct dir_struct *dir,\n \t\t\t((state == path_untracked) &&\n \t\t\t (get_dtype(cdir.de, istate, path.buf, path.len) == DT_DIR) &&\n \t\t\t ((dir->flags & DIR_SHOW_IGNORED_TOO) ||\n-\t\t\t do_match_pathspec(istate, pathspec, path.buf, path.len,\n-\t\t\t\t\t baselen, NULL, DO_MATCH_LEADING_PATHSPEC) == MATCHED_RECURSIVELY_LEADING_PATHSPEC))) {\n+\t\t\t (pathspec &&\n+\t\t\t do_match_pathspec(istate, pathspec, path.buf, path.len,\n+\t\t\t\t\t baselen, NULL, DO_MATCH_LEADING_PATHSPEC) == MATCHED_RECURSIVELY_LEADING_PATHSPEC)))) {\n \t\t\tstruct untracked_cache_dir *ud;\n \t\t\tud = lookup_untracked(dir->untracked, untracked,\n \t\t\t\t\t path.buf + baselen,\n@@ -1975,7 +1976,8 @@ static enum path_treatment read_directory_recursive(struct dir_struct *dir,\n \t\t\tif (subdir_state > dir_state)\n \t\t\t\tdir_state = subdir_state;\n \n-\t\t\tif (!match_pathspec(istate, pathspec, path.buf, path.len,\n+\t\t\tif (pathspec &&\n+\t\t\t !match_pathspec(istate, pathspec, path.buf, path.len,\n \t\t\t\t\t 0 /* prefix */, NULL,\n \t\t\t\t\t 0 /* do NOT special case dirs */))\n \t\t\t\tstate = path_none;\ndiff --git a/t/t0050-filesystem.sh b/t/t0050-filesystem.sh\nindex 192c94eccd..a840919967 100755\n--- a/t/t0050-filesystem.sh\n+++ b/t/t0050-filesystem.sh\n@@ -131,4 +131,25 @@ $test_unicode 'merge (silent unicode normalization)' '\n \tgit merge topic\n '\n \n+test_expect_success CASE_INSENSITIVE_FS 'checkout with no pathspec and a case insensitive fs' '\n+\tgit init repo &&\n+\t(\n+\t\tcd repo &&\n+\n+\t\t>Gitweb &&\n+\t\tgit add Gitweb &&\n+\t\tgit commit -m \"add Gitweb\" &&\n+\n+\t\tgit checkout --orphan todo &&\n+\t\tgit reset --hard &&\n+\t\tmkdir -p gitweb/subdir &&\n+\t\t>gitweb/subdir/file &&\n+\t\t# it is not strictly necessary to add and commit the\n+\t\tgit add gitweb &&\n+\t\tgit commit -m \"add gitweb/subdir/file\" &&\n+\n+\t\tgit checkout master\n+\t)\n+'\n+\n test_done\n-- \n2.23.0.25.g3f4444bfd7.dirty\n\n"},{"id":"383227","messageId":"20191001184049.GA30113@generichostname","threadId":"51732","inReplyTo":"20191001183005.16977-1-newren@gmail.com","subject":"Re: [PATCH v2] dir: special case check for the possibility that pathspec is NULL","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-10-01T18:40:49Z","receivedAt":"2019-10-01T18:41:17Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"Hi Elijah,\n\nOn Tue, Oct 01, 2019 at 11:30:05AM -0700, Elijah Newren wrote:\n\n[...]\n\n> diff --git a/dir.c b/dir.c\n> index 7ff79170fc..bd39b86be4 100644\n> --- a/dir.c\n> +++ b/dir.c\n> @@ -1962,8 +1962,9 @@ static enum path_treatment read_directory_recursive(struct dir_struct *dir,\n> \t\t\t((state == path_untracked) &&\n> \t\t\t (get_dtype(cdir.de, istate, path.buf, path.len) == DT_DIR) &&\n> \t\t\t ((dir->flags & DIR_SHOW_IGNORED_TOO) ||\n> -\t\t\t do_match_pathspec(istate, pathspec, path.buf, path.len,\n> -\t\t\t\t\t baselen, NULL, DO_MATCH_LEADING_PATHSPEC) == MATCHED_RECURSIVELY_LEADING_PATHSPEC))) {\n> +\t\t\t (pathspec &&\n> +\t\t\t do_match_pathspec(istate, pathspec, path.buf, path.len,\n> +\t\t\t\t\t baselen, NULL, DO_MATCH_LEADING_PATHSPEC) == MATCHED_RECURSIVELY_LEADING_PATHSPEC)))) {\n> \t\t\tstruct untracked_cache_dir *ud;\n> \t\t\tud = lookup_untracked(dir->untracked, untracked,\n> \t\t\t\t\t path.buf + baselen,\n> @@ -1975,7 +1976,8 @@ static enum path_treatment read_directory_recursive(struct dir_struct *dir,\n> \t\t\tif (subdir_state > dir_state)\n> \t\t\t\tdir_state = subdir_state;\n> \n> -\t\t\tif (!match_pathspec(istate, pathspec, path.buf, path.len,\n> +\t\t\tif (pathspec &&\n> +\t\t\t !match_pathspec(istate, pathspec, path.buf, path.len,\n> \t\t\t\t\t 0 /* prefix */, NULL,\n> \t\t\t\t\t 0 /* do NOT special case dirs */))\n> \t\t\t\tstate = path_none;\n> diff --git a/t/t0050-filesystem.sh b/t/t0050-filesystem.sh\n> index 192c94eccd..a840919967 100755\n> --- a/t/t0050-filesystem.sh\n> +++ b/t/t0050-filesystem.sh\n> @@ -131,4 +131,25 @@ $test_unicode 'merge (silent unicode normalization)' '\n> \tgit merge topic\n> '\n> \n> +test_expect_success CASE_INSENSITIVE_FS 'checkout with no pathspec and a case insensitive fs' '\n> +\tgit init repo &&\n> +\t(\n> +\t\tcd repo &&\n> +\n> +\t\t>Gitweb &&\n> +\t\tgit add Gitweb &&\n> +\t\tgit commit -m \"add Gitweb\" &&\n> +\n> +\t\tgit checkout --orphan todo &&\n> +\t\tgit reset --hard &&\n> +\t\tmkdir -p gitweb/subdir &&\n> +\t\t>gitweb/subdir/file &&\n> +\t\t# it is not strictly necessary to add and commit the\n\nProbably not worth a reroll but we're missing \"gitweb directory\" at the\nend of the comment. Other than that, it looks good to me.\n\nThanks again for the prompt fix,\n\nDenton\n\n> +\t\tgit add gitweb &&\n> +\t\tgit commit -m \"add gitweb/subdir/file\" &&\n> +\n> +\t\tgit checkout master\n> +\t)\n> +'\n> +\n> test_done\n> -- \n> 2.23.0.25.g3f4444bfd7.dirty\n> \n"},{"id":"383229","messageId":"CABPp-BEmtnXT9+=+P4-D-0yKM075zstnaovL1P22vfn5FNdfMA@mail.gmail.com","threadId":"51732","inReplyTo":"20191001184049.GA30113@generichostname","subject":"Re: [PATCH v2] dir: special case check for the possibility that pathspec is NULL","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2019-10-01T18:54:59Z","receivedAt":"2019-10-01T18:55:13Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Tue, Oct 1, 2019 at 11:41 AM Denton Liu wrote:\n>\n> Hi Elijah,\n>\n> On Tue, Oct 01, 2019 at 11:30:05AM -0700, Elijah Newren wrote:\n>\n> [...]\n>\n> > diff --git a/dir.c b/dir.c\n> > index 7ff79170fc..bd39b86be4 100644\n> > --- a/dir.c\n> > +++ b/dir.c\n> > @@ -1962,8 +1962,9 @@ static enum path_treatment read_directory_recursive(struct dir_struct *dir,\n> > ((state == path_untracked) &&\n> > (get_dtype(cdir.de, istate, path.buf, path.len) == DT_DIR) &&\n> > ((dir->flags & DIR_SHOW_IGNORED_TOO) ||\n> > - do_match_pathspec(istate, pathspec, path.buf, path.len,\n> > - baselen, NULL, DO_MATCH_LEADING_PATHSPEC) == MATCHED_RECURSIVELY_LEADING_PATHSPEC))) {\n> > + (pathspec &&\n> > + do_match_pathspec(istate, pathspec, path.buf, path.len,\n> > + baselen, NULL, DO_MATCH_LEADING_PATHSPEC) == MATCHED_RECURSIVELY_LEADING_PATHSPEC)))) {\n> > struct untracked_cache_dir *ud;\n> > ud = lookup_untracked(dir->untracked, untracked,\n> > path.buf + baselen,\n> > @@ -1975,7 +1976,8 @@ static enum path_treatment read_directory_recursive(struct dir_struct *dir,\n> > if (subdir_state > dir_state)\n> > dir_state = subdir_state;\n> >\n> > - if (!match_pathspec(istate, pathspec, path.buf, path.len,\n> > + if (pathspec &&\n> > + !match_pathspec(istate, pathspec, path.buf, path.len,\n> > 0 /* prefix */, NULL,\n> > 0 /* do NOT special case dirs */))\n> > state = path_none;\n> > diff --git a/t/t0050-filesystem.sh b/t/t0050-filesystem.sh\n> > index 192c94eccd..a840919967 100755\n> > --- a/t/t0050-filesystem.sh\n> > +++ b/t/t0050-filesystem.sh\n> > @@ -131,4 +131,25 @@ $test_unicode 'merge (silent unicode normalization)' '\n> > git merge topic\n> > '\n> >\n> > +test_expect_success CASE_INSENSITIVE_FS 'checkout with no pathspec and a case insensitive fs' '\n> > + git init repo &&\n> > + (\n> > + cd repo &&\n> > +\n> > + >Gitweb &&\n> > + git add Gitweb &&\n> > + git commit -m \"add Gitweb\" &&\n> > +\n> > + git checkout --orphan todo &&\n> > + git reset --hard &&\n> > + mkdir -p gitweb/subdir &&\n> > + >gitweb/subdir/file &&\n> > + # it is not strictly necessary to add and commit the\n>\n> Probably not worth a reroll but we're missing \"gitweb directory\" at the\n> end of the comment. Other than that, it looks good to me.\n\nYuck, I accidentally only removed half the comment when I intended to\nremove it all? Whoops. I think it's worth a reroll; it's just a\nsingle patch. I'll send it out.\n"},{"id":"383230","messageId":"20191001185524.18772-1-newren@gmail.com","threadId":"51732","inReplyTo":"20191001184049.GA30113@generichostname","subject":"[PATCH v3] dir: special case check for the possibility that pathspec is NULL","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2019-10-01T18:55:24Z","receivedAt":"2019-10-01T18:56:01Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Commits 404ebceda01c (\"dir: also check directories for matching\npathspecs\", 2019-09-17) and 89a1f4aaf765 (\"dir: if our pathspec might\nmatch files under a dir, recurse into it\", 2019-09-17) added calls to\nmatch_pathspec() and do_match_pathspec() passing along their pathspec\nparameter. Both match_pathspec() and do_match_pathspec() assume the\npathspec argument they are given is non-NULL. It turns out that\nunpack-tree.c's verify_clean_subdirectory() calls read_directory() with\npathspec == NULL, and it is possible on case insensitive filesystems for\nthat NULL to make it to these new calls to match_pathspec() and\ndo_match_pathspec(). Add appropriate checks on the NULLness of pathspec\nto avoid a segfault.\n\nIn case the negation throws anyone off (one of the calls was to\ndo_match_pathspec() while the other was to !match_pathspec(), yet no\nnegation of the NULLness of pathspec is used), there are two ways to\nunderstand the differences:\n * The code already handled the pathspec == NULL cases before this\n series, and this series only tried to change behavior when there was\n a pathspec, thus we only want to go into the if-block if pathspec is\n non-NULL.\n * One of the calls is for whether to recurse into a subdirectory, the\n other is for after we've recursed into it for whether we want to\n remove the subdirectory itself (i.e. the subdirectory didn't match\n but something under it could have). That difference in situation\n leads to the slight differences in logic used (well, that and the\n slightly unusual fact that we don't want empty pathspecs to remove\n untracked directories by default).\n\nDenton found and analyzed one issue and provided the patch for the\nmatch_pathspec() call, SZEDER figured out why the issue only reproduced\nfor some folks and not others and provided the testcase, and I looked\nthrough the remainder of the series and noted the do_match_pathspec()\ncall that should have the same check.\n\nCo-authored-by: Denton Liu \nCo-authored-by: SZEDER Gábor \nSigned-off-by: Elijah Newren \n---\nNote: Applies on top of en/clean-nested-with-ignored, in next.\n\nAs with v1, the authorship is really mixed, so I don't know if I\nshould use Co-authored-by (highlighted as a possibility by Denton), or\nthe far more common Helped-by (as suggested by Junio but based on a\nmore limited summary of the different contributions), or if perhaps\nDenton or SZEDER should be marked as the author and I be marked as\nHelped-by or Co-authored-by. Since Denton commented on round 1, I\nused his suggestion for attribution in this round, but I'm open to\nchanging it to whatever works best.\n\nChanges since v2:\n - This time actually removed the entire unnecessary comment\n\nRange-diff:\n1: c495b9303c ! 1: 40392c6bba dir: special case check for the possibility that pathspec is NULL\n @@ t/t0050-filesystem.sh: $test_unicode 'merge (silent unicode normalization)' '\n +\t\tgit reset --hard &&\n +\t\tmkdir -p gitweb/subdir &&\n +\t\t>gitweb/subdir/file &&\n -+\t\t# it is not strictly necessary to add and commit the\n +\t\tgit add gitweb &&\n +\t\tgit commit -m \"add gitweb/subdir/file\" &&\n +\n\n dir.c | 8 +++++---\n t/t0050-filesystem.sh | 21 +++++++++++++++++++++\n 2 files changed, 26 insertions(+), 3 deletions(-)\n\ndiff --git a/dir.c b/dir.c\nindex 7ff79170fc..bd39b86be4 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -1962,8 +1962,9 @@ static enum path_treatment read_directory_recursive(struct dir_struct *dir,\n \t\t\t((state == path_untracked) &&\n \t\t\t (get_dtype(cdir.de, istate, path.buf, path.len) == DT_DIR) &&\n \t\t\t ((dir->flags & DIR_SHOW_IGNORED_TOO) ||\n-\t\t\t do_match_pathspec(istate, pathspec, path.buf, path.len,\n-\t\t\t\t\t baselen, NULL, DO_MATCH_LEADING_PATHSPEC) == MATCHED_RECURSIVELY_LEADING_PATHSPEC))) {\n+\t\t\t (pathspec &&\n+\t\t\t do_match_pathspec(istate, pathspec, path.buf, path.len,\n+\t\t\t\t\t baselen, NULL, DO_MATCH_LEADING_PATHSPEC) == MATCHED_RECURSIVELY_LEADING_PATHSPEC)))) {\n \t\t\tstruct untracked_cache_dir *ud;\n \t\t\tud = lookup_untracked(dir->untracked, untracked,\n \t\t\t\t\t path.buf + baselen,\n@@ -1975,7 +1976,8 @@ static enum path_treatment read_directory_recursive(struct dir_struct *dir,\n \t\t\tif (subdir_state > dir_state)\n \t\t\t\tdir_state = subdir_state;\n \n-\t\t\tif (!match_pathspec(istate, pathspec, path.buf, path.len,\n+\t\t\tif (pathspec &&\n+\t\t\t !match_pathspec(istate, pathspec, path.buf, path.len,\n \t\t\t\t\t 0 /* prefix */, NULL,\n \t\t\t\t\t 0 /* do NOT special case dirs */))\n \t\t\t\tstate = path_none;\ndiff --git a/t/t0050-filesystem.sh b/t/t0050-filesystem.sh\nindex 192c94eccd..a840919967 100755\n--- a/t/t0050-filesystem.sh\n+++ b/t/t0050-filesystem.sh\n@@ -131,4 +131,25 @@ $test_unicode 'merge (silent unicode normalization)' '\n \tgit merge topic\n '\n \n+test_expect_success CASE_INSENSITIVE_FS 'checkout with no pathspec and a case insensitive fs' '\n+\tgit init repo &&\n+\t(\n+\t\tcd repo &&\n+\n+\t\t>Gitweb &&\n+\t\tgit add Gitweb &&\n+\t\tgit commit -m \"add Gitweb\" &&\n+\n+\t\tgit checkout --orphan todo &&\n+\t\tgit reset --hard &&\n+\t\tmkdir -p gitweb/subdir &&\n+\t\t>gitweb/subdir/file &&\n+\t\tgit add gitweb &&\n+\t\tgit commit -m \"add gitweb/subdir/file\" &&\n+\n+\t\tgit checkout master\n+\t)\n+'\n+\n test_done\n-- \n2.23.0.25.g3f4444bfd7.dirty\n\n"},{"id":"383232","messageId":"20191001193510.GA43899@generichostname","threadId":"51732","inReplyTo":"20191001185524.18772-1-newren@gmail.com","subject":"Re: [PATCH v3] dir: special case check for the possibility that pathspec is NULL","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-10-01T19:35:10Z","receivedAt":"2019-10-01T19:35:16Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"Hi Elijah,\n\nSorry for dragging out this thread for so long...\n\nOn Tue, Oct 01, 2019 at 11:55:24AM -0700, Elijah Newren wrote:\n\n[...]\n\n> diff --git a/t/t0050-filesystem.sh b/t/t0050-filesystem.sh\n> index 192c94eccd..a840919967 100755\n> --- a/t/t0050-filesystem.sh\n> +++ b/t/t0050-filesystem.sh\n> @@ -131,4 +131,25 @@ $test_unicode 'merge (silent unicode normalization)' '\n\nI had to change the 25 to a 24 for this to apply cleanly.\n\n> \tgit merge topic\n> '\n> \n> +test_expect_success CASE_INSENSITIVE_FS 'checkout with no pathspec and a case insensitive fs' '\n> +\tgit init repo &&\n> +\t(\n> +\t\tcd repo &&\n> +\n> +\t\t>Gitweb &&\n> +\t\tgit add Gitweb &&\n> +\t\tgit commit -m \"add Gitweb\" &&\n> +\n> +\t\tgit checkout --orphan todo &&\n> +\t\tgit reset --hard &&\n> +\t\tmkdir -p gitweb/subdir &&\n> +\t\t>gitweb/subdir/file &&\n> +\t\tgit add gitweb &&\n> +\t\tgit commit -m \"add gitweb/subdir/file\" &&\n> +\n> +\t\tgit checkout master\n> +\t)\n> +'\n> +\n> test_done\n\nJust wondering, how did you generate this patch? Did you manually edit\nthe last patch and resend it or is this a bug in our diff machinery?\n\n(Side note, I _hate_ how bad the feedback for git apply/am is. We should\nprobably give more information than \"error: corrupt patch at line 62\"\nsuch as why patches are corrupt (unexpected characters, too many/few\nlines, something else?).)\n\n> -- \n> 2.23.0.25.g3f4444bfd7.dirty\n> \n"},{"id":"383233","messageId":"CABPp-BEMNbnncJ=Zv7q+PoMrt7yN0ZxaUVW-rEyjZ9vvXxMHLQ@mail.gmail.com","threadId":"51732","inReplyTo":"20191001193510.GA43899@generichostname","subject":"Re: [PATCH v3] dir: special case check for the possibility that pathspec is NULL","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2019-10-01T19:39:44Z","receivedAt":"2019-10-01T19:40:00Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Tue, Oct 1, 2019 at 12:35 PM Denton Liu wrote:\n>\n> Hi Elijah,\n>\n> Sorry for dragging out this thread for so long...\n>\n> On Tue, Oct 01, 2019 at 11:55:24AM -0700, Elijah Newren wrote:\n>\n> [...]\n>\n> > diff --git a/t/t0050-filesystem.sh b/t/t0050-filesystem.sh\n> > index 192c94eccd..a840919967 100755\n> > --- a/t/t0050-filesystem.sh\n> > +++ b/t/t0050-filesystem.sh\n> > @@ -131,4 +131,25 @@ $test_unicode 'merge (silent unicode normalization)' '\n>\n> I had to change the 25 to a 24 for this to apply cleanly.\n>\n> > git merge topic\n> > '\n> >\n> > +test_expect_success CASE_INSENSITIVE_FS 'checkout with no pathspec and a case insensitive fs' '\n> > + git init repo &&\n> > + (\n> > + cd repo &&\n> > +\n> > + >Gitweb &&\n> > + git add Gitweb &&\n> > + git commit -m \"add Gitweb\" &&\n> > +\n> > + git checkout --orphan todo &&\n> > + git reset --hard &&\n> > + mkdir -p gitweb/subdir &&\n> > + >gitweb/subdir/file &&\n> > + git add gitweb &&\n> > + git commit -m \"add gitweb/subdir/file\" &&\n> > +\n> > + git checkout master\n> > + )\n> > +'\n> > +\n> > test_done\n>\n> Just wondering, how did you generate this patch? Did you manually edit\n> the last patch and resend it or is this a bug in our diff machinery?\n\nI manually edited because it \"was so simple\" and of course just\ncompounded the problem because I didn't fix the count, as you pointed\nout. Gah. Thanks for checking. Clearly, I'm bouncing between too\nmany things this morning, and need to wait until I'm not so distracted\nand rushing so I don't mess things up. I'll sound out a v4 in a few\nhours when I've cleaned a few other things off my plate.\n"},{"id":"383280","messageId":"CABPp-BEwopZr9NRQ51mpd1Ym4q_DT1qsefPS4PBr=8j-C03w=Q@mail.gmail.com","threadId":"51732","inReplyTo":"CABPp-BEMNbnncJ=Zv7q+PoMrt7yN0ZxaUVW-rEyjZ9vvXxMHLQ@mail.gmail.com","subject":"Re: [PATCH v3] dir: special case check for the possibility that pathspec is NULL","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2019-10-02T15:51:10Z","receivedAt":"2019-10-02T15:51:23Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Tue, Oct 1, 2019 at 12:39 PM Elijah Newren wrote:\n>\n> On Tue, Oct 1, 2019 at 12:35 PM Denton Liu wrote:\n> >\n> > Hi Elijah,\n> >\n> > Sorry for dragging out this thread for so long...\n> >\n> > On Tue, Oct 01, 2019 at 11:55:24AM -0700, Elijah Newren wrote:\n> >\n> > [...]\n> >\n> > > diff --git a/t/t0050-filesystem.sh b/t/t0050-filesystem.sh\n> > > index 192c94eccd..a840919967 100755\n> > > --- a/t/t0050-filesystem.sh\n> > > +++ b/t/t0050-filesystem.sh\n> > > @@ -131,4 +131,25 @@ $test_unicode 'merge (silent unicode normalization)' '\n> >\n> > I had to change the 25 to a 24 for this to apply cleanly.\n> >\n> > > git merge topic\n> > > '\n> > >\n> > > +test_expect_success CASE_INSENSITIVE_FS 'checkout with no pathspec and a case insensitive fs' '\n> > > + git init repo &&\n> > > + (\n> > > + cd repo &&\n> > > +\n> > > + >Gitweb &&\n> > > + git add Gitweb &&\n> > > + git commit -m \"add Gitweb\" &&\n> > > +\n> > > + git checkout --orphan todo &&\n> > > + git reset --hard &&\n> > > + mkdir -p gitweb/subdir &&\n> > > + >gitweb/subdir/file &&\n> > > + git add gitweb &&\n> > > + git commit -m \"add gitweb/subdir/file\" &&\n> > > +\n> > > + git checkout master\n> > > + )\n> > > +'\n> > > +\n> > > test_done\n> >\n> > Just wondering, how did you generate this patch? Did you manually edit\n> > the last patch and resend it or is this a bug in our diff machinery?\n>\n> I manually edited because it \"was so simple\" and of course just\n> compounded the problem because I didn't fix the count, as you pointed\n> out. Gah. Thanks for checking. Clearly, I'm bouncing between too\n> many things this morning, and need to wait until I'm not so distracted\n> and rushing so I don't mess things up. I'll sound out a v4 in a few\n> hours when I've cleaned a few other things off my plate.\n\nI was going to send out a new version this morning, but it looks like\nJunio already picked up the patch and fixed it up (the tip of\nen/clean-nested-with-ignored already has what we want), so I won't\nresend after all. Thanks Denton, SZEDER, and Junio.\n"},{"id":"383596","messageId":"20191007180409.GD11529@szeder.dev","threadId":"51732","inReplyTo":"20191001185524.18772-1-newren@gmail.com","subject":"Re: [PATCH v3] dir: special case check for the possibility that pathspec is NULL","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-10-07T18:04:09Z","receivedAt":"2019-10-07T18:04:18Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Tue, Oct 01, 2019 at 11:55:24AM -0700, Elijah Newren wrote:\n> Commits 404ebceda01c (\"dir: also check directories for matching\n> pathspecs\", 2019-09-17) and 89a1f4aaf765 (\"dir: if our pathspec might\n> match files under a dir, recurse into it\", 2019-09-17) added calls to\n> match_pathspec() and do_match_pathspec() passing along their pathspec\n> parameter. Both match_pathspec() and do_match_pathspec() assume the\n> pathspec argument they are given is non-NULL. It turns out that\n> unpack-tree.c's verify_clean_subdirectory() calls read_directory() with\n> pathspec == NULL, and it is possible on case insensitive filesystems for\n> that NULL to make it to these new calls to match_pathspec() and\n> do_match_pathspec(). Add appropriate checks on the NULLness of pathspec\n> to avoid a segfault.\n> \n> In case the negation throws anyone off (one of the calls was to\n> do_match_pathspec() while the other was to !match_pathspec(), yet no\n> negation of the NULLness of pathspec is used), there are two ways to\n> understand the differences:\n> * The code already handled the pathspec == NULL cases before this\n> series, and this series only tried to change behavior when there was\n> a pathspec, thus we only want to go into the if-block if pathspec is\n> non-NULL.\n> * One of the calls is for whether to recurse into a subdirectory, the\n> other is for after we've recursed into it for whether we want to\n> remove the subdirectory itself (i.e. the subdirectory didn't match\n> but something under it could have). That difference in situation\n> leads to the slight differences in logic used (well, that and the\n> slightly unusual fact that we don't want empty pathspecs to remove\n> untracked directories by default).\n> \n> Denton found and analyzed one issue and provided the patch for the\n> match_pathspec() call, SZEDER figured out why the issue only reproduced\n> for some folks and not others and provided the testcase, and I looked\n> through the remainder of the series and noted the do_match_pathspec()\n> call that should have the same check.\n> \n> Co-authored-by: Denton Liu \n> Co-authored-by: SZEDER Gábor \n> Signed-off-by: Elijah Newren \n> ---\n> Note: Applies on top of en/clean-nested-with-ignored, in next.\n> \n> As with v1, the authorship is really mixed, so I don't know if I\n> should use Co-authored-by (highlighted as a possibility by Denton), or\n> the far more common Helped-by (as suggested by Junio but based on a\n> more limited summary of the different contributions), or if perhaps\n> Denton or SZEDER should be marked as the author and I be marked as\n> Helped-by or Co-authored-by. Since Denton commented on round 1, I\n> used his suggestion for attribution in this round, but I'm open to\n> changing it to whatever works best.\n> \n> Changes since v2:\n> - This time actually removed the entire unnecessary comment\n> \n> Range-diff:\n> 1: c495b9303c ! 1: 40392c6bba dir: special case check for the possibility that pathspec is NULL\n> @@ t/t0050-filesystem.sh: $test_unicode 'merge (silent unicode normalization)' '\n> +\t\tgit reset --hard &&\n> +\t\tmkdir -p gitweb/subdir &&\n> +\t\t>gitweb/subdir/file &&\n> -+\t\t# it is not strictly necessary to add and commit the\n> +\t\tgit add gitweb &&\n> +\t\tgit commit -m \"add gitweb/subdir/file\" &&\n> +\n> \n> dir.c | 8 +++++---\n> t/t0050-filesystem.sh | 21 +++++++++++++++++++++\n> 2 files changed, 26 insertions(+), 3 deletions(-)\n> \n> diff --git a/dir.c b/dir.c\n> index 7ff79170fc..bd39b86be4 100644\n> --- a/dir.c\n> +++ b/dir.c\n> @@ -1962,8 +1962,9 @@ static enum path_treatment read_directory_recursive(struct dir_struct *dir,\n> \t\t\t((state == path_untracked) &&\n> \t\t\t (get_dtype(cdir.de, istate, path.buf, path.len) == DT_DIR) &&\n> \t\t\t ((dir->flags & DIR_SHOW_IGNORED_TOO) ||\n> -\t\t\t do_match_pathspec(istate, pathspec, path.buf, path.len,\n> -\t\t\t\t\t baselen, NULL, DO_MATCH_LEADING_PATHSPEC) == MATCHED_RECURSIVELY_LEADING_PATHSPEC))) {\n> +\t\t\t (pathspec &&\n> +\t\t\t do_match_pathspec(istate, pathspec, path.buf, path.len,\n> +\t\t\t\t\t baselen, NULL, DO_MATCH_LEADING_PATHSPEC) == MATCHED_RECURSIVELY_LEADING_PATHSPEC)))) {\n> \t\t\tstruct untracked_cache_dir *ud;\n> \t\t\tud = lookup_untracked(dir->untracked, untracked,\n> \t\t\t\t\t path.buf + baselen,\n> @@ -1975,7 +1976,8 @@ static enum path_treatment read_directory_recursive(struct dir_struct *dir,\n> \t\t\tif (subdir_state > dir_state)\n> \t\t\t\tdir_state = subdir_state;\n> \n> -\t\t\tif (!match_pathspec(istate, pathspec, path.buf, path.len,\n> +\t\t\tif (pathspec &&\n> +\t\t\t !match_pathspec(istate, pathspec, path.buf, path.len,\n> \t\t\t\t\t 0 /* prefix */, NULL,\n> \t\t\t\t\t 0 /* do NOT special case dirs */))\n> \t\t\t\tstate = path_none;\n> diff --git a/t/t0050-filesystem.sh b/t/t0050-filesystem.sh\n> index 192c94eccd..a840919967 100755\n> --- a/t/t0050-filesystem.sh\n> +++ b/t/t0050-filesystem.sh\n> @@ -131,4 +131,25 @@ $test_unicode 'merge (silent unicode normalization)' '\n> \tgit merge topic\n> '\n> \n> +test_expect_success CASE_INSENSITIVE_FS 'checkout with no pathspec and a case insensitive fs' '\n> +\tgit init repo &&\n> +\t(\n> +\t\tcd repo &&\n> +\n> +\t\t>Gitweb &&\n> +\t\tgit add Gitweb &&\n> +\t\tgit commit -m \"add Gitweb\" &&\n> +\n> +\t\tgit checkout --orphan todo &&\n> +\t\tgit reset --hard &&\n> +\t\tmkdir -p gitweb/subdir &&\n> +\t\t>gitweb/subdir/file &&\n> +\t\tgit add gitweb &&\n> +\t\tgit commit -m \"add gitweb/subdir/file\" &&\n> +\n> +\t\tgit checkout master\n> +\t)\n> +'\n\nI don't like this test ;)\n\nI only intended it as a \"here is how to reliably reproduce the\nsegfault without all the clutter of the full git.git repository\" that\nI wrote way past my bedtime. But I think that:\n\n - it shouldn't have the CASE_INSENSITIVE_FS prereq. Yes, that\n segfault could only be triggered on a case insensitive filesystem,\n but the given sequence of commands should succeed in a case\n sensitive file system just as well.\n\n (Have no idea why I added that prereq in the first place; as I\n said above, it was way past my bedtime...)\n\n - it's in the wrong test script; it would be better among other\n tests checking what 'git checkout' should or must not overwrite\n when switching branches, but not sure which test script that is.\n\n (I think I added it to this test script, because it stood out a\n bit when grepping for case insensitive fs in the test suite; I\n play the \"past my bedtime\" card again :)\n\n - it's already satisfied by 'git checkout master' not failing, but\n it doesn't check whether the resulting contents of the worktree\n are as expected.\n\n - it still bothers me why that additional subdir was necessary to\n trigger the segfault. Did you look into it?\n\n"}]}