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

[Junio C Hamano <gitster@pobox.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.
>
> 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.