Re: [PATCH v2 13/17] remove_dir_recurse(): handle disappearing files and directories

[Michael Haggerty <mhagger@alum.mit.edu>]

On 01/06/2014 07:18 PM, Junio C Hamano wrote:
> Michael Haggerty <mhagger@alum.mit.edu> 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 <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.

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
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/