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

From: Michael Haggerty <mhagger@alum.mit.edu>
Date: Jan 7, 2014, 10:07 UTC
Message-ID: <52CBD1D5.5080701@alum.mit.edu>
In-Reply-To: <xmqqy52tht2l.fsf@gitster.dls.corp.google.com>

On 01/06/2014 07:18 PM, Junio C Hamano wrote:

Show 29 quoted lines

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

Show 14 quoted lines

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