From: Karthik Nayak <karthik.188@gmail.com>
Date: Tue, 17 Feb 2026 11:26:10 GMT
Subject: Re: [PATCH v4] setup: allow cwd/.git to be a symlink to a directory
Message-ID: <CAOLa=ZTeTWhb0Yc8rPEv8vONTHtSg3bSvW6FBC-AWrZzi12oCA@mail.gmail.com>
In-Reply-To: <20260217084124.150366-1-a3205153416@gmail.com>

Tian Yuchen <a3205153416@gmail.com> writes:

> Strictly enforcing 'lstat()' and 'S_ISREG()' on '.git' prevents valid
> workflows where '.git' is a symbolic link pointing to a real git
> directory (e.g. created via 'ln -s').
>
> Refactor 'setup_git_directory_gently_1()' to use 'stat()' instead of
> 'lstat()'. This allows the filesystem to automatically resolve symbolic
> links.
>
> To ensure safety and correctness, the logic flow is updated to:
>
> 1. Ignore 'ENOENT' (file missing).
> 2. Check 'IS_A_DIR' cases via 'is_git_directory()'.
> 3. Explicitly reject 'NOT_A_FILE' cases (FIFOs or sockets).
>

Small nit, it would have been a bit nicer to separate these out into
individual commits with tests added per commit.

> Add a new test script t/t0009-setup-security.sh which verifies:
>

Wouldn't something like 't0009-git-dir-validation.sh' be a better name?

> - Valid .git symlinks to real directories are accepted.
> - .git as a named pipe (FIFO) is rejected.
> - .git as a symlink to a named pipe is rejected.
> - .git with garbage content is rejected.
> - Empty .git directories are ignored.
>
> Signed-off-by: Tian Yuchen <a3205153416@gmail.com>
> ---
>  setup.c                   | 39 ++++++++++++++-------
>  setup.h                   |  2 ++
>  t/t0009-setup-security.sh | 72 +++++++++++++++++++++++++++++++++++++++
>  3 files changed, 101 insertions(+), 12 deletions(-)
>  create mode 100755 t/t0009-setup-security.sh
>
> diff --git a/setup.c b/setup.c
> index 3a6a048620..269aa9faaa 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -939,8 +939,14 @@ const char *read_gitfile_gently(const char *path, int *return_error_code)
>  	static struct strbuf realpath = STRBUF_INIT;
>
>  	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;

So this differentiates between 'stat()' failing and the path not
existing. Ok.

> +	}
> +	if (S_ISDIR(st.st_mode)) {
> +		error_code = READ_GITFILE_ERR_IS_A_DIR;
>  		goto cleanup_return;
>  	}

Okay so if the '.git' file is a directory, we set the appropriate error.
So this error code is new and we introduce it in this patch.


>  	if (!S_ISREG(st.st_mode)) {
> @@ -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.

>  	free(buf);
> @@ -1576,20 +1584,27 @@ 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, die_on_error ?
> -						NULL : &error_code);
> +		gitdirenv = read_gitfile_gently(dir->buf, &error_code);

Okay so we unconditionally read the error into errorcode, quick question
that comes to mind: Wouldn't this break the previous flow for when
`die_on_error = 1`? Where `read_gitfile_error_die()` would've been
called?

>  		if (!gitdirenv) {
> -			if (die_on_error ||
> -			    error_code == READ_GITFILE_ERR_NOT_A_FILE) {
> -				/* NEEDSWORK: fail if .git is not file nor dir */
> +			if (error_code == READ_GITFILE_ERR_STAT_ENOENT) {
> +				;
> +			} else if (error_code == READ_GITFILE_ERR_IS_A_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;
> -		} else
> -			gitfile = xstrdup(dir->buf);
> +			} else if (error_code == READ_GITFILE_ERR_NOT_A_FILE) {
> +				if (die_on_error)
> +					die(_("Invalid %s: not a regular file or directory"), dir->buf);
> +				else
> +					return GIT_DIR_INVALID_GITFILE;
> +			} else if (error_code != READ_GITFILE_ERR_STAT_FAILED) {
> +				if (die_on_error)
> +					read_gitfile_error_die(error_code, dir->buf, NULL);

Well seems a bit convoluted as we explicitly skip this in our call to
`read_gitfile_gently()` to then call it ourselves.

> +				else
> +					return GIT_DIR_INVALID_GITFILE;
> +			}
> +		}
>  		/*
>  		 * Earlier, we tentatively added DEFAULT_GIT_DIR_ENVIRONMENT
>  		 * to check that directory for a repository.

[snip]