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

Tian Yuchen [off-list ref] 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 <redacted>
---
 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.