Re: [PATCH v4] setup: allow cwd/.git to be a symlink to a directory

> Junio C Hamano <gitster@pobox.com> writes:
> 
>> Karthik Nayak <karthik.188@gmail.com> writes:
>>
>>>> @@ -994,7 +1000,9 @@ const char *read_gitfile_gently(const char *path, int *return_error_code)
>>>>   cleanup_return:
>>>>   	if (return_error_code)
>>>>   		*return_error_code = error_code;
>>>> -	else if (error_code)
>>>> +	else if (error_code &&
>>>> +		error_code != READ_GITFILE_ERR_STAT_ENOENT &&
>>>> +		error_code != READ_GITFILE_ERR_IS_A_DIR)
>>>>   		read_gitfile_error_die(error_code, path, dir);
>>>>
>>>
>>> I understand the exclusion here (they are non-fatal flows), but wouldn't
>>> it more make sense to add these two exclusions within
>>> `read_gitfile_error_die()` which already has two such exclusions? By
>>> separating this out, it gets really confusing.
>>
>> Absolutely.  The point of this change, IIUC, is that these two
>> existing exclusions were too broad.  stat() can fail for many
>> reasons, but because we did not differenciate ENOENT (which we *are*
>> happy to see and do not want to consider an error) from all other
>> error cases (which we may have been better off if we diagnosed them
>> as error), we pretended both ENOENT and all other stat() failures
>> were happy case and "case ERR_STAT_FAILED:" covered both.
> 
> Yeah, so this is the situation before the patch, and I'm in agreement.
> 
>> To fix
>> this, the patch splits stat() failures into two, ERR_STAT_ENOENT is
>> the happy case we should have been returning without dying from
>> read_gitfile_error_die(), and ERR_STAT_FAILED is the rest that we
>> should have been dying there but in order to return from there
>> without dying when we got ENOENT, we were not dying there.  Now we
>> have a separate ERR_STAT_ENOENT, read_gitfile_error_die() can (and
>> should) die when we see ERR_STAT_FAILED, and it can (and should)
>> return to us when we see ERR_STAT_ENOENT as a happy case.
> 
> Okay, this is what I was expecting too, historically
> `read_gitfile_error_die()` treated ERR_STAT_FAILED as the non-fatal path
> which made sense. But now that we have ERR_STAT_ENOENT. It should treat
> the latter as the non-fatal path and the former as an actual issue.
> 
>> The story
>> is exactly the same between ERR_NOT_A_FILE (which had been non-error
>> only because we wanted to treat a directory as OK, but we can make
>> it an error) and ERR_IS_A DIR (which is new, and is an OK case).
>>
> 
> Yup makes sense.
> 
>> The above exception on the caller's side you quoted is a complete
>> opposite from that line of reasoning, and that is why it is
>> confusing.
>>
>> If there are other callers of read_gitfile_error_die() and different
>> semantics, such a "now we die on every possible errors" may also be
>> a valid position to take, *but* then it does not make sense unless
>> this patch makes read_gitfile_error_die() to die on ERR_STAT_FAILED
>> and ERR_NOT_A_FILE.
> 
> Exactly! Thanks for clearing it out.
> 
> Karthik