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

[Tian Yuchen <a3205153416@gmail.com>]

On 2/19/26 02:25, Junio C Hamano wrote:
> Tian Yuchen <a3205153416@gmail.com> writes:
> 
>> Strictly enforcing 'lstat()' prevents valid '.git' symlinks.
> 
> But nobody sane would propose running one more lstat() anyway, so
> how is that relevant?
> 
>>   		if (!gitdirenv) {
>> -			if (die_on_error ||
>> -			    error_code == READ_GITFILE_ERR_NOT_A_FILE) {
>> -				/* NEEDSWORK: fail if .git is not file nor dir */
>> -				if (is_git_directory(dir->buf)) {
>> -					gitdirenv = DEFAULT_GIT_DIR_ENVIRONMENT;
>> -					gitdir_path = xstrdup(dir->buf);
>> -				}
>> -			} else if (error_code != READ_GITFILE_ERR_STAT_FAILED)
>> -				return GIT_DIR_INVALID_GITFILE;
> 
> The _intent_ of the original code was to
> 
>      * do is_git_directory() thing to deal with a plain vanilla
>        ".git" directory when read_gitfile_gently thing said "we found
>        a directory" (NOT_A_FILE is overly coarse, which is what we
>        are correcting in this topic, but the _intent_ was to do the
>        is_git_directory() thing when we know it is a directory).
> 
>      * return INVALID_GITFILE on any error, but do not return when
>        the reason why read_gitfile_gently thing failed was because
>        there is no ".git" there (again, STAT_FAILED is overly coarse,
>        which is what we are correcting in this topic, but the
>        _intent_ was to return INVALID thing when we not the failure
>        is not due to ENOENT).  Note that returning INVALID_GITFILE is
>        done when die_on_error is not set.
> 
>> -		} else
>> +			if (error_code)
>> +				read_gitfile_error_die(error_code, dir->buf, NULL);
> 
> Should this be unconditional?  If our caller did not ask us to die
> upon an error with die_on_error, what happens?  The original I think
> returned INVALID_GITFILE for the caller to deal with.
> 
>> +			if (is_git_directory(dir->buf)) {
> 
> Should this be unconditional?  If the thing is a directory, the
> original would have given us NOT_A_FILE but now it would give us
> IS_A_DIR.  And that is the only case original wanted to call
> is_git_directory() no?
> 
>> +				gitdirenv = DEFAULT_GIT_DIR_ENVIRONMENT;
>> +				gitdir_path = xstrdup(dir->buf);
>> +			}
>> +		} else {
>>   			gitfile = xstrdup(dir->buf);
>> +		}
>>   		/*
>>   		 * Earlier, we tentatively added DEFAULT_GIT_DIR_ENVIRONMENT
>>   		 * to check that directory for a repository.


Thanks for the review.

I have already sent out v6 yesterday. Here is the link:

https://lore.kernel.org/git/20260218124638.176936-1-a3205153416@gmail.com/

Based on your feedback and the changes that have been made on v6, it 
seems that the tasks that need to be completed are as follows:

  - Conditional 'is_git_directory' check: restrict the 
'is_git_directory()' check to only run when we explicitly get 
'READ_GITFILE_ERR_IS_A_DIR'. It makes no sense to check it for other 
error types.

  - Squash two patches into one single commit, as you suggested. 
(Actually, I'm a bit confused—are you saying to “make both patches 
standalone executable” or to “merge the two patches directly”? Either 
way, I'll go ahead and send the v7 patches first.)

  - Rephrase the commit message to describe the lstat() limitation, 
rather than saying 'we switch to stat()'

The holiday is over, and my efficiency in sending patches and replying 
to emails may be somewhat reduced. Please bear with me.

Regards,

Yuchen