Don't treat git repos as non-existent when ignore_git is set

[jyn514]

The new submodule handling depends on is_git() to be accurate to
decide whether it should handle submodules at all or not. Unfortunately,
is_git() treated "this directory does not have a git repository" and
"this repository should not be used for SHA/version/commit date info"
the same. This changes it to distinguish the two.

To clarify: ignore_get is set by default whenever channel == "dev", which it is by default whenever you're compiling locally. So basically everyone would hit this, not just people who had explicitly configured ignore_git.

Here's an example of an error this fixes:

$ x build
Updating only changed submodules
Submodules updated in 0.01 seconds
Finished dev [unoptimized + debuginfo] target(s) in 0.17s
warning: x.py has made several changes recently you may want to look at
help: consider looking at the changes in `src/bootstrap/CHANGELOG.md`
note: to silence this warning, add `changelog-seen = 2` at the top of `config.toml`
Building stage0 std artifacts (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
Finished release [optimized] target(s) in 0.16s
Copying stage0 std from stage0 (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu / x86_64-unknown-linux-gnu)
Building LLVM for x86_64-unknown-linux-gnu
detected home dir change, cleaning out entire build directory
running: "cmake" "/home/joshua/rustc3/src/llvm-project/llvm" "-G" "Ninja" "-DLLVM_ENABLE_ASSERTIONS=OFF" "-DLLVM_TARGETS_TO_BUILD=AArch64;ARM;BPF;Hexagon;MSP430;Mips;NVPTX;PowerPC;RISCV;Sparc;SystemZ;WebAssembly;X86" "-DLLVM_EXPERIMENTAL_TARGETS_TO_BUILD=AVR" "-DLLVM_INCLUDE_EXAMPLES=OFF" "-DLLVM_INCLUDE_DOCS=OFF" "-DLLVM_INCLUDE_BENCHMARKS=OFF" "-DLLVM_ENABLE_TERMINFO=OFF" "-DLLVM_ENABLE_LIBEDIT=OFF" "-DLLVM_ENABLE_BINDINGS=OFF" "-DLLVM_ENABLE_Z3_SOLVER=OFF" "-DLLVM_PARALLEL_COMPILE_JOBS=48" "-DLLVM_TARGET_ARCH=x86_64" "-DLLVM_DEFAULT_TARGET_TRIPLE=x86_64-unknown-linux-gnu" "-DLLVM_ENABLE_ZLIB=ON" "-DLLVM_ENABLE_LIBXML2=OFF" "-DLLVM_VERSION_SUFFIX=-rust-dev" "-DCMAKE_INSTALL_MESSAGE=LAZY" "-DCMAKE_C_COMPILER=gcc" "-DCMAKE_CXX_COMPILER=g++" "-DCMAKE_ASM_COMPILER=gcc" "-DCMAKE_C_FLAGS=-ffunction-sections -fdata-sections -fPIC -m64" "-DCMAKE_CXX_FLAGS=-ffunction-sections -fdata-sections -fPIC -m64" "-DCMAKE_INSTALL_PREFIX=/home/joshua/rustc3/build/x86_64-unknown-linux-gnu/llvm" "-DCMAKE_ASM_FLAGS= -ffunction-sections -fdata-sections -fPIC -m64" "-DCMAKE_BUILD_TYPE=Release"
CMake Error: The source directory "/home/joshua/rustc3/src/llvm-project/llvm" does not exist.
Specify --help for usage, or press the help button on the CMake GUI.
thread 'main' panicked at '
command did not execute successfully, got: exit status: 1
build script failed, must exit now', /home/joshua/.local/lib/cargo/registry/src/github.com-1ecc6299db9ec823/cmake-0.1.44/src/lib.rs:885:5
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
finished in 0.783 seconds
Build completed unsuccessfully in 0:00:01

I believe this regression was only introduced in #87380, not #82653. @petrochenkov can you check that this fixes the issue you encountered in #82653 (comment) ?

r? @Mark-Simulacrum

[Mark-Simulacrum]

Can we add comments to these variants explaining their meaning? (In particular, Ignore not indicating no information, but rather positive presence, is not obvious).

Alternatively restructuring to Missing and a Present(Option<Info>) variant might be better; it would reduce the explaining needed I think.

[jyn514]

Can we add comments to these variants explaining their meaning? (In particular, Ignore not indicating no information, but rather positive presence, is not obvious).

Sure, done.

Alternatively restructuring to Missing and a Present(Option) variant might be better; it would reduce the explaining needed I think.

Hmm, this doesn't seem any more clear to me - you'd still need a comment explaining what Present(None) means.

[Mark-Simulacrum]

Ignore doesn't make sense in the context of "what is the git info" - it's an outside parameter. Present(None) does need a comment, I agree, but is still imo clearer in terms of present (but no info). Mainly because if I see a match in (future) code for Present, I won't likely think to check for Absent, but this way the check will be more obviously necessary.

[jyn514]

👍 Makes sense, I changed it.

[jyn514]

@bors r+ - it seemed you were ok with the general idea, and I'd like to get this fix in since the behavior is pretty bad without. Happy to make improvements in follow-ups.

[bors]

📌 Commit 7cab739 has been approved by jyn514

[petrochenkov]

@jyn514
Looks like it did fix the issue I've been complaining about.