Re: [PATCH v3 05/23] raceproof_create_file(): new function

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

On 12/31/2016 07:11 AM, Jeff King wrote:
> On Sat, Dec 31, 2016 at 04:12:45AM +0100, Michael Haggerty wrote:
>> [...]
>> +/*
>> + * Callback function for raceproof_create_file(). This function is
>> + * expected to do something that makes dirname(path) permanent despite
>> + * the fact that other processes might be cleaning up empty
>> + * directories at the same time. Usually it will create a file named
>> + * path, but alternatively it could create another file in that
>> + * directory, or even chdir() into that directory. The function should
>> + * return 0 if the action was completed successfully. On error, it
>> + * should return a nonzero result and set errno.
>> + * raceproof_create_file() treats two errno values specially:
>> + *
>> + * - ENOENT -- dirname(path) does not exist. In this case,
>> + *             raceproof_create_file() tries creating dirname(path)
>> + *             (and any parent directories, if necessary) and calls
>> + *             the function again.
>> + *
>> + * - EISDIR -- the file already exists and is a directory. In this
>> + *             case, raceproof_create_file() deletes the directory
>> + *             (recursively) if it is empty and calls the function
>> + *             again.
> 
> It took me a minute to figure out why EISDIR is recursive.
> 
> If we are trying to create "foo/bar/baz", we can only get EISDIR when
> "baz" exists and is a directory. I at first took your recursive to me
> that we delete it and "foo/bar" and "foo". Which is just silly and
> counterproductive.
> 
> But presumably you mean that we delete "foo/bar/baz/xyzzy", etc, up to
> "foo/bar/baz", provided they are all empty directories. I think your
> comment is probably OK and I was just being thick, but maybe stating it
> like:
> 
>   ...removes the directory if it is empty (and recursively any empty
>   directories it contains) and calls the function again.
> 
> would be more clear. That is still leaving the definition of "empty"
> implied, but it's hopefully obvious from the context.

Yes, that's clearer. I'll change it. Thanks!

> [...]

Michael