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

[Junio C Hamano <gitster@pobox.com>]

Tian Yuchen <a3205153416@gmail.com> writes:

> diff --git a/setup.c b/setup.c
> index c8336eb20e..5a573e5865 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -897,10 +897,13 @@ int verify_repository_format(const struct repository_format *format,
>  void read_gitfile_error_die(int error_code, const char *path, const char *dir)
>  {
>  	switch (error_code) {
> +	case READ_GITFILE_ERR_STAT_ENOENT:
> +	case READ_GITFILE_ERR_IS_A_DIR:
> +		break;
>  	case READ_GITFILE_ERR_STAT_FAILED:
> +		die(_("error reading %s"), path);
>  	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?

>  	if (stat(path, &st)) {
> -		/* NEEDSWORK: discern between ENOENT vs other errors */
> -		error_code = READ_GITFILE_ERR_STAT_FAILED;
> +		if (errno == ENOENT)
> +			error_code = READ_GITFILE_ERR_STAT_ENOENT;
> +		else
> +			error_code = READ_GITFILE_ERR_STAT_FAILED;
> +		goto cleanup_return;
> +	}
> +	if (S_ISDIR(st.st_mode)) {
> +		error_code = READ_GITFILE_ERR_IS_A_DIR;
>  		goto cleanup_return;
>  	}

OK.

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

	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.