v6, 2 partssetup: allow cwd/.git to be a symlink to a directory
45 messages between Feb 18, 2026 and Mar 9, 2026, from Tian Yuchen, Junio C Hamano, Karthik Nayak, Phillip Wood.
Plain Markdown or JSON for tools and agents. Diffs are folded; open one to read it.
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
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(-)
Show changes to 2 files +15 −4
setup.c, setup.h
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
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
Show changes to 3 files +86 −8
setup.c, t/meson.build, 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
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
Show changes to 4 files +104 −15
setup.c, setup.h, t/meson.build, 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
Tian Yuchen <a3205153416@gmail.com> writes:
Show 16 quoted lines
> 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?
Show 13 quoted lines
> 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;
> }Show 20 quoted lines
> @@ -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
On 2/20/26 11:40, Junio C Hamano wrote:
Thanks for the review!
Show 6 quoted lines
>> 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.
Show 23 quoted lines
>> @@ -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 likeTo 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.
Show 24 quoted lines
> 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
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
Show changes to 4 files +105 −12
setup.c, setup.h, t/meson.build, 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
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 logEven 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.
Show 38 quoted lines
> 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.
Show 19 quoted lines
> @@ -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 ...
Show 5 quoted lines
> + 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
> 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 ;)
Show 9 quoted lines
> 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
'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
Show changes to 4 files +105 −12
setup.c, setup.h, t/meson.build, 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
Tian Yuchen <a3205153416@gmail.com> writes:
Show 13 quoted lines
>> 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.
Re: [PATCH v8] setup: allow cwd/.git to be a symlink to a directory
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
Tian Yuchen <a3205153416@gmail.com> writes:
Show 16 quoted lines
> 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
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
'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
Show changes to 6 files +110 −15
setup.c, setup.h, submodule.c, t/meson.build, t/t0009-git-dir-validation.sh, worktree.c
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
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'.
>
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.
Show 17 quoted lines
> 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?
Show 18 quoted lines
> 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.
Show 19 quoted lines
> 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;
> }Show 8 quoted lines
> 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`.
Show 17 quoted lines
> 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.
Show 10 quoted lines
> + 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.
Show 65 quoted lines
> /*
> * 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.
Show 80 quoted lines
> + 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.0The rest looks good. Thanks!
Re: [PATCH v10] setup: improve error diagnosis for invalid .git files
Tian Yuchen <a3205153416@gmail.com> writes:
Show 33 quoted lines
> '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 <gitster@pobox.com> writes:
Show 28 quoted lines
>> 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(-)
Show changes to 3 files +6 −6
setup.c, setup.h, submodule.c
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
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
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
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.
Show 5 quoted lines
> 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
'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
Show changes to 6 files +118 −15
setup.c, setup.h, submodule.c, t/meson.build, t/t0009-git-dir-validation.sh, worktree.c
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 <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
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
Tian Yuchen <a3205153416@gmail.com> writes:
Show 15 quoted lines
> 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
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
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
Tian Yuchen <a3205153416@gmail.com> writes:
Show 8 quoted lines
> 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
> 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?
Show 5 quoted lines
> 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
Tian Yuchen <a3205153416@gmail.com> writes:
Show 32 quoted lines
> '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
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
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.
Show 8 quoted lines
> ...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
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:9Sorry 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
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:
Show 65 quoted lines
> 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
On 02/03/2026 16:26, Junio C Hamano wrote:
Show 5 quoted lines
> 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
Show 70 quoted lines
> 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 <phillip.wood123@gmail.com> writes:
Show 9 quoted lines
> 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
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
'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
Show changes to 6 files +121 −14
setup.c, setup.h, submodule.c, t/meson.build, t/t0009-git-dir-validation.sh, worktree.c
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
Tian Yuchen <a3205153416@gmail.com> writes:
Show 14 quoted lines
> 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().
Show 8 quoted lines
> 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.
Re: [PATCH v11] setup: improve error diagnosis for invalid .git files
Show 5 quoted lines
> 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
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
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.
Show 7 quoted lines
> 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.
Show 10 quoted lines
> 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.
Show 33 quoted lines
> 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
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.
Show 6 quoted lines
> 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
On 3/5/26 06:50, Junio C Hamano wrote:
Show 19 quoted lines
> 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
Tian Yuchen <a3205153416@gmail.com> writes:
Show 9 quoted lines
>> 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'.
Tian Yuchen
