GitPython icon indicating copy to clipboard operation
GitPython copied to clipboard

Override bash executable, defaulting to Git for Windows git bash over WSL bash

Open emanspeaks opened this issue 1 year ago • 6 comments

Provides a mechanism to set a path to bash to use on Windows for executing commit hook commands. If none is set, the default behavior should be to prefer Git Bash from Git for Windows.

If a user is running in Windows, they probably are using Git for Windows, which includes Git Bash and should be associated with the configured Git command set in refresh(). Search first for Git Bash relative to the configured Git command, and, if it exists, use it. Otherwise, fail back to the hardcoded default of "bash.exe".

The original "default" bash without a path assumes that bash.exe is on the PATH. If a user chose not to make MSYS2 tools available on the PATH on Git for Windows install, another method must be used to find that bash if it should be preferred over one found on the PATH. Newer versions of Windows include a bash.exe in System32 that is then found by default, but this bash.exe is intended for use with WSL. If a user intended to invoke git in WSL, presumably this package would have been installed there and not registered as "Windows" and reduces the ambiguity. In most cases where running the python in "native" Windows, then, it is undesirable to invoke the WSL Bash that is not associated with the Git for Windows install. Furthermore, direct access to bash.exe in System32 is reportedly deprecated in lieu of commands that explicitly invoke WSL.

This addresses issues where git hooks are intended to run assuming the "native" Windows environment as seen by git.exe rather than inside the git sandbox of WSL, which is likely configured independently of the Windows Git as well. A noteworthy example are repos with Git LFS, where Git LFS may be installed in Windows but not in WSL, causing commit hooks for LFS to fail despite being installed alongside the configured Git command in this package.

emanspeaks avatar Jan 09 '24 01:01 emanspeaks

@EliahKagan already prepared me for this case and I hope he can take a look at this PR as well [...]

I will try to look at this in detail today or tomorrow.

EliahKagan avatar Jan 09 '24 15:01 EliahKagan

Thanks for all the comments! Been busy with the day job the last week or so, but planning to try to address these things this weekend so we can get another round of reviews going and get it merged.

emanspeaks avatar Jan 19 '24 21:01 emanspeaks

I believe I have addressed all the comments. I removed the xfails from the unit tests, and had assertion errors in test_hook_uses_shell_not_from_cwd() due to the hook actually executing in type(_win_bash_status) in {WinBashStatus.Absent, WinBashStatus.WslNoDistro} block rather than raising the exception. I'm admittedly not understanding what this test is trying to test, but it seems like the else block is the behavior we now want since we should generally have working hooks regardless of WSL status. I opted to just comment out the former block and make every case follow the former else branch to get the test to pass. If that is not the correct result for this test, though, please let me know/enlighten me on what this test is trying to do so I can fix the issue. Otherwise, with your concurrence, this PR should hopefully be ready :-)

emanspeaks avatar Jan 21 '24 07:01 emanspeaks

The new conflict looks to be from #1819 and easily resolvable. #1819 bumped the version on the setup-wsl action that this PR comments out.

EliahKagan avatar Feb 13 '24 19:02 EliahKagan