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

[Junio C Hamano <gitster@pobox.com>]

Let's try again.

Can folks equipped with knowledge and environment to debug breakages
that hapepns only on Windows lend a hand to figure out what this
patch gets wrong to help the topic move forward?

Thanks.

Junio C Hamano <gitster@pobox.com> writes:

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