Re: [PATCH v5 1/2] setup: distingush ENOENT from other stat errors
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).
> 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.
Show 5 quoted lines
> 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.
Show 19 quoted lines
> 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.
Show 6 quoted lines
> 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'?
Show 35 quoted lines
> 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.0So 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.
Karthik Nayak <karthik.188@gmail.com>