Skip to content

Correct Windows hook Bash precedence - #2201

Draft
Byron wants to merge 1 commit into
mainfrom
fixup-bash-exe-lookup-on-windows
Draft

Correct Windows hook Bash precedence#2201
Byron wants to merge 1 commit into
mainfrom
fixup-bash-exe-lookup-on-windows

Conversation

@Byron

@Byron Byron commented Jul 30, 2026

Copy link
Copy Markdown
Member

Tasks

This section is for Byron only. Models continuing this PR must not add, remove, check, uncheck, rename, or reorder checkboxes here.

  • refackiew

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, and Git\<platform>\bin\git.exe layouts, then follows gix-path's bin/bash.exe before usr/bin/bash.exe precedence. 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 by safer_popen.

Windows and WSL validation

This machine has Ubuntu on WSL 2, C:\Windows\System32\bash.exe, and Git for Windows.

  • Bare bash.exe reported Linux ... microsoft-standard-WSL2.
  • git var GIT_SHELL_PATH returned C:/Program Files/Git/usr/bin/sh.exe, which reported MINGW64.
  • Under 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.
  • Under the normal System32;Git\cmd PATH, Bash was absent from PATH, but the corrected resolver selected C:\Program Files\Git\bin\bash.exe; the same end-to-end hook wrote a MINGW64 marker.

Tests

  • Regression models System32 before Git\cmd, with Bash absent from PATH, and exercises the real resolver.
  • Explicit current-directory PATH entry is accepted while empty entries are ignored.
  • Explicit Git\bin\git.exe configuration resolves its sibling Bash, including a relative configured path resolved from the parent process working directory.
  • The relative-path test enters its temporary root before constructing the path, because Windows cannot express a relative path across the separate checkout and temporary drives used by CI.
  • Focused hook suite: 6 passed.
  • Full test/test_index.py: 32 passed, 1 expected xfail, 1 existing xpass.
  • Repository-wide Ruff lint and format checks passed using the Python 3.7 target.
  • git diff --check passed.
  • A standalone Python 3.7 interpreter was unavailable for an additional py_compile run.
  • codex review --commit b5ae6d8ee6efee580aa21680786775977131bfde could 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.

@Byron
Byron force-pushed the fixup-bash-exe-lookup-on-windows branch from 6d86c78 to 3d57b2f Compare July 30, 2026 20:40
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.
@Byron
Byron force-pushed the fixup-bash-exe-lookup-on-windows branch from 3d57b2f to b5ae6d8 Compare July 30, 2026 20:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants