{"thread":{"id":"13022","subject":"git clean removes directories when not asked to","startedAt":"2008-04-08T18:22:56Z","lastAt":"2008-04-15T14:46:13Z","messageCount":14,"participants":["Joachim B Haga","Shawn Bohrer","Joachim Berdal Haga","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"73860","messageId":"85prt0jjen.fsf@lupus.strangled.net","threadId":"13022","inReplyTo":null,"subject":"git clean removes directories when not asked to","fromName":"Joachim B Haga","fromEmail":"jobh@broadpark.no","sentAt":"2008-04-08T18:22:56Z","receivedAt":"2008-04-08T18:22:56Z","isPatch":false,"sender":{"key":"jobh@broadpark.no","avatar":null},"body":"This is with debian packaged 1.5.4.4.\n\nWhen invoked from a subdirectory, git clean removes more than it\nshould. According to the documentation, it should not remove\ndirectories unless \"-d\" is given. However:\n\n\npep ~/src/test 0$ git init\nInitialized empty Git repository in .git/\npep ~/src/test|master 0$ mkdir dir\npep ~/src/test|master 0$ mkdir dir/subdir\npep ~/src/test|master 0$ git clean -f\nNot removing dir/\npep ~/src/test|master 0$ cd dir\npep ~/src/test/dir|master 0$ git clean -f\nRemoving subdir/\npep ~/src/test/dir|master 0$ ls subdir\nls: cannot access subdir: No such file or directory\n\n\nLuckily I just lost some compilation results in this case, but this is\nunexpected and dangerous behaviour.\n\n\n(Additionally, I find the \"-f\" slightly annoying but that's not an issue here.)\n\n\n-j\n"},{"id":"73861","messageId":"85k5j8jioc.fsf@lupus.strangled.net","threadId":"13022","inReplyTo":"85prt0jjen.fsf@lupus.strangled.net","subject":"Re: git clean removes directories when not asked to","fromName":"Joachim B Haga","fromEmail":"jobh@broadpark.no","sentAt":"2008-04-08T18:38:43Z","receivedAt":"2008-04-08T18:38:43Z","isPatch":false,"sender":{"key":"jobh@broadpark.no","avatar":null},"body":"Joachim B Haga writes:\n\n> This is with debian packaged 1.5.4.4.\n>\n> When invoked from a subdirectory, git clean removes more than it\n> should. According to the documentation, it should not remove\n> directories unless \"-d\" is given. However:\n\nI see the same behaviour with 1.5.5, just pulled.\n\n-j.\n"},{"id":"73967","messageId":"85fxtvj6y8.fsf_-_@lupus.strangled.net","threadId":"13022","inReplyTo":"85k5j8jioc.fsf@lupus.strangled.net","subject":"[PATCH] Re: git clean removes directories when not asked to","fromName":"Joachim B Haga","fromEmail":"jobh@broadpark.no","sentAt":"2008-04-09T17:04:15Z","receivedAt":"2008-04-09T17:04:15Z","isPatch":true,"sender":{"key":"jobh@broadpark.no","avatar":null},"body":"Joachim B Haga writes:\n\n> Joachim B Haga writes:\n>\n>> When invoked from a subdirectory, git clean removes more than it\n>> should. According to the documentation, it should not remove\n>> directories unless \"-d\" is given. However:\n\nI have tried to fix this, but I don't know the code. The previous logic was\nobviously (?) broken, as it had this (paraphrased):\n\nif (remove_directories || matches)\n\tremove_dir_recursively(...);\n\nwhich should have been &&. But with only this change, top-level directories\nwere not removed even if \"-d\" was given. Looking at the (!ISDIR) branch, I\nguessed that it should instead trigger if pathspec is NULL; i.e, generally\ntreat (!pathspec) as a match. It looks like the behaviour is correct now, but\nsomebody who knows this code should check my guesses.\n\n-j.\n\n\n\n\n>From 73647e7bb73b6037b9d14535ec027da8ee7d6091 Mon Sep 17 00:00:00 2001\nFrom: Joachim B Haga \nDate: Wed, 9 Apr 2008 18:49:34 +0200\nSubject: [PATCH] Stop builtin-clean from removing directories unless \"-d\" is given.\n\n---\n builtin-clean.c | 29 ++++++++++++++++-------------\n 1 files changed, 16 insertions(+), 13 deletions(-)\n\ndiff --git a/builtin-clean.c b/builtin-clean.c\nindex fefec30..15201d5 100644\n--- a/builtin-clean.c\n+++ b/builtin-clean.c\n@@ -130,29 +130,32 @@ int cmd_clean(int argc, const char **argv, const char *prefix)\n matches = match_pathspec(pathspec, ent->name, ent->len,\n baselen, seen);\n } else {\n- matches = 0;\n+ matches = 1;\n }\n \n if (S_ISDIR(st.st_mode)) {\n strbuf_addstr(&directory, ent->name);\n qname = quote_path_relative(directory.buf, directory.len, &buf, prefix);\n- if (show_only && (remove_directories || matches)) {\n- printf(\"Would remove %s\\n\", qname);\n- } else if (remove_directories || matches) {\n- if (!quiet)\n- printf(\"Removing %s\\n\", qname);\n- if (remove_dir_recursively(&directory, 0) != 0) {\n- warning(\"failed to remove '%s'\", qname);\n- errors++;\n+ if (remove_directories && matches) {\n+ if (show_only)\n+ printf(\"Would remove %s\\n\", qname);\n+ else {\n+ if (!quiet)\n+ printf(\"Removing %s\\n\", qname);\n+ if (remove_dir_recursively(&directory, 0) != 0) {\n+ warning(\"failed to remove '%s'\", qname);\n+ errors++;\n+ }\n }\n- } else if (show_only) {\n- printf(\"Would not remove %s\\n\", qname);\n } else {\n- printf(\"Not removing %s\\n\", qname);\n+ if (show_only)\n+ printf(\"Would not remove %s\\n\", qname);\n+ else\n+ printf(\"Not removing %s\\n\", qname);\n }\n strbuf_reset(&directory);\n } else {\n- if (pathspec && !matches)\n+ if (!matches)\n continue;\n qname = quote_path_relative(ent->name, -1, &buf, prefix);\n if (show_only) {\n-- \n1.5.4.4\n"},{"id":"74274","messageId":"1208130578-24748-1-git-send-email-shawn.bohrer@gmail.com","threadId":"13022","inReplyTo":"85fxtvj6y8.fsf_-_@lupus.strangled.net","subject":"[PATCH] git clean: Don't automatically remove directories when run within subdirectory","fromName":"Shawn Bohrer","fromEmail":"shawn.bohrer@gmail.com","sentAt":"2008-04-13T23:49:37Z","receivedAt":"2008-04-13T23:49:37Z","isPatch":true,"sender":{"key":"shawn.bohrer@gmail.com","avatar":"https://gravatar.com/avatar/6eb093ef7d276306d18366254e0c95ff6a5db58231ac7e82fe78c2800aaae1b6?d=mp&s=160"},"body":"When git clean is run from a subdirectory it should follow the normal\npolicy and only remove directories if they are passed in as a pathspec,\nor -d is specified.\n\nSigned-off-by: Shawn Bohrer \n---\n\n\nOn Wed, Apr 09, 2008 at 07:04:15PM +0200, Joachim B Haga wrote:\n> Joachim B Haga writes:\n> I have tried to fix this, but I don't know the code. The previous logic was\n> obviously (?) broken, as it had this (paraphrased):\n>\n> if (remove_directories || matches)\n> remove_dir_recursively(...);\n\ngit clean will remove directories if you specify -d (setting the\nremove_directories flag), or if you explicitly passed the directory in\nas a pathspec. For example:\n\nmkdir foo\ngit clean foo\n\nAfter looking at the code a bit I think the real problem is with\nmatch_pathspec, or at least with how git clean uses match_pathspec.\nHow does the following patch look?\n\n\n\n builtin-clean.c | 11 +++++------\n dir.c | 2 +-\n 2 files changed, 6 insertions(+), 7 deletions(-)\n\ndiff --git a/builtin-clean.c b/builtin-clean.c\nindex fefec30..5c5ec98 100644\n--- a/builtin-clean.c\n+++ b/builtin-clean.c\n@@ -95,7 +95,8 @@ int cmd_clean(int argc, const char **argv, const char *prefix)\n \n \tfor (i = 0; i < dir.nr; i++) {\n \t\tstruct dir_entry *ent = dir.entries[i];\n-\t\tint len, pos, matches;\n+\t\tint len, pos;\n+\t\tint matches = 0;\n \t\tstruct cache_entry *ce;\n \t\tstruct stat st;\n \n@@ -127,18 +128,16 @@ int cmd_clean(int argc, const char **argv, const char *prefix)\n \n \t\tif (pathspec) {\n \t\t\tmemset(seen, 0, argc > 0 ? argc : 1);\n-\t\t\tmatches = match_pathspec(pathspec, ent->name, ent->len,\n+\t\t\tmatches = match_pathspec(pathspec, ent->name, len,\n \t\t\t\t\t\t baselen, seen);\n-\t\t} else {\n-\t\t\tmatches = 0;\n \t\t}\n \n \t\tif (S_ISDIR(st.st_mode)) {\n \t\t\tstrbuf_addstr(&directory, ent->name);\n \t\t\tqname = quote_path_relative(directory.buf, directory.len, &buf, prefix);\n-\t\t\tif (show_only && (remove_directories || matches)) {\n+\t\t\tif (show_only && (remove_directories || (matches >= 2))) {\n \t\t\t\tprintf(\"Would remove %s\\n\", qname);\n-\t\t\t} else if (remove_directories || matches) {\n+\t\t\t} else if (remove_directories || (matches >= 2)) {\n \t\t\t\tif (!quiet)\n \t\t\t\t\tprintf(\"Removing %s\\n\", qname);\n \t\t\t\tif (remove_dir_recursively(&directory, 0) != 0) {\ndiff --git a/dir.c b/dir.c\nindex b5bfbca..63715c9 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -80,7 +80,7 @@ static int match_one(const char *match, const char *name, int namelen)\n \tif (strncmp(match, name, matchlen))\n \t\treturn !fnmatch(match, name, 0) ? MATCHED_FNMATCH : 0;\n \n-\tif (!name[matchlen])\n+\tif (namelen == matchlen)\n \t\treturn MATCHED_EXACTLY;\n \tif (match[matchlen-1] == '/' || name[matchlen] == '/')\n \t\treturn MATCHED_RECURSIVELY;\n-- \n1.5.5.106.g42c8b\n"},{"id":"74275","messageId":"1208130578-24748-2-git-send-email-shawn.bohrer@gmail.com","threadId":"13022","inReplyTo":"1208130578-24748-1-git-send-email-shawn.bohrer@gmail.com","subject":"[PATCH] git clean: Add test to verify directories aren't removed with a prefix","fromName":"Shawn Bohrer","fromEmail":"shawn.bohrer@gmail.com","sentAt":"2008-04-13T23:49:38Z","receivedAt":"2008-04-13T23:49:38Z","isPatch":true,"sender":{"key":"shawn.bohrer@gmail.com","avatar":"https://gravatar.com/avatar/6eb093ef7d276306d18366254e0c95ff6a5db58231ac7e82fe78c2800aaae1b6?d=mp&s=160"},"body":"Signed-off-by: Shawn Bohrer \n---\n t/t7300-clean.sh | 5 +++--\n 1 files changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t7300-clean.sh b/t/t7300-clean.sh\nindex afccfc9..a50492f 100755\n--- a/t/t7300-clean.sh\n+++ b/t/t7300-clean.sh\n@@ -75,8 +75,8 @@ test_expect_success 'git-clean src/ src/' '\n \n test_expect_success 'git-clean with prefix' '\n \n-\tmkdir -p build docs &&\n-\ttouch a.out src/part3.c docs/manual.txt obj.o build/lib.so &&\n+\tmkdir -p build docs src/test &&\n+\ttouch a.out src/part3.c docs/manual.txt obj.o build/lib.so src/test/1.c &&\n \t(cd src/ && git-clean) &&\n \ttest -f Makefile &&\n \ttest -f README &&\n@@ -84,6 +84,7 @@ test_expect_success 'git-clean with prefix' '\n \ttest -f src/part2.c &&\n \ttest -f a.out &&\n \ttest ! -f src/part3.c &&\n+\ttest -f src/test/1.c &&\n \ttest -f docs/manual.txt &&\n \ttest -f obj.o &&\n \ttest -f build/lib.so\n-- \n1.5.5.106.g42c8b\n"},{"id":"74321","messageId":"480301C7.3010702@broadpark.no","threadId":"13022","inReplyTo":"1208130578-24748-1-git-send-email-shawn.bohrer@gmail.com","subject":"Re: [PATCH] git clean: Don't automatically remove directories when run within subdirectory","fromName":"Joachim Berdal Haga","fromEmail":"jobh@broadpark.no","sentAt":"2008-04-14T07:03:35Z","receivedAt":"2008-04-14T07:03:35Z","isPatch":true,"sender":{"key":"jobh@broadpark.no","avatar":null},"body":"Shawn Bohrer wrote:\n> When git clean is run from a subdirectory it should follow the normal\n> policy and only remove directories if they are passed in as a pathspec,\n> or -d is specified.\n> \n> Signed-off-by: Shawn Bohrer \n\nI have tested this version and it fixes my problem/testcase. Thanks,\n\n-j.\n"},{"id":"74324","messageId":"7v8wzgaoqy.fsf@gitster.siamese.dyndns.org","threadId":"13022","inReplyTo":"1208130578-24748-1-git-send-email-shawn.bohrer@gmail.com","subject":"Re: [PATCH] git clean: Don't automatically remove directories when run within subdirectory","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-04-14T07:18:13Z","receivedAt":"2008-04-14T07:18:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Shawn Bohrer writes:\n\n> diff --git a/builtin-clean.c b/builtin-clean.c\n> index fefec30..5c5ec98 100644\n> --- a/builtin-clean.c\n> +++ b/builtin-clean.c\n> @@ -95,7 +95,8 @@ int cmd_clean(int argc, const char **argv, const char *prefix)\n> \n> \tfor (i = 0; i < dir.nr; i++) {\n> \t\tstruct dir_entry *ent = dir.entries[i];\n> -\t\tint len, pos, matches;\n> +\t\tint len, pos;\n> +\t\tint matches = 0;\n> \t\tstruct cache_entry *ce;\n> \t\tstruct stat st;\n\nInitialization of \"matches\" seems to be an independent clean-up. Although\nit forces the initialization in the codepath that do not need the value of\nmatches, that is not a big deal --- right?\n\n> @@ -127,18 +128,16 @@ int cmd_clean(int argc, const char **argv, const char *prefix)\n> \n> \t\tif (pathspec) {\n> \t\t\tmemset(seen, 0, argc > 0 ? argc : 1);\n> -\t\t\tmatches = match_pathspec(pathspec, ent->name, ent->len,\n> +\t\t\tmatches = match_pathspec(pathspec, ent->name, len,\n> \t\t\t\t\t\t baselen, seen);\n> -\t\t} else {\n> -\t\t\tmatches = 0;\n> \t\t}\n\nAnd the essential change (fix) is to send len which could be shorter than\nent->len because we have stripped '/' here, plus the one in match_one()\nthat now allows name[] that is not NUL terminated.\n\n> \t\tif (S_ISDIR(st.st_mode)) {\n> \t\t\tstrbuf_addstr(&directory, ent->name);\n> \t\t\tqname = quote_path_relative(directory.buf, directory.len, &buf, prefix);\n> -\t\t\tif (show_only && (remove_directories || matches)) {\n> +\t\t\tif (show_only && (remove_directories || (matches >= 2))) {\n> \t\t\t\tprintf(\"Would remove %s\\n\", qname);\n> -\t\t\t} else if (remove_directories || matches) {\n> +\t\t\t} else if (remove_directories || (matches >= 2)) {\n\nThese magic numbers are bad. Please update it to use symbolic constants.\n\n> \t\t\t\tif (!quiet)\n> \t\t\t\t\tprintf(\"Removing %s\\n\", qname);\n> \t\t\t\tif (remove_dir_recursively(&directory, 0) != 0) {\n> diff --git a/dir.c b/dir.c\n> index b5bfbca..63715c9 100644\n> --- a/dir.c\n> +++ b/dir.c\n> @@ -80,7 +80,7 @@ static int match_one(const char *match, const char *name, int namelen)\n> \tif (strncmp(match, name, matchlen))\n> \t\treturn !fnmatch(match, name, 0) ? MATCHED_FNMATCH : 0;\n> \n> -\tif (!name[matchlen])\n> +\tif (namelen == matchlen)\n> \t\treturn MATCHED_EXACTLY;\n> \tif (match[matchlen-1] == '/' || name[matchlen] == '/')\n> \t\treturn MATCHED_RECURSIVELY;\n"},{"id":"74366","messageId":"20080414170643.GA10548@mediacenter","threadId":"13022","inReplyTo":"7v8wzgaoqy.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] git clean: Don't automatically remove directories when run within subdirectory","fromName":"Shawn Bohrer","fromEmail":"shawn.bohrer@gmail.com","sentAt":"2008-04-14T17:06:43Z","receivedAt":"2008-04-14T17:06:43Z","isPatch":true,"sender":{"key":"shawn.bohrer@gmail.com","avatar":"https://gravatar.com/avatar/6eb093ef7d276306d18366254e0c95ff6a5db58231ac7e82fe78c2800aaae1b6?d=mp&s=160"},"body":"On Mon, Apr 14, 2008 at 12:18:13AM -0700, Junio C Hamano wrote:\n> Shawn Bohrer writes:\n> > -\t\tint len, pos, matches;\n> > +\t\tint len, pos;\n> > +\t\tint matches = 0;\n> > \t\tstruct cache_entry *ce;\n> > \t\tstruct stat st;\n> \n> Initialization of \"matches\" seems to be an independent clean-up. Although\n> it forces the initialization in the codepath that do not need the value of\n> matches, that is not a big deal --- right?\n\nYes this is an independent clean-up. I can't see any harm in forcing\nthe initializtion.\n\n> > -\t\t\tmatches = match_pathspec(pathspec, ent->name, ent->len,\n> > +\t\t\tmatches = match_pathspec(pathspec, ent->name, len,\n> > \t\t\t\t\t\t baselen, seen);\n> > -\t\t} else {\n> > -\t\t\tmatches = 0;\n> > \t\t}\n> \n> And the essential change (fix) is to send len which could be shorter than\n> ent->len because we have stripped '/' here, plus the one in match_one()\n> that now allows name[] that is not NUL terminated.\n\nYep, I'll add that to the changelog.\n\n> > -\t\t\tif (show_only && (remove_directories || matches)) {\n> > +\t\t\tif (show_only && (remove_directories || (matches >= 2))) {\n> > \t\t\t\tprintf(\"Would remove %s\\n\", qname);\n> > -\t\t\t} else if (remove_directories || matches) {\n> > +\t\t\t} else if (remove_directories || (matches >= 2)) {\n> \n> These magic numbers are bad. Please update it to use symbolic constants.\n\nAgreed I'll send an updated patch later tonight. One additional thought\nthough. 2 is MATCHED_FNMATCH which worries me a little because I think\nthis would mean 'git clean -f *' will also remove directories (I haven't\ntried though). Perhaps this should really be 3 MATCHED_EXACTLY just to\nbe safe. Does anyone have opinions either way?\n\n--\nShawn\n"},{"id":"74369","messageId":"48039FE5.5060309@broadpark.no","threadId":"13022","inReplyTo":"20080414170643.GA10548@mediacenter","subject":"Re: [PATCH] git clean: Don't automatically remove directories when run within subdirectory","fromName":"Joachim Berdal Haga","fromEmail":"cjbhaga@broadpark.no","sentAt":"2008-04-14T18:18:13Z","receivedAt":"2008-04-14T18:18:13Z","isPatch":true,"sender":{"key":"cjbhaga@broadpark.no","avatar":null},"body":"Shawn Bohrer wrote:\n> Agreed I'll send an updated patch later tonight. One additional thought\n> though. 2 is MATCHED_FNMATCH which worries me a little because I think\n> this would mean 'git clean -f *' will also remove directories (I haven't\n> tried though). Perhaps this should really be 3 MATCHED_EXACTLY just to\n> be safe. Does anyone have opinions either way?\n\nI don't have strong opinions on this since I don't use this form of the\ncommand, but still:\n\nI think that the best option would be to never remove a directory, even if\ngiven explicitly, unless -d is given. Because my gut feeling is that when a\ndirectory name is specified, it is most often meant as \"clean inside the\ngiven directory\", ie. as a path delimiter. Indeed, if the directory has\ntracked files inside of it,\n git clean dir\nand\n git clean dir/\nhave the same effect. If there are no tracked files inside, the current\npatch gives the path-delimiting effect on this form\n git clean dir/\nbut removes the whole directory irrespective of \"-d\" for this form\n git clean dir\nI think that a \"honor (lack of) -d even if pathspec matches\" would reduce\nthe consequences of this particular kind of user error (by deleting too\nlittle instead of too much).\n\n-j.\n"},{"id":"74408","messageId":"1208229249-32033-1-git-send-email-shawn.bohrer@gmail.com","threadId":"13022","inReplyTo":"20080414170643.GA10548@mediacenter","subject":"[PATCH] git clean: Don't automatically remove directories when run within subdirectory","fromName":"Shawn Bohrer","fromEmail":"shawn.bohrer@gmail.com","sentAt":"2008-04-15T03:14:09Z","receivedAt":"2008-04-15T03:14:09Z","isPatch":true,"sender":{"key":"shawn.bohrer@gmail.com","avatar":"https://gravatar.com/avatar/6eb093ef7d276306d18366254e0c95ff6a5db58231ac7e82fe78c2800aaae1b6?d=mp&s=160"},"body":"When git clean is run from a subdirectory it should follow the normal\npolicy and only remove directories if they are passed in as a pathspec,\nor -d is specified.\n\nThe fix is to send len which could be shorter than ent->len because we\nhave stripped the trailing '/' that read_directory adds. Additionaly\nmatch_one() was modified to allow a name[] that is not NUL terminated.\nThis allows us to check if the name matched the pathspec exactly\ninstead of recursively.\n\nSigned-off-by: Shawn Bohrer \n---\n builtin-clean.c | 13 +++++++------\n dir.c | 2 +-\n 2 files changed, 8 insertions(+), 7 deletions(-)\n\ndiff --git a/builtin-clean.c b/builtin-clean.c\nindex fefec30..6778a03 100644\n--- a/builtin-clean.c\n+++ b/builtin-clean.c\n@@ -95,7 +95,8 @@ int cmd_clean(int argc, const char **argv, const char *prefix)\n \n \tfor (i = 0; i < dir.nr; i++) {\n \t\tstruct dir_entry *ent = dir.entries[i];\n-\t\tint len, pos, matches;\n+\t\tint len, pos;\n+\t\tint matches = 0;\n \t\tstruct cache_entry *ce;\n \t\tstruct stat st;\n \n@@ -127,18 +128,18 @@ int cmd_clean(int argc, const char **argv, const char *prefix)\n \n \t\tif (pathspec) {\n \t\t\tmemset(seen, 0, argc > 0 ? argc : 1);\n-\t\t\tmatches = match_pathspec(pathspec, ent->name, ent->len,\n+\t\t\tmatches = match_pathspec(pathspec, ent->name, len,\n \t\t\t\t\t\t baselen, seen);\n-\t\t} else {\n-\t\t\tmatches = 0;\n \t\t}\n \n \t\tif (S_ISDIR(st.st_mode)) {\n \t\t\tstrbuf_addstr(&directory, ent->name);\n \t\t\tqname = quote_path_relative(directory.buf, directory.len, &buf, prefix);\n-\t\t\tif (show_only && (remove_directories || matches)) {\n+\t\t\tif (show_only && (remove_directories ||\n+\t\t\t (matches == MATCHED_EXACTLY))) {\n \t\t\t\tprintf(\"Would remove %s\\n\", qname);\n-\t\t\t} else if (remove_directories || matches) {\n+\t\t\t} else if (remove_directories ||\n+\t\t\t\t (matches == MATCHED_EXACTLY)) {\n \t\t\t\tif (!quiet)\n \t\t\t\t\tprintf(\"Removing %s\\n\", qname);\n \t\t\t\tif (remove_dir_recursively(&directory, 0) != 0) {\ndiff --git a/dir.c b/dir.c\nindex b5bfbca..63715c9 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -80,7 +80,7 @@ static int match_one(const char *match, const char *name, int namelen)\n \tif (strncmp(match, name, matchlen))\n \t\treturn !fnmatch(match, name, 0) ? MATCHED_FNMATCH : 0;\n \n-\tif (!name[matchlen])\n+\tif (namelen == matchlen)\n \t\treturn MATCHED_EXACTLY;\n \tif (match[matchlen-1] == '/' || name[matchlen] == '/')\n \t\treturn MATCHED_RECURSIVELY;\n-- \n1.5.5.106.g62ee2.dirty\n"},{"id":"74411","messageId":"20080415034417.GA2882@lintop","threadId":"13022","inReplyTo":"48039FE5.5060309@broadpark.no","subject":"Re: [PATCH] git clean: Don't automatically remove directories when run within subdirectory","fromName":"Shawn Bohrer","fromEmail":"shawn.bohrer@gmail.com","sentAt":"2008-04-15T03:44:17Z","receivedAt":"2008-04-15T03:44:17Z","isPatch":true,"sender":{"key":"shawn.bohrer@gmail.com","avatar":"https://gravatar.com/avatar/6eb093ef7d276306d18366254e0c95ff6a5db58231ac7e82fe78c2800aaae1b6?d=mp&s=160"},"body":"On Mon, Apr 14, 2008 at 08:18:13PM +0200, Joachim Berdal Haga wrote:\n> I think that the best option would be to never remove a directory, even if\n> given explicitly, unless -d is given. Because my gut feeling is that when a\n> directory name is specified, it is most often meant as \"clean inside the\n> given directory\", ie. as a path delimiter. Indeed, if the directory has\n> tracked files inside of it,\n> git clean dir\n> and\n> git clean dir/\n> have the same effect. If there are no tracked files inside, the current\n> patch gives the path-delimiting effect on this form\n> git clean dir/\n> but removes the whole directory irrespective of \"-d\" for this form\n> git clean dir\n> I think that a \"honor (lack of) -d even if pathspec matches\" would reduce\n> the consequences of this particular kind of user error (by deleting too\n> little instead of too much).\n\nIf there are no tracked files the only difference between the dir/ and\ndir case is that the former will leave behind an empty directory. So\nthe difference between too much and too little is of little\nimportance. However,\n\ngit clean dir\nWould not remove dir/\n\nis a little strange.\n\n--\nShawn\n"},{"id":"74416","messageId":"48044C33.20006@broadpark.no","threadId":"13022","inReplyTo":"20080415034417.GA2882@lintop","subject":"Re: [PATCH] git clean: Don't automatically remove directories when run within subdirectory","fromName":"Joachim Berdal Haga","fromEmail":"cjbhaga@broadpark.no","sentAt":"2008-04-15T06:33:23Z","receivedAt":"2008-04-15T06:33:23Z","isPatch":true,"sender":{"key":"cjbhaga@broadpark.no","avatar":null},"body":"Shawn Bohrer wrote:\n> On Mon, Apr 14, 2008 at 08:18:13PM +0200, Joachim Berdal Haga wrote:\n>> I think that the best option would be to never remove a directory, even if\n>> given explicitly, unless -d is given. Because my gut feeling is that when a\n>> directory name is specified, it is most often meant as \"clean inside the\n>> given directory\", ie. as a path delimiter.\n> \n> If there are no tracked files the only difference between the dir/ and\n> dir case is that the former will leave behind an empty directory. So\n> the difference between too much and too little is of little importance.\n\nNo, check this out; note that only in the very last case dir/subdir/subfile\nwould be removed.\n\n$ git init; mkdir -p dir/subdir; touch dir/file dir/subdir/subfile\nInitialized empty Git repository in .git/\n$ touch dir/tracked-file; git add dir/tracked-file\n$ ~/src/git/git-clean -n dir/\nWould remove dir/file\nWould not remove dir/subdir/\n$ ~/src/git/git-clean -n dir\nWould remove dir/file\nWould not remove dir/subdir/\n$ git rm -f dir/tracked-file\nrm 'dir/tracked-file'\n$ ~/src/git/git-clean -n dir/\nWould remove dir/file\nWould not remove dir/subdir/\n$ ~/src/git/git-clean -n dir\nWould remove dir/\n\n> However,\n> \n> git clean dir\n> Would not remove dir/\n> \n> is a little strange.\n\nYes, although it could be made less strange by adding a short explanation,\nlike \"Would not remove dir/ (-d not given)\". But I also think that the\ndifference between \"dir\" and \"dir/\" is very (too?) subtle in this case and\ntherefore should require explicit approval/action from the user.\n\n\n-j.\n"},{"id":"74449","messageId":"20080415142601.GB10548@mediacenter","threadId":"13022","inReplyTo":"48044C33.20006@broadpark.no","subject":"Re: [PATCH] git clean: Don't automatically remove directories when run within subdirectory","fromName":"Shawn Bohrer","fromEmail":"shawn.bohrer@gmail.com","sentAt":"2008-04-15T14:26:01Z","receivedAt":"2008-04-15T14:26:01Z","isPatch":true,"sender":{"key":"shawn.bohrer@gmail.com","avatar":"https://gravatar.com/avatar/6eb093ef7d276306d18366254e0c95ff6a5db58231ac7e82fe78c2800aaae1b6?d=mp&s=160"},"body":"On Tue, Apr 15, 2008 at 08:33:23AM +0200, Joachim Berdal Haga wrote:\n> Shawn Bohrer wrote:\n> > On Mon, Apr 14, 2008 at 08:18:13PM +0200, Joachim Berdal Haga wrote:\n> >> I think that the best option would be to never remove a directory, even if\n> >> given explicitly, unless -d is given. Because my gut feeling is that when a\n> >> directory name is specified, it is most often meant as \"clean inside the\n> >> given directory\", ie. as a path delimiter.\n> > \n> > If there are no tracked files the only difference between the dir/ and\n> > dir case is that the former will leave behind an empty directory. So\n> > the difference between too much and too little is of little importance.\n> \n> No, check this out; note that only in the very last case dir/subdir/subfile\n> would be removed.\n> \n> $ git init; mkdir -p dir/subdir; touch dir/file dir/subdir/subfile\n> Initialized empty Git repository in .git/\n> $ touch dir/tracked-file; git add dir/tracked-file\n> $ ~/src/git/git-clean -n dir/\n> Would remove dir/file\n> Would not remove dir/subdir/\n> $ ~/src/git/git-clean -n dir\n> Would remove dir/file\n> Would not remove dir/subdir/\n> $ git rm -f dir/tracked-file\n> rm 'dir/tracked-file'\n> $ ~/src/git/git-clean -n dir/\n> Would remove dir/file\n> Would not remove dir/subdir/\n> $ ~/src/git/git-clean -n dir\n> Would remove dir/\n\nAh of course, this is the behavior with my patch. Before it would have\nremoved everything which is the same bug you initially reported :)\n\n> > However,\n> > \n> > git clean dir\n> > Would not remove dir/\n> > \n> > is a little strange.\n> \n> Yes, although it could be made less strange by adding a short explanation,\n> like \"Would not remove dir/ (-d not given)\". But I also think that the\n> difference between \"dir\" and \"dir/\" is very (too?) subtle in this case and\n> therefore should require explicit approval/action from the user.\n\nYeah, I don't know how I feel about this. I do think that the behavior\nwith my current patch is technically correct, but you may be right that\na trailing slash is subtle. In most cases I use my shell's tab\ncompletion witch adds the trailing slash, and only remove it when\nneeded. Additionally, I could argue that by default we require explicit\naction to clean files by requiring -n or -f so hopefully users try -n\nfirst (I do).\n\n--\nShawn\n"},{"id":"74451","messageId":"4804BFB5.8030605@broadpark.no","threadId":"13022","inReplyTo":"20080415142601.GB10548@mediacenter","subject":"Re: [PATCH] git clean: Don't automatically remove directories when run within subdirectory","fromName":"Joachim Berdal Haga","fromEmail":"jobh@broadpark.no","sentAt":"2008-04-15T14:46:13Z","receivedAt":"2008-04-15T14:46:13Z","isPatch":true,"sender":{"key":"jobh@broadpark.no","avatar":null},"body":"Shawn Bohrer wrote:\n> On Tue, Apr 15, 2008 at 08:33:23AM +0200, Joachim Berdal Haga wrote:\n>> like \"Would not remove dir/ (-d not given)\". But I also think that the\n>> difference between \"dir\" and \"dir/\" is very (too?) subtle in this case and\n>> therefore should require explicit approval/action from the user.\n> \n> Yeah, I don't know how I feel about this. I do think that the behavior\n> with my current patch is technically correct, but you may be right that\n> a trailing slash is subtle. In most cases I use my shell's tab\n> completion witch adds the trailing slash, and only remove it when\n> needed. Additionally, I could argue that by default we require explicit\n> action to clean files by requiring -n or -f so hopefully users try -n\n> first (I do).\n\nI guess part of the story is that I dislike the -f requirement, because I see it \nas a case of \"training users to use -f without thinking\" (it's required for \nnormal operation). But that's another story, and now that I've raised my points \nI'm quite happy to leave the final decision to you.\n\nCheers,\n-j.\n"}]}