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.
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 |
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.