From b5ae6d8ee6efee580aa21680786775977131bfde Mon Sep 17 00:00:00 2001 From: Codex GPT-5 Date: Thu, 30 Jul 2026 19:55:01 +0000 Subject: [PATCH] Address review comments about Windows Bash lookup The resolver merged in #2199 only worked when Git for Windows Bash itself preceded System32 on PATH. That is typical in Git Bash and CI but not in a normal system-wide installation, where PATH commonly contains System32 followed by Git\cmd. It also skipped an explicitly listed current directory even though explicit PATH entries are trusted configuration, and its test mocked the resolver rather than exercising its precedence. This machine provides Ubuntu under WSL 2, C:\Windows\System32\bash.exe, and Git for Windows. Direct CreateProcess-style invocation of bare bash.exe reported Linux 6.18.33.2-microsoft-standard-WSL2, while `git var GIT_SHELL_PATH` reported C:/Program Files/Git/usr/bin/sh.exe and that shell reported MINGW64. With PATH reordered to System32 followed by Git\usr\bin, the merged resolver selected the System32 WSL launcher and an actual GitPython hook wrote a Linux marker. With the more typical System32 followed by Git\cmd PATH, Bash was not present on PATH at all. Locate the Bash associated with GitPython's selected Git executable before general PATH lookup. Recognize the standard Git for Windows layouts Git\cmd\git.exe, Git\bin\git.exe, and Git\\bin\git.exe, with the platform names used by MSYS2. Configured relative Git executable paths are resolved against the parent process working directory, matching measured CreateProcess behavior even when Popen supplies a different hook working directory. Root-level bin is accepted for a selected Git executable, while usr\bin is deliberately not used to infer an unbounded parent layout. From the trusted Git root, follow gix-path's precedence of bin/bash.exe before usr/bin/bash.exe. If the Git layout is unrecognized, search explicit PATH entries while excluding candidates below SystemRoot so the WSL launcher cannot win. Empty PATH entries are ignored according to Windows semantics, but an explicitly named directory remains eligible even when it is the current directory. Finally, retain the prior bare fallback for nonstandard installations; safer_popen sets NoDefaultCurrentDirectoryInExePath, so that fallback does not reintroduce current-directory lookup. The main regression models System32 before Git\cmd with Bash absent from PATH and checks the real resolver selects the associated Git\bin\bash.exe. Additional tests distinguish an explicit current-directory entry from an empty entry and cover an explicitly configured Git\bin\git.exe. On this machine, an end-to-end hook run under exactly System32;Git\cmd selected C:\Program Files\Git\bin\bash.exe and wrote a MINGW64 marker instead of the earlier Linux/WSL marker. Validated with the focused hook suite (6 passed), the complete test_index.py module (32 passed, one expected xfail, one existing xpass), repository-wide Ruff lint and format checks targeting Python 3.7, and git diff --check. A standalone Python 3.7 interpreter was not available for an additional py_compile run. --- git/index/fun.py | 100 ++++++++++++++++++++++++++++++++++++++++++++- test/test_index.py | 55 +++++++++++++++++++++++-- 2 files changed, 150 insertions(+), 5 deletions(-) diff --git a/git/index/fun.py b/git/index/fun.py index 5d52486f9..e7271dfef 100644 --- a/git/index/fun.py +++ b/git/index/fun.py @@ -26,7 +26,7 @@ from gitdb.base import IStream from gitdb.typ import str_tree_type -from git.cmd import handle_process_output, safer_popen +from git.cmd import Git, handle_process_output, safer_popen from git.compat import defenc, force_bytes, force_text, safe_decode from git.exc import HookExecutionError, UnmergedEntriesError from git.objects.fun import ( @@ -79,6 +79,97 @@ def _has_file_extension(path: str) -> str: return osp.splitext(path)[1] +def _is_in_windows_system_root(path: str) -> bool: + """Return whether ``path`` is inside the Windows installation directory.""" + system_root = os.environ.get("SystemRoot") + if not system_root: + return False + + system_root = osp.normcase(osp.realpath(system_root)) + path = osp.normcase(osp.realpath(path)) + try: + return osp.commonpath((system_root, path)) == system_root + except ValueError: + # Paths on different drives have no common path on Windows. + return False + + +def _which_from_path(command: str) -> Union[str, None]: + """Resolve ``command`` from PATH, excluding the Windows installation.""" + for directory in os.get_exec_path(): + # Unlike POSIX, Windows does not define an empty PATH entry as the current + # directory. Skip it rather than letting abspath() turn it into one. + if not directory: + continue + directory = osp.abspath(directory) + candidate = osp.join(directory, command) + # SystemRoot contains the WSL launcher stubs. They are valid executables but + # not suitable for running a Windows Git hook: the hook path and environment + # were prepared for Git for Windows, and WSL may have no distribution at all. + if _is_in_windows_system_root(candidate): + continue + if osp.isfile(candidate) and os.access(candidate, os.X_OK): + return candidate + return None + + +_GIT_FOR_WINDOWS_PREFIXES = ("mingw64", "mingw32", "clangarm64", "clang64", "clang32", "ucrt64") + + +def _git_for_windows_root() -> Union[str, None]: + """Infer a standard Git for Windows root from GitPython's selected executable.""" + git_executable = os.fspath(Git.GIT_PYTHON_GIT_EXECUTABLE or Git.git_exec_name) + if osp.dirname(git_executable): + # CreateProcess resolves a relative executable path containing a directory + # from the parent process cwd, even when Popen supplies a different child cwd. + git_executable = osp.abspath(git_executable) + else: + # GitPython deliberately retains a bare executable name so later PATH changes + # affect Git commands. Resolve it with the same PATH snapshot used for Bash. + names = (git_executable,) if _has_file_extension(git_executable) else (git_executable, f"{git_executable}.exe") + for name in names: + resolved = _which_from_path(name) + if resolved is not None: + git_executable = resolved + break + else: + git_executable = "" + if not git_executable: + return None + + executable_dir = osp.dirname(git_executable) + directory_name = osp.basename(executable_dir).lower() + if directory_name == "cmd": + # The normal system-wide PATH entry is /cmd. + return osp.dirname(executable_dir) + if directory_name == "bin": + prefix = osp.dirname(executable_dir) + if osp.basename(prefix).lower() in _GIT_FOR_WINDOWS_PREFIXES: + # Git Bash commonly exposes //bin/git.exe. + return osp.dirname(prefix) + if osp.basename(prefix).lower() != "usr": + # An explicitly configured Git may be the root-level bin/git.exe. Do + # not make the same inference from usr/bin: unlike the recognized + # platform prefixes, "usr" has no reliably bounded parent layout. + return prefix + return None + + +def _git_for_windows_bash() -> Union[str, None]: + """Return Bash from the Git for Windows installation selected by GitPython.""" + git_root = _git_for_windows_root() + if git_root is None: + return None + + # Match gix-path's precedence: prefer the lightweight bin shim, then the + # underlying usr/bin executable. Both belong to the same installation as Git. + for relative_path in ("bin/bash.exe", "usr/bin/bash.exe"): + candidate = osp.join(git_root, *relative_path.split("/")) + if osp.isfile(candidate) and os.access(candidate, os.X_OK): + return candidate + return None + + def run_commit_hook(name: str, index: "IndexFile", *args: str) -> None: """Run the commit hook of the given name. Silently ignore hooks that do not exist. @@ -112,7 +203,12 @@ def run_commit_hook(name: str, index: "IndexFile", *args: str) -> None: # an absolute path in this form, although a relative path is preferable # because it also works with the Windows Subsystem for Linux wrapper. bash_hp = hp - cmd = ["bash.exe", Path(bash_hp).as_posix()] + # Prefer Bash associated with GitPython's selected Git installation. If + # that layout is not recognized, use an explicitly configured non-system + # PATH entry. Preserve the bare fallback for installations that previously + # relied on WSL or another CreateProcess-resolved Bash. + bash_executable = _git_for_windows_bash() or _which_from_path("bash.exe") or "bash.exe" + cmd = [bash_executable, Path(bash_hp).as_posix()] process = safer_popen( cmd + list(args), diff --git a/test/test_index.py b/test/test_index.py index 3ad5a457f..ae65763dc 100644 --- a/test/test_index.py +++ b/test/test_index.py @@ -33,7 +33,7 @@ UnmergedEntriesError, UnsafeOptionError, ) -from git.index.fun import hook_path, run_commit_hook +from git.index.fun import _git_for_windows_bash, _which_from_path, hook_path, run_commit_hook from git.index.typ import BaseIndexEntry, IndexEntry from git.index.util import TemporaryFileSwap from git.objects import Blob @@ -1128,16 +1128,65 @@ def test_run_commit_hook_outside_worktree_on_windows(self, rw_dir): repo = Repo.init(root / "repo") hooks_dir = root / "hooks" _make_hook(root, "fake-hook", "exit 0") + system_root = root / "Windows" + system_bash = system_root / "System32" / "bash.exe" + git_executable = root / "Git" / "cmd" / "git.exe" + git_bash = root / "Git" / "bin" / "bash.exe" + for executable in (system_bash, git_executable, git_bash): + executable.parent.mkdir(parents=True) + executable.touch() + executable.chmod(0o755) with repo.config_writer() as writer: writer.set_value("core", "hooksPath", str(hooks_dir)) - with mock.patch("git.index.fun.sys.platform", "win32"): + # Model a normal Windows PATH: System32 (containing the WSL launcher) comes + # before Git's cmd directory, while Git's Bash is not itself on PATH. This + # exercises both Git-installation discovery and shell selection without + # mocking either resolver's answer. + with mock.patch("git.index.fun.sys.platform", "win32"), mock.patch.object( + Git, "GIT_PYTHON_GIT_EXECUTABLE", "git" + ), mock.patch.dict(os.environ, {"SystemRoot": str(system_root)}), mock.patch( + "git.index.fun.os.get_exec_path", return_value=["", str(system_bash.parent), str(git_executable.parent)] + ): with mock.patch("git.index.fun.safer_popen") as popen, mock.patch("git.index.fun.handle_process_output"): popen.return_value.returncode = 0 run_commit_hook("fake-hook", repo.index) command = popen.call_args[0][0] - self.assertEqual(command, ["bash.exe", "../hooks/fake-hook"]) + self.assertEqual(command, [str(git_bash), "../hooks/fake-hook"]) + + @with_rw_directory + def test_windows_bash_lookup_respects_explicit_current_directory_in_path(self, rw_dir): + root = Path(rw_dir).resolve() + bash = root / "bash.exe" + bash.touch() + bash.chmod(0o755) + + # An explicitly listed directory is trusted PATH configuration, even when + # it happens to be the current directory. This differs from an empty entry, + # which Windows requires PATH lookup to ignore. + with cwd(root), mock.patch("git.index.fun.os.get_exec_path", return_value=[str(root)]): + self.assertEqual(_which_from_path("bash.exe"), str(bash)) + + @with_rw_directory + def test_windows_bash_lookup_from_explicit_git_bin(self, rw_dir): + git_root = Path(rw_dir).resolve() / "Git" + git_executable = git_root / "bin" / "git.exe" + bash = git_root / "bin" / "bash.exe" + git_executable.parent.mkdir(parents=True) + for executable in (git_executable, bash): + executable.touch() + executable.chmod(0o755) + + # A relative executable containing a directory is resolved by CreateProcess + # from the parent process cwd, not the separately supplied child cwd. Enter the + # temporary root first because Windows cannot express a relative path between + # drives, and CI may keep the checkout and its temporary directory on different + # drives. + with cwd(Path(rw_dir).resolve()): + relative_git = osp.relpath(git_executable, os.curdir) + with mock.patch.object(Git, "GIT_PYTHON_GIT_EXECUTABLE", relative_git): + self.assertEqual(_git_for_windows_bash(), str(bash)) @ddt.data((False,), (True,)) @with_rw_directory