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

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

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

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