From 1598dbd030b09c34be524789f86f777b2462d524 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Mon, 17 Feb 2025 13:23:01 -0500 Subject: [PATCH 1/4] feat: Use `gix_path::env::shell()` in `gix-command` for sh `gix_command::Prepare` previously used `sh` on Windows rather than first checking for a usable `sh` implementation associated with a Git for Windows installation. This changes it to use the `gix-path` facility for finding what is likely the best 'sh' implementation for POSIX shell scripts that will operate on Git repositories. This increases consistency across how different crates find 'sh', and brings the benefit of preferring the Git for Windows `sh.exe` on Windows when it can be found reliably. --- Cargo.lock | 1 + gix-command/Cargo.toml | 1 + gix-command/src/lib.rs | 6 ++---- gix-command/tests/command.rs | 36 ++++++++++++++++++++++-------------- 4 files changed, 26 insertions(+), 18 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 90992bd2178..ed0f4f844dc 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1582,6 +1582,7 @@ dependencies = [ "gix-quote 0.4.15", "gix-testtools", "gix-trace 0.1.12", + "once_cell", "shell-words", ] diff --git a/gix-command/Cargo.toml b/gix-command/Cargo.toml index e81cfedb4be..a2f9d763ddf 100644 --- a/gix-command/Cargo.toml +++ b/gix-command/Cargo.toml @@ -24,3 +24,4 @@ shell-words = "1.0" [dev-dependencies] gix-testtools = { path = "../tests/tools" } +once_cell = "1.17.1" diff --git a/gix-command/src/lib.rs b/gix-command/src/lib.rs index ca64d5cc3e7..c8d47d93e32 100644 --- a/gix-command/src/lib.rs +++ b/gix-command/src/lib.rs @@ -280,10 +280,8 @@ mod prepare { cmd } None => { - let mut cmd = Command::new( - prep.shell_program - .unwrap_or(if cfg!(windows) { "sh" } else { "/bin/sh" }.into()), - ); + let shell = prep.shell_program.unwrap_or_else(|| gix_path::env::shell().into()); + let mut cmd = Command::new(shell); cmd.arg("-c"); if !prep.args.is_empty() { if prep.command.to_str().map_or(true, |cmd| !cmd.contains("$@")) { diff --git a/gix-command/tests/command.rs b/gix-command/tests/command.rs index 118c593d45a..b73a4299632 100644 --- a/gix-command/tests/command.rs +++ b/gix-command/tests/command.rs @@ -222,10 +222,13 @@ mod context { } mod prepare { - #[cfg(windows)] - const SH: &str = "sh"; - #[cfg(not(windows))] - const SH: &str = "/bin/sh"; + use once_cell::sync::Lazy; + + static SH: Lazy<&'static str> = Lazy::new(|| { + gix_path::env::shell() + .to_str() + .expect("`prepare` tests must be run where 'sh' path is valid Unicode") + }); fn quoted(input: &[&str]) -> String { input.iter().map(|s| format!("\"{s}\"")).collect::>().join(" ") @@ -275,7 +278,7 @@ mod prepare { if cfg!(windows) { quoted(&["ls", "first", "second", "third"]) } else { - quoted(&[SH, "-c", "ls first second third", "--"]) + quoted(&[*SH, "-c", "ls first second third", "--"]) }, "with shell, this works as it performs word splitting" ); @@ -311,7 +314,8 @@ mod prepare { if cfg!(windows) { quoted(&["ls", "--foo", "a b", "additional"]) } else { - format!(r#""{SH}" "-c" "ls --foo \"a b\" \"$@\"" "--" "additional""#) + let sh = *SH; + format!(r#""{sh}" "-c" "ls --foo \"a b\" \"$@\"" "--" "additional""#) }, "with shell, this works as it performs word splitting" ); @@ -334,7 +338,7 @@ mod prepare { let cmd = std::process::Command::from( gix_command::prepare("ls --foo=\"a b\"").command_may_be_shell_script_disallow_manual_argument_splitting(), ); - assert_eq!(format!("{cmd:?}"), quoted(&[SH, "-c", r#"ls --foo=\"a b\""#, "--"])); + assert_eq!(format!("{cmd:?}"), quoted(&[*SH, "-c", r#"ls --foo=\"a b\""#, "--"])); } #[test] @@ -347,7 +351,7 @@ mod prepare { ); assert_eq!( format!("{cmd:?}"), - quoted(&[SH, "-c", "ls \\\"$@\\\"", "--", "--foo=a b"]) + quoted(&[*SH, "-c", "ls \\\"$@\\\"", "--", "--foo=a b"]) ); } @@ -362,7 +366,7 @@ mod prepare { ); assert_eq!( format!("{cmd:?}"), - quoted(&[SH, "-c", "\\'ls\\' \\\"$@\\\"", "--", "--foo=a b"]), + quoted(&[*SH, "-c", "\\'ls\\' \\\"$@\\\"", "--", "--foo=a b"]), "looks strange thanks to debug printing, but is the right amount of quotes actually" ); } @@ -379,7 +383,7 @@ mod prepare { assert_eq!( format!("{cmd:?}"), quoted(&[ - SH, + *SH, "-c", "\\'C:\\\\Users\\\\O\\'\\\\\\'\\'Shaughnessy\\\\with space.exe\\' \\\"$@\\\"", "--", @@ -394,9 +398,10 @@ mod prepare { let cmd = std::process::Command::from( gix_command::prepare("ls --foo=~/path").command_may_be_shell_script_allow_manual_argument_splitting(), ); + let sh = *SH; assert_eq!( format!("{cmd:?}"), - format!(r#""{SH}" "-c" "ls --foo=~/path" "--""#), + format!(r#""{sh}" "-c" "ls --foo=~/path" "--""#), "splitting can also handle quotes" ); } @@ -405,9 +410,10 @@ mod prepare { fn tilde_path_and_multiple_arguments_as_part_of_command_with_shell() { let cmd = std::process::Command::from(gix_command::prepare("~/bin/exe --foo \"a b\"").command_may_be_shell_script()); + let sh = *SH; assert_eq!( format!("{cmd:?}"), - format!(r#""{SH}" "-c" "~/bin/exe --foo \"a b\"" "--""#), + format!(r#""{sh}" "-c" "~/bin/exe --foo \"a b\"" "--""#), "this always needs a shell as we need tilde expansion" ); } @@ -419,9 +425,10 @@ mod prepare { .command_may_be_shell_script() .arg("store"), ); + let sh = *SH; assert_eq!( format!("{cmd:?}"), - format!(r#""{SH}" "-c" "echo \"$@\" >&2" "--" "store""#), + format!(r#""{sh}" "-c" "echo \"$@\" >&2" "--" "store""#), "this is how credential helpers have to work as for some reason they don't get '$@' added in Git.\ We deal with it by not doubling the '$@' argument, which seems more flexible." ); @@ -435,9 +442,10 @@ mod prepare { .with_quoted_command() .arg("store"), ); + let sh = *SH; assert_eq!( format!("{cmd:?}"), - format!(r#""{SH}" "-c" "echo \"$@\" >&2" "--" "store""#) + format!(r#""{sh}" "-c" "echo \"$@\" >&2" "--" "store""#) ); } } From 872159e7ee13bf505b6ec5319968efbc6af70c79 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Mon, 17 Feb 2025 13:49:44 -0500 Subject: [PATCH 2/4] Adjust `gix-credentials` tests for `gix-command` changes Because `gix-command` uses `gix_path::env::shell()` to find sh, and `gix-credentials` uses `gix-command`. --- .../tests/program/from_custom_definition.rs | 26 ++++++++++++------- 1 file changed, 17 insertions(+), 9 deletions(-) diff --git a/gix-credentials/tests/program/from_custom_definition.rs b/gix-credentials/tests/program/from_custom_definition.rs index 25e9dd2ae4b..c746e0b48af 100644 --- a/gix-credentials/tests/program/from_custom_definition.rs +++ b/gix-credentials/tests/program/from_custom_definition.rs @@ -1,12 +1,17 @@ use gix_credentials::{helper, program::Kind, Program}; +use once_cell::sync::Lazy; -static GIT: once_cell::sync::Lazy<&'static str> = - once_cell::sync::Lazy::new(|| gix_path::env::exe_invocation().to_str().expect("not illformed")); +static GIT: once_cell::sync::Lazy<&'static str> = once_cell::sync::Lazy::new(|| { + gix_path::env::exe_invocation() + .to_str() + .expect("some `from_custom_definition` tests must be run where 'git' path is valid Unicode") +}); -#[cfg(windows)] -const SH: &str = "sh"; -#[cfg(not(windows))] -const SH: &str = "/bin/sh"; +static SH: Lazy<&'static str> = Lazy::new(|| { + gix_path::env::shell() + .to_str() + .expect("some `from_custom_definition` tests must be run where 'sh' path is valid Unicode") +}); #[test] fn empty() { @@ -47,11 +52,12 @@ fn name_with_args() { fn name_with_special_args() { let input = "name --arg --bar=~/folder/in/home"; let prog = Program::from_custom_definition(input); + let sh = *SH; let git = *GIT; assert!(matches!(&prog.kind, Kind::ExternalName{name_and_args} if name_and_args == input)); assert_eq!( format!("{:?}", prog.to_command(&helper::Action::Store("egal".into()))), - format!(r#""{SH}" "-c" "{git} credential-name --arg --bar=~/folder/in/home \"$@\"" "--" "store""#) + format!(r#""{sh}" "-c" "{git} credential-name --arg --bar=~/folder/in/home \"$@\"" "--" "store""#) ); } @@ -73,12 +79,13 @@ fn path_with_args_that_definitely_need_shell() { let input = "/abs/name --arg --bar=\"a b\""; let prog = Program::from_custom_definition(input); assert!(matches!(&prog.kind, Kind::ExternalPath{path_and_args} if path_and_args == input)); + let sh = *SH; assert_eq!( format!("{:?}", prog.to_command(&helper::Action::Store("egal".into()))), if cfg!(windows) { r#""/abs/name" "--arg" "--bar=a b" "store""#.to_owned() } else { - format!(r#""{SH}" "-c" "/abs/name --arg --bar=\"a b\" \"$@\"" "--" "store""#) + format!(r#""{sh}" "-c" "/abs/name --arg --bar=\"a b\" \"$@\"" "--" "store""#) } ); } @@ -100,12 +107,13 @@ fn path_with_simple_args() { let input = "/abs/name a b"; let prog = Program::from_custom_definition(input); assert!(matches!(&prog.kind, Kind::ExternalPath{path_and_args} if path_and_args == input)); + let sh = *SH; assert_eq!( format!("{:?}", prog.to_command(&helper::Action::Store("egal".into()))), if cfg!(windows) { r#""/abs/name" "a" "b" "store""#.to_owned() } else { - format!(r#""{SH}" "-c" "/abs/name a b \"$@\"" "--" "store""#) + format!(r#""{sh}" "-c" "/abs/name a b \"$@\"" "--" "store""#) }, "a shell is used as there are arguments, and it's generally more flexible, but on windows we split ourselves" ); From b7029f8f6aa04f0930d5d2e127726b1484c084f7 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Mon, 17 Feb 2025 16:11:42 -0500 Subject: [PATCH 3/4] fix: Use `/` in `gix_path::env::shell()` and check existence This makes the path returned by `gix_path::env::shell()` on Windows more usable by: 1. Adding components with `/` separators. While in principle a `\` should work, the path of the shell itself is used in shell scripts (script files and `sh -c` operands) that may not account for the presence of backslashes, and it is also harder to read paths with `\` in contexts where it appears escaped, which may include various messages from Rust code and shell scripts. The path before what we add will already use `/` and never `\`, unless `GIT_EXEC_PATH` has been set to a strange value, because it is based on `git --exec-path`, which by default gives a path with `/` separators. Thus, ensuring that the part we add uses `/` should be sufficient to get a path without `\` in all cases when it is clearly reasonable to do so. This therefore also usually increases stylistic consistency of the path, which is another factor that makes it more user-friendly in messages. This is needed to get tests to pass since changing `gix-command` to use `gix_path::env::shell()` on Windows, where a path is formatted in away that sometimes quotes `\` characters. Their expectations could be adjusted, but it seems likely that various other software, much of which may otherwise be working, has similar expectations. Using `/` instead of `\` works whether `\` is expected to be displayed quoted or not. 2. Check that the path to the shell plausibly has a shell there, only using it if it a file or a non-broken file symlink. When this is not the case, the fallback short name is used instead. 3. The fallback short name is changed from `sh` to `sh.exe`, since the `.exe` suffix is appended in other short names on Windows, such as `git.exe`, as well as being part of the filename component of the path we build for the shell when using the implementation provided as part of Git for Windows. Those changes only affect Windows. This also adds tests for (1) and (2) above, as well as for the expectation that we get an absolute path, to make sure we don't build a path that would be absolute on a Unix-like system but is relative on Windows (a path that starts with just one `/` or `\`). These tests are not Windows-specific, since all these expectations should already hold on Unix-like systems, where currently we are using the hard-coded path `/bin/sh`, which is an absolute path on those systems. (Some Unix-like systems may technically not have `/bin/sh` or it may not be the best path to use for a shell that should be POSIX-compatible, but we are already relying on this, and handling that better is outside the scope of the changes here.) --- gix-path/src/env/mod.rs | 33 +++++++++++++++++++++++++-------- gix-path/tests/path/env.rs | 25 +++++++++++++++++++++++-- 2 files changed, 48 insertions(+), 10 deletions(-) diff --git a/gix-path/src/env/mod.rs b/gix-path/src/env/mod.rs index 71188fb2a17..333527e5eb7 100644 --- a/gix-path/src/env/mod.rs +++ b/gix-path/src/env/mod.rs @@ -30,18 +30,35 @@ pub fn installation_config_prefix() -> Option<&'static Path> { /// Return the shell that Git would use, the shell to execute commands from. /// -/// 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. +/// On Windows, this is the full path to `sh.exe` bundled with Git for Windows if we can find it. +/// If the bundled shell on Windows cannot be found, `sh.exe` is returned as the name of a shell, +/// as it could possibly be found in `PATH`. On Unix it's `/bin/sh` as the POSIX-compatible shell. +/// +/// Note that the returned path might not be a path on disk, if it is a fallback path or if the +/// file was moved or deleted since the first time this function is called. pub fn shell() -> &'static OsStr { static PATH: Lazy = Lazy::new(|| { if cfg!(windows) { 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) + .and_then(|git_core| { + // Go up above something that is expected to be like mingw64/libexec/git-core. + git_core.ancestors().nth(3) + }) + .map(OsString::from) + .map(|mut raw_path| { + // Go down to where `sh.exe` usually is. To avoid breaking shell scripts that + // wrongly assume the shell's own path contains no `\`, as well as to produce + // more readable messages, append literally with `/` separators. The path from + // `git --exec-path` will already have all `/` separators (and no trailing `/`) + // unless it was explicitly overridden to an unusual value via `GIT_EXEC_PATH`. + raw_path.push("/usr/bin/sh.exe"); + raw_path + }) + .filter(|raw_path| { + // Check if there is something that could be a usable shell there. + Path::new(raw_path).is_file() + }) + .unwrap_or_else(|| "sh.exe".into()) } else { "/bin/sh".into() } diff --git a/gix-path/tests/path/env.rs b/gix-path/tests/path/env.rs index 28f23f9cb7b..c3c7019cbce 100644 --- a/gix-path/tests/path/env.rs +++ b/gix-path/tests/path/env.rs @@ -1,3 +1,5 @@ +use std::path::Path; + #[test] fn exe_invocation() { let actual = gix_path::env::exe_invocation(); @@ -10,8 +12,27 @@ fn exe_invocation() { #[test] 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" + Path::new(gix_path::env::shell()).exists(), + "On CI and on Unix we expect a usable path to the shell that exists on disk" + ); +} + +#[test] +fn shell_absolute() { + assert!( + Path::new(gix_path::env::shell()).is_absolute(), + "On CI and on Unix we currently expect the path to the shell always to be absolute" + ); +} + +#[test] +fn shell_unix_path() { + let shell = gix_path::env::shell() + .to_str() + .expect("This test depends on the shell path being valid Unicode"); + assert!( + !shell.contains('\\'), + "The path to the shell should have no backslashes, barring strange `GIT_EXEC_PATH` values" ); } From 65f706b256cdee068d0be64b1bcc3957332e307c Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Mon, 17 Feb 2025 16:30:59 -0500 Subject: [PATCH 4/4] Revise `gix_path::env` docstrings for clarity --- gix-path/src/env/mod.rs | 39 +++++++++++++++++++++------------------ 1 file changed, 21 insertions(+), 18 deletions(-) diff --git a/gix-path/src/env/mod.rs b/gix-path/src/env/mod.rs index 333527e5eb7..78e2da294c4 100644 --- a/gix-path/src/env/mod.rs +++ b/gix-path/src/env/mod.rs @@ -67,14 +67,16 @@ pub fn shell() -> &'static OsStr { } /// Return the name of the Git executable to invoke it. +/// /// If it's in the `PATH`, it will always be a short name. /// /// Note that on Windows, we will find the executable in the `PATH` if it exists there, or search it /// in alternative locations which when found yields the full path to it. pub fn exe_invocation() -> &'static Path { if cfg!(windows) { - /// The path to the Git executable as located in the `PATH` or in other locations that it's known to be installed to. - /// It's `None` if environment variables couldn't be read or if no executable could be found. + /// The path to the Git executable as located in the `PATH` or in other locations that it's + /// known to be installed to. It's `None` if environment variables couldn't be read or if + /// no executable could be found. static EXECUTABLE_PATH: Lazy> = Lazy::new(|| { std::env::split_paths(&std::env::var_os("PATH")?) .chain(git::ALTERNATIVE_LOCATIONS.iter().map(Into::into)) @@ -99,11 +101,11 @@ pub fn exe_invocation() -> &'static Path { } } -/// Returns the fully qualified path in the *xdg-home* directory (or equivalent in the home dir) to `file`, -/// accessing `env_var()` to learn where these bases are. +/// Returns the fully qualified path in the *xdg-home* directory (or equivalent in the home dir) to +/// `file`, accessing `env_var()` to learn where these bases are. /// -/// Note that the `HOME` directory should ultimately come from [`home_dir()`] as it handles windows correctly. -/// The same can be achieved by using [`var()`] as `env_var`. +/// Note that the `HOME` directory should ultimately come from [`home_dir()`] as it handles Windows +/// correctly. The same can be achieved by using [`var()`] as `env_var`. pub fn xdg_config(file: &str, env_var: &mut dyn FnMut(&str) -> Option) -> Option { env_var("XDG_CONFIG_HOME") .map(|home| { @@ -153,17 +155,17 @@ 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). +/// Returns the platform dependent system prefix or `None` if it cannot be found (right now only on Windows). /// /// ### Performance /// -/// On windows, the slowest part is the launch of the Git executable in the PATH, which only happens when launched -/// from outside of the `msys2` shell. +/// On Windows, the slowest part is the launch of the Git executable in the PATH. This is often +/// avoided by inspecting the environment, when launched from inside a Git Bash MSYS2 shell. /// /// ### When `None` is returned /// -/// This happens only windows if the git binary can't be found at all for obtaining its executable path, or if the git binary -/// wasn't built with a well-known directory structure or environment. +/// This happens only Windows if the git binary can't be found at all for obtaining its executable +/// path, or if the git binary wasn't built with a well-known directory structure or environment. pub fn system_prefix() -> Option<&'static Path> { if cfg!(windows) { static PREFIX: Lazy> = Lazy::new(|| { @@ -194,19 +196,20 @@ pub fn home_dir() -> Option { std::env::var("HOME").map(PathBuf::from).ok() } -/// Tries to obtain the home directory from `HOME` on all platforms, but falls back to [`home::home_dir()`] for -/// more complex ways of obtaining a home directory, particularly useful on Windows. +/// Tries to obtain the home directory from `HOME` on all platforms, but falls back to +/// [`home::home_dir()`] for more complex ways of obtaining a home directory, particularly useful +/// on Windows. /// -/// The reason `HOME` is tried first is to allow Windows users to have a custom location for their linux-style -/// home, as otherwise they would have to accumulate dot files in a directory these are inconvenient and perceived -/// as clutter. +/// The reason `HOME` is tried first is to allow Windows users to have a custom location for their +/// linux-style home, as otherwise they would have to accumulate dot files in a directory these are +/// inconvenient and perceived as clutter. #[cfg(not(target_family = "wasm"))] pub fn home_dir() -> Option { std::env::var_os("HOME").map(Into::into).or_else(home::home_dir) } -/// Returns the contents of an environment variable of `name` with some special handling -/// for certain environment variables (like `HOME`) for platform compatibility. +/// Returns the contents of an environment variable of `name` with some special handling for +/// certain environment variables (like `HOME`) for platform compatibility. pub fn var(name: &str) -> Option { if name == "HOME" { home_dir().map(PathBuf::into_os_string)