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

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

[Tian Yuchen]

Hi Junio, Karthik,

Here is the v6 reroll.

Changes since v5 (and previous discussions):

1. Fixed potential regression in gentle setup:
   In Patch 2/2, I fixed a logic bug where `setup_git_directory_gently()`
   could crash on generic errors (like `INVALID_FORMAT`) when `die_on_error`
   was false.
   The new logic only delegates to the die-handler for:
   - Benign cases we want to ignore (`ENOENT`, `IS_A_DIR`).
   - Security risks we MUST reject (`NOT_A_FILE`).
   - When `die_on_error` is explicitly true.

2. Refinements:
   - Used `break` instead of `return` in `read_gitfile_error_die()` (Patch 1/2).
   - Updated the error message for `NOT_A_FILE` to be more precise.

Thanks for the guidance! 


Tian Yuchen (2):
  setup: distinguish ENOENT from other stat errors
  setup: allow cwd/.git to be a symlink to a directory

 setup.c                       | 38 ++++++++++++------
 setup.h                       |  2 +
 t/meson.build                 |  1 +
 t/t0009-git-dir-validation.sh | 72 +++++++++++++++++++++++++++++++++++
 4 files changed, 101 insertions(+), 12 deletions(-)
 create mode 100755 t/t0009-git-dir-validation.sh

-- 
2.43.0

[PATCH v6 1/2] setup: distinguish ENOENT from other stat errors

[Tian Yuchen]

Currently, 'read_gitfile_gently()' treats all 'stat()' failures as
generic errors. This prevents distinguishing between a missing file and
real errors like permission denied (fatal).

Split 'READ_GITFILE_ERR_STAT_FAILED' into two:
 - 'READ_GITFILE_ERR_STAT_ENOENT': The file simply does not exist;
 - 'READ_GITFILE_ERR_STAT_FAILED': Real I/O or permission errors.

Introduce 'READ_GITFILE_ERR_IS_A_DIR':
 - Used when the path exists but is a directory.

Update 'read_gitfile_error_die()' to handle these cases:
 - Happy path ('ENOENT', 'IS_A_DIR'): Return without dying;
 - Fatal path ('STAT_FAILED', 'NOA_A_FILE'): Die immediately.

This prepares for error handling in the next commit.

Signed-off-by: Tian Yuchen <a3205153416@gmail.com>
---
 setup.c | 17 +++++++++++++----
 setup.h |  2 ++
 2 files changed, 15 insertions(+), 4 deletions(-)

diff --git a/setup.c b/setup.c
index c8336eb20e..d48b6a3a3d 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;
+		die(_("not a regular file: %s"), path);
 	case READ_GITFILE_ERR_OPEN_FAILED:
 		die_errno(_("error opening '%s'"), path);
 	case READ_GITFILE_ERR_TOO_LARGE:
@@ -941,8 +944,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;
+	}
+	if (S_ISDIR(st.st_mode)) {
+		error_code = READ_GITFILE_ERR_IS_A_DIR;
 		goto cleanup_return;
 	}
 	if (!S_ISREG(st.st_mode)) {
diff --git a/setup.h b/setup.h
index 0738dec244..ed4b13f061 100644
--- a/setup.h
+++ b/setup.h
@@ -36,6 +36,8 @@ int is_nonbare_repository_dir(struct strbuf *path);
 #define READ_GITFILE_ERR_NO_PATH 6
 #define READ_GITFILE_ERR_NOT_A_REPO 7
 #define READ_GITFILE_ERR_TOO_LARGE 8
+#define READ_GITFILE_ERR_STAT_ENOENT 9
+#define READ_GITFILE_ERR_IS_A_DIR 10
 void read_gitfile_error_die(int error_code, const char *path, const char *dir);
 const char *read_gitfile_gently(const char *path, int *return_error_code);
 #define read_gitfile(path) read_gitfile_gently((path), NULL)
-- 
2.43.0

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

[Tian Yuchen]

Strictly enforcing 'lstat()' prevents valid '.git' symlinks.

Rely on 'stat()' (via 'read_gitfile_gently()') to handle filesystem
resolution, instead of blocking it with strict logic checks in
'setup_git_directory_gently_1()'.

To ensure safety and correctness, we unconditionally delegate benign
cases ('ENOENT', 'IS_A_DIR') and security risks ('NOT_A_FILE') to
'read_gitfile_error_die()'.

For other errors (like invalid format), we only invoke the handler if
'die_on_error' is true; otherwise, we return the error code to respect
the gentle fallback behavior.

Add 't/t0009-git-dir-validation.sh' to verify symlink support and FIFO
rejection, and register it in 't/meson.build'.

Signed-off-by: Tian Yuchen <a3205153416@gmail.com>
---
 setup.c                       | 21 ++++++----
 t/meson.build                 |  1 +
 t/t0009-git-dir-validation.sh | 72 +++++++++++++++++++++++++++++++++++
 3 files changed, 86 insertions(+), 8 deletions(-)
 create mode 100755 t/t0009-git-dir-validation.sh

diff --git a/setup.c b/setup.c
index d48b6a3a3d..f4b9d41f78 100644
--- a/setup.c
+++ b/setup.c
@@ -1590,15 +1590,20 @@ static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,
 		gitdirenv = read_gitfile_gently(dir->buf, die_on_error ?
 						NULL : &error_code);
 		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);
+			if (error_code){
+				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;
 				}
-			} else if (error_code != READ_GITFILE_ERR_STAT_FAILED)
-				return GIT_DIR_INVALID_GITFILE;
+			}
+			if (is_git_directory(dir->buf)) {
+				gitdirenv = DEFAULT_GIT_DIR_ENVIRONMENT;
+				gitdir_path = xstrdup(dir->buf);
+			}
 		} else
 			gitfile = xstrdup(dir->buf);
 		/*
diff --git a/t/meson.build b/t/meson.build
index f80e366cff..c4afaacee5 100644
--- a/t/meson.build
+++ b/t/meson.build
@@ -80,6 +80,7 @@ integration_tests = [
   't0006-date.sh',
   't0007-git-var.sh',
   't0008-ignores.sh',
+  't0009-git-dir-validation.sh',
   't0010-racy-git.sh',
   't0012-help.sh',
   't0013-sha1dc.sh',
diff --git a/t/t0009-git-dir-validation.sh b/t/t0009-git-dir-validation.sh
new file mode 100755
index 0000000000..9b3925c85f
--- /dev/null
+++ b/t/t0009-git-dir-validation.sh
@@ -0,0 +1,72 @@
+#!/bin/sh
+
+test_description='setup: validation of .git file/directory types
+
+Verify that setup_git_directory() correctly handles:
+1. Valid .git directories (including symlinks to them).
+2. Invalid .git files (FIFOs, sockets) by erroring out.
+3. Invalid .git files (garbage) by erroring out.
+'
+
+. ./test-lib.sh
+
+test_expect_success 'setup: create parent git repository' '
+	git init parent &&
+	test_commit -C parent "root-commit"
+'
+
+test_expect_success SYMLINKS 'setup: .git as a symlink to a directory is valid' '
+	mkdir -p parent/link-to-dir &&
+	(
+		cd parent/link-to-dir &&
+		git init real-repo &&
+		ln -s real-repo/.git .git &&
+		git rev-parse --git-dir >actual &&
+		echo .git >expect &&
+		test_cmp expect actual
+	)
+'
+
+test_expect_success PIPE 'setup: .git as a FIFO (named pipe) is rejected' '
+	mkdir -p parent/fifo-trap &&
+	(
+		cd parent/fifo-trap &&
+		mkfifo .git &&
+		test_must_fail git rev-parse --git-dir 2>stderr &&
+		grep "not a regular file" stderr
+	)
+'
+
+test_expect_success SYMLINKS,PIPE 'setup: .git as a symlink to a FIFO is rejected' '
+	mkdir -p parent/symlink-fifo-trap &&
+	(
+		cd parent/symlink-fifo-trap &&
+		mkfifo target-fifo &&
+		ln -s target-fifo .git &&
+		test_must_fail git rev-parse --git-dir 2>stderr &&
+		grep "not a regular file" stderr
+	)
+'
+
+test_expect_success 'setup: .git with garbage content is rejected' '
+	mkdir -p parent/garbage-trap &&
+	(
+		cd parent/garbage-trap &&
+		echo "garbage" >.git &&
+		test_must_fail git rev-parse --git-dir 2>stderr &&
+		grep "invalid gitfile format" stderr
+	)
+'
+
+test_expect_success 'setup: .git as an empty directory is ignored' '
+	mkdir -p parent/empty-dir &&
+	(
+		cd parent/empty-dir &&
+		mkdir .git &&
+		git rev-parse --git-dir >actual &&
+		echo "$TRASH_DIRECTORY/parent/.git" >expect &&
+		test_cmp expect actual
+	)
+'
+
+test_done
-- 
2.43.0

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

[Tian Yuchen]

Currently, `setup_git_directory_gently_1()` fails to recognize a `.git`
symlink pointing to a directory because `read_gitfile_gently()` strictly
expects a regular file and returns `READ_GITFILE_ERR_NOT_A_FILE` for
anything else, including valid directories.

Fix this by distinguishing directories from regular files and other
non-regular file types (like FIFOs or sockets) via newly introduced
error_code.

To preserve the original intent of the setup process:
1. Update `read_gitfile_error_die()` to treat `IS_A_DIR` as a no-op
   (like `ENOENT`), while still calling `die()` on true `NOT_A_FILE`
   errors.
2. Unconditionally pass `&error_code` to `read_gitfile_gently()`. This
   eliminates an uninitialized variable hazard that occurred when
   `die_on_error` was true and `NULL` was passed.
3. Only invoke `is_git_directory()` when we explicitly receive
   `READ_GITFILE_ERR_IS_A_DIR`, avoiding redundant filesystem checks.
4. Correctly return `GIT_DIR_INVALID_GITFILE` on unrecognized errors
   when `die_on_error` is false.

Signed-off-by: Tian Yuchen <a3205153416@gmail.com>
---
Changes since v6:

 - Squashed into a single commit.
 - Fixed a hidden uninitialized variable trap. In v6:
   'setup_git_directory_gently_1()' passed 'die_on_error ? NULL : &error_code'
 to 'read_gitfile_gently()'. When 'NULL' was passed, the local 'error_code'
 remained uninitialized. The old code survived this because of short-circuit
 evaluation ('if (die_on_error || error_code == ...)'). 
   In this v7, I now unconditionally pass '&error_code'. This gives the caller
 explicit control over error routing.
   (Actually, I seem to have made the same modification back in v3 or v4, but I didn't
 realize at the time that the part I changed was originally a bug. So it was a 
 happy accident. :P
 - We now only invoke 'is_git_directory()' explicitly when 'error_code ==
 READ_GITFILE_ERR_IS_A_DIR'.

Thanks for your patience.

 setup.c                       | 44 +++++++++++++--------
 setup.h                       |  2 +
 t/meson.build                 |  1 +
 t/t0009-git-dir-validation.sh | 72 +++++++++++++++++++++++++++++++++++
 4 files changed, 104 insertions(+), 15 deletions(-)
 create mode 100755 t/t0009-git-dir-validation.sh

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;
+		die(_("not a regular file: %s"), path);
 	case READ_GITFILE_ERR_OPEN_FAILED:
 		die_errno(_("error opening '%s'"), path);
 	case READ_GITFILE_ERR_TOO_LARGE:
@@ -941,8 +944,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;
+	}
+	if (S_ISDIR(st.st_mode)) {
+		error_code = READ_GITFILE_ERR_IS_A_DIR;
 		goto cleanup_return;
 	}
 	if (!S_ISREG(st.st_mode)) {
@@ -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, die_on_error ?
-						NULL : &error_code);
+		gitdirenv = read_gitfile_gently(dir->buf, &error_code);
 		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);
+			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;
 				}
-			} else if (error_code != READ_GITFILE_ERR_STAT_FAILED)
-				return GIT_DIR_INVALID_GITFILE;
-		} else
+			}
+		} else {
 			gitfile = xstrdup(dir->buf);
+		}
 		/*
 		 * Earlier, we tentatively added DEFAULT_GIT_DIR_ENVIRONMENT
 		 * to check that directory for a repository.
diff --git a/setup.h b/setup.h
index 0738dec244..ed4b13f061 100644
--- a/setup.h
+++ b/setup.h
@@ -36,6 +36,8 @@ int is_nonbare_repository_dir(struct strbuf *path);
 #define READ_GITFILE_ERR_NO_PATH 6
 #define READ_GITFILE_ERR_NOT_A_REPO 7
 #define READ_GITFILE_ERR_TOO_LARGE 8
+#define READ_GITFILE_ERR_STAT_ENOENT 9
+#define READ_GITFILE_ERR_IS_A_DIR 10
 void read_gitfile_error_die(int error_code, const char *path, const char *dir);
 const char *read_gitfile_gently(const char *path, int *return_error_code);
 #define read_gitfile(path) read_gitfile_gently((path), NULL)
diff --git a/t/meson.build b/t/meson.build
index f80e366cff..c4afaacee5 100644
--- a/t/meson.build
+++ b/t/meson.build
@@ -80,6 +80,7 @@ integration_tests = [
   't0006-date.sh',
   't0007-git-var.sh',
   't0008-ignores.sh',
+  't0009-git-dir-validation.sh',
   't0010-racy-git.sh',
   't0012-help.sh',
   't0013-sha1dc.sh',
diff --git a/t/t0009-git-dir-validation.sh b/t/t0009-git-dir-validation.sh
new file mode 100755
index 0000000000..9b3925c85f
--- /dev/null
+++ b/t/t0009-git-dir-validation.sh
@@ -0,0 +1,72 @@
+#!/bin/sh
+
+test_description='setup: validation of .git file/directory types
+
+Verify that setup_git_directory() correctly handles:
+1. Valid .git directories (including symlinks to them).
+2. Invalid .git files (FIFOs, sockets) by erroring out.
+3. Invalid .git files (garbage) by erroring out.
+'
+
+. ./test-lib.sh
+
+test_expect_success 'setup: create parent git repository' '
+	git init parent &&
+	test_commit -C parent "root-commit"
+'
+
+test_expect_success SYMLINKS 'setup: .git as a symlink to a directory is valid' '
+	mkdir -p parent/link-to-dir &&
+	(
+		cd parent/link-to-dir &&
+		git init real-repo &&
+		ln -s real-repo/.git .git &&
+		git rev-parse --git-dir >actual &&
+		echo .git >expect &&
+		test_cmp expect actual
+	)
+'
+
+test_expect_success PIPE 'setup: .git as a FIFO (named pipe) is rejected' '
+	mkdir -p parent/fifo-trap &&
+	(
+		cd parent/fifo-trap &&
+		mkfifo .git &&
+		test_must_fail git rev-parse --git-dir 2>stderr &&
+		grep "not a regular file" stderr
+	)
+'
+
+test_expect_success SYMLINKS,PIPE 'setup: .git as a symlink to a FIFO is rejected' '
+	mkdir -p parent/symlink-fifo-trap &&
+	(
+		cd parent/symlink-fifo-trap &&
+		mkfifo target-fifo &&
+		ln -s target-fifo .git &&
+		test_must_fail git rev-parse --git-dir 2>stderr &&
+		grep "not a regular file" stderr
+	)
+'
+
+test_expect_success 'setup: .git with garbage content is rejected' '
+	mkdir -p parent/garbage-trap &&
+	(
+		cd parent/garbage-trap &&
+		echo "garbage" >.git &&
+		test_must_fail git rev-parse --git-dir 2>stderr &&
+		grep "invalid gitfile format" stderr
+	)
+'
+
+test_expect_success 'setup: .git as an empty directory is ignored' '
+	mkdir -p parent/empty-dir &&
+	(
+		cd parent/empty-dir &&
+		mkdir .git &&
+		git rev-parse --git-dir >actual &&
+		echo "$TRASH_DIRECTORY/parent/.git" >expect &&
+		test_cmp expect actual
+	)
+'
+
+test_done
-- 
2.43.0

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

[Junio C Hamano]

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.

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

[Tian Yuchen]

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

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

[Tian Yuchen]

Currently, `setup_git_directory_gently_1()` fails to recognize a `.git`
symlink pointing to a directory because `read_gitfile_gently()` strictly
expects a regular file and returns `READ_GITFILE_ERR_NOT_A_FILE` for
anything else, including valid directories.

Fix this by distinguishing directories from regular files and other
non-regular file types (like FIFOs or sockets) via newly introduced
error_code.

To preserve the original intent of the setup process:
1. Update `read_gitfile_error_die()` to treat `IS_A_DIR` as a no-op
   (like `ENOENT`), while still calling `die()` on true `NOT_A_FILE`
   errors.
2. Unconditionally pass `&error_code` to `read_gitfile_gently()`. This
   eliminates an uninitialized variable hazard that occurred when
   `die_on_error` was true and `NULL` was passed.
3. Only invoke `is_git_directory()` when we explicitly receive
   `READ_GITFILE_ERR_IS_A_DIR`, avoiding redundant filesystem checks.
4. Correctly return `GIT_DIR_INVALID_GITFILE` on unrecognized errors
   when `die_on_error` is false.

Signed-off-by: Tian Yuchen <a3205153416@gmail.com>
---
 setup.c                       | 42 ++++++++++++++------
 setup.h                       |  2 +
 t/meson.build                 |  1 +
 t/t0009-git-dir-validation.sh | 72 +++++++++++++++++++++++++++++++++++
 4 files changed, 105 insertions(+), 12 deletions(-)
 create mode 100755 t/t0009-git-dir-validation.sh

diff --git a/setup.c b/setup.c
index c8336eb20e..2869d10669 100644
--- a/setup.c
+++ b/setup.c
@@ -897,10 +897,14 @@ 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_FAILED:
-	case READ_GITFILE_ERR_NOT_A_FILE:
+	case READ_GITFILE_ERR_STAT_ENOENT:
+	case READ_GITFILE_ERR_IS_A_DIR:
 		/* non-fatal; follow return path */
 		break;
+	case READ_GITFILE_ERR_STAT_FAILED:
+		die(_("error reading %s"), path);
+	case READ_GITFILE_ERR_NOT_A_FILE:
+		die(_("not a regular file: %s"), path);
 	case READ_GITFILE_ERR_OPEN_FAILED:
 		die_errno(_("error opening '%s'"), path);
 	case READ_GITFILE_ERR_TOO_LARGE:
@@ -941,8 +945,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;
+	}
+	if (S_ISDIR(st.st_mode)) {
+		error_code = READ_GITFILE_ERR_IS_A_DIR;
 		goto cleanup_return;
 	}
 	if (!S_ISREG(st.st_mode)) {
@@ -1578,20 +1588,28 @@ 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);
 		if (!gitdirenv) {
-			if (die_on_error ||
-			    error_code == READ_GITFILE_ERR_NOT_A_FILE) {
-				/* NEEDSWORK: fail if .git is not file nor dir */
+			switch (error_code) {
+			case READ_GITFILE_ERR_STAT_ENOENT:
+				/* no .git in this directory, move on */
+				break;
+			case 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
+				/* otherwise, it is an empty/unrelated directory, move on */
+				break;
+			default:
+				if (die_on_error || error_code == READ_GITFILE_ERR_NOT_A_FILE)
+					read_gitfile_error_die(error_code, dir->buf, NULL);
+				else
+					return GIT_DIR_INVALID_GITFILE;
+			}
+		} else {
 			gitfile = xstrdup(dir->buf);
+		}
 		/*
 		 * Earlier, we tentatively added DEFAULT_GIT_DIR_ENVIRONMENT
 		 * to check that directory for a repository.
diff --git a/setup.h b/setup.h
index 0738dec244..ed4b13f061 100644
--- a/setup.h
+++ b/setup.h
@@ -36,6 +36,8 @@ int is_nonbare_repository_dir(struct strbuf *path);
 #define READ_GITFILE_ERR_NO_PATH 6
 #define READ_GITFILE_ERR_NOT_A_REPO 7
 #define READ_GITFILE_ERR_TOO_LARGE 8
+#define READ_GITFILE_ERR_STAT_ENOENT 9
+#define READ_GITFILE_ERR_IS_A_DIR 10
 void read_gitfile_error_die(int error_code, const char *path, const char *dir);
 const char *read_gitfile_gently(const char *path, int *return_error_code);
 #define read_gitfile(path) read_gitfile_gently((path), NULL)
diff --git a/t/meson.build b/t/meson.build
index f80e366cff..c4afaacee5 100644
--- a/t/meson.build
+++ b/t/meson.build
@@ -80,6 +80,7 @@ integration_tests = [
   't0006-date.sh',
   't0007-git-var.sh',
   't0008-ignores.sh',
+  't0009-git-dir-validation.sh',
   't0010-racy-git.sh',
   't0012-help.sh',
   't0013-sha1dc.sh',
diff --git a/t/t0009-git-dir-validation.sh b/t/t0009-git-dir-validation.sh
new file mode 100755
index 0000000000..9b3925c85f
--- /dev/null
+++ b/t/t0009-git-dir-validation.sh
@@ -0,0 +1,72 @@
+#!/bin/sh
+
+test_description='setup: validation of .git file/directory types
+
+Verify that setup_git_directory() correctly handles:
+1. Valid .git directories (including symlinks to them).
+2. Invalid .git files (FIFOs, sockets) by erroring out.
+3. Invalid .git files (garbage) by erroring out.
+'
+
+. ./test-lib.sh
+
+test_expect_success 'setup: create parent git repository' '
+	git init parent &&
+	test_commit -C parent "root-commit"
+'
+
+test_expect_success SYMLINKS 'setup: .git as a symlink to a directory is valid' '
+	mkdir -p parent/link-to-dir &&
+	(
+		cd parent/link-to-dir &&
+		git init real-repo &&
+		ln -s real-repo/.git .git &&
+		git rev-parse --git-dir >actual &&
+		echo .git >expect &&
+		test_cmp expect actual
+	)
+'
+
+test_expect_success PIPE 'setup: .git as a FIFO (named pipe) is rejected' '
+	mkdir -p parent/fifo-trap &&
+	(
+		cd parent/fifo-trap &&
+		mkfifo .git &&
+		test_must_fail git rev-parse --git-dir 2>stderr &&
+		grep "not a regular file" stderr
+	)
+'
+
+test_expect_success SYMLINKS,PIPE 'setup: .git as a symlink to a FIFO is rejected' '
+	mkdir -p parent/symlink-fifo-trap &&
+	(
+		cd parent/symlink-fifo-trap &&
+		mkfifo target-fifo &&
+		ln -s target-fifo .git &&
+		test_must_fail git rev-parse --git-dir 2>stderr &&
+		grep "not a regular file" stderr
+	)
+'
+
+test_expect_success 'setup: .git with garbage content is rejected' '
+	mkdir -p parent/garbage-trap &&
+	(
+		cd parent/garbage-trap &&
+		echo "garbage" >.git &&
+		test_must_fail git rev-parse --git-dir 2>stderr &&
+		grep "invalid gitfile format" stderr
+	)
+'
+
+test_expect_success 'setup: .git as an empty directory is ignored' '
+	mkdir -p parent/empty-dir &&
+	(
+		cd parent/empty-dir &&
+		mkdir .git &&
+		git rev-parse --git-dir >actual &&
+		echo "$TRASH_DIRECTORY/parent/.git" >expect &&
+		test_cmp expect actual
+	)
+'
+
+test_done
-- 
2.43.0

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

[Junio C Hamano]

Tian Yuchen <a3205153416@gmail.com> writes:

> Currently, `setup_git_directory_gently_1()` fails to recognize a `.git`
> symlink pointing to a directory because `read_gitfile_gently()` strictly
> expects a regular file and returns `READ_GITFILE_ERR_NOT_A_FILE` for
> anything else, including valid directories.

I have to admit that I was not paying too much attention to this
part of the proposed log message.  We kept seeing the above (and the
title) for the series forever, but is it really a problem if we did
not allow you to make a symbolic link to somebody else's .git/
directory?  Besides, I do not think we have such a problem at all,
as read_gitfile_gently() uses stat() not lstat().

	Side note: In any case, we do not want to see "Currently".
        We give an observation on how the current system works in
        the present tense (so no need to say "Currently X is Y", or
        "Previously X was Y" to describe the state before your
        change; just "X is Y" is enough), and discuss what you
        perceive as a problem in it.

I just did this, and it worked just fine as expected.

    $ git clone https://git.kernel.org/pub/scm/git/git.git/ git
    $ mv git/.git git.git
    $ ln -s ../git.git git/.git
    $ git -C git log

Even if there were a problem in making such a symbolic link work,
these days we have a textual "gitdir: $there" file as an official
way to let you keep the repository data on a filesystem that is
different from the filesystem that hosts its working tree.

Anyway, I've always thought that this topic is about addressing the
two NEEDSWORK comments to allow us to give better filesystem problem
diagnoses.

> Fix this by distinguishing directories from regular files and other
> non-regular file types (like FIFOs or sockets) via newly introduced
> error_code.

So, "Fix this" written here does not resonate with my understanding
of what we have been discussing so far.  Puzzled.

> diff --git a/setup.c b/setup.c
> index c8336eb20e..2869d10669 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -897,10 +897,14 @@ 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_FAILED:
> -	case READ_GITFILE_ERR_NOT_A_FILE:
> +	case READ_GITFILE_ERR_STAT_ENOENT:
> +	case READ_GITFILE_ERR_IS_A_DIR:
>  		/* non-fatal; follow return path */
>  		break;
> +	case READ_GITFILE_ERR_STAT_FAILED:
> +		die(_("error reading %s"), path);
> +	case READ_GITFILE_ERR_NOT_A_FILE:
> +		die(_("not a regular file: %s"), path);
>  	case READ_GITFILE_ERR_OPEN_FAILED:
>  		die_errno(_("error opening '%s'"), path);
>  	case READ_GITFILE_ERR_TOO_LARGE:
> @@ -941,8 +945,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;
> +	}
> +	if (S_ISDIR(st.st_mode)) {
> +		error_code = READ_GITFILE_ERR_IS_A_DIR;
>  		goto cleanup_return;
>  	}
>  	if (!S_ISREG(st.st_mode)) {

All of the above are exactly what I expected to see.  Nice.

> @@ -1578,20 +1588,28 @@ 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);
>  		if (!gitdirenv) {
> +			switch (error_code) {
> +			case READ_GITFILE_ERR_STAT_ENOENT:
> +				/* no .git in this directory, move on */
> +				break;
> +			case READ_GITFILE_ERR_IS_A_DIR:
>  				if (is_git_directory(dir->buf)) {
>  					gitdirenv = DEFAULT_GIT_DIR_ENVIRONMENT;
>  					gitdir_path = xstrdup(dir->buf);
>  				}
> +				/* otherwise, it is an empty/unrelated directory, move on */
> +				break;

This design decision may be debatable, but not tightening everything
at once may be a prudent thing to do to avoid accidental regression.

Having said that.

If you have a directory ".git/" somewhere in your working tree, and
the directory is somehow corrupt that is_git_directory() says "nope,
that is not a valid Git directory", wouldn't you rather want to know
about it as a potential problem?  Perhaps there are valid use cases
to have such a directory (you have "empty" listed here, which may or
may not have a valid use case), but it smells like falling into the
same bucket as "Gee we have .git that is a fifo here---what is going
on???", which is ...

> +			default:
> +				if (die_on_error || error_code == READ_GITFILE_ERR_NOT_A_FILE)
> +					read_gitfile_error_die(error_code, dir->buf, NULL);
> +				else
> +					return GIT_DIR_INVALID_GITFILE;

... what we do here.


So, I dunno.

> +			}
> +		} else {
>  			gitfile = xstrdup(dir->buf);
> +		}

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

[Tian Yuchen]

Hi junio,

> So, "Fix this" written here does not resonate with my understanding
> of what we have been discussing so far.  Puzzled.

You are completely right to be puzzled.

In an earlier iteration of this patch, another reviewer brought up a 
case regarding symlinks. I adjusted the logic and wrote the commit 
message around that "symlinks" narrative.

Through the subsequent refactors up to v8, the code evolved to tackle 
the much deeper issue-addressing the two decade-old 'NEEDSWORK' 
comments. The code structure was completely rewritten but I forgot that 
the symlinks stuff was not relevant anymore.

You are right. The actual value of this patch is the error code 
refactoring, not fixing symlinks.

> All of the above are exactly what I expected to see.  Nice.

I'm glad I finally got it right ;)

> This design decision may be debatable, but not tightening everything
> at once may be a prudent thing to do to avoid accidental regression.
> 
> Having said that.
>
> If you have a directory ".git/" somewhere in your working tree, and
> the directory is somehow corrupt that is_git_directory() says "nope,
> that is not a valid Git directory", wouldn't you rather want to know
> about it as a potential problem?

Great point. A corrupt '.git' dir is definitely a red flag. However, 
silently ignoring it and moving on has been the historical behavior, 
hasn't it?

Still, if we decide to tighten this in the future, it will be very 
simple change within this new 'switch' structure. Nothing much to worry 
about IMO.

Will send v9 soon, with commit message rewritten.

Regards,

Yuchen

[PATCH v9] setup: improve error diagnosis for invalid .git files

[Tian Yuchen]

'read_gitfile_gently()' treats any non-regular file as
'READ_GITFILE_ERR_NOT_A_FILE' and fails to discern between 'ENOENT'
and other stat failures. This flawed error reporting is noted by two
'NEEDSWORK' comments.

Address these comments by introducing two new error codes:
'READ_GITFILE_ERR_STAT_ENOENT' and 'READ_GITFILE_ERR_IS_A_DIR'.

To preserve the original intent of the setup process:
1. Update 'read_gitfile_error_die()' to treat 'IS_A_DIR' as a no-op
   (like 'ENOENT'), while still calling 'die()' on true 'NOT_A_FILE'
   errors.
2. Unconditionally pass '&error_code' to 'read_gitfile_gently()'. This
   eliminates an uninitialized variable hazard that occurred when
   'die_on_error' was true and 'NULL' was passed.
3. Only invoke 'is_git_directory()' when we explicitly receive
   'READ_GITFILE_ERR_IS_A_DIR', avoiding redundant filesystem checks.
4. Correctly return 'GIT_DIR_INVALID_GITFILE' on unrecognized errors
   when 'die_on_error' is false.

Signed-off-by: Tian Yuchen <a3205153416@gmail.com>
---
 setup.c                       | 42 ++++++++++++++------
 setup.h                       |  2 +
 t/meson.build                 |  1 +
 t/t0009-git-dir-validation.sh | 72 +++++++++++++++++++++++++++++++++++
 4 files changed, 105 insertions(+), 12 deletions(-)
 create mode 100755 t/t0009-git-dir-validation.sh

diff --git a/setup.c b/setup.c
index c8336eb20e..2869d10669 100644
--- a/setup.c
+++ b/setup.c
@@ -897,10 +897,14 @@ 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_FAILED:
-	case READ_GITFILE_ERR_NOT_A_FILE:
+	case READ_GITFILE_ERR_STAT_ENOENT:
+	case READ_GITFILE_ERR_IS_A_DIR:
 		/* non-fatal; follow return path */
 		break;
+	case READ_GITFILE_ERR_STAT_FAILED:
+		die(_("error reading %s"), path);
+	case READ_GITFILE_ERR_NOT_A_FILE:
+		die(_("not a regular file: %s"), path);
 	case READ_GITFILE_ERR_OPEN_FAILED:
 		die_errno(_("error opening '%s'"), path);
 	case READ_GITFILE_ERR_TOO_LARGE:
@@ -941,8 +945,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;
+	}
+	if (S_ISDIR(st.st_mode)) {
+		error_code = READ_GITFILE_ERR_IS_A_DIR;
 		goto cleanup_return;
 	}
 	if (!S_ISREG(st.st_mode)) {
@@ -1578,20 +1588,28 @@ 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);
 		if (!gitdirenv) {
-			if (die_on_error ||
-			    error_code == READ_GITFILE_ERR_NOT_A_FILE) {
-				/* NEEDSWORK: fail if .git is not file nor dir */
+			switch (error_code) {
+			case READ_GITFILE_ERR_STAT_ENOENT:
+				/* no .git in this directory, move on */
+				break;
+			case 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
+				/* otherwise, it is an empty/unrelated directory, move on */
+				break;
+			default:
+				if (die_on_error || error_code == READ_GITFILE_ERR_NOT_A_FILE)
+					read_gitfile_error_die(error_code, dir->buf, NULL);
+				else
+					return GIT_DIR_INVALID_GITFILE;
+			}
+		} else {
 			gitfile = xstrdup(dir->buf);
+		}
 		/*
 		 * Earlier, we tentatively added DEFAULT_GIT_DIR_ENVIRONMENT
 		 * to check that directory for a repository.
diff --git a/setup.h b/setup.h
index 0738dec244..ed4b13f061 100644
--- a/setup.h
+++ b/setup.h
@@ -36,6 +36,8 @@ int is_nonbare_repository_dir(struct strbuf *path);
 #define READ_GITFILE_ERR_NO_PATH 6
 #define READ_GITFILE_ERR_NOT_A_REPO 7
 #define READ_GITFILE_ERR_TOO_LARGE 8
+#define READ_GITFILE_ERR_STAT_ENOENT 9
+#define READ_GITFILE_ERR_IS_A_DIR 10
 void read_gitfile_error_die(int error_code, const char *path, const char *dir);
 const char *read_gitfile_gently(const char *path, int *return_error_code);
 #define read_gitfile(path) read_gitfile_gently((path), NULL)
diff --git a/t/meson.build b/t/meson.build
index f80e366cff..c4afaacee5 100644
--- a/t/meson.build
+++ b/t/meson.build
@@ -80,6 +80,7 @@ integration_tests = [
   't0006-date.sh',
   't0007-git-var.sh',
   't0008-ignores.sh',
+  't0009-git-dir-validation.sh',
   't0010-racy-git.sh',
   't0012-help.sh',
   't0013-sha1dc.sh',
diff --git a/t/t0009-git-dir-validation.sh b/t/t0009-git-dir-validation.sh
new file mode 100755
index 0000000000..9b3925c85f
--- /dev/null
+++ b/t/t0009-git-dir-validation.sh
@@ -0,0 +1,72 @@
+#!/bin/sh
+
+test_description='setup: validation of .git file/directory types
+
+Verify that setup_git_directory() correctly handles:
+1. Valid .git directories (including symlinks to them).
+2. Invalid .git files (FIFOs, sockets) by erroring out.
+3. Invalid .git files (garbage) by erroring out.
+'
+
+. ./test-lib.sh
+
+test_expect_success 'setup: create parent git repository' '
+	git init parent &&
+	test_commit -C parent "root-commit"
+'
+
+test_expect_success SYMLINKS 'setup: .git as a symlink to a directory is valid' '
+	mkdir -p parent/link-to-dir &&
+	(
+		cd parent/link-to-dir &&
+		git init real-repo &&
+		ln -s real-repo/.git .git &&
+		git rev-parse --git-dir >actual &&
+		echo .git >expect &&
+		test_cmp expect actual
+	)
+'
+
+test_expect_success PIPE 'setup: .git as a FIFO (named pipe) is rejected' '
+	mkdir -p parent/fifo-trap &&
+	(
+		cd parent/fifo-trap &&
+		mkfifo .git &&
+		test_must_fail git rev-parse --git-dir 2>stderr &&
+		grep "not a regular file" stderr
+	)
+'
+
+test_expect_success SYMLINKS,PIPE 'setup: .git as a symlink to a FIFO is rejected' '
+	mkdir -p parent/symlink-fifo-trap &&
+	(
+		cd parent/symlink-fifo-trap &&
+		mkfifo target-fifo &&
+		ln -s target-fifo .git &&
+		test_must_fail git rev-parse --git-dir 2>stderr &&
+		grep "not a regular file" stderr
+	)
+'
+
+test_expect_success 'setup: .git with garbage content is rejected' '
+	mkdir -p parent/garbage-trap &&
+	(
+		cd parent/garbage-trap &&
+		echo "garbage" >.git &&
+		test_must_fail git rev-parse --git-dir 2>stderr &&
+		grep "invalid gitfile format" stderr
+	)
+'
+
+test_expect_success 'setup: .git as an empty directory is ignored' '
+	mkdir -p parent/empty-dir &&
+	(
+		cd parent/empty-dir &&
+		mkdir .git &&
+		git rev-parse --git-dir >actual &&
+		echo "$TRASH_DIRECTORY/parent/.git" >expect &&
+		test_cmp expect actual
+	)
+'
+
+test_done
-- 
2.43.0

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

[Junio C Hamano]

Tian Yuchen <a3205153416@gmail.com> writes:

>> This design decision may be debatable, but not tightening everything
>> at once may be a prudent thing to do to avoid accidental regression.
>> 
>> Having said that.
>>
>> If you have a directory ".git/" somewhere in your working tree, and
>> the directory is somehow corrupt that is_git_directory() says "nope,
>> that is not a valid Git directory", wouldn't you rather want to know
>> about it as a potential problem?
>
> Great point. A corrupt '.git' dir is definitely a red flag. However, 
> silently ignoring it and moving on has been the historical behavior, 
> hasn't it?

Exactly.  That is where my reference to "not tightening everything
at once" comes from.

> Still, if we decide to tighten this in the future, it will be very 
> simple change within this new 'switch' structure. Nothing much to worry 
> about IMO.

Yup.  Perhaps it would deserve a new "/* NEEDSWORK: should we catch a
directory .git that is not a git directory here? */" comment there.

> Will send v9 soon, with commit message rewritten.

Thanks.

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

[Tian Yuchen]

Hi Junio,

Thanks for the review.
> Exactly.  That is where my reference to "not tightening everything
> at once" comes from.

Happy that I'm on the right track.

> Yup.  Perhaps it would deserve a new "/* NEEDSWORK: should we catch a
> directory .git that is not a git directory here? */" comment there.

Indeed. Will add soon.

Regards,

Yuchen

Re: [PATCH v9] setup: improve error diagnosis for invalid .git files

[Junio C Hamano]

Tian Yuchen <a3205153416@gmail.com> writes:

>  void read_gitfile_error_die(int error_code, const char *path, const char *dir)
>  {
>  	switch (error_code) {
> -	case READ_GITFILE_ERR_STAT_FAILED:
> -	case READ_GITFILE_ERR_NOT_A_FILE:
> +	case READ_GITFILE_ERR_STAT_ENOENT:
> +	case READ_GITFILE_ERR_IS_A_DIR:
>  		/* non-fatal; follow return path */
>  		break;
> +	case READ_GITFILE_ERR_STAT_FAILED:
> +		die(_("error reading %s"), path);
> +	case READ_GITFILE_ERR_NOT_A_FILE:
> +		die(_("not a regular file: %s"), path);
>  	case READ_GITFILE_ERR_OPEN_FAILED:
>  		die_errno(_("error opening '%s'"), path);
>  	case READ_GITFILE_ERR_TOO_LARGE:

The changes to these two functions require us to audit callers of
them that are outside the call graph of the main focus of this
patch.  For example, we see the following code in submodule.c:

        void absorb_git_dir_into_superproject(const char *path,
                                              const char *super_prefix)
        {
                int err_code;
                const char *sub_git_dir;
                struct strbuf gitdir = STRBUF_INIT;

                if (validate_submodule_path(path) < 0)
                        exit(128);

                strbuf_addf(&gitdir, "%s/.git", path);
                sub_git_dir = resolve_gitdir_gently(gitdir.buf, &err_code);

                /* Not populated? */
                if (!sub_git_dir) {
                        const struct submodule *sub;
                        struct strbuf sub_gitdir = STRBUF_INIT;

                        if (err_code == READ_GITFILE_ERR_STAT_FAILED) {
                                /* unpopulated as expected */
                                strbuf_release(&gitdir);
                                return;
                        }

                        if (err_code != READ_GITFILE_ERR_NOT_A_REPO)
                                /* We don't know what broke here. */
                                read_gitfile_error_die(err_code, path, NULL);

Let's take a look.

ERR_STAT_FAILED used to be "If we got ENOENT, it is an unpopulated
submodule so it is OK; it is so unlikely that we get other kinds of
failure and we cannot deal with them anyway".

With the patch we have been looking at, shouldn't the above code
check for ERR_STAT_ENOENT instead, to do the same "unpopulated,
nothing to do here, happy!" return?

This is merely just one example.  All hits from "git grep" for the
functions whose semantics have been updated by the patch must be
looked at in a similar way, but I didn't look at how many releavnt
code paths there are.  It could be an easier way to find code paths
that may need to be updated to grep for ERR_STAT_FAILED and
ERR_NOT_A_FILE, the two symbols whose meaning have changed.

Thanks.

Re: [PATCH v9] setup: improve error diagnosis for invalid .git files

[Tian Yuchen]

Hi Junio,

On 2/22/26 13:42, Junio C Hamano wrote:

> The changes to these two functions require us to audit callers of
> them that are outside the call graph of the main focus of this
> patch.  For example, we see the following code in submodule.c:

Oops, I was so focused on setup.c that I completely overlooked the 
external callers of the API. Thank you for pointing out!

I ran git grep for READ_GITFILE_ERR_STAT_FAILED and 
READ_GITFILE_ERR_NOT_A_FILE across the tree and audited the hits. 
Besides the expected changes in setup.c, here is what I found and fixed:

  - In submodule.c, exactly as you pointed out.
  - In worktree.c there were two places relying on NOT_A_FILE to print 
the specific ".git is not a file" error. I added 
READ_GITFILE_ERR_IS_A_DIR to those conditions to prevent the error from 
degrading to the generic ".git file broken".

I have also added the NEEDSWORK comment in the switch statedment, as you 
mentioned earlier.

Thanks,

Yuchen

[PATCH v10] setup: improve error diagnosis for invalid .git files

[Tian Yuchen]

'read_gitfile_gently()' treats any non-regular file as
'READ_GITFILE_ERR_NOT_A_FILE' and fails to discern between 'ENOENT'
and other stat failures. This flawed error reporting is noted by two
'NEEDSWORK' comments.

Address these comments by introducing two new error codes:
'READ_GITFILE_ERR_STAT_ENOENT' and 'READ_GITFILE_ERR_IS_A_DIR'.

To preserve the original intent of the setup process:
1. Update 'read_gitfile_error_die()' to treat 'IS_A_DIR' as a no-op
   (like 'ENOENT'), while still calling 'die()' on true 'NOT_A_FILE'
   errors.
2. Unconditionally pass '&error_code' to 'read_gitfile_gently()'. This
   eliminates an uninitialized variable hazard that occurred when
   'die_on_error' was true and 'NULL' was passed.
3. Only invoke 'is_git_directory()' when we explicitly receive
   'READ_GITFILE_ERR_IS_A_DIR', avoiding redundant filesystem checks.
4. Correctly return 'GIT_DIR_INVALID_GITFILE' on unrecognized errors
   when 'die_on_error' is false.

Additionally, audit external callers of 'read_gitfile_gently()' in
'submodule.c' and 'worktree.c' to accommodate the refined error codes.

Signed-off-by: Tian Yuchen <a3205153416@gmail.com>
---
 setup.c                       | 42 ++++++++++++++------
 setup.h                       |  2 +
 submodule.c                   |  2 +-
 t/meson.build                 |  1 +
 t/t0009-git-dir-validation.sh | 72 +++++++++++++++++++++++++++++++++++
 worktree.c                    |  6 ++-
 6 files changed, 110 insertions(+), 15 deletions(-)
 create mode 100755 t/t0009-git-dir-validation.sh

diff --git a/setup.c b/setup.c
index c8336eb20e..9d49b9ae53 100644
--- a/setup.c
+++ b/setup.c
@@ -897,10 +897,14 @@ 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_FAILED:
-	case READ_GITFILE_ERR_NOT_A_FILE:
+	case READ_GITFILE_ERR_STAT_ENOENT:
+	case READ_GITFILE_ERR_IS_A_DIR:
 		/* non-fatal; follow return path */
 		break;
+	case READ_GITFILE_ERR_STAT_FAILED:
+		die(_("error reading %s"), path);
+	case READ_GITFILE_ERR_NOT_A_FILE:
+		die(_("not a regular file: %s"), path);
 	case READ_GITFILE_ERR_OPEN_FAILED:
 		die_errno(_("error opening '%s'"), path);
 	case READ_GITFILE_ERR_TOO_LARGE:
@@ -941,8 +945,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;
+	}
+	if (S_ISDIR(st.st_mode)) {
+		error_code = READ_GITFILE_ERR_IS_A_DIR;
 		goto cleanup_return;
 	}
 	if (!S_ISREG(st.st_mode)) {
@@ -1578,20 +1588,28 @@ 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);
 		if (!gitdirenv) {
-			if (die_on_error ||
-			    error_code == READ_GITFILE_ERR_NOT_A_FILE) {
-				/* NEEDSWORK: fail if .git is not file nor dir */
+			switch (error_code) {
+			case READ_GITFILE_ERR_STAT_ENOENT:
+				/* no .git in this directory, move on */
+				break;
+			case 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
+				/* NEEDSWORK: should we catch a directory .git that is not a git directory here? */
+				break;
+			default:
+				if (die_on_error || error_code == READ_GITFILE_ERR_NOT_A_FILE)
+					read_gitfile_error_die(error_code, dir->buf, NULL);
+				else
+					return GIT_DIR_INVALID_GITFILE;
+			}
+		} else {
 			gitfile = xstrdup(dir->buf);
+		}
 		/*
 		 * Earlier, we tentatively added DEFAULT_GIT_DIR_ENVIRONMENT
 		 * to check that directory for a repository.
diff --git a/setup.h b/setup.h
index 0738dec244..ed4b13f061 100644
--- a/setup.h
+++ b/setup.h
@@ -36,6 +36,8 @@ int is_nonbare_repository_dir(struct strbuf *path);
 #define READ_GITFILE_ERR_NO_PATH 6
 #define READ_GITFILE_ERR_NOT_A_REPO 7
 #define READ_GITFILE_ERR_TOO_LARGE 8
+#define READ_GITFILE_ERR_STAT_ENOENT 9
+#define READ_GITFILE_ERR_IS_A_DIR 10
 void read_gitfile_error_die(int error_code, const char *path, const char *dir);
 const char *read_gitfile_gently(const char *path, int *return_error_code);
 #define read_gitfile(path) read_gitfile_gently((path), NULL)
diff --git a/submodule.c b/submodule.c
index 508938e4da..b179f952fb 100644
--- a/submodule.c
+++ b/submodule.c
@@ -2559,7 +2559,7 @@ void absorb_git_dir_into_superproject(const char *path,
 		const struct submodule *sub;
 		struct strbuf sub_gitdir = STRBUF_INIT;
 
-		if (err_code == READ_GITFILE_ERR_STAT_FAILED) {
+		if (err_code == READ_GITFILE_ERR_STAT_ENOENT) {
 			/* unpopulated as expected */
 			strbuf_release(&gitdir);
 			return;
diff --git a/t/meson.build b/t/meson.build
index f80e366cff..c4afaacee5 100644
--- a/t/meson.build
+++ b/t/meson.build
@@ -80,6 +80,7 @@ integration_tests = [
   't0006-date.sh',
   't0007-git-var.sh',
   't0008-ignores.sh',
+  't0009-git-dir-validation.sh',
   't0010-racy-git.sh',
   't0012-help.sh',
   't0013-sha1dc.sh',
diff --git a/t/t0009-git-dir-validation.sh b/t/t0009-git-dir-validation.sh
new file mode 100755
index 0000000000..9b3925c85f
--- /dev/null
+++ b/t/t0009-git-dir-validation.sh
@@ -0,0 +1,72 @@
+#!/bin/sh
+
+test_description='setup: validation of .git file/directory types
+
+Verify that setup_git_directory() correctly handles:
+1. Valid .git directories (including symlinks to them).
+2. Invalid .git files (FIFOs, sockets) by erroring out.
+3. Invalid .git files (garbage) by erroring out.
+'
+
+. ./test-lib.sh
+
+test_expect_success 'setup: create parent git repository' '
+	git init parent &&
+	test_commit -C parent "root-commit"
+'
+
+test_expect_success SYMLINKS 'setup: .git as a symlink to a directory is valid' '
+	mkdir -p parent/link-to-dir &&
+	(
+		cd parent/link-to-dir &&
+		git init real-repo &&
+		ln -s real-repo/.git .git &&
+		git rev-parse --git-dir >actual &&
+		echo .git >expect &&
+		test_cmp expect actual
+	)
+'
+
+test_expect_success PIPE 'setup: .git as a FIFO (named pipe) is rejected' '
+	mkdir -p parent/fifo-trap &&
+	(
+		cd parent/fifo-trap &&
+		mkfifo .git &&
+		test_must_fail git rev-parse --git-dir 2>stderr &&
+		grep "not a regular file" stderr
+	)
+'
+
+test_expect_success SYMLINKS,PIPE 'setup: .git as a symlink to a FIFO is rejected' '
+	mkdir -p parent/symlink-fifo-trap &&
+	(
+		cd parent/symlink-fifo-trap &&
+		mkfifo target-fifo &&
+		ln -s target-fifo .git &&
+		test_must_fail git rev-parse --git-dir 2>stderr &&
+		grep "not a regular file" stderr
+	)
+'
+
+test_expect_success 'setup: .git with garbage content is rejected' '
+	mkdir -p parent/garbage-trap &&
+	(
+		cd parent/garbage-trap &&
+		echo "garbage" >.git &&
+		test_must_fail git rev-parse --git-dir 2>stderr &&
+		grep "invalid gitfile format" stderr
+	)
+'
+
+test_expect_success 'setup: .git as an empty directory is ignored' '
+	mkdir -p parent/empty-dir &&
+	(
+		cd parent/empty-dir &&
+		mkdir .git &&
+		git rev-parse --git-dir >actual &&
+		echo "$TRASH_DIRECTORY/parent/.git" >expect &&
+		test_cmp expect actual
+	)
+'
+
+test_done
diff --git a/worktree.c b/worktree.c
index 9308389cb6..d1165e1d1c 100644
--- a/worktree.c
+++ b/worktree.c
@@ -653,7 +653,8 @@ static void repair_gitfile(struct worktree *wt,
 		}
 	}
 
-	if (err == READ_GITFILE_ERR_NOT_A_FILE)
+	if (err == READ_GITFILE_ERR_NOT_A_FILE ||
+		err == READ_GITFILE_ERR_IS_A_DIR)
 		fn(1, wt->path, _(".git is not a file"), cb_data);
 	else if (err)
 		repair = _(".git file broken");
@@ -833,7 +834,8 @@ void repair_worktree_at_path(const char *path,
 			strbuf_addstr(&backlink, dotgit_contents);
 			strbuf_realpath_forgiving(&backlink, backlink.buf, 0);
 		}
-	} else if (err == READ_GITFILE_ERR_NOT_A_FILE) {
+	} else if (err == READ_GITFILE_ERR_NOT_A_FILE ||
+			err == READ_GITFILE_ERR_IS_A_DIR) {
 		fn(1, dotgit.buf, _("unable to locate repository; .git is not a file"), cb_data);
 		goto done;
 	} else if (err == READ_GITFILE_ERR_NOT_A_REPO) {
-- 
2.43.0

Re: [PATCH v10] setup: improve error diagnosis for invalid .git files

[Karthik Nayak]

[-- Attachment #1: Type: text/plain, Size: 10491 bytes --]

Tian Yuchen <a3205153416@gmail.com> writes:

> 'read_gitfile_gently()' treats any non-regular file as
> 'READ_GITFILE_ERR_NOT_A_FILE' and fails to discern between 'ENOENT'
> and other stat failures. This flawed error reporting is noted by two
> 'NEEDSWORK' comments.

Okay.

> Address these comments by introducing two new error codes:
> 'READ_GITFILE_ERR_STAT_ENOENT' and 'READ_GITFILE_ERR_IS_A_DIR'.
>

Nit: This is much better, we seem to talk about the issues and the new
errors introduced. I wonder if we can tie the errors to the issues.

Perhaps

    The 'read_gitfile_gently()' is used to obtain the location of a git
    directory by parsing a '.git' file.

    When parsing the file with 'stat(2)', it fails to differentiate
    between a 'ENOENT' and other errors. Introduce
    'READ_GITFILE_ERR_STAT_ENOENT' to make this differentiation.

    The function also marks directories as
    'READ_GITFILE_ERR_NOT_A_FILE', introduce 'READ_GITFILE_ERR_IS_A_DIR'
    to specifically make this distinction.


> To preserve the original intent of the setup process:
> 1. Update 'read_gitfile_error_die()' to treat 'IS_A_DIR' as a no-op
>    (like 'ENOENT'), while still calling 'die()' on true 'NOT_A_FILE'
>    errors.

Nice, shouldn't we also mention READ_GITFILE_ERR_STAT_ENOENT is now
treated as a no-op, while its counterpart is not.

> 2. Unconditionally pass '&error_code' to 'read_gitfile_gently()'. This
>    eliminates an uninitialized variable hazard that occurred when
>    'die_on_error' was true and 'NULL' was passed.

Where is the 'uninitialized variable hazard'? The function says:

  If return_error_code is NULL the function will die instead

> 3. Only invoke 'is_git_directory()' when we explicitly receive
>    'READ_GITFILE_ERR_IS_A_DIR', avoiding redundant filesystem checks.

Nice.

> 4. Correctly return 'GIT_DIR_INVALID_GITFILE' on unrecognized errors
>    when 'die_on_error' is false.
>
> Additionally, audit external callers of 'read_gitfile_gently()' in
> 'submodule.c' and 'worktree.c' to accommodate the refined error codes.
>
> Signed-off-by: Tian Yuchen <a3205153416@gmail.com>
> ---
>  setup.c                       | 42 ++++++++++++++------
>  setup.h                       |  2 +
>  submodule.c                   |  2 +-
>  t/meson.build                 |  1 +
>  t/t0009-git-dir-validation.sh | 72 +++++++++++++++++++++++++++++++++++
>  worktree.c                    |  6 ++-
>  6 files changed, 110 insertions(+), 15 deletions(-)
>  create mode 100755 t/t0009-git-dir-validation.sh
>

I couldn't find a discussion, why did we merge the commits?

> diff --git a/setup.c b/setup.c
> index c8336eb20e..9d49b9ae53 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -897,10 +897,14 @@ 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_FAILED:
> -	case READ_GITFILE_ERR_NOT_A_FILE:
> +	case READ_GITFILE_ERR_STAT_ENOENT:
> +	case READ_GITFILE_ERR_IS_A_DIR:
>  		/* non-fatal; follow return path */
>  		break;
> +	case READ_GITFILE_ERR_STAT_FAILED:
> +		die(_("error reading %s"), path);
> +	case READ_GITFILE_ERR_NOT_A_FILE:
> +		die(_("not a regular file: %s"), path);

Not your fault, but some of the errors quote the path and some don't, it
would be nice to be uniform here.

>  	case READ_GITFILE_ERR_OPEN_FAILED:
>  		die_errno(_("error opening '%s'"), path);
>  	case READ_GITFILE_ERR_TOO_LARGE:
> @@ -941,8 +945,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;
> +	}
> +	if (S_ISDIR(st.st_mode)) {
> +		error_code = READ_GITFILE_ERR_IS_A_DIR;
>  		goto cleanup_return;
>  	}

This block makes sense.

>  	if (!S_ISREG(st.st_mode)) {
> @@ -1578,20 +1588,28 @@ 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);

So we now ask the error code to be provided, even if `die_on_error` is
set. I assume, we will manually handle `die_on_error`.

>  		if (!gitdirenv) {
> -			if (die_on_error ||
> -			    error_code == READ_GITFILE_ERR_NOT_A_FILE) {
> -				/* NEEDSWORK: fail if .git is not file nor dir */
> +			switch (error_code) {
> +			case READ_GITFILE_ERR_STAT_ENOENT:
> +				/* no .git in this directory, move on */
> +				break;
> +			case 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
> +				/* NEEDSWORK: should we catch a directory .git that is not a git directory here? */

Nit: we should probably wrap this.

> +				break;
> +			default:
> +				if (die_on_error || error_code == READ_GITFILE_ERR_NOT_A_FILE)
> +					read_gitfile_error_die(error_code, dir->buf, NULL);
> +				else
> +					return GIT_DIR_INVALID_GITFILE;
> +			}
> +		} else {
>  			gitfile = xstrdup(dir->buf);
> +		}

The changes themselves make sense to me.

>  		/*
>  		 * Earlier, we tentatively added DEFAULT_GIT_DIR_ENVIRONMENT
>  		 * to check that directory for a repository.
> diff --git a/setup.h b/setup.h
> index 0738dec244..ed4b13f061 100644
> --- a/setup.h
> +++ b/setup.h
> @@ -36,6 +36,8 @@ int is_nonbare_repository_dir(struct strbuf *path);
>  #define READ_GITFILE_ERR_NO_PATH 6
>  #define READ_GITFILE_ERR_NOT_A_REPO 7
>  #define READ_GITFILE_ERR_TOO_LARGE 8
> +#define READ_GITFILE_ERR_STAT_ENOENT 9
> +#define READ_GITFILE_ERR_IS_A_DIR 10
>  void read_gitfile_error_die(int error_code, const char *path, const char *dir);
>  const char *read_gitfile_gently(const char *path, int *return_error_code);
>  #define read_gitfile(path) read_gitfile_gently((path), NULL)
> diff --git a/submodule.c b/submodule.c
> index 508938e4da..b179f952fb 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -2559,7 +2559,7 @@ void absorb_git_dir_into_superproject(const char *path,
>  		const struct submodule *sub;
>  		struct strbuf sub_gitdir = STRBUF_INIT;
>
> -		if (err_code == READ_GITFILE_ERR_STAT_FAILED) {
> +		if (err_code == READ_GITFILE_ERR_STAT_ENOENT) {
>  			/* unpopulated as expected */
>  			strbuf_release(&gitdir);
>  			return;
> diff --git a/t/meson.build b/t/meson.build
> index f80e366cff..c4afaacee5 100644
> --- a/t/meson.build
> +++ b/t/meson.build
> @@ -80,6 +80,7 @@ integration_tests = [
>    't0006-date.sh',
>    't0007-git-var.sh',
>    't0008-ignores.sh',
> +  't0009-git-dir-validation.sh',
>    't0010-racy-git.sh',
>    't0012-help.sh',
>    't0013-sha1dc.sh',
> diff --git a/t/t0009-git-dir-validation.sh b/t/t0009-git-dir-validation.sh
> new file mode 100755
> index 0000000000..9b3925c85f
> --- /dev/null
> +++ b/t/t0009-git-dir-validation.sh
> @@ -0,0 +1,72 @@
> +#!/bin/sh
> +
> +test_description='setup: validation of .git file/directory types
> +
> +Verify that setup_git_directory() correctly handles:
> +1. Valid .git directories (including symlinks to them).
> +2. Invalid .git files (FIFOs, sockets) by erroring out.
> +3. Invalid .git files (garbage) by erroring out.
> +'
> +
> +. ./test-lib.sh
> +
> +test_expect_success 'setup: create parent git repository' '
> +	git init parent &&
> +	test_commit -C parent "root-commit"
> +'
> +
> +test_expect_success SYMLINKS 'setup: .git as a symlink to a directory is valid' '

Nit: should we also cleanup? with a 'test_when_finished "rm -rf
parent/link-to-dir"'.
Should apply for all the tests.

> +	mkdir -p parent/link-to-dir &&
> +	(
> +		cd parent/link-to-dir &&
> +		git init real-repo &&
> +		ln -s real-repo/.git .git &&
> +		git rev-parse --git-dir >actual &&
> +		echo .git >expect &&
> +		test_cmp expect actual
> +	)
> +'
> +
> +test_expect_success PIPE 'setup: .git as a FIFO (named pipe) is rejected' '
> +	mkdir -p parent/fifo-trap &&
> +	(
> +		cd parent/fifo-trap &&
> +		mkfifo .git &&
> +		test_must_fail git rev-parse --git-dir 2>stderr &&
> +		grep "not a regular file" stderr
> +	)
> +'
> +
> +test_expect_success SYMLINKS,PIPE 'setup: .git as a symlink to a FIFO is rejected' '
> +	mkdir -p parent/symlink-fifo-trap &&
> +	(
> +		cd parent/symlink-fifo-trap &&
> +		mkfifo target-fifo &&
> +		ln -s target-fifo .git &&
> +		test_must_fail git rev-parse --git-dir 2>stderr &&
> +		grep "not a regular file" stderr
> +	)
> +'
> +
> +test_expect_success 'setup: .git with garbage content is rejected' '
> +	mkdir -p parent/garbage-trap &&
> +	(
> +		cd parent/garbage-trap &&
> +		echo "garbage" >.git &&
> +		test_must_fail git rev-parse --git-dir 2>stderr &&
> +		grep "invalid gitfile format" stderr
> +	)
> +'
> +
> +test_expect_success 'setup: .git as an empty directory is ignored' '
> +	mkdir -p parent/empty-dir &&
> +	(
> +		cd parent/empty-dir &&
> +		mkdir .git &&
> +		git rev-parse --git-dir >actual &&
> +		echo "$TRASH_DIRECTORY/parent/.git" >expect &&
> +		test_cmp expect actual
> +	)
> +'
> +
> +test_done
> diff --git a/worktree.c b/worktree.c
> index 9308389cb6..d1165e1d1c 100644
> --- a/worktree.c
> +++ b/worktree.c
> @@ -653,7 +653,8 @@ static void repair_gitfile(struct worktree *wt,
>  		}
>  	}
>
> -	if (err == READ_GITFILE_ERR_NOT_A_FILE)
> +	if (err == READ_GITFILE_ERR_NOT_A_FILE ||
> +		err == READ_GITFILE_ERR_IS_A_DIR)
>  		fn(1, wt->path, _(".git is not a file"), cb_data);
>  	else if (err)
>  		repair = _(".git file broken");
> @@ -833,7 +834,8 @@ void repair_worktree_at_path(const char *path,
>  			strbuf_addstr(&backlink, dotgit_contents);
>  			strbuf_realpath_forgiving(&backlink, backlink.buf, 0);
>  		}
> -	} else if (err == READ_GITFILE_ERR_NOT_A_FILE) {
> +	} else if (err == READ_GITFILE_ERR_NOT_A_FILE ||
> +			err == READ_GITFILE_ERR_IS_A_DIR) {
>  		fn(1, dotgit.buf, _("unable to locate repository; .git is not a file"), cb_data);
>  		goto done;
>  	} else if (err == READ_GITFILE_ERR_NOT_A_REPO) {
> --
> 2.43.0

The rest looks good. Thanks!

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]

Re: [PATCH v10] setup: improve error diagnosis for invalid .git files

[Junio C Hamano]

Tian Yuchen <a3205153416@gmail.com> writes:

> 'read_gitfile_gently()' treats any non-regular file as
> 'READ_GITFILE_ERR_NOT_A_FILE' and fails to discern between 'ENOENT'
> and other stat failures. This flawed error reporting is noted by two
> 'NEEDSWORK' comments.
>
> Address these comments by introducing two new error codes:
> 'READ_GITFILE_ERR_STAT_ENOENT' and 'READ_GITFILE_ERR_IS_A_DIR'.
>
> To preserve the original intent of the setup process:
> 1. Update 'read_gitfile_error_die()' to treat 'IS_A_DIR' as a no-op
>    (like 'ENOENT'), while still calling 'die()' on true 'NOT_A_FILE'
>    errors.
> 2. Unconditionally pass '&error_code' to 'read_gitfile_gently()'. This
>    eliminates an uninitialized variable hazard that occurred when
>    'die_on_error' was true and 'NULL' was passed.
> 3. Only invoke 'is_git_directory()' when we explicitly receive
>    'READ_GITFILE_ERR_IS_A_DIR', avoiding redundant filesystem checks.
> 4. Correctly return 'GIT_DIR_INVALID_GITFILE' on unrecognized errors
>    when 'die_on_error' is false.
>
> Additionally, audit external callers of 'read_gitfile_gently()' in
> 'submodule.c' and 'worktree.c' to accommodate the refined error codes.
>
> Signed-off-by: Tian Yuchen <a3205153416@gmail.com>
> ---
>  setup.c                       | 42 ++++++++++++++------
>  setup.h                       |  2 +
>  submodule.c                   |  2 +-
>  t/meson.build                 |  1 +
>  t/t0009-git-dir-validation.sh | 72 +++++++++++++++++++++++++++++++++++
>  worktree.c                    |  6 ++-
>  6 files changed, 110 insertions(+), 15 deletions(-)
>  create mode 100755 t/t0009-git-dir-validation.sh

We'd probably need to treat ENOTDIR the same way as ENOENT to deal
with cases where we expect a directory "sm1" to be the root of a
submodule working tree, and we have a modification that removes the
submodule directory and replace it with a regular file "sm1".  In
the code path touched by this patch in submodule.c, we would ask "is
sm1/.git a git directory?" and the stat(2) call on that path in
read_gitfile_gently() used to say "Ah, a failure, that means we
cannot positively say that 'sm1/.git' is a git directory or a gitdir
file."  Now we inspect the error code in an attempt to tell if it is
a system failure (e.g., a corrupt filesystem), but catching only
ENOENT is probably a bit too tight.  In the above scenario, asking
about 'sm1/.git' when 'sm1' is a regular file will not result in
ENOENT but in ENOTDIR (i.e., "the leading 'sm1' is not a directory so
it makes no sense to ask about 'sm1/.git'").

Is it always sensible to treat ENOTDIR and ENOENT as two equivalent
errors for the purpose of read_gitfile_gently()?  I have no clear
answer offhand myself.  This is part of what we need to think about
and resolve while addressing the original "NEEDSWORK:" comment.

Re: [PATCH v10] setup: improve error diagnosis for invalid .git files

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

>>  setup.c                       | 42 ++++++++++++++------
>>  setup.h                       |  2 +
>>  submodule.c                   |  2 +-
>>  t/meson.build                 |  1 +
>>  t/t0009-git-dir-validation.sh | 72 +++++++++++++++++++++++++++++++++++
>>  worktree.c                    |  6 ++-
>>  6 files changed, 110 insertions(+), 15 deletions(-)
>>  create mode 100755 t/t0009-git-dir-validation.sh
>
> We'd probably need to treat ENOTDIR the same way as ENOENT to deal
> with cases where we expect a directory "sm1" to be the root of a
> submodule working tree, and we have a modification that removes the
> submodule directory and replace it with a regular file "sm1".  In
> the code path touched by this patch in submodule.c, we would ask "is
> sm1/.git a git directory?" and the stat(2) call on that path in
> read_gitfile_gently() used to say "Ah, a failure, that means we
> cannot positively say that 'sm1/.git' is a git directory or a gitdir
> file."  Now we inspect the error code in an attempt to tell if it is
> a system failure (e.g., a corrupt filesystem), but catching only
> ENOENT is probably a bit too tight.  In the above scenario, asking
> about 'sm1/.git' when 'sm1' is a regular file will not result in
> ENOENT but in ENOTDIR (i.e., "the leading 'sm1' is not a directory so
> it makes no sense to ask about 'sm1/.git'").
>
> Is it always sensible to treat ENOTDIR and ENOENT as two equivalent
> errors for the purpose of read_gitfile_gently()?  I have no clear
> answer offhand myself.  This is part of what we need to think about
> and resolve while addressing the original "NEEDSWORK:" comment.


----- >8 -----
Subject: [PATCH] read_gitfile(): group ENOENT and ENOTDIR into a
 single MISSING error

The code from the previous step does not deal wellwith a case where
we check if "sm/.git" is a good directory after replacing "sm" with
a regular file.  ENOTDIR is returned when we ask about "sm/.git",
not ENOENT, and the code would want to handle both.

I am not convinced if this is a good change, though.  Outside the
submodule caller, the story might be different and we may want to
treat ENOTDIR differently from ENOENT.  I dunno.  That is why this
is not squashed into the patch (yet).

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 setup.c     | 8 ++++----
 setup.h     | 2 +-
 submodule.c | 2 +-
 3 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/setup.c b/setup.c
index b79a9233f5..d4afbed3eb 100644
--- a/setup.c
+++ b/setup.c
@@ -895,7 +895,7 @@ 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_STAT_MISSING:
 	case READ_GITFILE_ERR_IS_A_DIR:
 		/* non-fatal; follow return path */
 		break;
@@ -943,8 +943,8 @@ const char *read_gitfile_gently(const char *path, int *return_error_code)
 	static struct strbuf realpath = STRBUF_INIT;
 
 	if (stat(path, &st)) {
-		if (errno == ENOENT)
-			error_code = READ_GITFILE_ERR_STAT_ENOENT;
+		if (errno == ENOENT || errno == ENOTDIR)
+			error_code = READ_GITFILE_ERR_STAT_MISSING;
 		else
 			error_code = READ_GITFILE_ERR_STAT_FAILED;
 		goto cleanup_return;
@@ -1589,7 +1589,7 @@ static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,
 		gitdirenv = read_gitfile_gently(dir->buf, &error_code);
 		if (!gitdirenv) {
 			switch (error_code) {
-			case READ_GITFILE_ERR_STAT_ENOENT:
+			case READ_GITFILE_ERR_STAT_MISSING:
 				/* no .git in this directory, move on */
 				break;
 			case READ_GITFILE_ERR_IS_A_DIR:
diff --git a/setup.h b/setup.h
index c23629cb4f..cc45f962fa 100644
--- a/setup.h
+++ b/setup.h
@@ -36,7 +36,7 @@ int is_nonbare_repository_dir(struct strbuf *path);
 #define READ_GITFILE_ERR_NO_PATH 6
 #define READ_GITFILE_ERR_NOT_A_REPO 7
 #define READ_GITFILE_ERR_TOO_LARGE 8
-#define READ_GITFILE_ERR_STAT_ENOENT 9
+#define READ_GITFILE_ERR_STAT_MISSING 9
 #define READ_GITFILE_ERR_IS_A_DIR 10
 void read_gitfile_error_die(int error_code, const char *path, const char *dir);
 const char *read_gitfile_gently(const char *path, int *return_error_code);
diff --git a/submodule.c b/submodule.c
index 52a7cf0e43..fc85a7a1d8 100644
--- a/submodule.c
+++ b/submodule.c
@@ -2413,7 +2413,7 @@ void absorb_git_dir_into_superproject(const char *path,
 		const struct submodule *sub;
 		struct strbuf sub_gitdir = STRBUF_INIT;
 
-		if (err_code == READ_GITFILE_ERR_STAT_ENOENT) {
+		if (err_code == READ_GITFILE_ERR_STAT_MISSING) {
 			/* unpopulated as expected */
 			strbuf_release(&gitdir);
 			return;
-- 
2.53.0-455-g62fcd67e6e

Re: [PATCH v10] setup: improve error diagnosis for invalid .git files

[Tian Yuchen]

Hi Junio,

 >> We'd probably need to treat ENOTDIR the same way as ENOENT to deal
 >> with cases where we expect a directory "sm1" to be the root of a
 >> submodule working tree, and we have a modification that removes the
 >> submodule directory and replace it with a regular file "sm1".  In
 >> the code path touched by this patch in submodule.c, we would ask "is
 >> sm1/.git a git directory?" and the stat(2) call on that path in
 >> read_gitfile_gently() used to say "Ah, a failure, that means we
 >> cannot positively say that 'sm1/.git' is a git directory or a gitdir
 >> file."  Now we inspect the error code in an attempt to tell if it is
 >> a system failure (e.g., a corrupt filesystem), but catching only
 >> ENOENT is probably a bit too tight.  In the above scenario, asking
 >> about 'sm1/.git' when 'sm1' is a regular file will not result in
 >> ENOENT but in ENOTDIR (i.e., "the leading 'sm1' is not a directory so
 >> it makes no sense to ask about 'sm1/.git'").

I must admit I hadn't considered this edge case at all. Thank you for 
pointing it out :]

>> Is it always sensible to treat ENOTDIR and ENOENT as two equivalent
>> errors for the purpose of read_gitfile_gently()?  I have no clear
>> answer offhand myself.  This is part of what we need to think about
>> and resolve while addressing the original "NEEDSWORK:" comment.

Hummm, I believe it is safe. From the perspective of 
'read_gitfile_gently()', the sole purpose is to locate and read a 
repository file. Whether 'stat()' returns 'ENOENT' (the file physically 
does not exist) or 'ENOTDIR' (a component of the path is not a 
directory, making it impossible for the file to exist there), the 
functional result is exactly the same: the '.git' file is missing.

But I must say I can't 100% guarantee its safety. Anyway, lemme just do 
what I can for now.

I will:
  - squash your diff into my patch
  - rename the error code to `READ_GITFILE_ERR_STAT_MISSING`
  - combine this with the commit message refinements and test cleanups 
suggested by Karthik in the previous thread.

Thank you for the patch and for walking me through this edge case. I'll 
send out v11 soon! (2~3 hours later)

Regards,

Yuchen

Re: [PATCH v10] setup: improve error diagnosis for invalid .git files

[Junio C Hamano]

Tian Yuchen <a3205153416@gmail.com> writes:

> I must admit I hadn't considered this edge case at all. Thank you for 
> pointing it out :]

It is easy to see, if you run the tests, though ;-)

> I will:
>   - squash your diff into my patch
>   - rename the error code to `READ_GITFILE_ERR_STAT_MISSING`

I think MISSING is more appropriate than STAT_MISSING.  Our stat(2)
call positively identified that the given path does not exist on the
filesystem.

>   - combine this with the commit message refinements and test cleanups 
> suggested by Karthik in the previous thread.

Yeah, cleaning after yourselves in the tests is a good idea.

Re: [PATCH v10] setup: improve error diagnosis for invalid .git files

[Tian Yuchen]

Hi Karthik,

Thank you so much for the detailed review.


> Where is the 'uninitialized variable hazard'?

Hummm...seems that 'die_on_error ? NULL : &error_code' would just 
immediately 'die()' internally if NULL was passed. So there was no 
*real* hazard of uninitialized error_code being evaluated externally. 
Anyway, this change hardly qualifies as a major focus, and I will remove 
it from the commit message.

> I couldn't find a discussion, why did we merge the commits?

It was suggested by Junio, ensuring that every commit in the history 
remains strictly atomic and bisectable.

> Not your fault, but some of the errors quote the path and some don't, it
> would be nice to be uniform here.
> Nit: should we also cleanup? with a 'test_when_finished "rm -rf
> parent/link-to-dir"'.
> Should apply for all the tests.

Great catch. Will change in v11.

> The rest looks good. Thanks!

Thank you again for the guidance ;)

Regards,

yuchen

[PATCH v11] setup: improve error diagnosis for invalid .git files

[Tian Yuchen]

'read_gitfile_gently()' treats any non-regular file as
'READ_GITFILE_ERR_NOT_A_FILE' and fails to discern between 'ENOENT'
and other stat failures. This flawed error reporting is noted by two
'NEEDSWORK' comments.

Address these comments by introducing two new error codes:
'READ_GITFILE_ERR_MISSING'(which groups the "file missing" scenarios
together) and 'READ_GITFILE_ERR_IS_A_DIR'.

To preserve the original intent of the setup process:
1. Update 'read_gitfile_error_die()' to treat both 'IS_A_DIR' and
   'MISSING' as no-ops, while continuing to call 'die()' on true
   'NOT_A_FILE' errors to prevent security hazards (like FIFOs).
2. Unconditionally pass '&error_code' to 'read_gitfile_gently()'.
3. Only invoke 'is_git_directory()' when we explicitly receive
   'READ_GITFILE_ERR_IS_A_DIR', avoiding redundant filesystem checks.
4. Correctly return 'GIT_DIR_INVALID_GITFILE' on unrecognized errors
   when 'die_on_error' is false.

Additionally, audit external callers of 'read_gitfile_gently()' in
'submodule.c' and 'worktree.c' to accommodate the refined error codes.

Signed-off-by: Tian Yuchen <a3205153416@gmail.com>
---
 setup.c                       | 45 ++++++++++++++------
 setup.h                       |  2 +
 submodule.c                   |  2 +-
 t/meson.build                 |  1 +
 t/t0009-git-dir-validation.sh | 77 +++++++++++++++++++++++++++++++++++
 worktree.c                    |  6 ++-
 6 files changed, 118 insertions(+), 15 deletions(-)
 create mode 100755 t/t0009-git-dir-validation.sh

diff --git a/setup.c b/setup.c
index c8336eb20e..015088119c 100644
--- a/setup.c
+++ b/setup.c
@@ -897,10 +897,14 @@ 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_FAILED:
-	case READ_GITFILE_ERR_NOT_A_FILE:
+	case READ_GITFILE_ERR_MISSING:
+	case READ_GITFILE_ERR_IS_A_DIR:
 		/* non-fatal; follow return path */
 		break;
+	case READ_GITFILE_ERR_STAT_FAILED:
+		die(_("error reading '%s'"), path);
+	case READ_GITFILE_ERR_NOT_A_FILE:
+		die(_("not a regular file: '%s'"), path);
 	case READ_GITFILE_ERR_OPEN_FAILED:
 		die_errno(_("error opening '%s'"), path);
 	case READ_GITFILE_ERR_TOO_LARGE:
@@ -941,8 +945,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 || errno == ENOTDIR)
+			error_code = READ_GITFILE_ERR_MISSING;
+		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;
 	}
 	if (!S_ISREG(st.st_mode)) {
@@ -1578,20 +1588,31 @@ 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);
 		if (!gitdirenv) {
-			if (die_on_error ||
-			    error_code == READ_GITFILE_ERR_NOT_A_FILE) {
-				/* NEEDSWORK: fail if .git is not file nor dir */
+			switch (error_code) {
+			case READ_GITFILE_ERR_MISSING:
+				/* no .git in this directory, move on */
+				break;
+			case 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
+				/* 
+				 * NEEDSWORK: should we catch a directory .git 
+				 * that is not a git directory here? 
+				 */
+				break;
+			default:
+				if (die_on_error || error_code == READ_GITFILE_ERR_NOT_A_FILE)
+					read_gitfile_error_die(error_code, dir->buf, NULL);
+				else
+					return GIT_DIR_INVALID_GITFILE;
+			}
+		} else {
 			gitfile = xstrdup(dir->buf);
+		}
 		/*
 		 * Earlier, we tentatively added DEFAULT_GIT_DIR_ENVIRONMENT
 		 * to check that directory for a repository.
diff --git a/setup.h b/setup.h
index 0738dec244..76fb260c20 100644
--- a/setup.h
+++ b/setup.h
@@ -36,6 +36,8 @@ int is_nonbare_repository_dir(struct strbuf *path);
 #define READ_GITFILE_ERR_NO_PATH 6
 #define READ_GITFILE_ERR_NOT_A_REPO 7
 #define READ_GITFILE_ERR_TOO_LARGE 8
+#define READ_GITFILE_ERR_MISSING 9
+#define READ_GITFILE_ERR_IS_A_DIR 10
 void read_gitfile_error_die(int error_code, const char *path, const char *dir);
 const char *read_gitfile_gently(const char *path, int *return_error_code);
 #define read_gitfile(path) read_gitfile_gently((path), NULL)
diff --git a/submodule.c b/submodule.c
index 508938e4da..767d4c3c35 100644
--- a/submodule.c
+++ b/submodule.c
@@ -2559,7 +2559,7 @@ void absorb_git_dir_into_superproject(const char *path,
 		const struct submodule *sub;
 		struct strbuf sub_gitdir = STRBUF_INIT;
 
-		if (err_code == READ_GITFILE_ERR_STAT_FAILED) {
+		if (err_code == READ_GITFILE_ERR_MISSING) {
 			/* unpopulated as expected */
 			strbuf_release(&gitdir);
 			return;
diff --git a/t/meson.build b/t/meson.build
index f80e366cff..c4afaacee5 100644
--- a/t/meson.build
+++ b/t/meson.build
@@ -80,6 +80,7 @@ integration_tests = [
   't0006-date.sh',
   't0007-git-var.sh',
   't0008-ignores.sh',
+  't0009-git-dir-validation.sh',
   't0010-racy-git.sh',
   't0012-help.sh',
   't0013-sha1dc.sh',
diff --git a/t/t0009-git-dir-validation.sh b/t/t0009-git-dir-validation.sh
new file mode 100755
index 0000000000..7e2c711a63
--- /dev/null
+++ b/t/t0009-git-dir-validation.sh
@@ -0,0 +1,77 @@
+#!/bin/sh
+
+test_description='setup: validation of .git file/directory types
+
+Verify that setup_git_directory() correctly handles:
+1. Valid .git directories (including symlinks to them).
+2. Invalid .git files (FIFOs, sockets) by erroring out.
+3. Invalid .git files (garbage) by erroring out.
+'
+
+. ./test-lib.sh
+
+test_expect_success 'setup: create parent git repository' '
+	git init parent &&
+	test_commit -C parent "root-commit"
+'
+
+test_expect_success SYMLINKS 'setup: .git as a symlink to a directory is valid' '
+	test_when_finished "rm -rf parent/link-to-dir" &&
+	mkdir -p parent/link-to-dir &&
+	(
+		cd parent/link-to-dir &&
+		git init real-repo &&
+		ln -s real-repo/.git .git &&
+		git rev-parse --git-dir >actual &&
+		echo .git >expect &&
+		test_cmp expect actual
+	)
+'
+
+test_expect_success PIPE 'setup: .git as a FIFO (named pipe) is rejected' '
+	test_when_finished "rm -rf parent/fifo-trap" &&
+	mkdir -p parent/fifo-trap &&
+	(
+		cd parent/fifo-trap &&
+		mkfifo .git &&
+		test_must_fail git rev-parse --git-dir 2>stderr &&
+		grep "not a regular file" stderr
+	)
+'
+
+test_expect_success SYMLINKS,PIPE 'setup: .git as a symlink to a FIFO is rejected' '
+	test_when_finished "rm -rf parent/symlink-fifo-trap" &&
+	mkdir -p parent/symlink-fifo-trap &&
+	(
+		cd parent/symlink-fifo-trap &&
+		mkfifo target-fifo &&
+		ln -s target-fifo .git &&
+		test_must_fail git rev-parse --git-dir 2>stderr &&
+		grep "not a regular file" stderr
+	)
+'
+
+test_expect_success 'setup: .git with garbage content is rejected' '
+	test_when_finished "rm -rf parent/garbage-trap" &&
+	mkdir -p parent/garbage-trap &&
+	(
+		cd parent/garbage-trap &&
+		echo "garbage" >.git &&
+		test_must_fail git rev-parse --git-dir 2>stderr &&
+		grep "invalid gitfile format" stderr
+	)
+'
+
+test_expect_success 'setup: .git as an empty directory is ignored' '
+	test_when_finished "rm -rf parent/empty-dir" &&
+	mkdir -p parent/empty-dir &&
+	(
+		cd parent/empty-dir &&
+		mkdir .git &&
+		git rev-parse --git-dir >actual &&
+		echo "$TRASH_DIRECTORY/parent/.git" >expect &&
+		test_cmp expect actual
+	)
+'
+
+test_done
diff --git a/worktree.c b/worktree.c
index 9308389cb6..d1165e1d1c 100644
--- a/worktree.c
+++ b/worktree.c
@@ -653,7 +653,8 @@ static void repair_gitfile(struct worktree *wt,
 		}
 	}
 
-	if (err == READ_GITFILE_ERR_NOT_A_FILE)
+	if (err == READ_GITFILE_ERR_NOT_A_FILE ||
+		err == READ_GITFILE_ERR_IS_A_DIR)
 		fn(1, wt->path, _(".git is not a file"), cb_data);
 	else if (err)
 		repair = _(".git file broken");
@@ -833,7 +834,8 @@ void repair_worktree_at_path(const char *path,
 			strbuf_addstr(&backlink, dotgit_contents);
 			strbuf_realpath_forgiving(&backlink, backlink.buf, 0);
 		}
-	} else if (err == READ_GITFILE_ERR_NOT_A_FILE) {
+	} else if (err == READ_GITFILE_ERR_NOT_A_FILE ||
+			err == READ_GITFILE_ERR_IS_A_DIR) {
 		fn(1, dotgit.buf, _("unable to locate repository; .git is not a file"), cb_data);
 		goto done;
 	} else if (err == READ_GITFILE_ERR_NOT_A_REPO) {
-- 
2.43.0

Re: [PATCH v10] setup: improve error diagnosis for invalid .git files

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> I think MISSING is more appropriate than STAT_MISSING.  Our stat(2)
> call positively identified that the given path does not exist on the
> filesystem.

If you prefer to have STAT_ there, I do not mind too much.  But at
some point, we may want to drop _ERR in those two new "these are not
errors" return values.

Re: [PATCH v10] setup: improve error diagnosis for invalid .git files

[Tian Yuchen]

Hi Junio,

 > But at some point, we may want to drop _ERR in those two new "these 
are not
 > errors" return values.

For the v11 I sent earlier, I kept `READ_GITFILE_ERR_MISSING` to 
maintain namespace consistency with the existing `READ_GITFILE_ERR_*` 
macros. However, I can't deny that decoupling these into neutral status 
codes (e.g., `READ_GITFILE_MISSING` vs actual fatal errors) also makes 
sense.

I'd be more than happy to drop the `_ERR` prefix in a v12 if you think 
it's better to address this cleanup right now, or we can definitely 
leave it for a future cleanup patch as you suggested.

Thanks,

Tian Yuchen

Re: [PATCH v10] setup: improve error diagnosis for invalid .git files

[Junio C Hamano]

Tian Yuchen <a3205153416@gmail.com> writes:

> Hi Junio,
>
>  > But at some point, we may want to drop _ERR in those two new "these 
> are not
>  > errors" return values.
>
> For the v11 I sent earlier, I kept `READ_GITFILE_ERR_MISSING` to 
> maintain namespace consistency with the existing `READ_GITFILE_ERR_*` 
> macros. However, I can't deny that decoupling these into neutral status 
> codes (e.g., `READ_GITFILE_MISSING` vs actual fatal errors) also makes 
> sense.
>
> I'd be more than happy to drop the `_ERR` prefix in a v12 if you think 
> it's better to address this cleanup right now, or we can definitely 
> leave it for a future cleanup patch as you suggested.

Nah, we are quickly approaching the point of diminishing return.
Let's see if we need to kill more fundamental bugs before
bikeshedding the constant names.

Thanks.

Re: [PATCH v10] setup: improve error diagnosis for invalid .git files

[Tian Yuchen]

Hi Junio,

Understood.

I'll leave the patch series as it is. Please reply to me any time
when new bugs are observed.

Regards,

Yuchen

Re: [PATCH v10] setup: improve error diagnosis for invalid .git files

[Tian Yuchen]

Hi Junio,

I believe we have made some progress so far. However, after reviewing 
the code again just now, I still have a few questions I'd like to ask:

  1. Perhaps I took it literally, but as far as the original intent 
goes, shouldn't 'read_gitfile_gently()' be solely responsible for 
*opening a .git file and parsing the gitdir: <path> format*? However, it 
currently executes 'stat()', checks 'S_ISDIR', checks 'S_ISREG', handles 
missing components, and *then* attempts to parse. Do we need an 'enum 
git_componet_type git_componet(cost char *path)' which returns pure 
filesystem states, then parse it with 'parse_gitfile_format(const char 
*path)'? I don't know.

  2. Let's say, when stat() encounters a EACCES when cheaking a 
restricted sub-folder. Git funnels this into STAT_FAILED and 
subsequently invokes die(), which calls exit(). I'm thinking of 
libification: if a long running multi threaded git server encounters a 
permission-denied directory, is killing the entire process the expected 
behavior? Should 'permission-denied' really be considered as an 'error'? 
Does a library has the authority to terminate the application?

I won't touch any of this right now to keep the current scope focused, 
but I plan to incorporate thoughts into my GSoC proposal for the global 
state/libification project.

If you have more important things to do, feel free to ignore this email. 
After all, I consider it a minor issue.

This is my first patch at my nineteen, and I'm more than delighted to 
spend my birthday reviewing git code ;)

Regards,

Yuchen

Re: [PATCH v10] setup: improve error diagnosis for invalid .git files

[Junio C Hamano]

Tian Yuchen <a3205153416@gmail.com> writes:

>   1. Perhaps I took it literally, but as far as the original intent 
> goes, shouldn't 'read_gitfile_gently()' be solely responsible for 
> *opening a .git file and parsing the gitdir: <path> format*? However, it 
> currently executes 'stat()', checks 'S_ISDIR', checks 'S_ISREG', handles 
> missing components, and *then* attempts to parse. Do we need an 'enum 
> git_componet_type git_componet(cost char *path)' which returns pure 
> filesystem states, then parse it with 'parse_gitfile_format(const char 
> *path)'? I don't know.

Sorry, but I do not see what such a change buys us.

>   2. Let's say, when stat() encounters a EACCES when cheaking a 
> restricted sub-folder. Git funnels this into STAT_FAILED and 
> subsequently invokes die(), which calls exit().

Yes all this happens in repository set-up which should happen very
early in the process, no?


> I'm thinking of 
> libification: if a long running multi threaded git server encounters a 
> permission-denied directory, is killing the entire process the expected 
> behavior?

It is a bug in the way you wrote that multi-threaded git server, no?

We have the "_gently" variant for such a use case, and I do not
think we expect the normal single-process git start-up sequence
should be reused there.

Re: [PATCH v10] setup: improve error diagnosis for invalid .git files

[Tian Yuchen]

Hi Junio,

> Sorry, but I do not see what such a change buys us.

My primary concern was the function's semantics, though it is indeed 
redundant.

> Yes all this happens in repository set-up which should happen very
> early in the process, no?

Should be like this.

> It is a bug in the way you wrote that multi-threaded git server, no?
> 
> We have the "_gently" variant for such a use case, and I do not
> think we expect the normal single-process git start-up sequence
> should be reused there.

I see. Thank you for the insights. I will drop these overly abstract 
ideas and keep my focus on practical fixes.

Regards,

Yuchen

Re: [PATCH v11] setup: improve error diagnosis for invalid .git files

[Junio C Hamano]

Tian Yuchen <a3205153416@gmail.com> writes:

> 'read_gitfile_gently()' treats any non-regular file as
> 'READ_GITFILE_ERR_NOT_A_FILE' and fails to discern between 'ENOENT'
> and other stat failures. This flawed error reporting is noted by two
> 'NEEDSWORK' comments.
>
> Address these comments by introducing two new error codes:
> 'READ_GITFILE_ERR_MISSING'(which groups the "file missing" scenarios
> together) and 'READ_GITFILE_ERR_IS_A_DIR'.
>
> To preserve the original intent of the setup process:
> 1. Update 'read_gitfile_error_die()' to treat both 'IS_A_DIR' and
>    'MISSING' as no-ops, while continuing to call 'die()' on true
>    'NOT_A_FILE' errors to prevent security hazards (like FIFOs).
> 2. Unconditionally pass '&error_code' to 'read_gitfile_gently()'.
> 3. Only invoke 'is_git_directory()' when we explicitly receive
>    'READ_GITFILE_ERR_IS_A_DIR', avoiding redundant filesystem checks.
> 4. Correctly return 'GIT_DIR_INVALID_GITFILE' on unrecognized errors
>    when 'die_on_error' is false.
>
> Additionally, audit external callers of 'read_gitfile_gently()' in
> 'submodule.c' and 'worktree.c' to accommodate the refined error codes.
>
> Signed-off-by: Tian Yuchen <a3205153416@gmail.com>
> ---
>  setup.c                       | 45 ++++++++++++++------
>  setup.h                       |  2 +
>  submodule.c                   |  2 +-
>  t/meson.build                 |  1 +
>  t/t0009-git-dir-validation.sh | 77 +++++++++++++++++++++++++++++++++++
>  worktree.c                    |  6 ++-
>  6 files changed, 118 insertions(+), 15 deletions(-)
>  create mode 100755 t/t0009-git-dir-validation.sh

Unfortunately this seems to break almost all the tests, not just the
test the patch adds, on Windows (which I almost know nothing about,
but I can observe that CI jobs die).

https://github.com/git/git/actions/runs/22464017037 is a CI run that
merged this patch on top of the commit that corresponds to the tip
of 'next' as of today.  We can see "win test (N)" jobs dying all
over.  I cancelled the workflow before seeing everything die,
though.

https://github.com/git/git/actions/runs/22464479533 is a CI run that
tests this patch applied directly on v2.53.0 in isolation.

As I said, I do not know Windows well, so this may be a red-herring,
but in this CI run, we see "GIT_DIR=/dev/null git diff --no-index ..."
results in "fatal: error reading 'nul'":

  https://github.com/git/git/actions/runs/22464479533/job/65067515458#step:5:95419

which is an expected thing to happen, but we probably used to ignore
it as a non-error?

For now, I'll kick this topic out of my tree to give other topics a
bit more test exposure so that we can notice new bugs in them (not
in this topic) that causes the tests fail.  With this topic in 'seen',
such bugs in other topics are all masked.

Can folks equipped with knowledge and environment to debug Windows
only breakage lend a hand to figure out what this patch gets wrong
to help it move forward?


Thanks.

Re: [PATCH v11] setup: improve error diagnosis for invalid .git files

[Tian Yuchen]

Hi Junio,

Thank you for the error reporting.

It seems that failure "fatal: error reading 'nul'" matches the 
'die(_("error reading %s"), path)', if my understanding is correct?

So during 'git diff --no-index', the test passes 'GIT_DIR=/dev/null'. I 
highly suspect that 'stat("nul")' on Windows fails with an 'errno' other 
than 'ENOENT', so it falls into the 'STAT_FAILED' branch...

...which can be simply fixed by reverting 'READ_GITFILE_ERR_STAT_FAILED' 
(and probably 'READ_GITFILE_ERR_NOT_A_FILE') back to being non-fatal 
inside 'read_gitfile_error_die()', if I'm correct? In that case, the 
logic of the test script should also be changed, shouldn't it?

However, I'm sure that this change runs counter to our previous 
discussions. I can't think of any good ideas since I'm no windows expert 
either. So you can pretend I never said anything :P

Regards,

Yuchen

Re: [PATCH v11] setup: improve error diagnosis for invalid .git files

[Junio C Hamano]

Tian Yuchen <a3205153416@gmail.com> writes:

> It seems that failure "fatal: error reading 'nul'" matches the 
> 'die(_("error reading %s"), path)', if my understanding is correct?

The message is likely to be with its own pair of single quotes,
i.e., "error reading '%s'".

> So during 'git diff --no-index', the test passes 'GIT_DIR=/dev/null'. I 
> highly suspect that 'stat("nul")' on Windows fails with an 'errno' other 
> than 'ENOENT', so it falls into the 'STAT_FAILED' branch...

In the expected case, how would

    $ GIT_DIR=/dev/null git diff --no-index . .

start up?  setup_git_directory_gently() is called with a non-NULL
&nongit in turn it calls setup_git_directory_gently_1() that gives
us GIT_DIR_EXPLICIT, which makes us call setup_explicit_git_dir().

It would make us call is_git_diretory() on "/dev/null", and a
non-NULL nongit_ok pointer causes us to return NULL from there.

I am not sure how the result of calling stat() gets in the picture.

> ...which can be simply fixed by reverting 'READ_GITFILE_ERR_STAT_FAILED' 
> (and probably 'READ_GITFILE_ERR_NOT_A_FILE') back to being non-fatal 
> inside 'read_gitfile_error_die()', if I'm correct? In that case, the 
> logic of the test script should also be changed, shouldn't it?
>
> However, I'm sure that this change runs counter to our previous 
> discussions. I can't think of any good ideas since I'm no windows expert 
> either. So you can pretend I never said anything :P

It sounds like saying "oh, this is too hard, let's punt and roll all
failures from stat() into 'nothing there, move on'", which is how
the code used to work before this patch X-<.

Re: [PATCH v11] setup: improve error diagnosis for invalid .git files

[Tian Yuchen]

Hi Junio,

Thanks for pushing back on my previous suggestion.

> It sounds like saying "oh, this is too hard, let's punt and roll all
> failures from stat() into 'nothing there, move on'", which is how
> the code used to work before this patch X-<.

Yeah, that's why I said this idea was pretty dumb. XD

Regarding your question about how 'stat()' gets into the picture, I 
tried GDB debugging and simulated Windows behavior by 'GIT_DIR=nul' (I'm 
NOT sure if it's correct). Here is the back trace caught:

(gdb) print path
$1 = 0x555555a86dd0 "nul"
(gdb) bt
#0  read_gitfile_gently (path=0x555555a86dd0 "nul", return_error_code=0x0)
     at setup.c:936
#1  0x000055555589dea1 in setup_explicit_git_dir (
     gitdirenv=0x555555a86dd0 "nul", cwd=0x555555a58070 <cwd>,
     repo_fmt=0x7fffffffcbe0, nongit_ok=0x7fffffffccc4) at setup.c:1107
#2  0x000055555589fd53 in setup_git_directory_gently 
(nongit_ok=0x7fffffffccc4)
     at setup.c:1867
#3  0x00005555555c1383 in cmd_diff (argc=4, argv=0x555555a86ce0, 
prefix=0x0,
     repo=0x0) at builtin/diff.c:458
#4  0x0000555555575e30 in run_builtin (p=0x555555a48368 <commands+840>,
     argc=4, argv=0x555555a86ce0, repo=0x555555a7bdc0 <the_repo>) at 
git.c:506
#5  0x0000555555576354 in handle_builtin (args=0x7fffffffdc10) at git.c:780
#6  0x000055555557667e in run_argv (args=0x7fffffffdc10) at git.c:863
#7  0x0000555555576b44 in cmd_main (argc=4, argv=0x7fffffffdda0) at 
git.c:985
#8  0x00005555556a73bf in main (argc=5, argv=0x7fffffffdd98) at 
common-main.c:9

Sorry it might be a bit messy. It turns out that 
'setup_explicit_git_dir()' calls 'read_gitfile_gently()' right away to 
check whether GIT_DIR is actually a gitfile, right?

Since stat("nul") fails on windows, STAT_FAILED was caught and 
immediately triggered die(), blowing up corresponding CI tests.

Still, I don't know Windows well. These thoughts are just to stimulate 
discussions. （　´_ゝ`）

Thanks,

Yuchen

Re: [PATCH v11] setup: improve error diagnosis for invalid .git files

[Junio C Hamano]

Let's try again.

Can folks equipped with knowledge and environment to debug breakages
that hapepns only on Windows lend a hand to figure out what this
patch gets wrong to help the topic move forward?

Thanks.

Junio C Hamano <gitster@pobox.com> writes:

> Tian Yuchen <a3205153416@gmail.com> writes:
>
>> 'read_gitfile_gently()' treats any non-regular file as
>> 'READ_GITFILE_ERR_NOT_A_FILE' and fails to discern between 'ENOENT'
>> and other stat failures. This flawed error reporting is noted by two
>> 'NEEDSWORK' comments.
>>
>> Address these comments by introducing two new error codes:
>> 'READ_GITFILE_ERR_MISSING'(which groups the "file missing" scenarios
>> together) and 'READ_GITFILE_ERR_IS_A_DIR'.
>>
>> To preserve the original intent of the setup process:
>> 1. Update 'read_gitfile_error_die()' to treat both 'IS_A_DIR' and
>>    'MISSING' as no-ops, while continuing to call 'die()' on true
>>    'NOT_A_FILE' errors to prevent security hazards (like FIFOs).
>> 2. Unconditionally pass '&error_code' to 'read_gitfile_gently()'.
>> 3. Only invoke 'is_git_directory()' when we explicitly receive
>>    'READ_GITFILE_ERR_IS_A_DIR', avoiding redundant filesystem checks.
>> 4. Correctly return 'GIT_DIR_INVALID_GITFILE' on unrecognized errors
>>    when 'die_on_error' is false.
>>
>> Additionally, audit external callers of 'read_gitfile_gently()' in
>> 'submodule.c' and 'worktree.c' to accommodate the refined error codes.
>>
>> Signed-off-by: Tian Yuchen <a3205153416@gmail.com>
>> ---
>>  setup.c                       | 45 ++++++++++++++------
>>  setup.h                       |  2 +
>>  submodule.c                   |  2 +-
>>  t/meson.build                 |  1 +
>>  t/t0009-git-dir-validation.sh | 77 +++++++++++++++++++++++++++++++++++
>>  worktree.c                    |  6 ++-
>>  6 files changed, 118 insertions(+), 15 deletions(-)
>>  create mode 100755 t/t0009-git-dir-validation.sh
>
> Unfortunately this seems to break almost all the tests, not just the
> test the patch adds, on Windows (which I almost know nothing about,
> but I can observe that CI jobs die).
>
> https://github.com/git/git/actions/runs/22464017037 is a CI run that
> merged this patch on top of the commit that corresponds to the tip
> of 'next' as of today.  We can see "win test (N)" jobs dying all
> over.  I cancelled the workflow before seeing everything die,
> though.
>
> https://github.com/git/git/actions/runs/22464479533 is a CI run that
> tests this patch applied directly on v2.53.0 in isolation.
>
> As I said, I do not know Windows well, so this may be a red-herring,
> but in this CI run, we see "GIT_DIR=/dev/null git diff --no-index ..."
> results in "fatal: error reading 'nul'":
>
>   https://github.com/git/git/actions/runs/22464479533/job/65067515458#step:5:95419
>
> which is an expected thing to happen, but we probably used to ignore
> it as a non-error?
>
> For now, I'll kick this topic out of my tree to give other topics a
> bit more test exposure so that we can notice new bugs in them (not
> in this topic) that causes the tests fail.  With this topic in 'seen',
> such bugs in other topics are all masked.
>
>
>
> Thanks.

Re: [PATCH v11] setup: improve error diagnosis for invalid .git files

[Phillip Wood]

On 02/03/2026 16:26, Junio C Hamano wrote:
> Let's try again.
> 
> Can folks equipped with knowledge and environment to debug breakages
> that hapepns only on Windows lend a hand to figure out what this
> patch gets wrong to help the topic move forward?

Looking at the test failures the tests are failing because

     GIT_DIR=/dev/null git diff --no-index ...

which is used by test_cmp() on Windows is dying. That happens because 
stat("nul", &st) fails and this series makes that an error (somewhere 
along the line "/dev/null" is rewritten to "nul" on Windows). I'm afraid 
I don't know enough about Windows to be sure how to fix it but maybe we 
should special case "nul" in the setup code or mingw_stat(). We should 
also check what happens when GIT_DIR=/dev/null on linux and other POSIX 
platforms.

Thanks

Phillip


> Thanks.
> 
> Junio C Hamano <gitster@pobox.com> writes:
> 
>> Tian Yuchen <a3205153416@gmail.com> writes:
>>
>>> 'read_gitfile_gently()' treats any non-regular file as
>>> 'READ_GITFILE_ERR_NOT_A_FILE' and fails to discern between 'ENOENT'
>>> and other stat failures. This flawed error reporting is noted by two
>>> 'NEEDSWORK' comments.
>>>
>>> Address these comments by introducing two new error codes:
>>> 'READ_GITFILE_ERR_MISSING'(which groups the "file missing" scenarios
>>> together) and 'READ_GITFILE_ERR_IS_A_DIR'.
>>>
>>> To preserve the original intent of the setup process:
>>> 1. Update 'read_gitfile_error_die()' to treat both 'IS_A_DIR' and
>>>     'MISSING' as no-ops, while continuing to call 'die()' on true
>>>     'NOT_A_FILE' errors to prevent security hazards (like FIFOs).
>>> 2. Unconditionally pass '&error_code' to 'read_gitfile_gently()'.
>>> 3. Only invoke 'is_git_directory()' when we explicitly receive
>>>     'READ_GITFILE_ERR_IS_A_DIR', avoiding redundant filesystem checks.
>>> 4. Correctly return 'GIT_DIR_INVALID_GITFILE' on unrecognized errors
>>>     when 'die_on_error' is false.
>>>
>>> Additionally, audit external callers of 'read_gitfile_gently()' in
>>> 'submodule.c' and 'worktree.c' to accommodate the refined error codes.
>>>
>>> Signed-off-by: Tian Yuchen <a3205153416@gmail.com>
>>> ---
>>>   setup.c                       | 45 ++++++++++++++------
>>>   setup.h                       |  2 +
>>>   submodule.c                   |  2 +-
>>>   t/meson.build                 |  1 +
>>>   t/t0009-git-dir-validation.sh | 77 +++++++++++++++++++++++++++++++++++
>>>   worktree.c                    |  6 ++-
>>>   6 files changed, 118 insertions(+), 15 deletions(-)
>>>   create mode 100755 t/t0009-git-dir-validation.sh
>>
>> Unfortunately this seems to break almost all the tests, not just the
>> test the patch adds, on Windows (which I almost know nothing about,
>> but I can observe that CI jobs die).
>>
>> https://github.com/git/git/actions/runs/22464017037 is a CI run that
>> merged this patch on top of the commit that corresponds to the tip
>> of 'next' as of today.  We can see "win test (N)" jobs dying all
>> over.  I cancelled the workflow before seeing everything die,
>> though.
>>
>> https://github.com/git/git/actions/runs/22464479533 is a CI run that
>> tests this patch applied directly on v2.53.0 in isolation.
>>
>> As I said, I do not know Windows well, so this may be a red-herring,
>> but in this CI run, we see "GIT_DIR=/dev/null git diff --no-index ..."
>> results in "fatal: error reading 'nul'":
>>
>>    https://github.com/git/git/actions/runs/22464479533/job/65067515458#step:5:95419
>>
>> which is an expected thing to happen, but we probably used to ignore
>> it as a non-error?
>>
>> For now, I'll kick this topic out of my tree to give other topics a
>> bit more test exposure so that we can notice new bugs in them (not
>> in this topic) that causes the tests fail.  With this topic in 'seen',
>> such bugs in other topics are all masked.
>>
>>
>>
>> Thanks.
> 

Re: [PATCH v11] setup: improve error diagnosis for invalid .git files

[Junio C Hamano]

Phillip Wood <phillip.wood123@gmail.com> writes:

> Looking at the test failures the tests are failing because
>
>      GIT_DIR=/dev/null git diff --no-index ...
>
> which is used by test_cmp() on Windows is dying. That happens because 
> stat("nul", &st) fails and this series makes that an error (somewhere 
> along the line "/dev/null" is rewritten to "nul" on Windows). I'm afraid 
> I don't know enough about Windows to be sure how to fix it but maybe we 
> should special case "nul" in the setup code or mingw_stat().

While I do not think we want to special case "nul", I think we need
to make the tightening of error checking conditional to who is
asking to know.  The _intent_ behind the NEEDSWORK comment was to
allow us to be more strict when we see a fishy ".git" filesystem
entity when we are in a directory /a/b/c/d and are trying to find
out if we are in a Git working tree and where our .git directory is.
We may not see /a/b/c/d/.git, go up one level and find /a/b/c/.git
and stat(2) it.  In the current code, we take any and all stat(2)
failures as if it failed because ENOENT i.e., as if /a/b/c/.git did
not exist, and we go upwards.  The NEEDSWORK comment wonders if we
want to be noticing that /a/b/c/.git did exist but we failed to
stat(2) for some other reason, and if we would want to let the user
know.

But the "GIT_DIR=/dev/null git ..." use case is vastly different.
We are not doing a discovery and we are not interested in going
upwards when the thing we check for "git-dir-ness" fails to be a
git-dir.  It may have worked around the "nul cannot be stat'ed"
limitation if we used "GIT_DIR=no-such-directory git ...", but I
think anything that we positively know is not a .git directory or a
"gitdir: over-there" file is given as GIT_DIR, we would want to say
"nope, we do not have .git dir and have to work outside any
repository", instead of dying with "whoa, what is that garbage you
are giving me as GIT_DIR???".

WIth an explicitly given GIT_DIR, setup_explicit_git_dir() is called
and the function calls read_gitfile_gently(path, NULL), that lets it
die when the thing turns out not to be a proper ".git", except when
stat(2) failed (for any reason) or stat(2) tells that the thing is
not a file.  If we use GIT_DIR=/dev/null on POSIX systems, this
"not-a-file is fine" leniency allows us to proceed without dying,
saying "We were given an invalid GIT_DIR, we are not doing
discovery, hence we are operating without a repository".  On systems
where stat(2) fails for "/dev/null" or its equivalent "NUL:", the
same "stat failed for any reason" leniency allows us to do the same.

But with the patches we have been looking at, that leniency is
tightened.  I think it is a good thing to do during the repository
discovery.  But it is not if we are checking the explicitly given
GIT_DIR.  All calls to read_gitfile_gently(path, NULL) need to be
audited and then we need to decide which ones to leave lenient, and
which ones are OK to tighten together with the call used during the
repository discovery.

Thanks.

Re: [PATCH v11] setup: improve error diagnosis for invalid .git files

[Tian Yuchen]

Hi Junio and Phillip,

Thanks for the detailed reply!

After reading through your discussion, I believe the most crucial point is:

> "We were given an invalid GIT_DIR, we are not doing
> discovery, hence we are operating without a repository"

If I understand correctly, the expected behavior should be: when a user 
explicitly passes 'GIT_DIR=/dev/null git diff', Git should no longer 
need to "search" or "guess" anything. Instead, if it's a trash file (or 
something similar) rather a repository, Git should simply act as if no 
repository exists. Is that correct?

So what I'm doing next is:

> All calls to read_gitfile_gently(path, NULL) need to be
> audited and then we need to decide which ones to leave lenient, and
> which ones are OK to tighten together with the call used during the
> repository discovery.

Will be working on it in the next few days.

Regards,

Yuchen

[PATCH v12] setup: improve error diagnosis for invalid .git files

[Tian Yuchen]

'read_gitfile_gently()' treats any non-regular file as
'READ_GITFILE_ERR_NOT_A_FILE' and fails to discern between 'ENOENT'
and other stat failures. This flawed error reporting is noted by two
'NEEDSWORK' comments.

Address these comments by introducing two new error codes:
'READ_GITFILE_ERR_MISSING'(which groups the "file missing" scenarios
together) and 'READ_GITFILE_ERR_IS_A_DIR':

1. Update 'read_gitfile_error_die()' to treat 'IS_A_DIR', 'MISSING',
'NOT_A_FILE' and 'STAT_FAILED' as non-fatal no-ops. This accommodates
intentional non-repo scenarios (e.g., GIT_DIR=/dev/null).

2. Explicitly catch 'NOT_A_FILE' and 'STAT_FAILED' during
discovery and call 'die()' if 'die_on_error' is set.

3. Unconditionally pass '&error_code' to 'read_gitfile_gently()'.

4. Only invoke 'is_git_directory()' when we explicitly receive
   'READ_GITFILE_ERR_IS_A_DIR', avoiding redundant checks.

Additionally, audit external callers of 'read_gitfile_gently()' in
'submodule.c' and 'worktree.c' to accommodate the refined error codes.

Signed-off-by: Tian Yuchen <a3205153416@gmail.com>
---
To be honest, I've really gotten myself all tangled up.
Skill issue :(
Feel free to point out all the stupid mistakes I made.

I'm very uncertain about whether my changes in 
setup_git_directory_gently_1() are appropriate.
But least all CI tests passed.

By the way, the replies in my email inbox look particularly messy.
When sending a new patch, which email should I reply to? Should I
reply to the previous patch, or, start a new thread?

 setup.c                       | 47 ++++++++++++++++-----
 setup.h                       |  2 +
 submodule.c                   |  2 +-
 t/meson.build                 |  1 +
 t/t0009-git-dir-validation.sh | 77 +++++++++++++++++++++++++++++++++++
 worktree.c                    |  6 ++-
 6 files changed, 121 insertions(+), 14 deletions(-)
 create mode 100755 t/t0009-git-dir-validation.sh

diff --git a/setup.c b/setup.c
index c8336eb20e..3bf96516ba 100644
--- a/setup.c
+++ b/setup.c
@@ -897,8 +897,10 @@ 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_FAILED:
 	case READ_GITFILE_ERR_NOT_A_FILE:
+	case READ_GITFILE_ERR_STAT_FAILED:
+	case READ_GITFILE_ERR_MISSING:
+	case READ_GITFILE_ERR_IS_A_DIR:
 		/* non-fatal; follow return path */
 		break;
 	case READ_GITFILE_ERR_OPEN_FAILED:
@@ -941,8 +943,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 || errno == ENOTDIR)
+			error_code = READ_GITFILE_ERR_MISSING;
+		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;
 	}
 	if (!S_ISREG(st.st_mode)) {
@@ -1578,20 +1586,37 @@ 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);
 		if (!gitdirenv) {
-			if (die_on_error ||
-			    error_code == READ_GITFILE_ERR_NOT_A_FILE) {
-				/* NEEDSWORK: fail if .git is not file nor dir */
+			switch (error_code) {
+			case READ_GITFILE_ERR_MISSING:
+				/* no .git in this directory, move on */
+				break;
+			case 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
+				break;
+			case READ_GITFILE_ERR_STAT_FAILED:
+				if (die_on_error)
+					die(_("error reading '%s'"), dir->buf);
+				else
+					return GIT_DIR_INVALID_GITFILE;
+			case READ_GITFILE_ERR_NOT_A_FILE:
+				if (die_on_error)
+					die(_("not a regular file: '%s'"), dir->buf);
+				else
+					return GIT_DIR_INVALID_GITFILE;
+			default:
+				if (die_on_error)
+					read_gitfile_error_die(error_code, dir->buf, NULL);
+				else
+					return GIT_DIR_INVALID_GITFILE;
+			}
+		} else {
 			gitfile = xstrdup(dir->buf);
+		}
 		/*
 		 * Earlier, we tentatively added DEFAULT_GIT_DIR_ENVIRONMENT
 		 * to check that directory for a repository.
diff --git a/setup.h b/setup.h
index 0738dec244..76fb260c20 100644
--- a/setup.h
+++ b/setup.h
@@ -36,6 +36,8 @@ int is_nonbare_repository_dir(struct strbuf *path);
 #define READ_GITFILE_ERR_NO_PATH 6
 #define READ_GITFILE_ERR_NOT_A_REPO 7
 #define READ_GITFILE_ERR_TOO_LARGE 8
+#define READ_GITFILE_ERR_MISSING 9
+#define READ_GITFILE_ERR_IS_A_DIR 10
 void read_gitfile_error_die(int error_code, const char *path, const char *dir);
 const char *read_gitfile_gently(const char *path, int *return_error_code);
 #define read_gitfile(path) read_gitfile_gently((path), NULL)
diff --git a/submodule.c b/submodule.c
index 508938e4da..767d4c3c35 100644
--- a/submodule.c
+++ b/submodule.c
@@ -2559,7 +2559,7 @@ void absorb_git_dir_into_superproject(const char *path,
 		const struct submodule *sub;
 		struct strbuf sub_gitdir = STRBUF_INIT;
 
-		if (err_code == READ_GITFILE_ERR_STAT_FAILED) {
+		if (err_code == READ_GITFILE_ERR_MISSING) {
 			/* unpopulated as expected */
 			strbuf_release(&gitdir);
 			return;
diff --git a/t/meson.build b/t/meson.build
index f80e366cff..c4afaacee5 100644
--- a/t/meson.build
+++ b/t/meson.build
@@ -80,6 +80,7 @@ integration_tests = [
   't0006-date.sh',
   't0007-git-var.sh',
   't0008-ignores.sh',
+  't0009-git-dir-validation.sh',
   't0010-racy-git.sh',
   't0012-help.sh',
   't0013-sha1dc.sh',
diff --git a/t/t0009-git-dir-validation.sh b/t/t0009-git-dir-validation.sh
new file mode 100755
index 0000000000..33d21ed9ea
--- /dev/null
+++ b/t/t0009-git-dir-validation.sh
@@ -0,0 +1,77 @@
+#!/bin/sh
+
+test_description='setup: validation of .git file/directory types
+
+Verify that setup_git_directory() correctly handles:
+1. Valid .git directories (including symlinks to them).
+2. Invalid .git files (FIFOs, sockets) by erroring out.
+3. Invalid .git files (garbage) by erroring out.
+'
+
+. ./test-lib.sh
+
+test_expect_success 'setup: create parent git repository' '
+	git init parent &&
+	test_commit -C parent "root-commit"
+'
+
+test_expect_success SYMLINKS 'setup: .git as a symlink to a directory is valid' '
+	test_when_finished "rm -rf parent/link-to-dir" &&
+	mkdir -p parent/link-to-dir &&
+	(
+		cd parent/link-to-dir &&
+		git init real-repo &&
+		ln -s real-repo/.git .git &&
+		git rev-parse --git-dir >actual &&
+		echo .git >expect &&
+		test_cmp expect actual
+	)
+'
+
+test_expect_success PIPE 'setup: .git as a FIFO (named pipe) is rejected' '
+	test_when_finished "rm -rf parent/fifo-trap" &&
+	mkdir -p parent/fifo-trap &&
+	(
+		cd parent/fifo-trap &&
+		mkfifo .git &&
+		test_must_fail git rev-parse --git-dir 2>stderr &&
+		grep "not a regular file" stderr
+	)
+'
+
+test_expect_success SYMLINKS,PIPE 'setup: .git as a symlink to a FIFO is rejected' '
+	test_when_finished "rm -rf parent/symlink-fifo-trap" &&
+	mkdir -p parent/symlink-fifo-trap &&
+	(
+		cd parent/symlink-fifo-trap &&
+		mkfifo target-fifo &&
+		ln -s target-fifo .git &&
+		test_must_fail git rev-parse --git-dir 2>stderr &&
+		grep "not a regular file" stderr
+	)
+'
+
+test_expect_success 'setup: .git with garbage content is rejected' '
+	test_when_finished "rm -rf parent/garbage-trap" &&
+	mkdir -p parent/garbage-trap &&
+	(
+		cd parent/garbage-trap &&
+		echo "garbage" >.git &&
+		test_must_fail git rev-parse --git-dir 2>stderr &&
+		grep "invalid gitfile format" stderr
+	)
+'
+
+test_expect_success 'setup: .git as an empty directory is ignored' '
+	test_when_finished "rm -rf parent/empty-dir" &&
+	mkdir -p parent/empty-dir &&
+	(
+		cd parent/empty-dir &&
+		git rev-parse --git-dir >expect &&
+		mkdir .git &&
+		git rev-parse --git-dir >actual &&
+		test_cmp expect actual
+	)
+'
+
+test_done
diff --git a/worktree.c b/worktree.c
index 9308389cb6..d1165e1d1c 100644
--- a/worktree.c
+++ b/worktree.c
@@ -653,7 +653,8 @@ static void repair_gitfile(struct worktree *wt,
 		}
 	}
 
-	if (err == READ_GITFILE_ERR_NOT_A_FILE)
+	if (err == READ_GITFILE_ERR_NOT_A_FILE ||
+		err == READ_GITFILE_ERR_IS_A_DIR)
 		fn(1, wt->path, _(".git is not a file"), cb_data);
 	else if (err)
 		repair = _(".git file broken");
@@ -833,7 +834,8 @@ void repair_worktree_at_path(const char *path,
 			strbuf_addstr(&backlink, dotgit_contents);
 			strbuf_realpath_forgiving(&backlink, backlink.buf, 0);
 		}
-	} else if (err == READ_GITFILE_ERR_NOT_A_FILE) {
+	} else if (err == READ_GITFILE_ERR_NOT_A_FILE ||
+			err == READ_GITFILE_ERR_IS_A_DIR) {
 		fn(1, dotgit.buf, _("unable to locate repository; .git is not a file"), cb_data);
 		goto done;
 	} else if (err == READ_GITFILE_ERR_NOT_A_REPO) {
-- 
2.43.0

Re: [PATCH v11] setup: improve error diagnosis for invalid .git files

[Junio C Hamano]

Tian Yuchen <a3205153416@gmail.com> writes:

> Hi Junio and Phillip,
>
> Thanks for the detailed reply!
>
> After reading through your discussion, I believe the most crucial point is:
>
>> "We were given an invalid GIT_DIR, we are not doing
>> discovery, hence we are operating without a repository"
>
> If I understand correctly, the expected behavior should be: when a user 
> explicitly passes 'GIT_DIR=/dev/null git diff', Git should no longer 
> need to "search" or "guess" anything. Instead, if it's a trash file (or 
> something similar) rather a repository, Git should simply act as if no 
> repository exists. Is that correct?

That is one of the things.  The broken test highlighted that
GIT_DIR_EXPLICIT case needs more thought than what we have discussed
so far, but there may be other cases that we need to also think
about.  See what different cases are in the big switch statement in
setup_git_directory_gently().

> So what I'm doing next is:
>
>> All calls to read_gitfile_gently(path, NULL) need to be
>> audited and then we need to decide which ones to leave lenient, and
>> which ones are OK to tighten together with the call used during the
>> repository discovery.
>
> Will be working on it in the next few days.

Thanks.

Re: [PATCH v11] setup: improve error diagnosis for invalid .git files

[Tian Yuchen]

Hi Junio,

> That is one of the things.  The broken test highlighted that
> GIT_DIR_EXPLICIT case needs more thought than what we have discussed
> so far, but there may be other cases that we need to also think
> about.  See what different cases are in the big switch statement in
> setup_git_directory_gently().

Shortly before your email I had already sent the v12 patch.

I did make changes in setup_git_directory_gently_1(), and logically it 
shouldn't cause major issues.

Unfortunately, looking back now, my implementation barely qualifies as 
“functional” and actually undermines the purpose of setup_git..() 
itself. It's a mess — I rushed into it without properly reviewing the 
context (like how other cases are handled) :(((

I'll polish it thoroughly before releasing v13.

Regards,

Yuchen

Re: [PATCH v11] setup: improve error diagnosis for invalid .git files

[Junio C Hamano]

Tian Yuchen <a3205153416@gmail.com> writes:

> Unfortunately, looking back now, my implementation barely qualifies as 
> “functional” and actually undermines the purpose of setup_git..() 
> itself. It's a mess — I rushed into it without properly reviewing the 
> context (like how other cases are handled) :(((

v12 does work differently from what we have been aiming for, but I
find that it arguably is a much safer approach.

Even though the updated read_gitfile_gently() returns finer-grained
READ_GITFILE_ERR_* codes than the original, read_gitfile_error_die()
does not change behaviour from the original.  Any caller that use
read_gitfile(path), which is read_gitfile_gently(path, NULL), like
the setup_explicit_git_dir() codepath we have been looking at lately,
lets read_gitfile_error_die() react to the error code, which is to
behave exactly as what the code did before this patch.

So, I dunno.  After all, these two NEEDSWORK comments have been with
us for quite some time, and reminded us that we may want to consider
if we need to do anything differently.  I do not think we mind if we
conclude negatively, taking "no, it is of dubious value to tighten
error checking in these code paths" as an answer to these NEEDSWORK
comments.  v12 is slightly less defeatest than that stance in that
we are only allowing the callers that care about what kind of errors
they are getting and and want to decide how to react to them, while
keeping the default error behaviour the same for those who do not
ask with &error_code what kind of errors we saw.

The patch makes the behaviour change for callers that pass an
&error_code pointer to read_gitfile_gently() and act on the returned
error code itself, like the discovery code path.  As long as these
callers are audited and adjusted as necessary, we have very little
risk of regression.

I won't be doing a full audit in this message, but just to give
taste of what is expected ...

$ git grep -n -e 'read_gitfile_gently('

builtin/init-db.c:212:		p = read_gitfile_gently(git_dir, &err);

This caller gives &err but it never looks at what is in it after the
call returns, so there shouldn't be any behaviour change.

setup.c:465:	if (read_gitfile_gently(path->buf, &gitfile_error) || is_git_directory(path->buf))

This is followed by 

		ret = 1;
        if (gitfile_error == READ_GITFILE_ERR_OPEN_FAILED ||
            gitfile_error == READ_GITFILE_ERR_READ_FAILED)
                ret = 1;

I do not offhand know if this list of "error codes that should
result in returning 1 from this function" needs to be tweaked to
adjust for the change in this patch.

worktree.c:390:	path = xstrdup_or_null(read_gitfile_gently(wt_path.buf, &err));

This is followed by code that reacts to path being NULL and shows
the contents of err in an error message.  Should be benign.

Thanks.

Re: [PATCH v11] setup: improve error diagnosis for invalid .git files

[Tian Yuchen]

Hi Junio,

On 3/5/26 02:06, Junio C Hamano wrote:

> v12 does work differently from what we have been aiming for, but I
> find that it arguably is a much safer approach.

Maybe, but my main concern was that adding 'die()' in 
'setup_git_directory_gently_1()' might not be the best choice. 
Considering the implementations of the preceding functions, I though 
locating 'die()' in 'setup_explicit_git_dir()' might be a better choice? 
By the way, I noticed there's a '(read_gitfile(path))' macro that 
expands to 'read_gitfile(path, NULL)'. I was planning to pass 
'error_code' here, essentially moving the logic from the original 
'setup_git_directory_gently_1()' to this location, where the former 
would only be responsible for returning the error status... The changes 
would be a bit too extensive if I did it that way.

Since you say the v12 patch is safer, I have no objections. I'm 
completely unsure about the overall structure of this part. Skill issue.

> Even though the updated read_gitfile_gently() returns finer-grained
> READ_GITFILE_ERR_* codes than the original, read_gitfile_error_die()
> does not change behaviour from the original.  Any caller that use
> read_gitfile(path), which is read_gitfile_gently(path, NULL), like
> the setup_explicit_git_dir() codepath we have been looking at lately,
> lets read_gitfile_error_die() react to the error code, which is to
> behave exactly as what the code did before this patch.

I should have considered this. I feel that the changes in v12 are 
actually more like a band-aid fix compared to the previous ones.

You mentioned this change didn't touch 'read_gitfile_error_die()', and I 
think that's one of the benefits of such simple modifications. I just 
can't be entirely sure what the trade-offs might be — perhaps it's my 
lack of experience, but I always worry about digging a deep hole for 
future developers to fill. I'll keep learning.

> So, I dunno.  After all, these two NEEDSWORK comments have been with
> us for quite some time, and reminded us that we may want to consider
> if we need to do anything differently.  I do not think we mind if we
> conclude negatively, taking "no, it is of dubious value to tighten
> error checking in these code paths" as an answer to these NEEDSWORK
> comments.  v12 is slightly less defeatest than that stance in that
> we are only allowing the callers that care about what kind of errors
> they are getting and and want to decide how to react to them, while
> keeping the default error behaviour the same for those who do not
> ask with &error_code what kind of errors we saw.

I see.

> The patch makes the behaviour change for callers that pass an
> &error_code pointer to read_gitfile_gently() and act on the returned
> error code itself, like the discovery code path.  As long as these
> callers are audited and adjusted as necessary, we have very little
> risk of regression.
> 
> I won't be doing a full audit in this message, but just to give
> taste of what is expected ...
> 
> $ git grep -n -e 'read_gitfile_gently('
> 
> builtin/init-db.c:212:		p = read_gitfile_gently(git_dir, &err);
> 
> This caller gives &err but it never looks at what is in it after the
> call returns, so there shouldn't be any behaviour change.
> 
> setup.c:465:	if (read_gitfile_gently(path->buf, &gitfile_error) || is_git_directory(path->buf))
> 
> This is followed by
> 
> 		ret = 1;
>          if (gitfile_error == READ_GITFILE_ERR_OPEN_FAILED ||
>              gitfile_error == READ_GITFILE_ERR_READ_FAILED)
>                  ret = 1;
> 
> I do not offhand know if this list of "error codes that should
> result in returning 1 from this function" needs to be tweaked to
> adjust for the change in this patch.
> 
> worktree.c:390:	path = xstrdup_or_null(read_gitfile_gently(wt_path.buf, &err));
> 
> This is followed by code that reacts to path being NULL and shows
> the contents of err in an error message.  Should be benign.

I don't think this needs to be changed. If a new error_code 
READ_GITFILE_MISSING is encountered, then this isn't a repository at 
all, so it won't enter the if statement, and ret = 0.

If encountering the READ_GITFILE_ERR_IS_A_DIR, the 
is_git_directory(path->buf) check takes precedence. If it detects a 
valid .git directory, it returns 1. Therefore, the gitfile_error 
function doesn't need to match IS_A_DIR at all.

Thank you for taking the time to review my (roughly made) patch.

Goodnight,

Yuchen

Re: [PATCH v11] setup: improve error diagnosis for invalid .git files

[Junio C Hamano]

Tian Yuchen <a3205153416@gmail.com> writes:

> Maybe, but my main concern was that adding 'die()' in 
> 'setup_git_directory_gently_1()' might not be the best choice. 
> Considering the implementations of the preceding functions, I though 
> locating 'die()' in 'setup_explicit_git_dir()' might be a better choice? 

You may be right.  I didn't take a careful enough look to comment.

> By the way, I noticed there's a '(read_gitfile(path))' macro that 
> expands to 'read_gitfile(path, NULL)'. I was planning to pass 
> 'error_code' here, essentially moving the logic from the original 
> 'setup_git_directory_gently_1()' to this location, where the former 
> would only be responsible for returning the error status... The changes 
> would be a bit too extensive if I did it that way.

True.  It would be a lot more invasive change.  I do not know if it
is worth our time _right_ _now_, or if it is better to be left for
future iterations.

Re: [PATCH v11] setup: improve error diagnosis for invalid .git files

[Tian Yuchen]

On 3/5/26 06:50, Junio C Hamano wrote:
> Tian Yuchen <a3205153416@gmail.com> writes:
> 
>> Maybe, but my main concern was that adding 'die()' in
>> 'setup_git_directory_gently_1()' might not be the best choice.
>> Considering the implementations of the preceding functions, I though
>> locating 'die()' in 'setup_explicit_git_dir()' might be a better choice?
> 
> You may be right.  I didn't take a careful enough look to comment.
> 
>> By the way, I noticed there's a '(read_gitfile(path))' macro that
>> expands to 'read_gitfile(path, NULL)'. I was planning to pass
>> 'error_code' here, essentially moving the logic from the original
>> 'setup_git_directory_gently_1()' to this location, where the former
>> would only be responsible for returning the error status... The changes
>> would be a bit too extensive if I did it that way.
> 
> True.  It would be a lot more invasive change.  I do not know if it
> is worth our time _right_ _now_, or if it is better to be left for
> future iterations.

I will hold off on any further iterations and leave v12 as is, unless 
you or others spot any specific details in it that still need tweaking.

Thank you so much for the patience and guidance throughout this entire 
series! I really learned a lot from it.

Regards,

Yuchen

Re: [PATCH v11] setup: improve error diagnosis for invalid .git files

[Junio C Hamano]

Tian Yuchen <a3205153416@gmail.com> writes:

>> True.  It would be a lot more invasive change.  I do not know if it
>> is worth our time _right_ _now_, or if it is better to be left for
>> future iterations.
>
> I will hold off on any further iterations and leave v12 as is, unless 
> you or others spot any specific details in it that still need tweaking.
>
> Thank you so much for the patience and guidance throughout this entire 
> series! I really learned a lot from it.

Sounds good.  Let me mark the topic for 'next'.