Fix gix_path::env::login_shell() by removing it
#1758
Conversation
This assures we don't suggest the usage of actual login shells to ever be used to execute hooks. Note that this is not marked as breaking change as no release was made with the old name yet.
There was a problem hiding this comment.
This looks great! Nothing need change, though I've suggested a few small things. Most are not mentioned here in this comment, and the things I do mention in this comment are not necessarily any more important than those mentioned in separate comments.
Parsing git --exec-path
I think it may be reasonable to include a fix for #1760 here, but this PR does not introduce that bug (nor had #1752). One of my review comments has my proposed one-line change fix for it, but it's just as easy to do separately if it would lead to delay or you're not sure it's right. I opened it as a separate issue rather than describing it here because it is definitely not a blocker for this PR.
Does Git for Windows really use its sh.exe as /bin/sh? (Yes.)
I decided to actually verify that Git for Windows is using its sh.exe rather than its bash.exe to run commands that are documented to run in a shell, where no shebang is involved or even relevant in principle, because it is not running a script file but a command, such as from a variable. I am pretty sure I've verified this before, but I wanted to be certain.
(Its sh.exe and bash.exe are the same, i.e. its sh is bash, but they behave subtly differently in some circumstances because, like most POSIX-compatible shells, bash has a POSIX mode where it is more strictly POSIX compatible that must either be enabled with options or an environment variable or, more typically, by naming it like sh rather than like bash. So the distinction is meaningful.)
It does use sh.exe for this purpose, as I had guessed in #1752 (comment) but was not totally certain of. So the approach in this PR is correct. The following shows how I checked, and omits the expected fetch-related error message from Git itself:
parnassus@windows-arm CLANGARM64 ~/repos/gitoxide (main)
$ git -c core.sshCommand='ps | grep -E "^\s*(PID|$$)\b" >&2; :' fetch
PID PPID PGID WINPID TTY UID STIME COMMAND
2323 1 2319 9716 cons0 197108 21:50:17 /usr/bin/sh
The strange use of grep is because the ps in the Git Bash environment does not support filtering processes by passing a PID. Further details about that, in case they are somehow of interest, can be seen in this gist.
Future directions
I've opened the research-oriented feature request #1761 about other edge cases and scenarios where something deliberate besides just /bin/sh may benefit from being done in situations where /bin/sh would ordinarily be used. That overlaps, though only partially, with the goals of #1752 and this PR.
I mention that chiefly because I think it is of interest. To be clear, nothing there has to be done here, nor should be, except the already-checked box that refers to this and is already fully done here.
| /// If the bundled shell on Windows cannot be found, `sh` is returned as the name of a shell | ||
| /// as it could possibly be found in `PATH`. |
There was a problem hiding this comment.
For executables on Windows, we have usually been searching for them with names that end in .exe. It generally works either way. I would not expect a usable sh implementation to be found that does not end in .exe, so I don't think we would be losing anything by returning sh.exe as the fallback on Windows, much as we use git.exe (rather than git) on Windows.
I haven't made a diff suggestion for this because it should probably be decided together with the question of whether the code should be refactored to make the local static item PATH an OsString rather than an Option<OsString>. Also, this is definitely not a blocker for the PR!
There was a problem hiding this comment.
That's a great point and I have made the change.
| .map(|p| p.join("usr").join("bin").join("bash.exe")) | ||
| core_dir() | ||
| .and_then(|p| p.ancestors().nth(3) /* skip mingw64/libexec/git-core */) | ||
| .map(|p| p.join("usr").join("bin").join("sh.exe")) |
There was a problem hiding this comment.
In addition to this (git-installation)\usr\bin\sh.exe, there is also (git installation)\bin\sh.exe. I do not know which is better.
The bin directory that is not under usr has less in it: only git.exe, bash.exe, and sh.exe. I believe usr-less bin directory's git.exe is a shim that delegates to the "real" (environment prefix)\bin\git.exe executable, since I have found that its contents are identical to those of (git installation)\cmd\git.exe, which is documented as such as shim (git-for-windows/git#2506).
My guess is that the distinction is similarly shim-related for the shells, though maybe the advantages and disadvantages that arise from using a shim versus the target executable are different for sh.exe and bash.exe than for git.exe. The reason I guess that the sh.exe in the usr-less bin is a shim is that it is much smaller than the usr\bin one:
:\Users\ek\scoop\apps\git\2.47.1> ls bin/sh.exe
Directory: C:\Users\ek\scoop\apps\git\2.47.1\bin
Mode LastWriteTime Length Name
---- ------------- ------ ----
-a--- 11/25/2024 7:28 AM 47496 sh.exe
C:\Users\ek\scoop\apps\git\2.47.1> ls usr/bin/sh.exe
Directory: C:\Users\ek\scoop\apps\git\2.47.1\usr\bin
Mode LastWriteTime Length Name
---- ------------- ------ ----
-a--- 11/25/2024 7:12 AM 2553064 sh.exe
I point this out in case you know you wish to use one over the other. I do not know which is better, nor do I have a guess. I've listed this in #1761 so it is not forgotten and to remind me to look into it in the future. This is definitely not a blocker for the PR.
There was a problem hiding this comment.
Generally I'd like the bin/sh shim more as it's closer to what's used on Linux with /bin/sh, but would avoid to actually doing so on Windows as the shim will have to redirect to the actual binary which probably slows things down measurably.
With that said, I didn't find the shim last time I looked, because I tested on a VM that just has the full-blown git SDK for Windows, which doesn't seem to have it.
The normal Git installation does have the shim though, but it's apparently not the most portable assumption to make about the Git installation.
Speaking of portable, I have never tested with the 'portable' version of the Git installer, if there is one.
There was a problem hiding this comment.
With that said, I didn't find the shim last time I looked, because I tested on a VM that just has the full-blown git SDK for Windows, which doesn't seem to have it.
Thanks--in #1761 I had forgotten about how the Git for Windows SDK is another distinctive environment, which is really a different kind of Git for Windows installation, that should be tested specifically. I've added it to the list in #1761 for further research. It might be something that be treated the same as an upstream MSYS2 with git installed in it, but I'm not sure. (For now, the box is not checked, since I'm not clear on how many or what the locations are for sh.exe with it.)
The normal Git installation does have the shim though, but it's apparently not the most portable assumption to make about the Git installation.
It sounds like (git installation)\bin\sh.exe shouldn't be preferred, then. I've added a note to #1761 about that too, also pointing here. Assuming the shim should neither be preferred nor used as a fallback, the only thing left to do for that item would be to add a comment to the code briefly explaining the choice, which I could do after testing locally on a system that has the Git for Windows SDK installed. (The absence of a check mark there is only because I haven't added that code comment yet.)
Speaking of portable, I have never tested with the 'portable' version of the Git installer, if there is one.
Yes, the downloads page lists a Portable ("thumbdrive edition") download.
Although it's been ages since I've used that directly, it is also used by other packagers, including the Scoop package, which is how I install Git for Windows, except when I am doing it differently to test something else. So I think the portable version is covered already. I've added it to the list in #1761. I put a check mark on it, which can be removed if someone finds I am missing something here.
One subtlety is that Scoop (and other package managers such as Chocolatey) provide their own shims. However, we are not attempting to use those shims, and they are in addition to what one gets with a standard full Git for Windows installation. (They point at Git for Windows's own shims.)
To be clear, we very well may be running those shims--in a Scoop installation, the git.exe shim is typically what is found in a PATH search and called, including in git --exec-path. But then the path git --exec-path provides as output is a path into the Git for Windows installation itself, and nothing we do with absolute paths based on that hits the Scoop shims anymore.
There was a problem hiding this comment.
Thanks--in #1761 I had forgotten about how the Git for Windows SDK is another distinctive environment, which is really a different kind of Git for Windows installation, that should be tested specifically. I've added it to the list in #1761 for further research. It might be something that be treated the same as an upstream MSYS2 with
gitinstalled in it, but I'm not sure. (For now, the box is not checked, since I'm not clear on how many or what the locations are forsh.exewith it.)
I would have loved to provide that information but just yesterday that VM demolished itself after a Windows update and is inoperable. During the weekend I plan to restore it from backup, so I could help here if still useful.
One subtlety is that Scoop (and other package managers such as Chocolatey) provide their own shims. However, we are not attempting to use those shims, and they are in addition to what one gets with a standard full Git for Windows installation. (They point at Git for Windows's own shims.)
Maybe the Shim could be a fallback if it turns out to be more portable, i.e. each or most of the Git packages one can use on Windows have it under that particular path, like <install-dir>/bin/<shim>.exe.
A practical solution could be to support the most common installation methods without shim for just slightly better invocation performance, but use the shim for less common packages if it makes the implementation significantly simpler.
To be clear, we very well may be running those shims--in a Scoop installation, the
git.exeshim is typically what is found in aPATHsearch and called, including ingit --exec-path. But then the pathgit --exec-pathprovides as output is a path into the Git for Windows installation itself, and nothing we do with absolute paths based on that hits the Scoop shims anymore
I agree. Expressing the above differently I could also say that I am happy with finding the actual file locations like we do now, but also think it might be simpler to rely on shims more if these paths are more consistent across the various installation methods.
There was a problem hiding this comment.
just yesterday that VM demolished itself after a Windows update and is inoperable
That must be annoying--good luck!
Maybe the Shim could be a fallback [...]
Currently, for git itself, we almost do the opposite. We take git from a PATH search if we can find it that way. When there is an applicable shim--whether from a package manager like scoop or from Git for Windows itself--that is usually what this finds. Though in some environments we may find the shim target--for example, on my Windows 10 development machine with Git for Windows installed via scoop, when I run git in PowerShell it is the scoop shim, but when I run git in Git Bash it is not.
If the PATH turned up no git, we check common locations, and look in directories where it is commonly installed. The current logic for that does not use the shims--it looks in whichever of the bin directories in mingw64 or mingw32 is applicable to the particular Git for Windows installation is being examined. (I believe the current behavior is still as it was when #1456 was merged.) It needs to be updated also to look into the clangarm64 directory when applicable. Git for Windows uses this on ARM64, which has had stable releases for a short while.
I've been meaning, and will eventually manage, to update that. Alternatively, if we use the Git for Windows git shim then the paths are "environment"-independent. One reason I've been slightly reluctant to do that is that there may sometimes be a performance benefit to bypassing the shims. I've been reluctant to give that up when it might turn out that it should instead be expanded (or kept but controlled by an environment variable--probably not a configuration variable, since we invoke git to find out where to look for some of those).
I'm not sure about Chocolatey, and also I haven't recently researched what other package managers besides choco and scoop are in wide use on Windows. But given the root of a scoop installation, we could find out if a scoop-managed Git for Windows is installed. We could also find Git for Windows's own directory and, in so doing, bypass all shims. Both operation seem pretty simple in ordinary setups and I believe the only subtleties are those related to edge cases, which I would guess would not pose a big problem.
Whether this makes sense to do probably depends on why we would be doing it. But it seems to me that it is not justified to do it if the reason is to be more likely to find git. I think it would be strange to use scoop but not put its shims directory in one's PATH, and scoop automatically adds its shims directory to the PATH when installed. But then again, some scoop users may use it mainly to install graphical programs that they invoke in ways that don't require the Scoop shims to be in their PATH, so maybe this is a real use case.
Here is what @EliahKagan wrote: > While it sometimes works, I don't think this technique is a reliable way of finding the `bash.exe` associated with Git for Windows. If I recall correctly, `installation_config_prefix()` is based on the location of the highest scoped non-local non-worktree configuration file of those that supply at least one configuration variable, except in the case of the `EXEPATH` optimization, which in practice only works in a Git Bash environment (and not always reliably). > > Git for Windows installations usually have variables configured in the system scope, but this is not guaranteed. That scope may also be suppressed by `GIT_CONFIG_NOSYSTEM` or have its associated configuration file set to a custom path by `GIT_CONFIG_SYSTEM`. In any of those cases, we may get no path but, currently, we are more likely to get a different path that would not be correct here, because while the local and worktree scopes are excluded, the global scope is not. > > Although it is valuable to perform fewer subprocess invocations on Windows where they can be slow, I don't think a technique along these lines for finding the `bash.exe` associated with a Git for Windows installation can be made reliable. The only reliable approach I know of is to infer the path from the output of `git --exec-path`, as in #1712. > > (If it is necessary to eke out a little extra performance, then it might be possible to attempt another approach while carefully covering cases where it does not work and checking for signs that it may be inaccurate, while still falling back to an `--exec-path`-based way.) Note that there is no attempt to eke out more performance yet.
| /// If the bundled shell on Windows cannot be found, `sh` is returned as the name of a shell | ||
| /// as it could possibly be found in `PATH`. |
There was a problem hiding this comment.
That's a great point and I have made the change.
| .map(|p| p.join("usr").join("bin").join("bash.exe")) | ||
| core_dir() | ||
| .and_then(|p| p.ancestors().nth(3) /* skip mingw64/libexec/git-core */) | ||
| .map(|p| p.join("usr").join("bin").join("sh.exe")) |
There was a problem hiding this comment.
Generally I'd like the bin/sh shim more as it's closer to what's used on Linux with /bin/sh, but would avoid to actually doing so on Windows as the shim will have to redirect to the actual binary which probably slows things down measurably.
With that said, I didn't find the shim last time I looked, because I tested on a VM that just has the full-blown git SDK for Windows, which doesn't seem to have it.
The normal Git installation does have the shim though, but it's apparently not the most portable assumption to make about the Git installation.
Speaking of portable, I have never tested with the 'portable' version of the Git installer, if there is one.
Co-authored-by: Eliah Kagan <[email protected]>
|
It looks like |
|
If a command is ever to be run in a shell for the environment the shell confers (rather than for its expansions or other language features), then there is one fragment of the logic for doing so that I think may make sense to have in |
- Add a TODO comment about changing `Exponential` to `Quadratic`. This cannot be done yet, but when upgrading dependencies, it will need to be done (since the rename is a breaking change). - Replace the TODO comment about `login_shell()` with one about how to use the value of `shell()` on Windows, and modify the last component, since GitoxideLabs#1758 replaced `login_shell()` with `shell()`. `shell()` returns a path to `sh`, and gix-testtools currently needs `bash` for fixtures. But the specific approach for finding `sh.exe` relative to the Git for Windows `git-core` directory also works for `bash.exe`.
- Add a TODO comment about changing `Exponential` to `Quadratic`. This cannot be done yet, but when upgrading dependencies, it will need to be done (since the rename is a breaking change). - Replace the TODO comment about `login_shell()` with one about how to use the value of `shell()` on Windows, and modify the last component, since GitoxideLabs#1758 replaced `login_shell()` with `shell()`. `shell()` returns a path to `sh`, and gix-testtools currently needs `bash` for fixtures. But the specific approach for finding `sh.exe` relative to the Git for Windows `git-core` directory also works for `bash.exe`.
- Add a TODO comment about changing `Exponential` to `Quadratic`. This cannot be done yet, but when upgrading dependencies, it will need to be done (since the rename is a breaking change). - Replace the TODO comment about `login_shell()` with one about how to use the value of `shell()` on Windows, and modify the last component, since GitoxideLabs#1758 replaced `login_shell()` with `shell()`. `shell()` returns a path to `sh`, and gix-testtools currently needs `bash` for fixtures. But the specific approach for finding `sh.exe` relative to the Git for Windows `git-core` directory also works for `bash.exe`.
Thanks to the review of @EliahKagan we can stop the proliferation of a bad idea in its tracks and replace it with what hopefully is a better idea.
Follow-up on #1752 .
Tasks
login_shell()with an alternativesh.exeis located on windows.gix envto print out such paths more easilyNotes for the Reviewer
git --exec-pathis always called even though one might do without under some circumstances. This optimization can be done later once this function is widely enough used.