Fix and expand bash.exe xfail marks on hook tests

881456b (gitpython-developers#1679) expanded the use of shutil.which in test_index.py
to attempt to mark test_commit_msg_hook_success xfail when bash.exe
is a WSL bash wrapper in System32 (because that test currently
is not passing when the hook is run via bash in a WSL system, which
the WSL bash.exe wraps). But this was not reliable, due to
significant differences between shell and non-shell search behavior
for executable commands on Windows. As the new docstring notes,
lookup due to Popen generally checks System32 before going through
directories in PATH, as this is how CreateProcessW behaves.

- https://learn.microsoft.com/en-us/windows/win32/api/processthreadsapi/nf-processthreadsapi-createprocessw
- python/cpython#91558 (comment)

The technique I had used in 881456b also had the shortcoming of
assuming that a bash.exe in System32 was the WSL wrapper. But such
a file may exist on some systems without WSL, and be a bash
interpreter unrelated to WSL that may be able to run hooks.

In addition, one common situation, which was the case on CI before
a step to install a WSL distribution was added, is that WSL is
present but no WSL distributions are installed. In that situation
bash.exe is found in System32, but it can't be used to run any
hooks, because there's no actual system with a bash in it to wrap.
This was not covered before. Unlike some conditions that prevent
a WSL system from being used, such as resource exhaustion blocking
it from being started, the absence of a WSL system should probably
not fail the pytest run, for the same reason as we are trying not
to have the complete *absence* of bash.exe fail the pytest run.
Both conditions should be xfail. Fortunately, the error message
when no distribution exists is distinctive and can be checked for.

There is probably no correct and reasonable way to check LBYL-style
which executable file bash.exe resolves to by using shutil.which,
due to shutil.which and subprocess.Popen's differing seach orders
and other subtleties. So this adds code to do it EAFP-style using
subprocess.run (which itself uses Popen, so giving the same
CreateProcessW behavior). It tries to run a command with bash.exe
whose output pretty reliably shows if the system is WSL or not.

We may fail to get to the point of running that command at all, if
bash.exe is not usable, in which case the failure's details tell us
if bash.exe is absent (xfail), present as the WSL wrapper with no WSL
systems (xfail), or has some other error (not xfail, so the user can
become aware of and proably fix the problem). If we do get to that
point, then a file that is almost always present on both WSL 1 and
WSL 2 systems and almost always absent on any other system is
checked for, to distinguish whether the working bash shell operates
in a WSL system, or outside any such system as e.g. Git Bash does.

See https://superuser.com/a/1749811 on various techniques for
checking for WSL, including the /proc/sys/fs/binfmt_misc/WSLInterop
technique used here (which seems overall may be the most reliable).

Although the Windows CI runners have Git Bash, and this is even the
bash.exe that appears first in PATH (giving rise to the problem
with shutil.which detailed above), it would be a bit awkward to
test the behavior when Git Bash is actually used to run hooks on
CI, because of how Popen selects the System32 bash.exe first,
whether or not any WSL distribution is present. However, local
testing shows that when Git Bash's bash.exe is selected, all four
hook tests in the module pass, both before and after this change,
and furthermore that bash.exe is correctly detected as "native",
continuing to avoid an erroneous xfail mark in that case.

Author: EliahKagan

Diff for: test/test_index.py

@@ -3,12 +3,14 @@
 # This module is part of GitPython and is released under the
 # 3-Clause BSD License: https://opensource.org/license/bsd-3-clause/
 
+import enum
 from io import BytesIO
+import logging
 import os
 import os.path as osp
 from pathlib import Path
 from stat import S_ISLNK, ST_MODE
-import shutil
+import subprocess
 import tempfile
 
 import pytest
@@ -34,19 +36,97 @@
 
 HOOKS_SHEBANG = "#!/usr/bin/env sh\n"
 
+log = logging.getLogger(__name__)
 
-def _found_in(cmd, directory):
-    """Check if a command is resolved in a directory (without following symlinks)."""
-    path = shutil.which(cmd)
-    return path and Path(path).parent == Path(directory)
 
+class _WinBashMeta(enum.EnumMeta):
+    """Metaclass allowing :class:`_WinBash` custom behavior when called."""
 
-is_win_without_bash = os.name == "nt" and not shutil.which("bash.exe")
+    def __call__(cls):
+        return cls._check()
 
-is_win_with_wsl_bash = os.name == "nt" and _found_in(
-    cmd="bash.exe",
-    directory=Path(os.getenv("WINDIR")) / "System32",
-)
+
+@enum.unique
+class _WinBash(enum.Enum, metaclass=_WinBashMeta):
+    """Status of bash.exe for native Windows. Affects which commit hook tests can pass.
+
+    Call ``_WinBash()`` to check the status.
+
+    This can't be reliably discovered using :func:`shutil.which`, as that approximates
+    how a shell is expected to search for an executable. On Windows, there are major
+    differences between how executables are found by a shell and otherwise. (This is the
+    cmd.exe Windows shell and should not be confused with bash.exe.) Our run_commit_hook
+    function in GitPython uses subprocess.Popen, including to run bash.exe on Windows.
+    It does not pass shell=True (and should not). Popen calls CreateProcessW, which
+    searches several locations prior to using the PATH environment variable. It is
+    expected to search the System32 directory, even if another directory containing the
+    executable precedes it in PATH. (There are other differences, less relevant here.)
+    When WSL is installed, even if no WSL *systems* are installed, bash.exe exists in
+    System32, and Popen finds it even if another bash.exe precedes it in PATH, as
+    happens on CI. If WSL is absent, System32 may still have bash.exe, as Windows users
+    and administrators occasionally copy executables there in lieu of extending PATH.
+    """
+
+    INAPPLICABLE = enum.auto()
+    """This system is not native Windows: either not Windows at all, or Cygwin."""
+
+    ABSENT = enum.auto()
+    """No command for ``bash.exe`` is found on the system."""
+
+    NATIVE = enum.auto()
+    """Running ``bash.exe`` operates outside any WSL environment (as with Git Bash)."""
+
+    WSL = enum.auto()
+    """Running ``bash.exe`` runs bash on a WSL system."""
+
+    WSL_NO_DISTRO = enum.auto()
+    """Running ``bash.exe` tries to run bash on a WSL system, but none exists."""
+
+    ERROR_WHILE_CHECKING = enum.auto()
+    """Could not determine the status.
+
+    This should not trigger a skip or xfail, as it typically indicates either a fixable
+    problem on the test machine, such as an "Insufficient system resources exist to
+    complete the requested service" error starting WSL, or a bug in this detection code.
+    ``ERROR_WHILE_CHECKING.error_or_process`` has details about the most recent failure.
+    """
+
+    @classmethod
+    def _check(cls):
+        if os.name != "nt":
+            return cls.INAPPLICABLE
+
+        try:
+            # Print rather than returning the test command's exit status so that if a
+            # failure occurs before we even get to this point, we will detect it.
+            script = 'test -e /proc/sys/fs/binfmt_misc/WSLInterop; echo "$?"'
+            command = ["bash.exe", "-c", script]
+            proc = subprocess.run(command, capture_output=True, check=True, text=True)
+        except FileNotFoundError:
+            return cls.ABSENT
+        except OSError as error:
+            return cls._error(error)
+        except subprocess.CalledProcessError as error:
+            no_distro_message = "Windows Subsystem for Linux has no installed distributions."
+            if error.returncode == 1 and error.stdout.startswith(no_distro_message):
+                return cls.WSL_NO_DISTRO
+            return cls._error(error)
+
+        status = proc.stdout.rstrip()
+        if status == "0":
+            return cls.WSL
+        if status == "1":
+            return cls.NATIVE
+        return cls._error(proc)
+
+    @classmethod
+    def _error(cls, error_or_process):
+        if isinstance(error_or_process, subprocess.CompletedProcess):
+            log.error("Strange output checking WSL status: %s", error_or_process.stdout)
+        else:
+            log.error("Error running bash.exe to check WSL status: %s", error_or_process)
+        cls.ERROR_WHILE_CHECKING.error_or_process = error_or_process
+        return cls.ERROR_WHILE_CHECKING
 
 
 def _make_hook(git_dir, name, content, make_exec=True):
@@ -917,20 +997,30 @@ class Mocked:
         rel = index._to_relative_path(path)
         self.assertEqual(rel, os.path.relpath(path, root))
 
+    @pytest.mark.xfail(
+        _WinBash() is _WinBash.WSL_NO_DISTRO,
+        reason="Currently uses the bash.exe for WSL even with no WSL distro installed",
+        raises=HookExecutionError,
+    )
     @with_rw_repo("HEAD", bare=True)
     def test_pre_commit_hook_success(self, rw_repo):
         index = rw_repo.index
         _make_hook(index.repo.git_dir, "pre-commit", "exit 0")
         index.commit("This should not fail")
 
+    @pytest.mark.xfail(
+        _WinBash() is _WinBash.WSL_NO_DISTRO,
+        reason="Currently uses the bash.exe for WSL even with no WSL distro installed",
+        raises=AssertionError,
+    )
     @with_rw_repo("HEAD", bare=True)
     def test_pre_commit_hook_fail(self, rw_repo):
         index = rw_repo.index
         hp = _make_hook(index.repo.git_dir, "pre-commit", "echo stdout; echo stderr 1>&2; exit 1")
         try:
             index.commit("This should fail")
         except HookExecutionError as err:
-            if is_win_without_bash:
+            if _WinBash() is _WinBash.ABSENT:
                 self.assertIsInstance(err.status, OSError)
                 self.assertEqual(err.command, [hp])
                 self.assertEqual(err.stdout, "")
@@ -946,10 +1036,15 @@ def test_pre_commit_hook_fail(self, rw_repo):
             raise AssertionError("Should have caught a HookExecutionError")
 
     @pytest.mark.xfail(
-        is_win_without_bash or is_win_with_wsl_bash,
+        _WinBash() in {_WinBash.ABSENT, _WinBash.WSL},
         reason="Specifically seems to fail on WSL bash (in spite of #1399)",
         raises=AssertionError,
     )
+    @pytest.mark.xfail(
+        _WinBash() is _WinBash.WSL_NO_DISTRO,
+        reason="Currently uses the bash.exe for WSL even with no WSL distro installed",
+        raises=HookExecutionError,
+    )
     @with_rw_repo("HEAD", bare=True)
     def test_commit_msg_hook_success(self, rw_repo):
         commit_message = "commit default head by Frèderic Çaufl€"
@@ -963,14 +1058,19 @@ def test_commit_msg_hook_success(self, rw_repo):
         new_commit = index.commit(commit_message)
         self.assertEqual(new_commit.message, "{} {}".format(commit_message, from_hook_message))
 
+    @pytest.mark.xfail(
+        _WinBash() is _WinBash.WSL_NO_DISTRO,
+        reason="Currently uses the bash.exe for WSL even with no WSL distro installed",
+        raises=AssertionError,
+    )
     @with_rw_repo("HEAD", bare=True)
     def test_commit_msg_hook_fail(self, rw_repo):
         index = rw_repo.index
         hp = _make_hook(index.repo.git_dir, "commit-msg", "echo stdout; echo stderr 1>&2; exit 1")
         try:
             index.commit("This should fail")
         except HookExecutionError as err:
-            if is_win_without_bash:
+            if _WinBash() is _WinBash.ABSENT:
                 self.assertIsInstance(err.status, OSError)
                 self.assertEqual(err.command, [hp])
                 self.assertEqual(err.stdout, "")