Skip to content

Commit 6d86c78

Browse files
codexByron
authored andcommitted
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\<platform>\bin\git.exe, with the platform names used by MSYS2. 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.
1 parent 804f7a4 commit 6d86c78

2 files changed

Lines changed: 130 additions & 17 deletions

File tree

git/index/fun.py

Lines changed: 85 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,7 @@
2626
from gitdb.base import IStream
2727
from gitdb.typ import str_tree_type
2828

29-
from git.cmd import handle_process_output, safer_popen
29+
from git.cmd import Git, handle_process_output, safer_popen
3030
from git.compat import defenc, force_bytes, force_text, safe_decode
3131
from git.exc import HookExecutionError, UnmergedEntriesError
3232
from git.objects.fun import (
@@ -79,16 +79,90 @@ def _has_file_extension(path: str) -> str:
7979
return osp.splitext(path)[1]
8080

8181

82+
def _is_in_windows_system_root(path: str) -> bool:
83+
"""Return whether ``path`` is inside the Windows installation directory."""
84+
system_root = os.environ.get("SystemRoot")
85+
if not system_root:
86+
return False
87+
88+
system_root = osp.normcase(osp.realpath(system_root))
89+
path = osp.normcase(osp.realpath(path))
90+
try:
91+
return osp.commonpath((system_root, path)) == system_root
92+
except ValueError:
93+
# Paths on different drives have no common path on Windows.
94+
return False
95+
96+
8297
def _which_from_path(command: str) -> Union[str, None]:
83-
"""Resolve an executable 'command' from PATH without considering the current directory."""
84-
cwd = osp.normcase(osp.abspath(os.curdir))
98+
"""Resolve ``command`` from PATH, excluding the Windows installation."""
8599
for directory in os.get_exec_path():
100+
# Unlike POSIX, Windows does not define an empty PATH entry as the current
101+
# directory. Skip it rather than letting abspath() turn it into one.
86102
if not directory:
87103
continue
88104
directory = osp.abspath(directory)
89-
if osp.normcase(directory) == cwd:
90-
continue
91105
candidate = osp.join(directory, command)
106+
# SystemRoot contains the WSL launcher stubs. They are valid executables but
107+
# not suitable for running a Windows Git hook: the hook path and environment
108+
# were prepared for Git for Windows, and WSL may have no distribution at all.
109+
if _is_in_windows_system_root(candidate):
110+
continue
111+
if osp.isfile(candidate) and os.access(candidate, os.X_OK):
112+
return candidate
113+
return None
114+
115+
116+
_GIT_FOR_WINDOWS_PREFIXES = ("mingw64", "mingw32", "clangarm64", "clang64", "clang32", "ucrt64")
117+
118+
119+
def _git_for_windows_root() -> Union[str, None]:
120+
"""Infer a standard Git for Windows root from GitPython's selected executable."""
121+
git_executable = os.fspath(Git.GIT_PYTHON_GIT_EXECUTABLE or Git.git_exec_name)
122+
if osp.dirname(git_executable):
123+
git_executable = osp.abspath(git_executable)
124+
else:
125+
# GitPython deliberately retains a bare executable name so later PATH changes
126+
# affect Git commands. Resolve it with the same PATH snapshot used for Bash.
127+
names = (git_executable,) if _has_file_extension(git_executable) else (git_executable, f"{git_executable}.exe")
128+
for name in names:
129+
resolved = _which_from_path(name)
130+
if resolved is not None:
131+
git_executable = resolved
132+
break
133+
else:
134+
git_executable = ""
135+
if not git_executable:
136+
return None
137+
138+
executable_dir = osp.dirname(git_executable)
139+
directory_name = osp.basename(executable_dir).lower()
140+
if directory_name == "cmd":
141+
# The normal system-wide PATH entry is <git-root>/cmd.
142+
return osp.dirname(executable_dir)
143+
if directory_name == "bin":
144+
prefix = osp.dirname(executable_dir)
145+
if osp.basename(prefix).lower() in _GIT_FOR_WINDOWS_PREFIXES:
146+
# Git Bash commonly exposes <git-root>/<platform>/bin/git.exe.
147+
return osp.dirname(prefix)
148+
if osp.basename(prefix).lower() != "usr":
149+
# An explicitly configured Git may be the root-level bin/git.exe. Do
150+
# not make the same inference from usr/bin: unlike the recognized
151+
# platform prefixes, "usr" has no reliably bounded parent layout.
152+
return prefix
153+
return None
154+
155+
156+
def _git_for_windows_bash() -> Union[str, None]:
157+
"""Return Bash from the Git for Windows installation selected by GitPython."""
158+
git_root = _git_for_windows_root()
159+
if git_root is None:
160+
return None
161+
162+
# Match gix-path's precedence: prefer the lightweight bin shim, then the
163+
# underlying usr/bin executable. Both belong to the same installation as Git.
164+
for relative_path in ("bin/bash.exe", "usr/bin/bash.exe"):
165+
candidate = osp.join(git_root, *relative_path.split("/"))
92166
if osp.isfile(candidate) and os.access(candidate, os.X_OK):
93167
return candidate
94168
return None
@@ -127,11 +201,12 @@ def run_commit_hook(name: str, index: "IndexFile", *args: str) -> None:
127201
# an absolute path in this form, although a relative path is preferable
128202
# because it also works with the Windows Subsystem for Linux wrapper.
129203
bash_hp = hp
130-
# Resolve through PATH before spawning. On Windows, CreateProcess searches
131-
# the current and system directories before PATH for a bare executable name,
132-
# which can otherwise select an impostor or the WSL launcher instead of Git
133-
# for Windows' Bash.
134-
cmd = [_which_from_path("bash.exe") or "bash.exe", Path(bash_hp).as_posix()]
204+
# Prefer Bash associated with GitPython's selected Git installation. If
205+
# that layout is not recognized, use an explicitly configured non-system
206+
# PATH entry. Preserve the bare fallback for installations that previously
207+
# relied on WSL or another CreateProcess-resolved Bash.
208+
bash_executable = _git_for_windows_bash() or _which_from_path("bash.exe") or "bash.exe"
209+
cmd = [bash_executable, Path(bash_hp).as_posix()]
135210

136211
process = safer_popen(
137212
cmd + list(args),

test/test_index.py

Lines changed: 45 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,7 @@
3333
UnmergedEntriesError,
3434
UnsafeOptionError,
3535
)
36-
from git.index.fun import hook_path, run_commit_hook
36+
from git.index.fun import _git_for_windows_bash, _which_from_path, hook_path, run_commit_hook
3737
from git.index.typ import BaseIndexEntry, IndexEntry
3838
from git.index.util import TemporaryFileSwap
3939
from git.objects import Blob
@@ -1128,20 +1128,58 @@ def test_run_commit_hook_outside_worktree_on_windows(self, rw_dir):
11281128
repo = Repo.init(root / "repo")
11291129
hooks_dir = root / "hooks"
11301130
_make_hook(root, "fake-hook", "exit 0")
1131-
bash = root / "git" / "bin" / "bash.exe"
1131+
system_root = root / "Windows"
1132+
system_bash = system_root / "System32" / "bash.exe"
1133+
git_executable = root / "Git" / "cmd" / "git.exe"
1134+
git_bash = root / "Git" / "bin" / "bash.exe"
1135+
for executable in (system_bash, git_executable, git_bash):
1136+
executable.parent.mkdir(parents=True)
1137+
executable.touch()
1138+
executable.chmod(0o755)
11321139
with repo.config_writer() as writer:
11331140
writer.set_value("core", "hooksPath", str(hooks_dir))
11341141

1135-
with mock.patch("git.index.fun.sys.platform", "win32"), mock.patch(
1136-
"git.index.fun._which_from_path", return_value=str(bash)
1137-
) as which:
1142+
# Model a normal Windows PATH: System32 (containing the WSL launcher) comes
1143+
# before Git's cmd directory, while Git's Bash is not itself on PATH. This
1144+
# exercises both Git-installation discovery and shell selection without
1145+
# mocking either resolver's answer.
1146+
with mock.patch("git.index.fun.sys.platform", "win32"), mock.patch.object(
1147+
Git, "GIT_PYTHON_GIT_EXECUTABLE", "git"
1148+
), mock.patch.dict(os.environ, {"SystemRoot": str(system_root)}), mock.patch(
1149+
"git.index.fun.os.get_exec_path", return_value=["", str(system_bash.parent), str(git_executable.parent)]
1150+
):
11381151
with mock.patch("git.index.fun.safer_popen") as popen, mock.patch("git.index.fun.handle_process_output"):
11391152
popen.return_value.returncode = 0
11401153
run_commit_hook("fake-hook", repo.index)
11411154

1142-
which.assert_called_once_with("bash.exe")
11431155
command = popen.call_args[0][0]
1144-
self.assertEqual(command, [str(bash), "../hooks/fake-hook"])
1156+
self.assertEqual(command, [str(git_bash), "../hooks/fake-hook"])
1157+
1158+
@with_rw_directory
1159+
def test_windows_bash_lookup_respects_explicit_current_directory_in_path(self, rw_dir):
1160+
root = Path(rw_dir).resolve()
1161+
bash = root / "bash.exe"
1162+
bash.touch()
1163+
bash.chmod(0o755)
1164+
1165+
# An explicitly listed directory is trusted PATH configuration, even when
1166+
# it happens to be the current directory. This differs from an empty entry,
1167+
# which Windows requires PATH lookup to ignore.
1168+
with cwd(root), mock.patch("git.index.fun.os.get_exec_path", return_value=[str(root)]):
1169+
self.assertEqual(_which_from_path("bash.exe"), str(bash))
1170+
1171+
@with_rw_directory
1172+
def test_windows_bash_lookup_from_explicit_git_bin(self, rw_dir):
1173+
git_root = Path(rw_dir).resolve() / "Git"
1174+
git_executable = git_root / "bin" / "git.exe"
1175+
bash = git_root / "bin" / "bash.exe"
1176+
git_executable.parent.mkdir(parents=True)
1177+
for executable in (git_executable, bash):
1178+
executable.touch()
1179+
executable.chmod(0o755)
1180+
1181+
with mock.patch.object(Git, "GIT_PYTHON_GIT_EXECUTABLE", str(git_executable)):
1182+
self.assertEqual(_git_for_windows_bash(), str(bash))
11451183

11461184
@ddt.data((False,), (True,))
11471185
@with_rw_directory

0 commit comments

Comments
 (0)