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

[Tian Yuchen <a3205153416@gmail.com>]

On 2/18/26 02:50, Karthik Nayak wrote:
> 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

I don't think leaving it up yo the caller to determine "which are error 
cases" is as confusing as you suggest. But honestly, your approach is 
indeed much clearer and more readable.

I'll split this into two commits and send them shortly.

Regards,

Yuchen