# Small "git clean" annoyance 6 messages from 2011-03-31 to 2011-04-01. Participants: Linus Torvalds, Alex Riesen, Matthieu Moy, Erik Faye-Lund. Thread: https://gitlist.dev/t/26951 ## Linus Torvalds, 2011-03-31 22:01 Subject: Small "git clean" annoyance Message-ID: URL: https://gitlist.dev/e/AANLkTinQscpkRDftLmCrQR%2BAq5RacGyurd3kb15y%3D1FK%40mail.gmail.com ``` I don't have a patch for this, and I guess it doesn't much matter, but I just found this annoying: [torvalds@i5 git]$ mkdir -m 0 tmp [torvalds@i5 git]$ git clean -dqfx warning: failed to remove tmp/ [torvalds@i5 git]$ rmdir tmp and the reason is simply that git gives up if the directory is unreadable and thus cannot be opened. Which is kind of understandable, but at the same time, if it's empty, a "rmdir()" will just work. So git gave up a bit too soon. (In case anybody wonders, the reason I had empty unreadable directories around is not because I commonly do "mkdir -m 0", but simply because they got created when I was running a system call fuzzer for testing). Now, I realize that if the directory isn't empty, and is unreadable, we really should give up (although a better error message about _why_ we failed may be in order) rather than try to chmod it or anything like that. But the simple "try to rmdir it" might be a good addition for the trivial case. Linus ``` ## Alex Riesen, 2011-04-01 07:34 Subject: Re: Small "git clean" annoyance Message-ID: URL: https://gitlist.dev/e/AANLkTikksQj3HfOJmi-uZTfc7sTNNuDA4bFVaj2cY3Ad%40mail.gmail.com In-Reply-To: ``` On Fri, Apr 1, 2011 at 00:01, Linus Torvalds wrote: > Now, I realize that if the directory isn't empty, and is unreadable, > we really should give up (although a better error message about _why_ > we failed may be in order) rather than try to chmod it or anything > like that. But the simple "try to rmdir it" might be a good addition > for the trivial case. Something like this? diff --git a/dir.c b/dir.c index 325fb56..7251426 100644 --- a/dir.c +++ b/dir.c @@ -1191,8 +1191,11 @@ int remove_dir_recursively(struct strbuf *path, int flag) return 0; dir = opendir(path->buf); - if (!dir) + if (!dir) { + if (rmdir(path->buf) == 0) + return 0; return -1; + } if (path->buf[original_len - 1] != '/') strbuf_addch(path, '/'); ``` ## Matthieu Moy, 2011-04-01 07:41 Subject: Re: Small "git clean" annoyance Message-ID: URL: https://gitlist.dev/e/vpq62qymn9f.fsf%40bauges.imag.fr In-Reply-To: ``` Alex Riesen writes: > - if (!dir) > + if (!dir) { > + if (rmdir(path->buf) == 0) > + return 0; > return -1; > + } Sounds good. -- Matthieu Moy http://www-verimag.imag.fr/~moy/ ``` ## Erik Faye-Lund, 2011-04-01 08:29 Subject: Re: Small "git clean" annoyance Message-ID: URL: https://gitlist.dev/e/AANLkTikP17k4FWgfuZa6Fto5qtp1Bp_%2B4vOBwp%3De2JZy%40mail.gmail.com In-Reply-To: ``` On Fri, Apr 1, 2011 at 9:34 AM, Alex Riesen wrote: > On Fri, Apr 1, 2011 at 00:01, Linus Torvalds > wrote: >> Now, I realize that if the directory isn't empty, and is unreadable, >> we really should give up (although a better error message about _why_ >> we failed may be in order) rather than try to chmod it or anything >> like that. But the simple "try to rmdir it" might be a good addition >> for the trivial case. > > Something like this? > > diff --git a/dir.c b/dir.c > index 325fb56..7251426 100644 > --- a/dir.c > +++ b/dir.c > @@ -1191,8 +1191,11 @@ int remove_dir_recursively(struct strbuf *path, int flag) >                return 0; > >        dir = opendir(path->buf); > -       if (!dir) > +       if (!dir) { > +               if (rmdir(path->buf) == 0) nit-pick: don't we usually do "if (!rmdir(path->buf))"? ``` ## Alex Riesen, 2011-04-01 09:20 Subject: Re: Small "git clean" annoyance Message-ID: URL: https://gitlist.dev/e/AANLkTinHD6ZEwnSNxU1DC2gBF9ZpKPuDdgW%3DEUiPSWZK%40mail.gmail.com In-Reply-To: ``` On Fri, Apr 1, 2011 at 10:29, Erik Faye-Lund wrote: >> +               if (rmdir(path->buf) == 0) > > nit-pick: > don't we usually do "if (!rmdir(path->buf))"? > Not in this file. ``` ## Linus Torvalds, 2011-04-01 14:48 Subject: Re: Small "git clean" annoyance Message-ID: URL: https://gitlist.dev/e/AANLkTinD%3DZx0tdY53n8mXvpufCUr96v0u1DbzasRXMfY%40mail.gmail.com In-Reply-To: ``` On Fri, Apr 1, 2011 at 12:34 AM, Alex Riesen wrote: > > Something like this? > > diff --git a/dir.c b/dir.c > index 325fb56..7251426 100644 > --- a/dir.c > +++ b/dir.c > @@ -1191,8 +1191,11 @@ int remove_dir_recursively(struct strbuf *path, int flag) >                return 0; > >        dir = opendir(path->buf); > -       if (!dir) > +       if (!dir) { > +               if (rmdir(path->buf) == 0) > +                       return 0; >                return -1; > +       } Heh. My minimalist sensibilities go "yuck", and say that you should just do if (!dir) return rmdir(path->buf); instead. But not a big deal. Looks fine, and fixes the trivial annoyance. I'd still like the better error message. With "rm -rf" I get good error messages even for the complex case: [torvalds@i5 git]$ mkdir tmp [torvalds@i5 git]$ mkdir tmp/tmp [torvalds@i5 git]$ chmod -w tmp [torvalds@i5 git]$ rm -rf tmp rm: cannot remove `tmp/tmp': Permission denied but "git clean" says: [torvalds@i5 git]$ git clean -dqfx warning: failed to remove tmp/ Hmm. Adding the obvious "strerror(errno)" to builtin/clean.c just changes it to warning: failed to remove tmp/ (Permission denied) which I guess is better, but not entirely obvious (it's "tmp/tmp" that failed to remove due to permissions, but clean.c only sees/cares about the uppermost level) But it's probably not worth worrying about any more. The simple one-liner rmdir() looks worth it. Linus ```