Small "git clean" annoyance

Small "git clean" annoyance

[Linus Torvalds]

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

Re: Small "git clean" annoyance

[Alex Riesen]

On Fri, Apr 1, 2011 at 00:01, Linus Torvalds
<torvalds@linux-foundation.org> 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, '/');

Re: Small "git clean" annoyance

[Matthieu Moy]

Alex Riesen <raa.lkml@gmail.com> writes:

> -	if (!dir)
> +	if (!dir) {
> +		if (rmdir(path->buf) == 0)
> +			return 0;
>  		return -1;
> +	}

Sounds good.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: Small "git clean" annoyance

[Erik Faye-Lund]

On Fri, Apr 1, 2011 at 9:34 AM, Alex Riesen <raa.lkml@gmail.com> wrote:
> On Fri, Apr 1, 2011 at 00:01, Linus Torvalds
> <torvalds@linux-foundation.org> 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))"?

Re: Small "git clean" annoyance

[Alex Riesen]

On Fri, Apr 1, 2011 at 10:29, Erik Faye-Lund <kusmabite@gmail.com> wrote:
>> +               if (rmdir(path->buf) == 0)
>
> nit-pick:
> don't we usually do "if (!rmdir(path->buf))"?
>

Not in this file.

Re: Small "git clean" annoyance

[Linus Torvalds]

On Fri, Apr 1, 2011 at 12:34 AM, Alex Riesen <raa.lkml@gmail.com> 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