Re: Small "git clean" annoyance

From: Linus Torvalds <torvalds@linux-foundation.org>
Date: Apr 1, 2011, 14:48 UTC
Message-ID: <AANLkTinD=Zx0tdY53n8mXvpufCUr96v0u1DbzasRXMfY@mail.gmail.com>
In-Reply-To: <AANLkTikksQj3HfOJmi-uZTfc7sTNNuDA4bFVaj2cY3Ad@mail.gmail.com>

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