threads / discuss / 26951

Small "git clean" annoyance

Subject: Small "git clean" annoyance

## tl;dr

6 messages between Mar 31, 2011 and Apr 1, 2011.

replies: 5people: 4as markdown or json

Linus Torvalds· Mar 31, 2011, 22:01 UTC · lore

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· Apr 1, 2011, 07:34 UTC · re: Linus Torvalds · lore

Re: Small "git clean" annoyance

On Fri, Apr 1, 2011 at 00:01, Linus Torvalds <torvalds@linux-foundation.org> wrote:

Show 5 quoted lines
> 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· Apr 1, 2011, 07:41 UTC · re: Alex Riesen · lore

Re: Small "git clean" annoyance

Alex Riesen <raa.lkml@gmail.com> writes:
Show 6 quoted lines
> -	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· Apr 1, 2011, 08:29 UTC · re: Alex Riesen · lore

Re: Small "git clean" annoyance

On Fri, Apr 1, 2011 at 9:34 AM, Alex Riesen <raa.lkml@gmail.com> wrote:
Show 21 quoted lines
> 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))"?

Alex Riesen· Apr 1, 2011, 09:20 UTC · re: Erik Faye-Lund · lore

Re: Small "git clean" annoyance

On Fri, Apr 1, 2011 at 10:29, Erik Faye-Lund <kusmabite@gmail.com> wrote:
Show 5 quoted lines
>> +               if (rmdir(path->buf) == 0)
>
> nit-pick:
> don't we usually do "if (!rmdir(path->buf))"?
>
Not in this file.
Linus Torvalds· Apr 1, 2011, 14:48 UTC · re: Alex Riesen · lore

Re: Small "git clean" annoyance

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

← back to recent threads