[PATCH] Try to remove the given path even if it can't be opened

[PATCH] Try to remove the given path even if it can't be opened

[Alex Riesen]

[-- Attachment #1: Type: text/plain, Size: 1411 bytes --]

Consider unreadable empty directories. rmdir(2) will remove
them just fine, assuming the parent directory is modifiable.

Noticed by Linus.

Signed-off-by: Alex Riesen <raa.lkml@gmail.com>
---
On Fri, Apr 1, 2011 at 00:01, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> 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.
...
> 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.

It is not tested, but looks trivial. The system I made it on is a Cygwin
machine, and a test from last master pull is still running (since two days).
And sorry, it is not based on master. Should apply without problems, though.

---
 dir.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

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, '/');

-- 
1.7.2.2.240.g7d094

[-- Attachment #2: 0001-Try-to-remove-the-given-path-even-if-it-can-t-be-open.diff --]
[-- Type: application/octet-stream, Size: 874 bytes --]

From 861871ebfe72b6839526eaa4fe8e5c4b6eec924e Mon Sep 17 00:00:00 2001
From: Alex Riesen <raa.lkml@gmail.com>
Date: Fri, 1 Apr 2011 09:37:07 +0200
Subject: [PATCH] Try to remove the given path even if it can't be opened

Consider unreadable empty directories. rmdir(2) will remove
them just fine, assuming the parent directory is modifiable.

Noticed by Linus.

Signed-off-by: Alex Riesen <raa.lkml@gmail.com>
---
 dir.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

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, '/');
 
-- 
1.7.2.2.240.g7d094

Re: [PATCH] Try to remove the given path even if it can't be opened

[Michael J Gruber]

Alex Riesen venit, vidit, dixit 01.04.2011 10:29:
> Consider unreadable empty directories. rmdir(2) will remove
> them just fine, assuming the parent directory is modifiable.
> 
> Noticed by Linus.
> 
> Signed-off-by: Alex Riesen <raa.lkml@gmail.com>
> ---
> On Fri, Apr 1, 2011 at 00:01, Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
>> 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.
> ...
>> 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.
> 
> It is not tested, but looks trivial. The system I made it on is a Cygwin

Famous last words...

> machine, and a test from last master pull is still running (since two days).
> And sorry, it is not based on master. Should apply without problems, though.
> 
> ---
>  dir.c |    5 ++++-
>  1 files changed, 4 insertions(+), 1 deletions(-)
> 
> 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, '/');
> 

How about simply

if (!dir)
	return rmdir(path->buf);

like we do later on in that function?

Michael

Re: [PATCH] Try to remove the given path even if it can't be opened

[Alex Riesen]

On Fri, Apr 1, 2011 at 15:37, Michael J Gruber <git@drmicha.warpmail.net> wrote:
> How about simply
>
> if (!dir)
>        return rmdir(path->buf);
>
> like we do later on in that function?
>

I'm used to try to keep the returned value of a function I modify,
and I'm also used to not trust the return values of the functions
I don't control. That's to my defense.

But you're unquestionably right.

Re: [PATCH] Try to remove the given path even if it can't be opened

[Junio C Hamano]

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

> --0016e6d9a16eca69d0049fd73526
> Content-Type: text/plain; charset=UTF-8
>
> Consider unreadable empty directories. rmdir(2) will remove
> them just fine, assuming the parent directory is modifiable.
>
> Noticed by Linus.
>
> Signed-off-by: Alex Riesen <raa.lkml@gmail.com>
> ---

Please don't do an attachment that has an inline patch and then attach the
patch itself again in base64.  It is extremely annoying.

[PATCH] Try to remove the given path even if it can't be opened

[Alex Riesen]

Consider unreadable empty directories. rmdir(2) will remove
them just fine, assuming the parent directory is modifiable.

Noticed by Linus.
Fix suggested by Michael Gruber and Linus.

Signed-off-by: Alex Riesen <raa.lkml@gmail.com>
---
Junio C Hamano, Fri, Apr 01, 2011 20:08:36 +0200:
> 
> Please don't do an attachment that has an inline patch and then attach the
> patch itself again in base64.  It is extremely annoying.

Sorry. Hard to notice on GMail.

The extended error information is a little bit tricky: there is at least four
error cases (opendir, stat, unlink and rmdir) and there is a closedir, which
resets errno to output the error in the caller of remove_dir_recursively.

 dir.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/dir.c b/dir.c
index 325fb56..532bcb6 100644
--- a/dir.c
+++ b/dir.c
@@ -1192,7 +1192,7 @@ int remove_dir_recursively(struct strbuf *path, int flag)
 
 	dir = opendir(path->buf);
 	if (!dir)
-		return -1;
+		return rmdir(path->buf);
 	if (path->buf[original_len - 1] != '/')
 		strbuf_addch(path, '/');
 
-- 
1.7.4

Re: [PATCH] Try to remove the given path even if it can't be opened

[Junio C Hamano]

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

>> Please don't do an attachment that has an inline patch and then attach the
>> patch itself again in base64.  It is extremely annoying.
>
> Sorry. Hard to notice on GMail.

Thanks.  I've already queued 0235017 (clean: unreadable directory may
still be rmdir-able if it is empty, 2011-04-01) with a trivial test.