Re: [PATCH v2 13/17] remove_dir_recurse(): handle disappearing files and directories
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jan 6, 2014, 18:18 UTC
- Message-ID
- <xmqqy52tht2l.fsf@gitster.dls.corp.google.com>
- In-Reply-To
- <1389015935-21936-14-git-send-email-mhagger@alum.mit.edu>
Michael Haggerty <mhagger@alum.mit.edu> writes:
Show 19 quoted lines
> 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 <mhagger@alum.mit.edu>
> ---
> 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.
Show 38 quoted lines
> /*
> * 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