{"thread":{"id":"26951","subject":"Small \"git clean\" annoyance","startedAt":"2011-03-31T22:01:33Z","lastAt":"2011-04-01T14:48:12Z","messageCount":6,"participants":["Linus Torvalds","Alex Riesen","Matthieu Moy","Erik Faye-Lund"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"164839","messageId":"AANLkTinQscpkRDftLmCrQR+Aq5RacGyurd3kb15y=1FK@mail.gmail.com","threadId":"26951","inReplyTo":null,"subject":"Small \"git clean\" annoyance","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2011-03-31T22:01:33Z","receivedAt":"2011-03-31T22:01:33Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"I don't have a patch for this, and I guess it doesn't much matter, but\nI just found this annoying:\n\n [torvalds@i5 git]$ mkdir -m 0 tmp\n [torvalds@i5 git]$ git clean -dqfx\n warning: failed to remove tmp/\n [torvalds@i5 git]$ rmdir tmp\n\nand the reason is simply that git gives up if the directory is\nunreadable and thus cannot be opened.\n\nWhich is kind of understandable, but at the same time, if it's empty,\na \"rmdir()\" will just work. So git gave up a bit too soon.\n\n(In case anybody wonders, the reason I had empty unreadable\ndirectories around is not because I commonly do \"mkdir -m 0\", but\nsimply because they got created when I was running a system call\nfuzzer for testing).\n\nNow, I realize that if the directory isn't empty, and is unreadable,\nwe really should give up (although a better error message about _why_\nwe failed may be in order) rather than try to chmod it or anything\nlike that. But the simple \"try to rmdir it\" might be a good addition\nfor the trivial case.\n\n Linus\n"},{"id":"164858","messageId":"AANLkTikksQj3HfOJmi-uZTfc7sTNNuDA4bFVaj2cY3Ad@mail.gmail.com","threadId":"26951","inReplyTo":"AANLkTinQscpkRDftLmCrQR+Aq5RacGyurd3kb15y=1FK@mail.gmail.com","subject":"Re: Small \"git clean\" annoyance","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2011-04-01T07:34:44Z","receivedAt":"2011-04-01T07:34:44Z","isPatch":false,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"On Fri, Apr 1, 2011 at 00:01, Linus Torvalds\n wrote:\n> Now, I realize that if the directory isn't empty, and is unreadable,\n> we really should give up (although a better error message about _why_\n> we failed may be in order) rather than try to chmod it or anything\n> like that. But the simple \"try to rmdir it\" might be a good addition\n> for the trivial case.\n\nSomething like this?\n\ndiff --git a/dir.c b/dir.c\nindex 325fb56..7251426 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -1191,8 +1191,11 @@ int remove_dir_recursively(struct strbuf *path, int flag)\n \t\treturn 0;\n\n \tdir = opendir(path->buf);\n-\tif (!dir)\n+\tif (!dir) {\n+\t\tif (rmdir(path->buf) == 0)\n+\t\t\treturn 0;\n \t\treturn -1;\n+\t}\n \tif (path->buf[original_len - 1] != '/')\n \t\tstrbuf_addch(path, '/');\n"},{"id":"164859","messageId":"vpq62qymn9f.fsf@bauges.imag.fr","threadId":"26951","inReplyTo":"AANLkTikksQj3HfOJmi-uZTfc7sTNNuDA4bFVaj2cY3Ad@mail.gmail.com","subject":"Re: Small \"git clean\" annoyance","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2011-04-01T07:41:48Z","receivedAt":"2011-04-01T07:41:48Z","isPatch":false,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Alex Riesen writes:\n\n> -\tif (!dir)\n> +\tif (!dir) {\n> +\t\tif (rmdir(path->buf) == 0)\n> +\t\t\treturn 0;\n> \t\treturn -1;\n> +\t}\n\nSounds good.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"164863","messageId":"AANLkTikP17k4FWgfuZa6Fto5qtp1Bp_+4vOBwp=e2JZy@mail.gmail.com","threadId":"26951","inReplyTo":"AANLkTikksQj3HfOJmi-uZTfc7sTNNuDA4bFVaj2cY3Ad@mail.gmail.com","subject":"Re: Small \"git clean\" annoyance","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@gmail.com","sentAt":"2011-04-01T08:29:19Z","receivedAt":"2011-04-01T08:29:19Z","isPatch":false,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"On Fri, Apr 1, 2011 at 9:34 AM, Alex Riesen wrote:\n> On Fri, Apr 1, 2011 at 00:01, Linus Torvalds\n> wrote:\n>> Now, I realize that if the directory isn't empty, and is unreadable,\n>> we really should give up (although a better error message about _why_\n>> we failed may be in order) rather than try to chmod it or anything\n>> like that. But the simple \"try to rmdir it\" might be a good addition\n>> for the trivial case.\n>\n> Something like this?\n>\n> diff --git a/dir.c b/dir.c\n> index 325fb56..7251426 100644\n> --- a/dir.c\n> +++ b/dir.c\n> @@ -1191,8 +1191,11 @@ int remove_dir_recursively(struct strbuf *path, int flag)\n>                return 0;\n>\n>        dir = opendir(path->buf);\n> -       if (!dir)\n> +       if (!dir) {\n> +               if (rmdir(path->buf) == 0)\n\nnit-pick:\ndon't we usually do \"if (!rmdir(path->buf))\"?\n"},{"id":"164867","messageId":"AANLkTinHD6ZEwnSNxU1DC2gBF9ZpKPuDdgW=EUiPSWZK@mail.gmail.com","threadId":"26951","inReplyTo":"AANLkTikP17k4FWgfuZa6Fto5qtp1Bp_+4vOBwp=e2JZy@mail.gmail.com","subject":"Re: Small \"git clean\" annoyance","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2011-04-01T09:20:14Z","receivedAt":"2011-04-01T09:20:14Z","isPatch":false,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"On Fri, Apr 1, 2011 at 10:29, Erik Faye-Lund wrote:\n>> +               if (rmdir(path->buf) == 0)\n>\n> nit-pick:\n> don't we usually do \"if (!rmdir(path->buf))\"?\n>\n\nNot in this file.\n"},{"id":"164894","messageId":"AANLkTinD=Zx0tdY53n8mXvpufCUr96v0u1DbzasRXMfY@mail.gmail.com","threadId":"26951","inReplyTo":"AANLkTikksQj3HfOJmi-uZTfc7sTNNuDA4bFVaj2cY3Ad@mail.gmail.com","subject":"Re: Small \"git clean\" annoyance","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2011-04-01T14:48:12Z","receivedAt":"2011-04-01T14:48:12Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"On Fri, Apr 1, 2011 at 12:34 AM, Alex Riesen wrote:\n>\n> Something like this?\n>\n> diff --git a/dir.c b/dir.c\n> index 325fb56..7251426 100644\n> --- a/dir.c\n> +++ b/dir.c\n> @@ -1191,8 +1191,11 @@ int remove_dir_recursively(struct strbuf *path, int flag)\n>                return 0;\n>\n>        dir = opendir(path->buf);\n> -       if (!dir)\n> +       if (!dir) {\n> +               if (rmdir(path->buf) == 0)\n> +                       return 0;\n>                return -1;\n> +       }\n\nHeh. My minimalist sensibilities go \"yuck\", and say that you should just do\n\n if (!dir)\n return rmdir(path->buf);\n\ninstead.\n\nBut not a big deal. Looks fine, and fixes the trivial annoyance.\n\nI'd still like the better error message. With \"rm -rf\" I get good\nerror messages even for the complex case:\n\n [torvalds@i5 git]$ mkdir tmp\n [torvalds@i5 git]$ mkdir tmp/tmp\n [torvalds@i5 git]$ chmod -w tmp\n [torvalds@i5 git]$ rm -rf tmp\n rm: cannot remove `tmp/tmp': Permission denied\n\nbut \"git clean\" says:\n\n [torvalds@i5 git]$ git clean -dqfx\n warning: failed to remove tmp/\n\nHmm.\n\nAdding the obvious \"strerror(errno)\" to builtin/clean.c just changes it to\n\n warning: failed to remove tmp/ (Permission denied)\n\nwhich I guess is better, but not entirely obvious (it's \"tmp/tmp\" that\nfailed to remove due to permissions, but clean.c only sees/cares about\nthe uppermost level)\n\nBut it's probably not worth worrying about any more. The simple\none-liner rmdir() looks worth it.\n\n Linus\n"}]}