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

[Tian Yuchen <a3205153416@gmail.com>]

On 2/20/26 11:40, Junio C Hamano wrote:

Thanks for the review!

>>   	case READ_GITFILE_ERR_NOT_A_FILE:
>> -		/* non-fatal; follow return path */
>> -		break;
> 
> This comment is now lost.  Shouldn't (at least /* non-fatal */ part of)
> it be moved to those two new non-error codes we see above?

Indeed. I made a mistake here.

>> @@ -1578,20 +1587,25 @@ static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,
>>   		if (offset > min_offset)
>>   			strbuf_addch(dir, '/');
>>   		strbuf_addstr(dir, DEFAULT_GIT_DIR_ENVIRONMENT);
>> +		gitdirenv = read_gitfile_gently(dir->buf, &error_code);
>>   		if (!gitdirenv) {
>> +			if (error_code == READ_GITFILE_ERR_IS_A_DIR &&
>> +			is_git_directory(dir->buf)) {
>> +				gitdirenv = DEFAULT_GIT_DIR_ENVIRONMENT;
>> +				gitdir_path = xstrdup(dir->buf);
>> +			} else {
>> +				if (error_code == READ_GITFILE_ERR_STAT_ENOENT ||
>> +				error_code == READ_GITFILE_ERR_IS_A_DIR ||
>> +				error_code == READ_GITFILE_ERR_NOT_A_FILE ||
>> +				die_on_error) {
>> +					read_gitfile_error_die(error_code, dir->buf, NULL);
>> +				} else {
>> +					return GIT_DIR_INVALID_GITFILE;
>>   				}
>> +			}
> 
> Is it just me who finds the above harder to follow than necessary?
> I would have expected something like

To be honest I can't find any reason why the above statement is 
difficult to understand. However, replacing it with a switch statement 
does make it much clearer. On this point, I completely agree with you. 
Will change soon.


> 	if (!gitdirenv) {
> 		switch (error_code) {
> 		case READ_GITFILE_ERR_IS_A_DIR:
> 			if (is_git_directory(dir->buf)) {
> 				...
> 			} else if (die_on_error) {
> 				die("'%s' is an invalid .git directory", dir->buf);
> 			} else {
> 				return GIT_DIR_INVALID_GITFILE;
> 			}
> 			break;
> 		case READ_GITFILE_ERR_STAT_NOENT:
> 			/* no .git in this directory, move on */
> 			break;
> 		default:
> 			if (die_on_error)
> 				read_gitfile_err_stat_noent(error_code, ...);
> 			else
> 				return GIT_DIR_INVALID_GITFILE;
> 		}
> 	}
> 
> or its equivalent, with the top-level switch rewritten into
> an if/elseif cascade.

Point taken.

Regards,

Yuchen