Resolve Windows hook Bash through PATH - #2199
Conversation
<!-- agent --> Extensionless commit hooks on Windows were launched through a bare bash.exe. Windows CreateProcess searches system directories before PATH for bare executable names, so the WSL launcher in System32 could preempt Git for Windows Bash and make every commit fail when no WSL distribution was installed. A regression test mocks PATH resolution and demonstrates that the hook command previously remained bare instead of using the resolved executable. Resolve bash.exe with shutil.which before spawning, while retaining the bare-name fallback when PATH has no Bash so existing WSL-based behavior remains available. This matches Git for Windows v2.55.0.windows.3, whose git var GIT_SHELL_PATH reports the absolute PATH-selected sh.exe. It also follows gix-command's principle of resolving a Git-associated Windows shell before falling back to a bare executable name. Assisted-by: GPT 5.6 Co-authored-by: GPT 5.6 <codex@openai.com>
531f5dc to
7131bcc
Compare
EliahKagan
left a comment
EliahKagan
left a comment
There was a problem hiding this comment.
I don't think this usually works, and I'm not sure it works well or often enough to be worth keeping. The good news is that this applies specifically to hooks, where the previous lookup was already not working, so I can see a case for keeping it until it can be improved. But even then I think the case is not very strong.
(Note that this is a very different situation from path lookup in general--applying something like this more broadly, e.g. as a modification of safer_popen, would break a lot of stuff and possibly introduce vulnerabilities.)
My concerns are:
-
This skips the current directory in
PATH, which is not necessary outside the case of ensuring that the empty string is not interpreted to mean the current directory, and which produces wrong behavior if the user happens to be in the directory wherebash.exeactually should and otherwise would be found.(One does need to ensure the empty string is not interpreted to mean the current directory in Windows, since one can get it from splitting
PATH, but on Windows it is supposed to simply be skipped. Facilities that automatically handlePATHshould always automatically do this and it is always a severe bug if they do not. It's when handling it manually, or when implementing such a general facility, that one must take care to skip empty entries.)I think the confusion here is that we do need to avoid automatically including the CWD in the search, since it's an untrusted search path vulnerability if we don't. But
safer_popenalready does this. Any method that searches only directories named inPATHalso does this automatically. This is so long as it either has no fallback or, as here, falls back to a mechanism that does it--and so long as it does not wrongly interpret empty entries on Windows as designating the current directory. -
This doesn't skip the
System32directory, which is usually present inPATHand early on, and which is where the WSL-associatedbash.exeis actually located.
There's a test here, but I also don't see how the test is really verifying that this works--it makes sure the new function is used, but it doesn't check that the new function works as designed, nor is there an end-to-end check to see if this design is effective at causing a correct bash.exe to be run when an incorrect one would have run before.
On CI, in a Git Bash shell, and in some but not most other development environments, the Git for Windows bash.exe precedes the WSL-associated bash.exe in PATH. Usually it doesn't. So if this appears to work then I suspect it is one of two cases:
- The narrow case where the
bindirectory containing the Git for Windowsbash.exeis present inPATHwithout theSystem32directory preceding it (and nothing going wrong such as beingcdd to that directory). I think that's happening on CI. - It not working in the current environment, but it appearing to work based on the new test passing. I expect that to happen in local testing if tested in most common local environments other than a Git Bash shell.
One thing that is right about this implementation is that it does not use shutil.which, which is unsuitable for this particular use case. I bring this up because the commit message here inaccurately states that it does use shutil.which, which might wrongly lead someone to attempt to reimplement it by using that.
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. 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.
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. 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.
Tasks
This section is for Byron only. Models continuing this PR must not add, remove, check, uncheck, rename, or reorder checkboxes here.
Everything below this line was generated by Codex GPT-5.
Created by Codex on behalf of Byron. Byron will review before this is ready to merge.
Summary
Fixes #2198.
On Windows, resolve
bash.exefromPATHbefore launching extensionless commit hooks. This preventsCreateProcessfrom selecting the WSL launcher in System32 ahead of Git for Windows Bash. The current directory is deliberately excluded from resolution so a repository-local executable cannot be selected. If no Bash is present onPATH, GitPython retains its existing bare-name fallback.Baselines
Git for Windows v2.55.0.windows.3 resolves its shell through PATH:
git var GIT_SHELL_PATHreturnedC:/Program Files/Git/usr/bin/sh.exein the reference checkout at/c/Users/byron/Desktop/dev/git.The
gix-commandreference delegates default-shell selection togix_path::env::shell_command(). On Windows, gix first locates a Git-associated shell from the selected Git installation (bin, thenusr/bin) and falls back to baresh.exe. This fix adopts the same key safety property—resolve before spawning and retain a bare fallback—while preserving GitPython's established Bash invocation.Validation
test/test_index.py: 30 passed, 1 expected xfail, 1 xpassruff check .ruff format --check .git diff --checkcodex review --commit 531f5dc242efc2b4f76668f3d322d5fd3940f213: no actionable findings