Re: [PATCH 02/20] safe_create_leading_directories(): set errno on SCLD_EXISTS

From: Michael Haggerty <mhagger@alum.mit.edu>
Date: Feb 18, 2016, 15:33 UTC
Message-ID: <56C5E430.1070705@alum.mit.edu>
In-Reply-To: <xmqqziuzi1jo.fsf@gitster.mtv.corp.google.com>

On 02/17/2016 08:23 PM, Junio C Hamano wrote:

Show 25 quoted lines

> Michael Haggerty <mhagger@alum.mit.edu> writes:
> 
>> The exit path for SCLD_EXISTS wasn't setting errno, as expected by at
>> least one caller. Fix the problem and document that the function sets
>> errno correctly to help avoid similar regressions in the future.
> 
>> diff --git a/sha1_file.c b/sha1_file.c
>> index 568120e..a1ac646 100644
>> --- a/sha1_file.c
>> +++ b/sha1_file.c
>> @@ -135,8 +135,10 @@ enum scld_error safe_create_leading_directories(char *path)
>>  		*slash = '\0';
>>  		if (!stat(path, &st)) {
>>  			/* path exists */
>> -			if (!S_ISDIR(st.st_mode))
>> +			if (!S_ISDIR(st.st_mode)) {
>> +				errno = EEXIST;
>>  				ret = SCLD_EXISTS;
> 
> Hmm, when does this trigger?  There is a non-directory A/B, you are
> preparing leading directories to create A/B/C/D, and you find A/B
> exists but is not a directory so you cannot create A/B/C underneath
> it?
> 
> That sounds more like ENOTDIR to me.

Yes, you're right. Thanks.

> Does the caller expect EEXIST, or both?

I don't know of any callers that branch based on errno, but several use strerror() or die_errno() to report the error to the user. The few callers that care about the reason for the failure base their behavior on the return value.

But there are a lot of callers and I haven't audited each of them carefully. Given that this error path currently doesn't necessarily set errno *at all*, I think using a plausible value is strictly an improvement.

I'll change it to ENOTDIR in the next round.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu