Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork938
Override bash executable, defaulting to Git for Windows git bash over WSL bash#1791
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Thanks a lot for tackling this and for explaining the reasoning so very well!
@EliahKagan already prepared me for this case and I hope he can take a look at this PR as well to see if it fits into the big picture.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
I will try to look at this in detail today or tomorrow. |
EliahKagan left a comment• edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Thank you for bearing with my delay in reviewing this, and I apologize for any inconvenience. I strongly agree with the idea here, and I think this pull request will be suitable for merging, after some changes.
As you say, running hook scripts in WSL is not a good default, and it often fails or causes incorrect behavior. I think the problems with the way GitPython currently does it are severe enough to justify changing the default in a bugfix release. In my opinion, using WSL automatically even when a Git Bash shell is available is a bug, is usually not what users intend, and should be changed even though doing so may break some existing code that relies on the current behavior. (In#1745 and#1777 I had expressed hope that this could somehow be fixed in a way that does not break backward compatibility at all, for anyone, but I realize that is not realistic.) The ability to restore the old behavior by settingGIT_PYTHON_BASH_EXECUTABLE
tobash.exe
should be clearly documented.
The case for using the Git for Windowsbash.exe
when available is actually even stronger than it seems. Unlike on other systems, path search on Windowsdiffers substantially depending on whether a shell is used. That is, running a program incmd.exe
resolves commands differently from a direct call toCreateProcessW
. One effect is thatPopen
(andsubprocess.run
, etc.) on Windows withoutshell=True
searches a few directories prior to those listed in thePATH
environment variable. One such directory isSystem32
, even if it is not inPATH
or no matter what is listed before it. So even when a "native"bash.exe
is in aPATH
directory listed beforeSystem32
, the WSLbash.exe
inSystem32
is usually whatPopen
runs. (I say "usually" as there are other subtleties.) Furthermore, the WSLbash.exe
often exists inSystem32
even if no user has intentionally set up WSL and even if no WSL distributions are installed that would let it succeed.
Using the Git for Windowsbash.exe
whenever it is available would solve those problems, as well as the ones you described, and the further problem that using the WSLbash
can hurt performance as it may need to start a WSL system. This is also much closer to the behavior that users are likely to expect, because Git for Windows itself runs hooks with a#!/bin/sh
or#!/bin/bash
shebang (and some others) with itsbash.exe
.
I also agree with much of the strategy this takes. However, there are some changes that should be made, to avoid introducing newuntrusted search path vulnerabilities, similar toCVE-2023-40590 orCVE-2024-22190, that result in arbitrary code execution; to cover more of the common ways Git for Windows may be installed and run, while also avoiding using abash.exe
associated with a separategit
from the one GitPython will use; and to ensure no code that uses GitPython without using hooks can break as a result of changes made here. That is all covered among my comments below. There are also two considerations not covered in review comments:
A fix forCVE-2024-22190 (#1792) was recently merged. That was after this pull request was initially opened, so this will have to be updated to get that bugfix, and to help prevent that security bug from being inadvertently reintroduced. This is also needed to resolve conflicts, which fortunately are very minor, and to get the changes from#1803 without which subsequent CI runs would fail (#1802). Although theguidelines don't express a preference on how PRs should be updated, based on#1659 (comment) and how minor the conflicts are, I recommend rebasing (maybe@Byron can speak to this more definitively). Updating this PR won't automatically fixnew untrusted search path vulnerabilities the current code in this PR would add, but I've left review comments on the specific affected code about how that can be done.
The tests should be modified to ensure these changes work as expected. I am unsure if any new tests are needed. The main thing I'd suggest doing is to remove all WSL-relatedxfail
mark decorators from test functions intest/test_index.py
, and remove the "Setup WSL" step from thepythonpackage.yml
workflow. Currently, the WSLbash.exe
is being used on CI, even with these changes. This is because, as written, the changes here make assumptions Git for Windows does not always satisfy, including as it is used on the Windows runners for GitHub Actions. As commented below, that can be fixed. I suggest removing marks and the "Setup WSL" step, so all hook tests fail on CI due to the WSLbash.exe
having no distribution to runbash
in (see9717b8d), before fixing the bug in how the Git for Windowsbash.exe
is found, and verifying that this makes the tests pass.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
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. |
I believe I have addressed all the comments. I removed the xfails from the unit tests, and had assertion errors in |
EliahKagan left a comment• edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Thanks for the revisions! I agree that this addresses everything from the first review, and I think this is very close to being ready to merge.
I think the the only tricky remaining issue is what to do intest_uses_shell_not_from_cwd
. I believe that one of the reasonable approaches is to leave it as it is, possibly removing the commented-out code altogether, and possibly adding a comment about future directions. But I'm also aware of two other reasonable approaches. I've left a review comment there that covers all three, and that also tries to answer the question of what that test is testing. Further information and context on what that test is checking for can be found in the "When hook scripts are run" subsection ofCVE-2024-22190.
Aside from that, I recommend deleting the code that this PR currently comments out. You may have been intending to do that anyway, but I've posted review comments where applicable for it. It should not be necessary to install a WSL distribution on CI anymore, due to the improvements you've made in this PR. Furthermore, at least outside of tests related to security or the ability to signal a failed hook run, I don't think there is any need to cover WSL in the tests anymore (thus none of the WSL-related xfail decorations should be needed). If it turns out some such code will be needed again, it can be found in the commit history and restored as needed, with whatever modifications end up being required for it.
I've also noticed and included review comments about a few other small things.
#- name: Set up WSL (Windows) | ||
# if: startsWith(matrix.os, 'windows') | ||
# uses: Vampire/setup-wsl@v2.0.2 | ||
# with: | ||
# distribution: Debian |
EliahKaganJan 21, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
With the improvement from this PR, we shouldn't need to install a WSL distribution on CI anymore, so this commented-out code can be removed altogether. It will still be in the history and can be brought back if needed in the future. (I'm guessing you may have been planning to do this anyway, but I figured I'd mention it where applicable.)
@@ -360,9 +361,107 @@ def __setstate__(self, d: Dict[str, Any]) -> None: | |||
the top level ``__init__``. | |||
""" | |||
_bash_exec_env_var = "GIT_PYTHON_BASH_EXECUTABLE" | |||
bash_exec_name = "bash" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This seems not to be used. I'm not sure it's needed. If it is kept, then_get_default_bash_path
should probably return it on non-Windows systems and possibly returnf"{cla.bash_exec_name}.exe"
as the fallback on Windows systems. Although I believe it was for symmetry withgit_exec_name
, the treatment ofgit
andbash
has become less similar in recent changes, so I'm not sure there's a need for this anymore.
GIT_PYTHON_BASH_EXECUTABLE = None | ||
""" | ||
Provides the path to the bash executable used for commit hooks. This is |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I recommend adding "on Windows" to the first sentence, both here and in therefresh_bash
docstring. It is set on other systems, and future software that uses GitPython may itself use it directly on other systems (though that would be unusual), but GitPython itself is only using it on Windows.
Refreshes the cached path to the bash executable used for executing | ||
commit hook scripts. This gets called by the top-level `refresh()` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
As above forGIT_PYTHON_BASH_EXECUTABLE
, I suggest adding "on Windows" to the first sentence, since the path this sets is not used by GitPython on other systems. Otherwise, this is saying that hook scripts are executed with this bash interpreter on all systems, which is not accurate, and the documentation of the behavior on non-Windows systems lower down in this same docstring--which is itself not a problem--will reinforce that wrong information.
this method may be invoked manually if the path changes after import. | ||
This method only checks one path for a valid bash executable at a time, | ||
using the first non-empty path provided in the following priority |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Thepath
argument seems to be used if notNone
, even if it is the empty string. Although people shouldn't do that, the behavior seems correct, since passing a string aspath
should cause that to be used.
This description could be changed to just say "the first path provided..." because it is reasonable to interpret an empty environment variable as not providing a path, which is what I would suggest. But if you feel that's not accurate enough, then it could be expanded to be more specific, or something about the environment variable's value being nonempty could be added in item 2 of the numbered list.
# if type(_win_bash_status) in {WinBashStatus.Absent, WinBashStatus.WslNoDistro}: | ||
# # The real shell can't run, but the impostor should still not be used. | ||
# with self.assertRaises(HookExecutionError): | ||
# with maybe_chdir: | ||
# run_commit_hook("polyglot", repo.index) | ||
# self.assertFalse(payload.exists()) | ||
# else: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I'm not sure what the best thing is to do here right now. Some of the modifications to the test suite to take advantage of the improvements in this pull request could be done later.
The purpose of this test method is to verify that an untrusted repository (or, more realistically, an untrusted branch, possibly from a fork) with a maliciousbash.exe
cannot cause thatbash.exe
to run, which was part ofCVE-2024-22190. There are two general scenarios that should be tested: when there is anotherbash.exe
that should run hooks, and when there isn't another one. In both cases, the impostorbash.exe
should not run.
It is a shortcoming of#1792, which fixedCVE-2024-22190 and added this test, that both scenarios would not be tested in a single run of the test suite. This would have been tricky to do prior to the changes in this pull request, but I probably could and maybe should have done it. Instead, this tested whichever of the two scenarios applied to the test system. Both scenarios were common prior to the changes in this pull request, which makes the broken-or-unavailablebash
scenario uncommon, though still possible.
Possible approaches:
- Assume a working
bash.exe
is available for the test. That's what this PR currently does, by commenting out the code for the broken-or-unavailablebash
scenario. That code could even be removed, like other commented-out code, since it is in the repository's history. It could be replaced with a comment“TODO: Test that an impostor bash doesn't run in a realistic case of the "real" bash being broken or absent.” (or maybe there is a better wording). - Modify
WinBashStatus.check
to work for checking the status of thebash
interpreter that GitPython uses now. One way is to have it take an argument for thebash
command and run that instead of hard-codingbash.exe
. That would be passed in the call used to bind_win_bash_status
. Then the existing check would work properly again. (It would even work in xfail conditions, but I think there is no good reason to cover it in those tests.) Further improvements could come later, including eventual removal of theWinBashStatus
class. This is the approach that I would lean toward taking at this time. - Produce both scenarios as separate test cases. This is ideal, but also seems like it may be outside this PR's scope. It may also be a bit tricky to do well, since the goal is for it to fail in a realistic regression ofCVE-2024-22190. If such a bug were inadvertently introduced in the future, the exploit would probably be a
bash.exe
impostor in an untrusted repository/branch, and it would probably affect only the situation where the"bash.exe"
fallback is used forGIT_PYTHON_BASH_EXECUTABLE
. This can be hard to test reliably because Windows searches in some directories before usingPATH
, such asSystem32
. However, the changes in this PR allow this to be tested a different way, by temporarily settingGIT_PYTHON_BASH_EXECUTABLE
to a command name that is very unlikely to exist already, such asbash-7de052ca-c9e6-4e4c-abe8-4d168ecd74c6.exe
, and naming the impostor that as well. This could be done without excessive code duplication by expanding the way the test is parametrized, or by splitting it into two separate methods that call a shared helper (or use a shared fixture).
I think any of those approaches, and probably various others I have not thought of, are reasonable.
#@pytest.mark.xfail( | ||
# type(_win_bash_status) is WinBashStatus.Absent, | ||
# reason="Can't run a hook on Windows without bash.exe.", | ||
# rasies=HookExecutionError, | ||
#) | ||
#@pytest.mark.xfail( | ||
# type(_win_bash_status) is WinBashStatus.WslNoDistro, | ||
# reason="Currently uses the bash.exe of WSL, even with no WSL distro installed", | ||
# raises=HookExecutionError, | ||
#) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
As ontest_run_commit_hook
, this commented-out code can be removed.
#@pytest.mark.xfail( | ||
# type(_win_bash_status) is WinBashStatus.WslNoDistro, | ||
# reason="Currently uses the bash.exe of WSL, even with no WSL distro installed", | ||
# raises=AssertionError, | ||
#) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
As ontest_run_commit_hook
andtest_pre_commit_hook_success
, this commented-out code can be removed.
The situation with theif type(_win_bash_status) is WinBashStatus.Absent:
check in the method body is more subtle, because it would be useful to continue testing that we get aHookExecutionError
, with the expected attributes, whenbash
is absent.
This isroughly analogous to the situation intest_hook_uses_shell_not_from_cwd
. But the stakes are much lower here, because this does not test for a security vulnerability. Also, this is passing on CI, because theWinBashStatus.Absent
case is fairly rare and not produced there:WinBashStatus.check
findsbash.exe
inSystem32
and concludesbash
is present, then the Git Bashbash.exe
is used due to the changes in this PR.
The possible approaches totest_hook_uses_shell_not_from_cwd
are also possible here, but because this test is not failing, there is the further option of just commenting to note that the check is not always accurate. As there, I think further improvements to the cases this covers should eventually be made, andcould be made here, but that their absence should not get in the way of this PR being merged.
#@pytest.mark.xfail( | ||
# type(_win_bash_status) is WinBashStatus.Absent, | ||
# reason="Can't run a hook on Windows without bash.exe.", | ||
# rasies=HookExecutionError, | ||
#) | ||
#@pytest.mark.xfail( | ||
# type(_win_bash_status) is WinBashStatus.Wsl, | ||
# reason="Specifically seems to fail on WSL bash (in spite of #1399)", | ||
# raises=AssertionError, | ||
#) | ||
#@pytest.mark.xfail( | ||
# type(_win_bash_status) is WinBashStatus.WslNoDistro, | ||
# reason="Currently uses the bash.exe of WSL, even with no WSL distro installed", | ||
# raises=HookExecutionError, | ||
#) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
As ontest_run_commit_hook
,test_pre_commit_hook_success
, andtest_pre_commit_hook_fail
, this commented-out code can be removed.
I am not concerned about losing mention of the "Specifically seems to fail on WSL bash (in spite of#1399)" situation here, and I don't think even that commented-out decoration needs to be kept as comments here. After the improvements of this PR, I think most use of the WSL bash for hooks will be for people whose workflows were already working and who need it for backward compatibility. The solution to this problem, for people who experience it, is just to use the new default behavior.
If the#1399-related failure with WSL is considered important to keep track of, then a new issue can be opened about it (I would be pleased to do this on request), or a comment could briefly mention it, but it seems to me that neither is necessary at this time.
#@pytest.mark.xfail( | ||
# type(_win_bash_status) is WinBashStatus.WslNoDistro, | ||
# reason="Currently uses the bash.exe of WSL, even with no WSL distro installed", | ||
# raises=AssertionError, | ||
#) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
As ontest_run_commit_hook
,test_pre_commit_hook_success
,test_pre_commit_hook_fail
, andtest_commit_msg_hook_success
, this commented-out code can be removed.
+ Test a successful refresh with a relative path, which will be safer to do once the refresh tests restore changed state.This is incomplete, because while it is probably not necessarywhile running the test suite to preserve an old False value ofgit.GIT_OK (most tests can't work if that happens anyway), thevalue of Git.GIT_PYTHON_GIT_EXECUTABLE is not the only other globalstate that effects the behavior of subsequently run tests and thatmay be changed as a result of the refresh tests.1. After the git.refresh function calls git.cmd.Git.refresh, it calls git.remote.FetchInfo.refresh, which rebinds the git.remote.FetchInfo._flag_map attribute.2. Future changes to git.cmd.Git.refresh may mutate other state in the Git class, and ideally the coupling would be loose enough that the refresh tests wouldn't have to be updated for that if the behavior being tested does not change.3. Future changes to git.refresh may perform other refreshing actions, and ideally it would be easy (and obvious) what has to be done to patch it back. In particular, it will likely call another Git method that mutates class-wide state due togitpython-developers#1791, and for such state that is also of the Git class, ideally no further changes would have to be made to the code that restores state after the refresh tests.If we assume git.refresh is working at least in the case that it iscalled with no arguments, then the cleanup can just be a call togit.refresh(). Otherwise, sufficiently general cleanup may be morecomplicated.
+ Test a successful refresh with a relative path, which will be safer to do once the refresh tests restore changed state.Seegitpython-developers#1811. This addresses it incompletely, because while it isprobably not necessary while running the test suite to preserve anold False value of git.GIT_OK (most tests can't work if thathappens anyway), the value of Git.GIT_PYTHON_GIT_EXECUTABLE is notthe only other global state that effects the behavior ofsubsequently run tests and that may be changed as a result of therefresh tests.1. After the git.refresh function calls git.cmd.Git.refresh, it calls git.remote.FetchInfo.refresh, which rebinds the git.remote.FetchInfo._flag_map attribute.2. Future changes to git.cmd.Git.refresh may mutate other state in the Git class, and ideally the coupling would be loose enough that the refresh tests wouldn't have to be updated for that if the behavior being tested does not change.3. Future changes to git.refresh may perform other refreshing actions, and ideally it would be easy (and obvious) what has to be done to patch it back. In particular, it will likely call another Git method that mutates class-wide state due togitpython-developers#1791, and for such state that is also of the Git class, ideally no further changes would have to be made to the code that restores state after the refresh tests.If we assume git.refresh is working at least in the case that it iscalled with no arguments, then the cleanup can just be a call togit.refresh(). Otherwise, sufficiently general cleanup may be morecomplicated.
This comment was marked as outdated.
This comment was marked as outdated.
schaveyt commentedOct 8, 2024
Nudge |
I believe this PR is waiting for the author, there is a lot of pending review comments and conflicts by now. |
Rather than hard-coding `bash` on all systems as the fallbackinterpreter when a fixture script cannot be run directly, thisfalls back in an operating system specific manner:- Except on Windows, always fall back to `bash`, as before.- On Windows, run `git --exec-path` to find the `git-core` directory. Then check if a `bash.exe` exists at the expected location relative to that. In Git for Windows installations, this will usually work. If so, use that path (with `..` components resolved away).- On Windows, if a specific `bash.exe` is not found in that way, then fall back to using the relative path `bash.exe`. This is to preserve the ability to run `bash` on Windows systems where it may have worked before even without `bash.exe` in an expected location provided by a Git for Windows installation.(The distinction between `bash` and `bash.exe` is only slightlysignificant: we check for the existence of the interpreter withoutinitially running it, and that check requires the full filename.It is called `bash.exe` elsewhere for consistency both with thechecked-for executable and for consistencey with how we run mostother programs on Windows, e.g., the `git` vs. `git.exe`.)ThisfixesGitoxideLabs#1359. That bug is not currently observed on CI, butthis change is verified to fix it on a local test system where itpreviously always occurred when running the test suite fromPowerShell in an unmodified environment. The fix applies both with`GIX_TEST_IGNORE_ARCHIVES` unset, in which case there are now nofailures, and with `GIX_TEST_IGNORE_ARCHIVES=1`, in which case thefailures are now limited to the 15 cases tracked inGitoxideLabs#1358.Previously, fixture scripts had been run on Windows with whatever`bash` was found in a `PATH` search, which had two problems:- On most Windows systems, even if no WSL distribution is installed and even if WSL itself is not set up, the `System32` directory contains a `bash.exe` program associated with WSL. This program attempts to use WSL to run `bash` in an installed distribution. The `wsl.exe` program also provides this functionality and is favored for this purpose, but the `bash.exe` program is still present and is likely to remain for many years for compatibility. Even when this `bash` is usable, it is not suited for running most shell scripts meant to operate on the native Windows system. In particular, it is not suitable for running our fixture scripts, which need to use the native `git` to prepare fixtures to be used natively, among other requirements that would not be satisfied with WSL (except when the tests are actually running in WSL). Since some fixtures are `.gitignore`d because creating them on the test system (rather than another system) is part of the test, this has caused breakage in most Windows environments unless `PATH` is modified -- either explicitly or by testing in an MSYS2 environment, such as the Git Bash environment -- whether or not `GIX_TEST_IGNORE_ARCHIVES` is set. This was the cause ofGitoxideLabs#1359.- Although using a Git Bash environment or otherwise adjusting the path *currently* works, the reasons it works are subtle and rely on non-guaranteed behavior of `std::process::Command` path search that may change without warning. On Windows, processes are created by calling the `CreateProcessW` API function. `CreateProcessW` is capable of performing a `PATH` search, but this `PATH` search is not secure in most uses, since it includes the current directory (and searches it before `PATH` directories) unless `NoDefaultCurrentDirectoryInExePath` is set in the caller's environment. While it is the most relevant to security, the CWD is not the only location `CreateProcessW` searches before searching `PATH` directories (and regardless of where, if anywhere, they may also appear in `PATH`). Another such location is the `System32` directory. This is to say that, even when another directory with `bash.exe` precedes `System32` in `PATH`, an executable search will still find the WSL-associated `bash.exe` in `System32` unless it deviates from the algorithm `CreateProcessW` uses. To avoid including the CWD in the search, `std::process::Command` performs its own path search, then passes the resolved path to `CreateProcessW`. The path search it performs is currently almost the same the algorithm `CreateProcessW` uses, other than not automatically including the CWD. But there are some other subtle differences. One such difference is that, when the `Command` instance is configured to create a modified child environment (for example, by `env` calls), the `PATH` for the child is searched early on. This precedes a search of the `System32` directory. It is done even if none of the customizations of the child environment modify its `PATH`. This behavior is not guaranteed, and it may change at any time. It is also the behavior we rely on inadvertently every time we run `bash` on Windows with a `std::process::Command` instance constructed by passing `bash` or `bash.exe` as the `program` argument: it so happens that we are also customizing the child environment, and due to implementation details in the Rust standard library, this manages to find a non-WSL `bash` when the tests are run in Git Bash, in GitHub Actions jobs, and in some other cases. If in the future this is not done, or narrowed to be done only when `PATH` is one of the environment variables customized for the child process, then putting the directory with the desired `bash.exe` earlier than the `System32` directory in `PATH` will no longer prevent `std::proces::Command` from finding the `bash.exe` in `System32` as `CreateProcessW` would and using it. Then it would be nontrivial to run the test suite on Windows.For references and other details, seeGitoxideLabs#1359 and comments including:GitoxideLabs#1359 (comment)On the approach of finding the Git for Windows `bash.exe` relativeto the `git-core` directory, see the GitPython pull requestgitpython-developers/GitPython#1791.Two possible future enhancements are *not* included in this commit:1. This only modifies how test fixture scripts are run. It only affects the behavior of `gix-testtools`, and not of any other gitoxide crates such as `gix-command`. This is because: - While gitoxide uses information from `git` to find out where it is installed, mainly so we know where to find installation level configuration, we cannot in assume that `git` is present at all. Unlike GitPython, gitoxide is usable without `git`. - We know our test fixture scripts are all (at least currently) `bash` scripts, and this seems likely for other software that currently uses this functionality of `gix-testtools`. But scripts that are run as hooks, or as custom commands, or filters, etc., are often written in other languages, such as Perl. (The fallback here does not examine leading `#!` lines.) - Although a `bash.exe` located at the usual place relative to (but outside of) the `git-core` directory is usually suitable, there may be scenarios where running an executable found this way is not safe. Limiting it to `gix-testtools` pending further research may help mitigate this risk.2. As in other runs of `git` by `gix-testools`, this calls `git.exe`, letting `std::process::Command` do an executable search, but not trying any additional locations where Git is known sometimes to be installed. This does not find `git.exe` in as many situations as `gix_path::env::exe_invocation` does. The reasons for not (or not quite yet) including that change are: - It would add `gix-path` as a dependency of `gix-testtools`. - Finding `git` in a `std::process::Command` path search is an established (though not promised) approach in `gix-testtools`, including to run `git --exec-path` (to find `git-daemon`). - It is not immediately obvious that `exe_invocation` behavior is semantically correct for `gix-testtools`, though it most likely is reasonable. The main issue is that, in many cases where `git` itself runs scripts, it prepends the path to the `git-core` directory to the `PATH` environment variable for the script. This directory has a `git` (or `git.exe`) executable in it, so scripts run an equivalent `git` associated with the same installation. In contrast, when we run test fixture scripts with a `bash.exe` associated with a Git for Windows installation, we do not customize its path. Since top-level scripts written to use `git` but not to be used *by* `git` are usually written without the expectation of such an environment, prepending this will not necessarily be an improvement.
Rather than hard-coding `bash` on all systems as the fallbackinterpreter when a fixture script cannot be run directly, thisfalls back in an operating system specific manner:- Except on Windows, always fall back to `bash`, as before.- On Windows, run `git --exec-path` to find the `git-core` directory. Then check if a `bash.exe` exists at the expected location relative to that. In Git for Windows installations, this will usually work. If so, use that path (with `..` components resolved away).- On Windows, if a specific `bash.exe` is not found in that way, then fall back to using the relative path `bash.exe`. This is to preserve the ability to run `bash` on Windows systems where it may have worked before even without `bash.exe` in an expected location provided by a Git for Windows installation.(The distinction between `bash` and `bash.exe` is only slightlysignificant: we check for the existence of the interpreter withoutinitially running it, and that check requires the full filename.It is called `bash.exe` elsewhere for consistency both with thechecked-for executable and for consistencey with how we run mostother programs on Windows, e.g., the `git` vs. `git.exe`.)ThisfixesGitoxideLabs#1359. That bug is not currently observed on CI, butthis change is verified to fix it on a local test system where itpreviously always occurred when running the test suite fromPowerShell in an unmodified environment. The fix applies both with`GIX_TEST_IGNORE_ARCHIVES` unset, in which case there are now nofailures, and with `GIX_TEST_IGNORE_ARCHIVES=1`, in which case thefailures are now limited to the 15 cases tracked inGitoxideLabs#1358.Previously, fixture scripts had been run on Windows with whatever`bash` was found in a `PATH` search, which had two problems:- On most Windows systems, even if no WSL distribution is installed and even if WSL itself is not set up, the `System32` directory contains a `bash.exe` program associated with WSL. This program attempts to use WSL to run `bash` in an installed distribution. The `wsl.exe` program also provides this functionality and is favored for this purpose, but the `bash.exe` program is still present and is likely to remain for many years for compatibility. Even when this `bash` is usable, it is not suited for running most shell scripts meant to operate on the native Windows system. In particular, it is not suitable for running our fixture scripts, which need to use the native `git` to prepare fixtures to be used natively, among other requirements that would not be satisfied with WSL (except when the tests are actually running in WSL). Since some fixtures are `.gitignore`d because creating them on the test system (rather than another system) is part of the test, this has caused breakage in most Windows environments unless `PATH` is modified -- either explicitly or by testing in an MSYS2 environment, such as the Git Bash environment -- whether or not `GIX_TEST_IGNORE_ARCHIVES` is set. This was the cause ofGitoxideLabs#1359.- Although using a Git Bash environment or otherwise adjusting the path *currently* works, the reasons it works are subtle and rely on non-guaranteed behavior of `std::process::Command` path search that may change without warning. On Windows, processes are created by calling the `CreateProcessW` API function. `CreateProcessW` is capable of performing a `PATH` search, but this `PATH` search is not secure in most uses, since it includes the current directory (and searches it before `PATH` directories) unless `NoDefaultCurrentDirectoryInExePath` is set in the caller's environment. While it is the most relevant to security, the CWD is not the only location `CreateProcessW` searches before searching `PATH` directories (and regardless of where, if anywhere, they may also appear in `PATH`). Another such location is the `System32` directory. This is to say that, even when another directory with `bash.exe` precedes `System32` in `PATH`, an executable search will still find the WSL-associated `bash.exe` in `System32` unless it deviates from the algorithm `CreateProcessW` uses. To avoid including the CWD in the search, `std::process::Command` performs its own path search, then passes the resolved path to `CreateProcessW`. The path search it performs is currently almost the same the algorithm `CreateProcessW` uses, other than not automatically including the CWD. But there are some other subtle differences. One such difference is that, when the `Command` instance is configured to create a modified child environment (for example, by `env` calls), the `PATH` for the child is searched early on. This precedes a search of the `System32` directory. It is done even if none of the customizations of the child environment modify its `PATH`. This behavior is not guaranteed, and it may change at any time. It is also the behavior we rely on inadvertently every time we run `bash` on Windows with a `std::process::Command` instance constructed by passing `bash` or `bash.exe` as the `program` argument: it so happens that we are also customizing the child environment, and due to implementation details in the Rust standard library, this manages to find a non-WSL `bash` when the tests are run in Git Bash, in GitHub Actions jobs, and in some other cases. If in the future this is not done, or narrowed to be done only when `PATH` is one of the environment variables customized for the child process, then putting the directory with the desired `bash.exe` earlier than the `System32` directory in `PATH` will no longer prevent `std::proces::Command` from finding the `bash.exe` in `System32` as `CreateProcessW` would and using it. Then it would be nontrivial to run the test suite on Windows.For references and other details, seeGitoxideLabs#1359 and comments including:GitoxideLabs#1359 (comment)On the approach of finding the Git for Windows `bash.exe` relativeto the `git-core` directory, see the GitPython pull requestgitpython-developers/GitPython#1791, itscomments, and the implementation of the approach by@emanspeaks:https://github.com/gitpython-developers/GitPython/blob/f065d1fba422a528a133719350e027f1241273df/git/cmd.py#L398-L403Two possible future enhancements are *not* included in this commit:1. This only modifies how test fixture scripts are run. It only affects the behavior of `gix-testtools`, and not of any other gitoxide crates such as `gix-command`. This is because: - While gitoxide uses information from `git` to find out where it is installed, mainly so we know where to find installation level configuration, we cannot in assume that `git` is present at all. Unlike GitPython, gitoxide is usable without `git`. - We know our test fixture scripts are all (at least currently) `bash` scripts, and this seems likely for other software that currently uses this functionality of `gix-testtools`. But scripts that are run as hooks, or as custom commands, or filters, etc., are often written in other languages, such as Perl. (The fallback here does not examine leading `#!` lines.) - Although a `bash.exe` located at the usual place relative to (but outside of) the `git-core` directory is usually suitable, there may be scenarios where running an executable found this way is not safe. Limiting it to `gix-testtools` pending further research may help mitigate this risk.2. As in other runs of `git` by `gix-testools`, this calls `git.exe`, letting `std::process::Command` do an executable search, but not trying any additional locations where Git is known sometimes to be installed. This does not find `git.exe` in as many situations as `gix_path::env::exe_invocation` does. The reasons for not (or not quite yet) including that change are: - It would add `gix-path` as a dependency of `gix-testtools`. - Finding `git` in a `std::process::Command` path search is an established (though not promised) approach in `gix-testtools`, including to run `git --exec-path` (to find `git-daemon`). - It is not immediately obvious that `exe_invocation` behavior is semantically correct for `gix-testtools`, though it most likely is reasonable. The main issue is that, in many cases where `git` itself runs scripts, it prepends the path to the `git-core` directory to the `PATH` environment variable for the script. This directory has a `git` (or `git.exe`) executable in it, so scripts run an equivalent `git` associated with the same installation. In contrast, when we run test fixture scripts with a `bash.exe` associated with a Git for Windows installation, we do not customize its path. Since top-level scripts written to use `git` but not to be used *by* `git` are usually written without the expectation of such an environment, prepending this will not necessarily be an improvement.
Uh oh!
There was an error while loading.Please reload this page.
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.