From: Karthik Nayak Date: Sun, 22 Feb 2026 16:53:35 GMT Subject: Re: [PATCH v10] setup: improve error diagnosis for invalid .git files Message-ID: In-Reply-To: <20260222102928.377519-1-a3205153416@gmail.com> Tian Yuchen writes: > 'read_gitfile_gently()' treats any non-regular file as > 'READ_GITFILE_ERR_NOT_A_FILE' and fails to discern between 'ENOENT' > and other stat failures. This flawed error reporting is noted by two > 'NEEDSWORK' comments. Okay. > Address these comments by introducing two new error codes: > 'READ_GITFILE_ERR_STAT_ENOENT' and 'READ_GITFILE_ERR_IS_A_DIR'. > Nit: This is much better, we seem to talk about the issues and the new errors introduced. I wonder if we can tie the errors to the issues. Perhaps The 'read_gitfile_gently()' is used to obtain the location of a git directory by parsing a '.git' file. When parsing the file with 'stat(2)', it fails to differentiate between a 'ENOENT' and other errors. Introduce 'READ_GITFILE_ERR_STAT_ENOENT' to make this differentiation. The function also marks directories as 'READ_GITFILE_ERR_NOT_A_FILE', introduce 'READ_GITFILE_ERR_IS_A_DIR' to specifically make this distinction. > To preserve the original intent of the setup process: > 1. Update 'read_gitfile_error_die()' to treat 'IS_A_DIR' as a no-op > (like 'ENOENT'), while still calling 'die()' on true 'NOT_A_FILE' > errors. Nice, shouldn't we also mention READ_GITFILE_ERR_STAT_ENOENT is now treated as a no-op, while its counterpart is not. > 2. Unconditionally pass '&error_code' to 'read_gitfile_gently()'. This > eliminates an uninitialized variable hazard that occurred when > 'die_on_error' was true and 'NULL' was passed. Where is the 'uninitialized variable hazard'? The function says: If return_error_code is NULL the function will die instead > 3. Only invoke 'is_git_directory()' when we explicitly receive > 'READ_GITFILE_ERR_IS_A_DIR', avoiding redundant filesystem checks. Nice. > 4. Correctly return 'GIT_DIR_INVALID_GITFILE' on unrecognized errors > when 'die_on_error' is false. > > Additionally, audit external callers of 'read_gitfile_gently()' in > 'submodule.c' and 'worktree.c' to accommodate the refined error codes. > > Signed-off-by: Tian Yuchen > --- > setup.c | 42 ++++++++++++++------ > setup.h | 2 + > submodule.c | 2 +- > t/meson.build | 1 + > t/t0009-git-dir-validation.sh | 72 +++++++++++++++++++++++++++++++++++ > worktree.c | 6 ++- > 6 files changed, 110 insertions(+), 15 deletions(-) > create mode 100755 t/t0009-git-dir-validation.sh > I couldn't find a discussion, why did we merge the commits? > diff --git a/setup.c b/setup.c > index c8336eb20e..9d49b9ae53 100644 > --- a/setup.c > +++ b/setup.c > @@ -897,10 +897,14 @@ int verify_repository_format(const struct repository_format *format, > void read_gitfile_error_die(int error_code, const char *path, const char *dir) > { > switch (error_code) { > - case READ_GITFILE_ERR_STAT_FAILED: > - case READ_GITFILE_ERR_NOT_A_FILE: > + case READ_GITFILE_ERR_STAT_ENOENT: > + case READ_GITFILE_ERR_IS_A_DIR: > /* non-fatal; follow return path */ > break; > + case READ_GITFILE_ERR_STAT_FAILED: > + die(_("error reading %s"), path); > + case READ_GITFILE_ERR_NOT_A_FILE: > + die(_("not a regular file: %s"), path); Not your fault, but some of the errors quote the path and some don't, it would be nice to be uniform here. > case READ_GITFILE_ERR_OPEN_FAILED: > die_errno(_("error opening '%s'"), path); > case READ_GITFILE_ERR_TOO_LARGE: > @@ -941,8 +945,14 @@ const char *read_gitfile_gently(const char *path, int *return_error_code) > static struct strbuf realpath = STRBUF_INIT; > > if (stat(path, &st)) { > - /* NEEDSWORK: discern between ENOENT vs other errors */ > - error_code = READ_GITFILE_ERR_STAT_FAILED; > + if (errno == ENOENT) > + error_code = READ_GITFILE_ERR_STAT_ENOENT; > + else > + error_code = READ_GITFILE_ERR_STAT_FAILED; > + goto cleanup_return; > + } > + if (S_ISDIR(st.st_mode)) { > + error_code = READ_GITFILE_ERR_IS_A_DIR; > goto cleanup_return; > } This block makes sense. > if (!S_ISREG(st.st_mode)) { > @@ -1578,20 +1588,28 @@ static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir, > if (offset > min_offset) > strbuf_addch(dir, '/'); > strbuf_addstr(dir, DEFAULT_GIT_DIR_ENVIRONMENT); > - gitdirenv = read_gitfile_gently(dir->buf, die_on_error ? > - NULL : &error_code); > + gitdirenv = read_gitfile_gently(dir->buf, &error_code); So we now ask the error code to be provided, even if `die_on_error` is set. I assume, we will manually handle `die_on_error`. > if (!gitdirenv) { > - if (die_on_error || > - error_code == READ_GITFILE_ERR_NOT_A_FILE) { > - /* NEEDSWORK: fail if .git is not file nor dir */ > + switch (error_code) { > + case READ_GITFILE_ERR_STAT_ENOENT: > + /* no .git in this directory, move on */ > + break; > + case READ_GITFILE_ERR_IS_A_DIR: > if (is_git_directory(dir->buf)) { > gitdirenv = DEFAULT_GIT_DIR_ENVIRONMENT; > gitdir_path = xstrdup(dir->buf); > } > - } else if (error_code != READ_GITFILE_ERR_STAT_FAILED) > - return GIT_DIR_INVALID_GITFILE; > - } else > + /* NEEDSWORK: should we catch a directory .git that is not a git directory here? */ Nit: we should probably wrap this. > + break; > + default: > + if (die_on_error || error_code == READ_GITFILE_ERR_NOT_A_FILE) > + read_gitfile_error_die(error_code, dir->buf, NULL); > + else > + return GIT_DIR_INVALID_GITFILE; > + } > + } else { > gitfile = xstrdup(dir->buf); > + } The changes themselves make sense to me. > /* > * Earlier, we tentatively added DEFAULT_GIT_DIR_ENVIRONMENT > * to check that directory for a repository. > diff --git a/setup.h b/setup.h > index 0738dec244..ed4b13f061 100644 > --- a/setup.h > +++ b/setup.h > @@ -36,6 +36,8 @@ int is_nonbare_repository_dir(struct strbuf *path); > #define READ_GITFILE_ERR_NO_PATH 6 > #define READ_GITFILE_ERR_NOT_A_REPO 7 > #define READ_GITFILE_ERR_TOO_LARGE 8 > +#define READ_GITFILE_ERR_STAT_ENOENT 9 > +#define READ_GITFILE_ERR_IS_A_DIR 10 > void read_gitfile_error_die(int error_code, const char *path, const char *dir); > const char *read_gitfile_gently(const char *path, int *return_error_code); > #define read_gitfile(path) read_gitfile_gently((path), NULL) > diff --git a/submodule.c b/submodule.c > index 508938e4da..b179f952fb 100644 > --- a/submodule.c > +++ b/submodule.c > @@ -2559,7 +2559,7 @@ void absorb_git_dir_into_superproject(const char *path, > const struct submodule *sub; > struct strbuf sub_gitdir = STRBUF_INIT; > > - if (err_code == READ_GITFILE_ERR_STAT_FAILED) { > + if (err_code == READ_GITFILE_ERR_STAT_ENOENT) { > /* unpopulated as expected */ > strbuf_release(&gitdir); > return; > diff --git a/t/meson.build b/t/meson.build > index f80e366cff..c4afaacee5 100644 > --- a/t/meson.build > +++ b/t/meson.build > @@ -80,6 +80,7 @@ integration_tests = [ > 't0006-date.sh', > 't0007-git-var.sh', > 't0008-ignores.sh', > + 't0009-git-dir-validation.sh', > 't0010-racy-git.sh', > 't0012-help.sh', > 't0013-sha1dc.sh', > diff --git a/t/t0009-git-dir-validation.sh b/t/t0009-git-dir-validation.sh > new file mode 100755 > index 0000000000..9b3925c85f > --- /dev/null > +++ b/t/t0009-git-dir-validation.sh > @@ -0,0 +1,72 @@ > +#!/bin/sh > + > +test_description='setup: validation of .git file/directory types > + > +Verify that setup_git_directory() correctly handles: > +1. Valid .git directories (including symlinks to them). > +2. Invalid .git files (FIFOs, sockets) by erroring out. > +3. Invalid .git files (garbage) by erroring out. > +' > + > +. ./test-lib.sh > + > +test_expect_success 'setup: create parent git repository' ' > + git init parent && > + test_commit -C parent "root-commit" > +' > + > +test_expect_success SYMLINKS 'setup: .git as a symlink to a directory is valid' ' Nit: should we also cleanup? with a 'test_when_finished "rm -rf parent/link-to-dir"'. Should apply for all the tests. > + mkdir -p parent/link-to-dir && > + ( > + cd parent/link-to-dir && > + git init real-repo && > + ln -s real-repo/.git .git && > + git rev-parse --git-dir >actual && > + echo .git >expect && > + test_cmp expect actual > + ) > +' > + > +test_expect_success PIPE 'setup: .git as a FIFO (named pipe) is rejected' ' > + mkdir -p parent/fifo-trap && > + ( > + cd parent/fifo-trap && > + mkfifo .git && > + test_must_fail git rev-parse --git-dir 2>stderr && > + grep "not a regular file" stderr > + ) > +' > + > +test_expect_success SYMLINKS,PIPE 'setup: .git as a symlink to a FIFO is rejected' ' > + mkdir -p parent/symlink-fifo-trap && > + ( > + cd parent/symlink-fifo-trap && > + mkfifo target-fifo && > + ln -s target-fifo .git && > + test_must_fail git rev-parse --git-dir 2>stderr && > + grep "not a regular file" stderr > + ) > +' > + > +test_expect_success 'setup: .git with garbage content is rejected' ' > + mkdir -p parent/garbage-trap && > + ( > + cd parent/garbage-trap && > + echo "garbage" >.git && > + test_must_fail git rev-parse --git-dir 2>stderr && > + grep "invalid gitfile format" stderr > + ) > +' > + > +test_expect_success 'setup: .git as an empty directory is ignored' ' > + mkdir -p parent/empty-dir && > + ( > + cd parent/empty-dir && > + mkdir .git && > + git rev-parse --git-dir >actual && > + echo "$TRASH_DIRECTORY/parent/.git" >expect && > + test_cmp expect actual > + ) > +' > + > +test_done > diff --git a/worktree.c b/worktree.c > index 9308389cb6..d1165e1d1c 100644 > --- a/worktree.c > +++ b/worktree.c > @@ -653,7 +653,8 @@ static void repair_gitfile(struct worktree *wt, > } > } > > - if (err == READ_GITFILE_ERR_NOT_A_FILE) > + if (err == READ_GITFILE_ERR_NOT_A_FILE || > + err == READ_GITFILE_ERR_IS_A_DIR) > fn(1, wt->path, _(".git is not a file"), cb_data); > else if (err) > repair = _(".git file broken"); > @@ -833,7 +834,8 @@ void repair_worktree_at_path(const char *path, > strbuf_addstr(&backlink, dotgit_contents); > strbuf_realpath_forgiving(&backlink, backlink.buf, 0); > } > - } else if (err == READ_GITFILE_ERR_NOT_A_FILE) { > + } else if (err == READ_GITFILE_ERR_NOT_A_FILE || > + err == READ_GITFILE_ERR_IS_A_DIR) { > fn(1, dotgit.buf, _("unable to locate repository; .git is not a file"), cb_data); > goto done; > } else if (err == READ_GITFILE_ERR_NOT_A_REPO) { > -- > 2.43.0 The rest looks good. Thanks!