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

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