threads / patch / 26955

patchTry to remove the given path even if it can't be opened

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

## tl;dr

6 messages between Apr 1, 2011 and Apr 2, 2011. Diffs are folded; open one to read it.

replies: 5people: 3as markdown or json

Alex Riesen· Apr 1, 2011, 08:29 UTC · lore

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.
...
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.

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(-)
Show changes to dir.c +4 −1
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


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
Michael J Gruber· Apr 1, 2011, 13:37 UTC · re: Alex Riesen · lore

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

Alex Riesen venit, vidit, dixit 01.04.2011 10:29:
Show 19 quoted lines
> 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...
Show 24 quoted lines
> 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
Alex Riesen· Apr 1, 2011, 14:01 UTC · re: Michael J Gruber · lore

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

On Fri, Apr 1, 2011 at 15:37, Michael J Gruber <git@drmicha.warpmail.net> wrote:
Show 7 quoted lines
> 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.
Junio C Hamano· Apr 1, 2011, 18:08 UTC · re: Alex Riesen · lore

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

Alex Riesen <raa.lkml@gmail.com> writes:
Show 10 quoted lines
> --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.

Alex Riesen· Apr 2, 2011, 20:09 UTC · re: Junio C Hamano · lore

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(-)
Show changes to dir.c +1 −1
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
Junio C Hamano· Apr 2, 2011, 20:33 UTC · re: Alex Riesen · lore

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

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.

← back to recent threads