Re: [PATCH v11] 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_MISSING'(which groups the "file missing" scenarios
> together) and 'READ_GITFILE_ERR_IS_A_DIR'.
>
> To preserve the original intent of the setup process:
> 1. Update 'read_gitfile_error_die()' to treat both 'IS_A_DIR' and
>    'MISSING' as no-ops, while continuing to call 'die()' on true
>    'NOT_A_FILE' errors to prevent security hazards (like FIFOs).
> 2. Unconditionally pass '&error_code' to 'read_gitfile_gently()'.
> 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                       | 45 ++++++++++++++------
>  setup.h                       |  2 +
>  submodule.c                   |  2 +-
>  t/meson.build                 |  1 +
>  t/t0009-git-dir-validation.sh | 77 +++++++++++++++++++++++++++++++++++
>  worktree.c                    |  6 ++-
>  6 files changed, 118 insertions(+), 15 deletions(-)
>  create mode 100755 t/t0009-git-dir-validation.sh

Unfortunately this seems to break almost all the tests, not just the
test the patch adds, on Windows (which I almost know nothing about,
but I can observe that CI jobs die).

https://github.com/git/git/actions/runs/22464017037 is a CI run that
merged this patch on top of the commit that corresponds to the tip
of 'next' as of today.  We can see "win test (N)" jobs dying all
over.  I cancelled the workflow before seeing everything die,
though.

https://github.com/git/git/actions/runs/22464479533 is a CI run that
tests this patch applied directly on v2.53.0 in isolation.

As I said, I do not know Windows well, so this may be a red-herring,
but in this CI run, we see "GIT_DIR=/dev/null git diff --no-index ..."
results in "fatal: error reading 'nul'":

  https://github.com/git/git/actions/runs/22464479533/job/65067515458#step:5:95419

which is an expected thing to happen, but we probably used to ignore
it as a non-error?

For now, I'll kick this topic out of my tree to give other topics a
bit more test exposure so that we can notice new bugs in them (not
in this topic) that causes the tests fail.  With this topic in 'seen',
such bugs in other topics are all masked.

Can folks equipped with knowledge and environment to debug Windows
only breakage lend a hand to figure out what this patch gets wrong
to help it move forward?


Thanks.