Correct Windows hook Bash precedence - #2201
Merged
Merged
Conversation
Byron
force-pushed
the
fixup-bash-exe-lookup-on-windows
branch
3 times, most recently
from
August 4, 2026 11:30
b5ae6d8 to
701259a
Compare
Byron
marked this pull request as ready for review
August 4, 2026 11:30
Member
Author
|
@EliahKagan I think this take on |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 701259a3e4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Contributor
There was a problem hiding this comment.
Pull request overview
This pull request refines GitPython’s Windows commit-hook execution path to reliably select the intended Git-for-Windows Bash interpreter (instead of the WSL System32\bash.exe launcher) by preferring a Bash that matches GitPython’s selected git.exe installation, and by hardening the PATH-based fallback.
Changes:
- Add Windows-specific Bash resolution helpers to infer the Git-for-Windows install root from the configured/selected
gitexecutable, and preferbin/bash.exeoverusr/bin/bash.exe. - Implement a PATH lookup helper that ignores empty PATH entries (Windows semantics) and excludes executables under
%SystemRoot%. - Extend regression tests to model
System32precedingGit\cmdon PATH and validate correct Bash selection, including explicit current-directory PATH handling and relative configuredgit.exepaths.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
git/index/fun.py |
Adds Git-for-Windows-root/Bash discovery and a safer PATH resolver; updates run_commit_hook to prefer Git-associated Bash before falling back. |
test/test_index.py |
Adds/adjusts tests to cover the new resolver behavior under realistic Windows PATH ordering and configuration scenarios. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
<!-- agent --> 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. The machine producing this commit 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. Assisted-by: GPT 5.6 Co-authored-by: GPT 5.6 <codex@openai.com>
Byron
force-pushed
the
fixup-bash-exe-lookup-on-windows
branch
from
August 4, 2026 12:51
701259a to
b6f4a75
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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
Follow-up to #2199 and its post-merge review of the Windows hook Bash resolver. Refs #2198.
The merged resolver still selected the WSL launcher when System32 preceded Git Bash on PATH, and it could not locate Git Bash in the common configuration where PATH contains only
Git\cmd. It also skipped an explicitly configured current-directory PATH entry and mocked the resolver in its regression test.This change first locates the Bash associated with GitPython's selected Git executable. It recognizes standard
Git\cmd\git.exe,Git\bin\git.exe, andGit\<platform>\bin\git.exelayouts, then follows gix-path'sbin/bash.exebeforeusr/bin/bash.exeprecedence. The PATH fallback excludes candidates below SystemRoot, ignores empty entries according to Windows semantics, and still honors explicitly named directories. The existing bare fallback is retained for nonstandard installations and remains protected from implicit current-directory lookup bysafer_popen.Windows and WSL validation
This machine has Ubuntu on WSL 2,
C:\Windows\System32\bash.exe, and Git for Windows.bash.exereportedLinux ... microsoft-standard-WSL2.git var GIT_SHELL_PATHreturnedC:/Program Files/Git/usr/bin/sh.exe, which reported MINGW64.System32;Git\usr\bin, the Resolve Windows hook Bash through PATH #2199 resolver selected System32 and an actual GitPython hook wrote a Linux/WSL marker.System32;Git\cmdPATH, Bash was absent from PATH, but the corrected resolver selectedC:\Program Files\Git\bin\bash.exe; the same end-to-end hook wrote a MINGW64 marker.Tests
Git\cmd, with Bash absent from PATH, and exercises the real resolver.Git\bin\git.execonfiguration resolves its sibling Bash, including a relative configured path resolved from the parent process working directory.test/test_index.py: 32 passed, 1 expected xfail, 1 existing xpass.git diff --checkpassed.py_compilerun.codex review --commit b5ae6d8ee6efee580aa21680786775977131bfdecould not inspect the local commit because its Windows sandbox helper was unavailable and fell back to the stale pre-fix PR patch. Its three findings described behavior already corrected in this commit and were verified as false positives against the current source and tests.