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

From: Karthik Nayak <karthik.188@gmail.com>
Date: Feb 17, 2026, 18:50 UTC
Message-ID: <CAOLa=ZQ8i0pUAbF8NGidR=6jVLJuiYdUWu=ZLDrJMek005D9bA@mail.gmail.com>
In-Reply-To: <xmqqv7fvcnj5.fsf@gitster.g>

Junio C Hamano <gitster@pobox.com> writes:

Show 25 quoted lines

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

Show 9 quoted lines

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

Show 5 quoted lines

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

Show 9 quoted lines

> 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