{"thread":{"id":"36108","subject":"Accidentially deleted directory, bug in git clean -d?","startedAt":"2014-03-10T10:31:37Z","lastAt":"2014-03-10T20:37:30Z","messageCount":6,"participants":["Robin Pedersen","Jeff King","Simon Ruderich"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"236369","messageId":"CABN9-fcpyDogh45WPwuS1qgkE1jLuOAtejuW=fGCKNNyNm3DEQ@mail.gmail.com","threadId":"36108","inReplyTo":null,"subject":"Accidentially deleted directory, bug in git clean -d?","fromName":"Robin Pedersen","fromEmail":"robinp@snap.tv","sentAt":"2014-03-10T10:31:37Z","receivedAt":"2014-03-10T10:31:37Z","isPatch":false,"sender":{"key":"robinp@snap.tv","avatar":null},"body":"I accidentially deleted a directory using git clean. I would think\nthis is a bug, but I'm not sure. Was using 1.8.1, but upgraded to\n1.9.0 just to see if it was still reproducable, and it was.\n\nHere's a minimal way to reproduce:\n\n$ git init\n$ mkdir foo foobar\n$ git clean -df foobar\nRemoving foo/\nRemoving foobar/\n$ ls\n$\n\nI expected only \"foobar\" to be deleted, but \"foo\" was also deleted.\n\nThe same thing happens in the opposite case:\n\n$ git init\n$ mkdir foo foobar\n$ git clean -df foo\nRemoving foo/\nRemoving foobar/\n$ ls\n$\n\nHowever, it only happens when there is a common prefix in the names:\n\n$ git init\n$ mkdir foo bar\n$ git clean -df foo\nRemoving foo/\n$ ls\nbar\n$\n\nIn this case, \"bar\" was not deleted.\n\n-- \nBest regards,\n\nRobin Pedersen\nSoftware Engineer\n\nSnapTV AS\nJordmor Magdalenes vei 17\nN-9519 Kviby.\nNorway\n\nrobinp@snap.tv\nhttp://www.snap.tv\n"},{"id":"236420","messageId":"20140310172002.GB29801@sigill.intra.peff.net","threadId":"36108","inReplyTo":"CABN9-fcpyDogh45WPwuS1qgkE1jLuOAtejuW=fGCKNNyNm3DEQ@mail.gmail.com","subject":"[PATCH] clean: respect pathspecs with \"-d\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-03-10T17:20:02Z","receivedAt":"2014-03-10T17:20:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"git-clean uses read_directory to fill in a `struct dir` with\npotential hits. However, read_directory does not actually\ncheck against our pathspec. It uses a simplified version\nthat may turn up false positives. As a result, we need to\ncheck that any hits match our pathspec. We do so reliably\nfor non-directories. For directories, if \"-d\" is not given\nwe check that the pathspec matched exactly (i.e., we are\neven stricter, and require an explicit \"git clean foo\" to\nclean \"foo/\"). But if \"-d\" is given, rather than relaxing\nthe exact match to allow a recursive match, we do not check\nthe pathspec at all.\n\nThis regression was introduced in 113f10f (Make git-clean a\nbuiltin, 2007-11-11).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nOn Mon, Mar 10, 2014 at 11:31:37AM +0100, Robin Pedersen wrote:\n\n> I accidentially deleted a directory using git clean. I would think\n> this is a bug, but I'm not sure. Was using 1.8.1, but upgraded to\n> 1.9.0 just to see if it was still reproducable, and it was.\n\nDefinitely a bug, and it dates back quite a while.  Thanks for a very\nclear bug report.\n\n-- >8 --\n builtin/clean.c  | 5 +++--\n t/t7300-clean.sh | 8 ++++++++\n 2 files changed, 11 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/clean.c b/builtin/clean.c\nindex 114d7bf..31c1488 100644\n--- a/builtin/clean.c\n+++ b/builtin/clean.c\n@@ -947,14 +947,15 @@ int cmd_clean(int argc, const char **argv, const char *prefix)\n \t\tif (pathspec.nr)\n \t\t\tmatches = dir_path_match(ent, &pathspec, 0, NULL);\n \n+\t\tif (pathspec.nr && !matches)\n+\t\t\tcontinue;\n+\n \t\tif (S_ISDIR(st.st_mode)) {\n \t\t\tif (remove_directories || (matches == MATCHED_EXACTLY)) {\n \t\t\t\trel = relative_path(ent->name, prefix, &buf);\n \t\t\t\tstring_list_append(&del_list, rel);\n \t\t\t}\n \t\t} else {\n-\t\t\tif (pathspec.nr && !matches)\n-\t\t\t\tcontinue;\n \t\t\trel = relative_path(ent->name, prefix, &buf);\n \t\t\tstring_list_append(&del_list, rel);\n \t\t}\ndiff --git a/t/t7300-clean.sh b/t/t7300-clean.sh\nindex 710be90..0c602de 100755\n--- a/t/t7300-clean.sh\n+++ b/t/t7300-clean.sh\n@@ -511,4 +511,12 @@ test_expect_success SANITY 'git clean -d with an unreadable empty directory' '\n \t! test -d foo\n '\n \n+test_expect_success 'git clean -d respects pathspecs' '\n+\tmkdir foo &&\n+\tmkdir foobar &&\n+\tgit clean -df foobar &&\n+\ttest_path_is_dir foo &&\n+\ttest_path_is_missing foobar\n+'\n+\n test_done\n-- \n1.9.0.403.g7a2f4b0\n"},{"id":"236421","messageId":"20140310172215.GC29801@sigill.intra.peff.net","threadId":"36108","inReplyTo":"20140310172002.GB29801@sigill.intra.peff.net","subject":"Re: [PATCH] clean: respect pathspecs with \"-d\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-03-10T17:22:15Z","receivedAt":"2014-03-10T17:22:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Mar 10, 2014 at 01:20:02PM -0400, Jeff King wrote:\n\n> On Mon, Mar 10, 2014 at 11:31:37AM +0100, Robin Pedersen wrote:\n> \n> > I accidentially deleted a directory using git clean. I would think\n> > this is a bug, but I'm not sure. Was using 1.8.1, but upgraded to\n> > 1.9.0 just to see if it was still reproducable, and it was.\n> \n> Definitely a bug, and it dates back quite a while.  Thanks for a very\n> clear bug report.\n> \n> -- >8 --\n\nWhoops, accidentally included a scissors line here that will break\npeople using \"git am --scissors\" to pick up the patch. Here it is\ncorrectly formatted.\n\n-- >8 --\nSubject: clean: respect pathspecs with \"-d\"\n\ngit-clean uses read_directory to fill in a `struct dir` with\npotential hits. However, read_directory does not actually\ncheck against our pathspec. It uses a simplified version\nthat may turn up false positives. As a result, we need to\ncheck that any hits match our pathspec. We do so reliably\nfor non-directories. For directories, if \"-d\" is not given\nwe check that the pathspec matched exactly (i.e., we are\neven stricter, and require an explicit \"git clean foo\" to\nclean \"foo/\"). But if \"-d\" is given, rather than relaxing\nthe exact match to allow a recursive match, we do not check\nthe pathspec at all.\n\nThis regression was introduced in 113f10f (Make git-clean a\nbuiltin, 2007-11-11).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/clean.c  | 5 +++--\n t/t7300-clean.sh | 8 ++++++++\n 2 files changed, 11 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/clean.c b/builtin/clean.c\nindex 114d7bf..31c1488 100644\n--- a/builtin/clean.c\n+++ b/builtin/clean.c\n@@ -947,14 +947,15 @@ int cmd_clean(int argc, const char **argv, const char *prefix)\n \t\tif (pathspec.nr)\n \t\t\tmatches = dir_path_match(ent, &pathspec, 0, NULL);\n \n+\t\tif (pathspec.nr && !matches)\n+\t\t\tcontinue;\n+\n \t\tif (S_ISDIR(st.st_mode)) {\n \t\t\tif (remove_directories || (matches == MATCHED_EXACTLY)) {\n \t\t\t\trel = relative_path(ent->name, prefix, &buf);\n \t\t\t\tstring_list_append(&del_list, rel);\n \t\t\t}\n \t\t} else {\n-\t\t\tif (pathspec.nr && !matches)\n-\t\t\t\tcontinue;\n \t\t\trel = relative_path(ent->name, prefix, &buf);\n \t\t\tstring_list_append(&del_list, rel);\n \t\t}\ndiff --git a/t/t7300-clean.sh b/t/t7300-clean.sh\nindex 710be90..0c602de 100755\n--- a/t/t7300-clean.sh\n+++ b/t/t7300-clean.sh\n@@ -511,4 +511,12 @@ test_expect_success SANITY 'git clean -d with an unreadable empty directory' '\n \t! test -d foo\n '\n \n+test_expect_success 'git clean -d respects pathspecs' '\n+\tmkdir foo &&\n+\tmkdir foobar &&\n+\tgit clean -df foobar &&\n+\ttest_path_is_dir foo &&\n+\ttest_path_is_missing foobar\n+'\n+\n test_done\n-- \n1.9.0.403.g7a2f4b0\n"},{"id":"236422","messageId":"20140310172447.GD29801@sigill.intra.peff.net","threadId":"36108","inReplyTo":"20140310172002.GB29801@sigill.intra.peff.net","subject":"[PATCH] clean: simplify dir/not-dir logic","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-03-10T17:24:47Z","receivedAt":"2014-03-10T17:24:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Mar 10, 2014 at 01:20:02PM -0400, Jeff King wrote:\n\n> git-clean uses read_directory to fill in a `struct dir` with\n> potential hits. However, read_directory does not actually\n> check against our pathspec. It uses a simplified version\n> that may turn up false positives. As a result, we need to\n> check that any hits match our pathspec. We do so reliably\n> for non-directories. For directories, if \"-d\" is not given\n> we check that the pathspec matched exactly (i.e., we are\n> even stricter, and require an explicit \"git clean foo\" to\n> clean \"foo/\"). But if \"-d\" is given, rather than relaxing\n> the exact match to allow a recursive match, we do not check\n> the pathspec at all.\n> \n> This regression was introduced in 113f10f (Make git-clean a\n> builtin, 2007-11-11).\n\nThe code has been cleaned up quite a bit from that original version, and\nit was pretty easy to see the discrepancy between the two code paths.\nHowever, if the code were structured like the cleanup patch below, I\nthink it would have been even easier.\n\nThis comes on top of my other patch. So the bug is already fixed, but I\nthink the end result is more readable.\n\n-- >8 --\nWhen we get a list of paths from read_directory, we further\nprune it to create the final list of items to remove. The\ncode paths for directories and non-directories repeat the\nsame \"add to list\" code.\n\nThis patch restructures the code so that we don't repeat\nourselves. Also, by following a \"if (condition) continue\"\npattern like the pathspec check above, it makes it more\nobvious that the conditional is about excluding directories\nunder certain circumstances.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/clean.c | 15 ++++++---------\n 1 file changed, 6 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin/clean.c b/builtin/clean.c\nindex 31c1488..cf76b1f 100644\n--- a/builtin/clean.c\n+++ b/builtin/clean.c\n@@ -950,15 +950,12 @@ int cmd_clean(int argc, const char **argv, const char *prefix)\n \t\tif (pathspec.nr && !matches)\n \t\t\tcontinue;\n \n-\t\tif (S_ISDIR(st.st_mode)) {\n-\t\t\tif (remove_directories || (matches == MATCHED_EXACTLY)) {\n-\t\t\t\trel = relative_path(ent->name, prefix, &buf);\n-\t\t\t\tstring_list_append(&del_list, rel);\n-\t\t\t}\n-\t\t} else {\n-\t\t\trel = relative_path(ent->name, prefix, &buf);\n-\t\t\tstring_list_append(&del_list, rel);\n-\t\t}\n+\t\tif (S_ISDIR(st.st_mode) && !remove_directories &&\n+\t\t    matches != MATCHED_EXACTLY)\n+\t\t\tcontinue;\n+\n+\t\trel = relative_path(ent->name, prefix, &buf);\n+\t\tstring_list_append(&del_list, rel);\n \t}\n \n \tif (interactive && del_list.nr > 0)\n-- \n1.9.0.403.g7a2f4b0\n"},{"id":"236449","messageId":"20140310200235.GA14112@ruderich.org","threadId":"36108","inReplyTo":"20140310172215.GC29801@sigill.intra.peff.net","subject":"Re: [PATCH] clean: respect pathspecs with \"-d\"","fromName":"Simon Ruderich","fromEmail":"simon@ruderich.org","sentAt":"2014-03-10T20:02:35Z","receivedAt":"2014-03-10T20:02:35Z","isPatch":true,"sender":{"key":"simon@ruderich.org","avatar":"https://avatars.githubusercontent.com/u/390994?v=4"},"body":"On Mon, Mar 10, 2014 at 01:22:15PM -0400, Jeff King wrote:\n> +test_expect_success 'git clean -d respects pathspecs' '\n> +\tmkdir foo &&\n> +\tmkdir foobar &&\n> +\tgit clean -df foobar &&\n> +\ttest_path_is_dir foo &&\n> +\ttest_path_is_missing foobar\n> +'\n> +\n>  test_done\n\nI think we should also test removing foo, which was also in the\noriginal report, to make sure we don't match prefixes, e.g.:\n\ntest_expect_success 'git clean -d respects pathspecs' '\n\tmkdir foo &&\n\tmkdir foobar &&\n\tgit clean -df foo &&\n\ttest_path_is_missing foo &&\n\ttest_path_is_dir foobar\n'\n\nRegards\nSimon\n-- \n+ privacy is necessary\n+ using gnupg http://gnupg.org\n+ public key id: 0x92FEFDB7E44C32F9\n"},{"id":"236457","messageId":"20140310203730.GD24568@sigill.intra.peff.net","threadId":"36108","inReplyTo":"20140310200235.GA14112@ruderich.org","subject":"Re: [PATCH] clean: respect pathspecs with \"-d\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-03-10T20:37:30Z","receivedAt":"2014-03-10T20:37:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Mar 10, 2014 at 09:02:35PM +0100, Simon Ruderich wrote:\n\n> On Mon, Mar 10, 2014 at 01:22:15PM -0400, Jeff King wrote:\n> > +test_expect_success 'git clean -d respects pathspecs' '\n> > +\tmkdir foo &&\n> > +\tmkdir foobar &&\n> > +\tgit clean -df foobar &&\n> > +\ttest_path_is_dir foo &&\n> > +\ttest_path_is_missing foobar\n> > +'\n> > +\n> >  test_done\n> \n> I think we should also test removing foo, which was also in the\n> original report, to make sure we don't match prefixes, e.g.:\n> \n> test_expect_success 'git clean -d respects pathspecs' '\n> \tmkdir foo &&\n> \tmkdir foobar &&\n> \tgit clean -df foo &&\n> \ttest_path_is_missing foo &&\n> \ttest_path_is_dir foobar\n> '\n\nYeah, it probably makes sense to test both ways (though the root cause\nand fix are the same). Those mkdirs need to be \"mkdir -p\", though.\n\nHere's an updated patch with your suggestion:\n\n-- >8 --\nSubject: clean: respect pathspecs with \"-d\"\n\ngit-clean uses read_directory to fill in a `struct dir` with\npotential hits. However, read_directory does not actually\ncheck against our pathspec. It uses a simplified version\nthat may turn up false positives. As a result, we need to\ncheck that any hits match our pathspec. We do so reliably\nfor non-directories. For directories, if \"-d\" is not given\nwe check that the pathspec matched exactly (i.e., we are\neven stricter, and require an explicit \"git clean foo\" to\nclean \"foo/\"). But if \"-d\" is given, rather than relaxing\nthe exact match to allow a recursive match, we do not check\nthe pathspec at all.\n\nThis regression was introduced in 113f10f (Make git-clean a\nbuiltin, 2007-11-11).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/clean.c  |  5 +++--\n t/t7300-clean.sh | 16 ++++++++++++++++\n 2 files changed, 19 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/clean.c b/builtin/clean.c\nindex 114d7bf..31c1488 100644\n--- a/builtin/clean.c\n+++ b/builtin/clean.c\n@@ -947,14 +947,15 @@ int cmd_clean(int argc, const char **argv, const char *prefix)\n \t\tif (pathspec.nr)\n \t\t\tmatches = dir_path_match(ent, &pathspec, 0, NULL);\n \n+\t\tif (pathspec.nr && !matches)\n+\t\t\tcontinue;\n+\n \t\tif (S_ISDIR(st.st_mode)) {\n \t\t\tif (remove_directories || (matches == MATCHED_EXACTLY)) {\n \t\t\t\trel = relative_path(ent->name, prefix, &buf);\n \t\t\t\tstring_list_append(&del_list, rel);\n \t\t\t}\n \t\t} else {\n-\t\t\tif (pathspec.nr && !matches)\n-\t\t\t\tcontinue;\n \t\t\trel = relative_path(ent->name, prefix, &buf);\n \t\t\tstring_list_append(&del_list, rel);\n \t\t}\ndiff --git a/t/t7300-clean.sh b/t/t7300-clean.sh\nindex 710be90..74de814 100755\n--- a/t/t7300-clean.sh\n+++ b/t/t7300-clean.sh\n@@ -511,4 +511,20 @@ test_expect_success SANITY 'git clean -d with an unreadable empty directory' '\n \t! test -d foo\n '\n \n+test_expect_success 'git clean -d respects pathspecs (dir is prefix of pathspec)' '\n+\tmkdir -p foo &&\n+\tmkdir -p foobar &&\n+\tgit clean -df foobar &&\n+\ttest_path_is_dir foo &&\n+\ttest_path_is_missing foobar\n+'\n+\n+test_expect_success 'git clean -d respects pathspecs (pathspec is prefix of dir)' '\n+\tmkdir -p foo &&\n+\tmkdir -p foobar &&\n+\tgit clean -df foo &&\n+\ttest_path_is_missing foo &&\n+\ttest_path_is_dir foobar\n+'\n+\n test_done\n-- \n1.9.0.403.g7a2f4b0\n"}]}