Re: [PATCH v2 13/17] remove_dir_recurse(): handle disappearing files and directories
Michael Haggerty writes: > I'm not sure I understand your point. Please note that the > REMOVE_DIR_KEEP_TOPLEVEL bit is cleared from flags before this function > recurses. So in recursive invocations, keep_toplevel will always be > false, and the rmdir(path->buf) codepath will be permitted. Does that > answer your question? Yes; thanks. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 13/17] remove_dir_recurse(): handle disappearing files and directories
On 01/06/2014 07:18 PM, Junio C Hamano wrote:
> Michael Haggerty writes:
>
>> If a file or directory that we are trying to remove disappears (e.g.,
>> because another process has pruned it), do not consider it an error.
>>
>> Signed-off-by: Michael Haggerty
>> ---
>> dir.c | 22 --
>> 1 file changed, 16 insertions(+), 6 deletions(-)
>>
>> diff --git a/dir.c b/dir.c
>> index 11e1520..716b613 100644
>> --- a/dir.c
>> +++ b/dir.c
>> @@ -1476,7 +1476,9 @@ static int remove_dir_recurse(struct strbuf *path, int
>> flag, int *kept_up)
>> flag &= ~REMOVE_DIR_KEEP_TOPLEVEL;
>> dir = opendir(path->buf);
>> if (!dir) {
>> -if (errno == EACCES && !keep_toplevel)
>> +if (errno == ENOENT)
>> +return keep_toplevel ? -1 : 0;
>
> If we were told that "foo/bar must go, but do not bother removing
> foo/", is it an error if foo itself did not exist? I think this
> step does not change the behaviour from the original (we used to say
> "oh, we were told to keep_toplevel, and the top-level cannot be read
> for whatever reason, so it is an error"), but because this series is
> giving us a finer grained error diagnosis, we may want to think
> about it further, perhaps as a follow-up.
Indeed, this is a design choice that I should have explained. I
implemented it this way to keep the behavior the same as the old code in
this situation, and because I thought that if the caller explicitly asks
for the toplevel path to be kept, and that path doesn't even exist at
the entrance to the function, then something is screwy and it is better
to return an error than to keep going.
It would even be possible to argue that if keep_toplevel is set but path
is missing, then this function should call
safe_create_leading_directories(path).
Changing this behavior would require an audit to see which behavior
would be most useful to the callers. I think that is out of the scope
of this patch series.
> I also wonder why the keep-toplevel logic is in this "recurse" part
> of the callchain. If everything in "foo/bar/" can be removed but
> "foo/bar/" is unreadable, it is OK, when opendir("foo/bar") failed
> with EACCESS, to attempt to rmdir("foo/bar") whether we were told
> not to attempt removing "foo/", no?
>
>> +else if (errno == EACCES && !keep_toplevel)
>
> That is, I am wondering why this part just checks !keep_toplevel,
> not
>
> (!keep_toplevel || has_dir_separator(path->buf))
>
> or something.
I'm not sure I understand your point. Please note that the
REMOVE_DIR_KEEP_TOPLEVEL bit is cleared from flags before this function
recurses. So in recursive invocations, keep_toplevel will always be
false, and the rmdir(path->buf) codepath will be permitted. Does that
answer your question?
Michael
--
Michael Haggerty
mhag...@alum.mit.edu
http://softwareswirl.blogspot.com/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 13/17] remove_dir_recurse(): handle disappearing files and directories
Michael Haggerty writes:
> If a file or directory that we are trying to remove disappears (e.g.,
> because another process has pruned it), do not consider it an error.
>
> Signed-off-by: Michael Haggerty
> ---
> dir.c | 22 --
> 1 file changed, 16 insertions(+), 6 deletions(-)
>
> diff --git a/dir.c b/dir.c
> index 11e1520..716b613 100644
> --- a/dir.c
> +++ b/dir.c
> @@ -1476,7 +1476,9 @@ static int remove_dir_recurse(struct strbuf *path, int
> flag, int *kept_up)
> flag &= ~REMOVE_DIR_KEEP_TOPLEVEL;
> dir = opendir(path->buf);
> if (!dir) {
> - if (errno == EACCES && !keep_toplevel)
> + if (errno == ENOENT)
> + return keep_toplevel ? -1 : 0;
If we were told that "foo/bar must go, but do not bother removing
foo/", is it an error if foo itself did not exist? I think this
step does not change the behaviour from the original (we used to say
"oh, we were told to keep_toplevel, and the top-level cannot be read
for whatever reason, so it is an error"), but because this series is
giving us a finer grained error diagnosis, we may want to think
about it further, perhaps as a follow-up.
I also wonder why the keep-toplevel logic is in this "recurse" part
of the callchain. If everything in "foo/bar/" can be removed but
"foo/bar/" is unreadable, it is OK, when opendir("foo/bar") failed
with EACCESS, to attempt to rmdir("foo/bar") whether we were told
not to attempt removing "foo/", no?
> + else if (errno == EACCES && !keep_toplevel)
That is, I am wondering why this part just checks !keep_toplevel,
not
(!keep_toplevel || has_dir_separator(path->buf))
or something.
> /*
>* An empty dir could be removable even if it
>* is unreadable:
> @@ -1496,13 +1498,21 @@ static int remove_dir_recurse(struct strbuf *path,
> int flag, int *kept_up)
>
> strbuf_setlen(path, len);
> strbuf_addstr(path, e->d_name);
> - if (lstat(path->buf, &st))
> - ; /* fall thru */
> - else if (S_ISDIR(st.st_mode)) {
> + if (lstat(path->buf, &st)) {
> + if (errno == ENOENT)
> + /*
> + * file disappeared, which is what we
> + * wanted anyway
> + */
> + continue;
> + /* fall thru */
> + } else if (S_ISDIR(st.st_mode)) {
> if (!remove_dir_recurse(path, flag, &kept_down))
> continue; /* happy */
> - } else if (!only_empty && !unlink(path->buf))
> + } else if (!only_empty &&
> +(!unlink(path->buf) || errno == ENOENT)) {
> continue; /* happy, too */
> + }
>
> /* path too long, stat fails, or non-directory still exists */
> ret = -1;
> @@ -1512,7 +1522,7 @@ static int remove_dir_recurse(struct strbuf *path, int
> flag, int *kept_up)
>
> strbuf_setlen(path, original_len);
> if (!ret && !keep_toplevel && !kept_down)
> - ret = rmdir(path->buf);
> + ret = (!rmdir(path->buf) || errno == ENOENT) ? 0 : -1;
> else if (kept_up)
> /*
>* report the uplevel that it is not an error that we
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 13/17] remove_dir_recurse(): handle disappearing files and directories
If a file or directory that we are trying to remove disappears (e.g.,
because another process has pruned it), do not consider it an error.
Signed-off-by: Michael Haggerty
---
dir.c | 22 --
1 file changed, 16 insertions(+), 6 deletions(-)
diff --git a/dir.c b/dir.c
index 11e1520..716b613 100644
--- a/dir.c
+++ b/dir.c
@@ -1476,7 +1476,9 @@ static int remove_dir_recurse(struct strbuf *path, int
flag, int *kept_up)
flag &= ~REMOVE_DIR_KEEP_TOPLEVEL;
dir = opendir(path->buf);
if (!dir) {
- if (errno == EACCES && !keep_toplevel)
+ if (errno == ENOENT)
+ return keep_toplevel ? -1 : 0;
+ else if (errno == EACCES && !keep_toplevel)
/*
* An empty dir could be removable even if it
* is unreadable:
@@ -1496,13 +1498,21 @@ static int remove_dir_recurse(struct strbuf *path, int
flag, int *kept_up)
strbuf_setlen(path, len);
strbuf_addstr(path, e->d_name);
- if (lstat(path->buf, &st))
- ; /* fall thru */
- else if (S_ISDIR(st.st_mode)) {
+ if (lstat(path->buf, &st)) {
+ if (errno == ENOENT)
+ /*
+* file disappeared, which is what we
+* wanted anyway
+*/
+ continue;
+ /* fall thru */
+ } else if (S_ISDIR(st.st_mode)) {
if (!remove_dir_recurse(path, flag, &kept_down))
continue; /* happy */
- } else if (!only_empty && !unlink(path->buf))
+ } else if (!only_empty &&
+ (!unlink(path->buf) || errno == ENOENT)) {
continue; /* happy, too */
+ }
/* path too long, stat fails, or non-directory still exists */
ret = -1;
@@ -1512,7 +1522,7 @@ static int remove_dir_recurse(struct strbuf *path, int
flag, int *kept_up)
strbuf_setlen(path, original_len);
if (!ret && !keep_toplevel && !kept_down)
- ret = rmdir(path->buf);
+ ret = (!rmdir(path->buf) || errno == ENOENT) ? 0 : -1;
else if (kept_up)
/*
* report the uplevel that it is not an error that we
--
1.8.5.2
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
