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

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 4, 2026, 05:39 UTC
Message-ID: <xmqqo6l49mrt.fsf@gitster.g>
In-Reply-To: <460f00d5-97b4-4a6c-be45-6f60a17cd33e@gmail.com>

Phillip Wood <phillip.wood123@gmail.com> writes:

Show 9 quoted lines

> Looking at the test failures the tests are failing because
>
>      GIT_DIR=/dev/null git diff --no-index ...
>
> which is used by test_cmp() on Windows is dying. That happens because 
> stat("nul", &st) fails and this series makes that an error (somewhere 
> along the line "/dev/null" is rewritten to "nul" on Windows). I'm afraid 
> I don't know enough about Windows to be sure how to fix it but maybe we 
> should special case "nul" in the setup code or mingw_stat().

While I do not think we want to special case "nul", I think we need to make the tightening of error checking conditional to who is asking to know. The _intent_ behind the NEEDSWORK comment was to allow us to be more strict when we see a fishy ".git" filesystem entity when we are in a directory /a/b/c/d and are trying to find out if we are in a Git working tree and where our .git directory is. We may not see /a/b/c/d/.git, go up one level and find /a/b/c/.git and stat(2) it. In the current code, we take any and all stat(2) failures as if it failed because ENOENT i.e., as if /a/b/c/.git did not exist, and we go upwards. The NEEDSWORK comment wonders if we want to be noticing that /a/b/c/.git did exist but we failed to stat(2) for some other reason, and if we would want to let the user know.

But the "GIT_DIR=/dev/null git ..." use case is vastly different. We are not doing a discovery and we are not interested in going upwards when the thing we check for "git-dir-ness" fails to be a git-dir. It may have worked around the "nul cannot be stat'ed" limitation if we used "GIT_DIR=no-such-directory git ...", but I think anything that we positively know is not a .git directory or a "gitdir: over-there" file is given as GIT_DIR, we would want to say "nope, we do not have .git dir and have to work outside any repository", instead of dying with "whoa, what is that garbage you are giving me as GIT_DIR???".

WIth an explicitly given GIT_DIR, setup_explicit_git_dir() is called and the function calls read_gitfile_gently(path, NULL), that lets it die when the thing turns out not to be a proper ".git", except when stat(2) failed (for any reason) or stat(2) tells that the thing is not a file. If we use GIT_DIR=/dev/null on POSIX systems, this "not-a-file is fine" leniency allows us to proceed without dying, saying "We were given an invalid GIT_DIR, we are not doing discovery, hence we are operating without a repository". On systems where stat(2) fails for "/dev/null" or its equivalent "NUL:", the same "stat failed for any reason" leniency allows us to do the same.

But with the patches we have been looking at, that leniency is tightened. I think it is a good thing to do during the repository discovery. But it is not if we are checking the explicitly given GIT_DIR. All calls to read_gitfile_gently(path, NULL) need to be audited and then we need to decide which ones to leave lenient, and which ones are OK to tighten together with the call used during the repository discovery.

Thanks.