Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix gix_path::env::login_shell() by removing it #1758

Merged
merged 5 commits into from
Jan 12, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 46 additions & 0 deletions gitoxide-core/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
#![deny(rust_2018_idioms)]
#![forbid(unsafe_code)]

use anyhow::bail;
use std::str::FromStr;

#[derive(Debug, Eq, PartialEq, Hash, Clone, Copy)]
Expand Down Expand Up @@ -82,6 +83,51 @@ pub mod repository;
mod discover;
pub use discover::discover;

pub fn env(mut out: impl std::io::Write, format: OutputFormat) -> anyhow::Result<()> {
if format != OutputFormat::Human {
bail!("JSON output isn't supported");
};

let width = 15;
writeln!(
out,
"{field:>width$}: {}",
std::path::Path::new(gix::path::env::shell()).display(),
field = "shell",
)?;
writeln!(
out,
"{field:>width$}: {:?}",
gix::path::env::installation_config_prefix(),
field = "config prefix",
)?;
writeln!(
out,
"{field:>width$}: {:?}",
gix::path::env::installation_config(),
field = "config",
)?;
writeln!(
out,
"{field:>width$}: {}",
gix::path::env::exe_invocation().display(),
field = "git exe",
)?;
writeln!(
out,
"{field:>width$}: {:?}",
gix::path::env::system_prefix(),
field = "system prefix",
)?;
writeln!(
out,
"{field:>width$}: {:?}",
gix::path::env::core_dir(),
field = "core dir",
)?;
Ok(())
}

#[cfg(all(feature = "async-client", feature = "blocking-client"))]
compile_error!("Cannot set both 'blocking-client' and 'async-client' features as they are mutually exclusive");

Expand Down
73 changes: 46 additions & 27 deletions gix-path/src/env/mod.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use std::ffi::OsString;
use std::ffi::{OsStr, OsString};
use std::path::{Path, PathBuf};

use bstr::{BString, ByteSlice};
Expand Down Expand Up @@ -28,21 +28,25 @@ pub fn installation_config_prefix() -> Option<&'static Path> {
installation_config().map(git::config_to_base_path)
}

/// Return the shell that Git would prefer as login shell, the shell to execute Git commands from.
/// Return the shell that Git would use, the shell to execute commands from.
///
/// On Windows, this is the `bash.exe` bundled with it, and on Unix it's the shell specified by `SHELL`,
/// or `None` if it is truly unspecified.
pub fn login_shell() -> Option<&'static Path> {
static PATH: Lazy<Option<PathBuf>> = Lazy::new(|| {
/// On Windows, this is the full path to `sh.exe` bundled with Git, and on
/// Unix it's `/bin/sh` as posix compatible shell.
/// 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`.
/// Note that the returned path might not be a path on disk.
Comment on lines +35 to +36
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a great point and I have made the change.

pub fn shell() -> &'static OsStr {
static PATH: Lazy<OsString> = Lazy::new(|| {
if cfg!(windows) {
installation_config_prefix()
.and_then(|p| p.parent())
.map(|p| p.join("usr").join("bin").join("bash.exe"))
core_dir()
.and_then(|p| p.ancestors().nth(3)) // Skip something like mingw64/libexec/git-core.
.map(|p| p.join("usr").join("bin").join("sh.exe"))
.map_or_else(|| OsString::from("sh"), Into::into)
Copy link
Member

@EliahKagan EliahKagan Jan 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

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.

Copy link
Member

@EliahKagan EliahKagan Jan 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

} else {
std::env::var_os("SHELL").map(PathBuf::from)
"/bin/sh".into()
}
});
PATH.as_deref()
PATH.as_ref()
}

/// Return the name of the Git executable to invoke it.
Expand Down Expand Up @@ -102,6 +106,36 @@ pub fn xdg_config(file: &str, env_var: &mut dyn FnMut(&str) -> Option<OsString>)
})
}

static GIT_CORE_DIR: Lazy<Option<PathBuf>> = Lazy::new(|| {
let mut cmd = std::process::Command::new(exe_invocation());

#[cfg(windows)]
{
use std::os::windows::process::CommandExt;
const CREATE_NO_WINDOW: u32 = 0x08000000;
cmd.creation_flags(CREATE_NO_WINDOW);
}
let output = cmd.arg("--exec-path").output().ok()?;

if !output.status.success() {
return None;
}

BString::new(output.stdout)
.strip_suffix(b"\n")?
.to_path()
.ok()?
.to_owned()
.into()
});

/// Return the directory obtained by calling `git --exec-path`.
///
/// Returns `None` if Git could not be found or if it returned an error.
pub fn core_dir() -> Option<&'static Path> {
GIT_CORE_DIR.as_deref()
}

/// Returns the platform dependent system prefix or `None` if it cannot be found (right now only on windows).
///
/// ### Performance
Expand All @@ -125,22 +159,7 @@ pub fn system_prefix() -> Option<&'static Path> {
}
}

let mut cmd = std::process::Command::new(exe_invocation());
#[cfg(windows)]
{
use std::os::windows::process::CommandExt;
const CREATE_NO_WINDOW: u32 = 0x08000000;
cmd.creation_flags(CREATE_NO_WINDOW);
}
cmd.arg("--exec-path").stderr(std::process::Stdio::null());
gix_trace::debug!(cmd = ?cmd, "invoking git to get system prefix/exec path");
let path = cmd.output().ok()?.stdout;
let path = BString::new(path)
.trim_with(|b| b.is_ascii_whitespace())
.to_path()
.ok()?
.to_owned();

let path = GIT_CORE_DIR.as_deref()?;
let one_past_prefix = path.components().enumerate().find_map(|(idx, c)| {
matches!(c,std::path::Component::Normal(name) if name.to_str() == Some("libexec")).then_some(idx)
})?;
Expand Down
22 changes: 15 additions & 7 deletions gix-path/tests/path/env.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,13 +8,11 @@ fn exe_invocation() {
}

#[test]
fn login_shell() {
// On CI, the $SHELL variable isn't necessarily set. Maybe other ways to get the login shell should be used then.
if !gix_testtools::is_ci::cached() {
assert!(gix_path::env::login_shell()
.expect("There should always be the notion of a shell used by git")
.exists());
}
fn shell() {
assert!(
std::path::Path::new(gix_path::env::shell()).exists(),
"On CI and on Unix we'd expect a full path to the shell that exists on disk"
);
}

#[test]
Expand All @@ -26,6 +24,16 @@ fn installation_config() {
);
}

#[test]
fn core_dir() {
assert!(
gix_path::env::core_dir()
.expect("Git is always in PATH when we run tests")
.is_dir(),
"The core directory is a valid directory"
);
}

#[test]
fn system_prefix() {
assert_ne!(
Expand Down
9 changes: 9 additions & 0 deletions src/plumbing/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,15 @@ pub fn main() -> Result<()> {
}

match cmd {
Subcommands::Env => prepare_and_run(
"env",
trace,
verbose,
progress,
progress_keep_open,
None,
move |_progress, out, _err| core::env(out, format),
),
Subcommands::Merge(merge::Platform { cmd }) => match cmd {
merge::SubCommands::File {
resolve_with,
Expand Down
1 change: 1 addition & 0 deletions src/plumbing/options/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,7 @@ pub enum Subcommands {
Corpus(corpus::Platform),
MergeBase(merge_base::Command),
Merge(merge::Platform),
Env,
Byron marked this conversation as resolved.
Show resolved Hide resolved
Diff(diff::Platform),
Log(log::Platform),
Worktree(worktree::Platform),
Expand Down
Loading