From: Karthik Nayak <karthik.188@gmail.com>
Date: Wed, 18 Feb 2026 10:12:16 GMT
Subject: Re: [PATCH v5 1/2] setup: distingush ENOENT from other stat errors
Message-ID: <CAOLa=ZR_tH6A6JEj7NwziwYaVtezkHMez_cZNYyU1TQi5D8=XQ@mail.gmail.com>
In-Reply-To: <20260218051850.164972-2-a3205153416@gmail.com>

Tian Yuchen <a3205153416@gmail.com> writes:

> 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).

Correct.

> Introduce 'READ_GITFILE_ERR_STAT_ENOENT' and 'READ_GITFILE_ERR_IS_A_DIR'.

Okay, to clarify we're making two distinct changes:
1. Split the 'stat()' error ERR_STAT_FAILED into ERR_STAT_FAILED and
   ERR_STAT_ENOENT. This is what your previous paragraph described. It
   would be nice if you could also explain this.
2. We introduce ERR_IS_A_DIR for when the .git path is a directory. We
   don't talk about this at all.

> Updata 'read_gitfile_error_die()' to handle these cases:
>
> 1. Return for happy cases ('ENOENT', 'IS_A_DIR');
> 2. Die for fatal cases ('STAT_FAILED', 'NOT_A_FILE');
>

Nit: It would be nice to explain on the decision behind picking errors
for the happy vs fatal path.

> This prepares for error handling in the next commit.
>
> Signed-off-by: Tian Yuchen <a3205153416@gmail.com>
> ---
>  setup.c | 17 +++++++++++++----
>  setup.h |  2 ++
>  2 files changed, 15 insertions(+), 4 deletions(-)
>
> diff --git a/setup.c b/setup.c
> index c8336eb20e..0ca129623e 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:
> +		return;

Nit: For this function it should be okay to return early. But I was
expecting a break here, since it was using 'break' before, ideally we
shouldn't change it unless there is a reason to.

>  	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(_("invalid %s: not a regular file"), path);

Would it make more sense to do 'not a regular file: %s'?

>  	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

So why didn't we add the tests here for the changes made?

Nit: I would even go further to even separate this into two commits:
1. Split 'stat()' error into  ERR_STAT_FAILED and ERR_STAT_ENOENT.
2. Introduce 'READ_GITFILE_ERR_IS_A_DIR'.

But I'll leave that to you.