git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Karthik Nayak <karthik.188@gmail.com>
Date
Feb 22, 2026, 16:53 UTC
Message-ID
<CAOLa=ZTePRR05M5VBxxk0OA=_RyNd0pLe=Bq6xwnE3MyZBjBAw@mail.gmail.com>
In-Reply-To
<20260222102928.377519-1-a3205153416@gmail.com>
Tian Yuchen <a3205153416@gmail.com> writes:
> 'read_gitfile_gently()' treats any non-regular file as
> 'READ_GITFILE_ERR_NOT_A_FILE' and fails to discern between 'ENOENT'
> and other stat failures. This flawed error reporting is noted by two
> 'NEEDSWORK' comments.
Okay.
> Address these comments by introducing two new error codes:
> 'READ_GITFILE_ERR_STAT_ENOENT' and 'READ_GITFILE_ERR_IS_A_DIR'.
>
Nit: This is much better, we seem to talk about the issues and the new
errors introduced. I wonder if we can tie the errors to the issues.
Perhaps
    The 'read_gitfile_gently()' is used to obtain the location of a git
    directory by parsing a '.git' file.
    When parsing the file with 'stat(2)', it fails to differentiate
    between a 'ENOENT' and other errors. Introduce
    'READ_GITFILE_ERR_STAT_ENOENT' to make this differentiation.
    The function also marks directories as
    'READ_GITFILE_ERR_NOT_A_FILE', introduce 'READ_GITFILE_ERR_IS_A_DIR'
    to specifically make this distinction.
> To preserve the original intent of the setup process:
> 1. Update 'read_gitfile_error_die()' to treat 'IS_A_DIR' as a no-op
>    (like 'ENOENT'), while still calling 'die()' on true 'NOT_A_FILE'
>    errors.

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

> 2. Unconditionally pass '&error_code' to 'read_gitfile_gently()'. This
>    eliminates an uninitialized variable hazard that occurred when
>    'die_on_error' was true and 'NULL' was passed.
Where is the 'uninitialized variable hazard'? The function says:
  If return_error_code is NULL the function will die instead
> 3. Only invoke 'is_git_directory()' when we explicitly receive
>    'READ_GITFILE_ERR_IS_A_DIR', avoiding redundant filesystem checks.
Nice.
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;
>  	}
This block makes sense.
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.0
The rest looks good. Thanks!
Previous: Tian YuchenNext: Tian Yuchen
Message 16 of 45 in “setup: allow cwd/.git to be a symlink to a directory”
  1. 0/2 setup: allow cwd/.git to be a symlink to a directoryTian Yuchen, Feb 18, 2026
  2. 1/2 setup: distinguish ENOENT from other stat errorsTian Yuchen, Feb 18, 2026
  3. 2/2 setup: allow cwd/.git to be a symlink to a directoryTian Yuchen, Feb 18, 2026
  4. setup: allow cwd/.git to be a symlink to a directoryTian Yuchen, Feb 19, 2026
  5. Junio C HamanoFeb 20, 2026
  6. Tian YuchenFeb 20, 2026
  7. setup: allow cwd/.git to be a symlink to a directoryTian Yuchen, Feb 20, 2026
  8. Junio C HamanoFeb 20, 2026
  9. Tian YuchenFeb 21, 2026
  10. Junio C HamanoFeb 21, 2026
  11. Tian YuchenFeb 22, 2026
  12. setup: improve error diagnosis for invalid .git filesTian Yuchen, Feb 21, 2026
  13. Junio C HamanoFeb 22, 2026
  14. Tian YuchenFeb 22, 2026
  15. setup: improve error diagnosis for invalid .git filesTian Yuchen, Feb 22, 2026
  16. Karthik NayakFeb 22, 2026
  17. Tian YuchenFeb 23, 2026
  18. Junio C HamanoFeb 22, 2026
  19. Junio C HamanoFeb 23, 2026
  20. Tian YuchenFeb 23, 2026
  21. Junio C HamanoFeb 23, 2026
  22. Junio C HamanoFeb 23, 2026
  23. Tian YuchenFeb 23, 2026
  24. Junio C HamanoFeb 23, 2026
  25. Tian YuchenFeb 24, 2026
  26. Tian YuchenFeb 24, 2026
  27. Junio C HamanoFeb 25, 2026
  28. Tian YuchenFeb 25, 2026
  29. setup: improve error diagnosis for invalid .git filesTian Yuchen, Feb 23, 2026
  30. Junio C HamanoFeb 26, 2026
  31. Tian YuchenFeb 27, 2026
  32. Junio C HamanoFeb 27, 2026
  33. Tian YuchenFeb 28, 2026
  34. Junio C HamanoMar 2, 2026
  35. Phillip WoodMar 3, 2026
  36. Junio C HamanoMar 4, 2026
  37. Tian YuchenMar 4, 2026
  38. Junio C HamanoMar 4, 2026
  39. Tian YuchenMar 4, 2026
  40. Junio C HamanoMar 4, 2026
  41. Tian YuchenMar 4, 2026
  42. Junio C HamanoMar 4, 2026
  43. Tian YuchenMar 5, 2026
  44. Junio C HamanoMar 9, 2026
  45. setup: improve error diagnosis for invalid .git filesTian Yuchen, Mar 4, 2026

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.