From 01737ad7b76f16d84c2d8a43d70e1c92f8514867 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Sat, 30 Nov 2024 12:35:57 -0500 Subject: [PATCH 1/6] Rename `parse_gix_version` to `parse_git_version` The function checks the `git` version. It was accidentally renamed to `parse_gix_version` in 00286c9 along with other `git` -> `gix` changes for gitoxide crates that were renamed `git-*` to `gix-*`. This also makes the same correction to a local variable. --- tests/tools/src/lib.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/tools/src/lib.rs b/tests/tools/src/lib.rs index 27fb06d5006..baddccb6769 100644 --- a/tests/tools/src/lib.rs +++ b/tests/tools/src/lib.rs @@ -87,7 +87,7 @@ static EXCLUDE_LUT: Lazy>> = Lazy::new(|| { Mutex::new(cache) }); /// The major, minor and patch level of the git version on the system. -pub static GIT_VERSION: Lazy<(u8, u8, u8)> = Lazy::new(|| parse_gix_version().expect("git version to be parsable")); +pub static GIT_VERSION: Lazy<(u8, u8, u8)> = Lazy::new(|| parse_git_version().expect("git version to be parsable")); /// Define how [`scripted_fixture_writable_with_args()`] uses produces the writable copy. pub enum Creation { @@ -116,9 +116,9 @@ pub fn should_skip_as_git_version_is_smaller_than(major: u8, minor: u8, patch: u *GIT_VERSION < (major, minor, patch) } -fn parse_gix_version() -> Result<(u8, u8, u8)> { - let gix_program = cfg!(windows).then(|| "git.exe").unwrap_or("git"); - let output = std::process::Command::new(gix_program).arg("--version").output()?; +fn parse_git_version() -> Result<(u8, u8, u8)> { + let git_program = cfg!(windows).then(|| "git.exe").unwrap_or("git"); + let output = std::process::Command::new(git_program).arg("--version").output()?; git_version_from_bytes(&output.stdout) } From e30c0700c3e64b87c20b4695ccfb41f05f961129 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Sat, 30 Nov 2024 13:08:51 -0500 Subject: [PATCH 2/6] Use consistent `git` command name in gix-testtools In the test tools, this always runs the `git` program as `git.exe` on Windows, while continuing always to run it as `git` on other systems. Prior to this change, on Windows `gix-testtools` used `git` in some operations and `git.exe` in others: - `parse_git_version` used `git.exe`. - Other functions used `git`. - For the git daemon, `git-daemon.exe` was used. For the way `gix-testtools` uses the `git` program, it would be fine to call it `git` on all platforms. For example, it does not form full paths to the executable that have to be found to exist in operations other than running it. (For running it, the `.exe` suffix is allowed to be omitted.) So it would probably be fine to use the even simpler logic of having it be `git` everywhere. But since `git.exe` was sometimes used, `git-daemon.exe` was used, and using `git.exe` is more similar to the behavior in `git-path`, it is changed to use `git.exe` when the platform is Windows. Because `gix-testtools` does not depend on `gix-path`, it doesn't use `gix_path::env::exe_invocation` to decide how to call `git`. That keeps it from finding `git` in some Windows environments (those where `git` is in a standard/predictable location but not in `PATH`). This change has no effect on that limitation. --- tests/tools/src/lib.rs | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/tests/tools/src/lib.rs b/tests/tools/src/lib.rs index baddccb6769..ef1742a7329 100644 --- a/tests/tools/src/lib.rs +++ b/tests/tools/src/lib.rs @@ -59,6 +59,7 @@ impl Drop for GitDaemon { } static SCRIPT_IDENTITY: Lazy>> = Lazy::new(|| Mutex::new(BTreeMap::new())); + static EXCLUDE_LUT: Lazy>> = Lazy::new(|| { let cache = (|| { let (repo_path, _) = gix_discover::upwards(Path::new(".")).ok()?; @@ -86,6 +87,12 @@ static EXCLUDE_LUT: Lazy>> = Lazy::new(|| { })(); Mutex::new(cache) }); + +#[cfg(windows)] +const GIT_PROGRAM: &str = "git.exe"; +#[cfg(not(windows))] +const GIT_PROGRAM: &str = "git"; + /// The major, minor and patch level of the git version on the system. pub static GIT_VERSION: Lazy<(u8, u8, u8)> = Lazy::new(|| parse_git_version().expect("git version to be parsable")); @@ -117,9 +124,7 @@ pub fn should_skip_as_git_version_is_smaller_than(major: u8, minor: u8, patch: u } fn parse_git_version() -> Result<(u8, u8, u8)> { - let git_program = cfg!(windows).then(|| "git.exe").unwrap_or("git"); - let output = std::process::Command::new(git_program).arg("--version").output()?; - + let output = std::process::Command::new(GIT_PROGRAM).arg("--version").output()?; git_version_from_bytes(&output.stdout) } @@ -173,7 +178,7 @@ impl Drop for AutoRevertToPreviousCWD { /// Run `git` in `working_dir` with all provided `args`. pub fn run_git(working_dir: &Path, args: &[&str]) -> std::io::Result { - std::process::Command::new("git") + std::process::Command::new(GIT_PROGRAM) .current_dir(working_dir) .args(args) .status() @@ -182,7 +187,7 @@ pub fn run_git(working_dir: &Path, args: &[&str]) -> std::io::Result) -> std::io::Result { static EXEC_PATH: Lazy = Lazy::new(|| { - let path = std::process::Command::new("git") + let path = std::process::Command::new(GIT_PROGRAM) .arg("--exec-path") .stderr(std::process::Stdio::null()) .output() @@ -738,7 +743,7 @@ fn populate_meta_dir(destination_dir: &Path, script_identity: u32) -> std::io::R )?; std::fs::write( meta_dir.join(META_GIT_VERSION), - std::process::Command::new("git").arg("--version").output()?.stdout, + std::process::Command::new(GIT_PROGRAM).arg("--version").output()?.stdout, )?; Ok(meta_dir) } @@ -951,7 +956,7 @@ mod tests { let temp = tempfile::TempDir::new().expect("can create temp dir"); populate_ad_hoc_config_files(temp.path()); - let mut cmd = std::process::Command::new("git"); + let mut cmd = std::process::Command::new(GIT_PROGRAM); cmd.env("GIT_CONFIG_SYSTEM", SCOPE_ENV_VALUE); cmd.env("GIT_CONFIG_GLOBAL", SCOPE_ENV_VALUE); configure_command(&mut cmd, ["config", "-l", "--show-origin"], temp.path()); From da0393213dd0b08958da847e856b96028d038b46 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Sat, 30 Nov 2024 14:13:15 -0500 Subject: [PATCH 3/6] Run `cargo fmt` --- tests/tools/src/lib.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/tests/tools/src/lib.rs b/tests/tools/src/lib.rs index ef1742a7329..2ae947f98b3 100644 --- a/tests/tools/src/lib.rs +++ b/tests/tools/src/lib.rs @@ -743,7 +743,10 @@ fn populate_meta_dir(destination_dir: &Path, script_identity: u32) -> std::io::R )?; std::fs::write( meta_dir.join(META_GIT_VERSION), - std::process::Command::new(GIT_PROGRAM).arg("--version").output()?.stdout, + std::process::Command::new(GIT_PROGRAM) + .arg("--version") + .output()? + .stdout, )?; Ok(meta_dir) } From 479c06b372224a33d29e8b12fd59d96ab29fc60f Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Sat, 30 Nov 2024 15:42:42 -0500 Subject: [PATCH 4/6] Refine `EXEC_PATH` validation in `spawn_git_daemon` This enforces requirements more precisely and, overall, more robustly, when validating and converting `git --exec-path` output to a `PathBuf`. This also no longer redirect standard error to the null device when running that command. No stderr output is expected; if there is any such output, it's better seen than thrown away. --- tests/tools/src/lib.rs | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/tests/tools/src/lib.rs b/tests/tools/src/lib.rs index 2ae947f98b3..9570c0a3716 100644 --- a/tests/tools/src/lib.rs +++ b/tests/tools/src/lib.rs @@ -187,14 +187,19 @@ pub fn run_git(working_dir: &Path, args: &[&str]) -> std::io::Result) -> std::io::Result { static EXEC_PATH: Lazy = Lazy::new(|| { - let path = std::process::Command::new(GIT_PROGRAM) + let output = std::process::Command::new(GIT_PROGRAM) .arg("--exec-path") - .stderr(std::process::Stdio::null()) .output() - .expect("can execute `git --exec-path`") - .stdout; - String::from_utf8(path.trim().into()) - .expect("no invalid UTF8 in exec-path") + .expect("can execute `git --exec-path`"); + + assert!(output.status.success(), "`git --exec-path` failed"); + + output + .stdout + .strip_suffix(b"\n") + .expect("malformed output from `git --exec-path`") + .to_os_str() + .expect("no invalid UTF-8 in `--exec-path` except as OS allows") .into() }); let mut ports: Vec<_> = (9419u16..9419 + 100).collect(); From fe3f2d128a1478af97999025b46c7b146e778524 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Sat, 30 Nov 2024 15:47:02 -0500 Subject: [PATCH 5/6] fix: Run test fixture scripts on Windows with Git Bash Rather than hard-coding `bash` on all systems as the fallback interpreter when a fixture script cannot be run directly, this falls 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 slightly significant: we check for the existence of the interpreter without initially running it, and that check requires the full filename. It is called `bash.exe` elsewhere for consistency both with the checked-for executable and for consistencey with how we run most other programs on Windows, e.g., the `git` vs. `git.exe`.) This fixes #1359. That bug is not currently observed on CI, but this change is verified to fix it on a local test system where it previously always occurred when running the test suite from PowerShell in an unmodified environment. The fix applies both with `GIX_TEST_IGNORE_ARCHIVES` unset, in which case there are now no failures, and with `GIX_TEST_IGNORE_ARCHIVES=1`, in which case the failures are now limited to the 15 cases tracked in #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 of #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, see #1359 and comments including: https://github.com/GitoxideLabs/gitoxide/issues/1359#issuecomment-2316614616 On the approach of finding the Git for Windows `bash.exe` relative to the `git-core` directory, see the GitPython pull request https://github.com/gitpython-developers/GitPython/pull/1791, its comments, and the implementation of the approach by @emanspeaks: https://github.com/gitpython-developers/GitPython/blob/f065d1fba422a528a133719350e027f1241273df/git/cmd.py#L398-L403 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. --- tests/tools/src/lib.rs | 101 ++++++++++++++++++++++++++++++++--------- 1 file changed, 79 insertions(+), 22 deletions(-) diff --git a/tests/tools/src/lib.rs b/tests/tools/src/lib.rs index 9570c0a3716..6f37ca5ff7a 100644 --- a/tests/tools/src/lib.rs +++ b/tests/tools/src/lib.rs @@ -93,6 +93,23 @@ const GIT_PROGRAM: &str = "git.exe"; #[cfg(not(windows))] const GIT_PROGRAM: &str = "git"; +static GIT_CORE_DIR: Lazy = Lazy::new(|| { + let output = std::process::Command::new(GIT_PROGRAM) + .arg("--exec-path") + .output() + .expect("can execute `git --exec-path`"); + + assert!(output.status.success(), "`git --exec-path` failed"); + + output + .stdout + .strip_suffix(b"\n") + .expect("malformed output from `git --exec-path`") + .to_os_str() + .expect("no invalid UTF-8 in `--exec-path` except as OS allows") + .into() +}); + /// The major, minor and patch level of the git version on the system. pub static GIT_VERSION: Lazy<(u8, u8, u8)> = Lazy::new(|| parse_git_version().expect("git version to be parsable")); @@ -186,22 +203,6 @@ pub fn run_git(working_dir: &Path, args: &[&str]) -> std::io::Result) -> std::io::Result { - static EXEC_PATH: Lazy = Lazy::new(|| { - let output = std::process::Command::new(GIT_PROGRAM) - .arg("--exec-path") - .output() - .expect("can execute `git --exec-path`"); - - assert!(output.status.success(), "`git --exec-path` failed"); - - output - .stdout - .strip_suffix(b"\n") - .expect("malformed output from `git --exec-path`") - .to_os_str() - .expect("no invalid UTF-8 in `--exec-path` except as OS allows") - .into() - }); let mut ports: Vec<_> = (9419u16..9419 + 100).collect(); fastrand::shuffle(&mut ports); let addr_at = |port| std::net::SocketAddr::from(([127, 0, 0, 1], port)); @@ -210,11 +211,12 @@ pub fn spawn_git_daemon(working_dir: impl AsRef) -> std::io::Result { - cmd = std::process::Command::new("bash"); + cmd = std::process::Command::new(bash_program()); configure_command(cmd.arg(script_absolute_path), &args, &script_result_directory).output()? } Err(err) => return Err(err.into()), @@ -642,6 +644,22 @@ fn configure_command<'a, I: IntoIterator, S: AsRef>( .env("GIT_CONFIG_VALUE_3", "always") } +fn bash_program() -> &'static Path { + if cfg!(windows) { + static GIT_BASH: Lazy> = Lazy::new(|| { + GIT_CORE_DIR + .parent()? + .parent()? + .parent() + .map(|installation_dir| installation_dir.join("bin").join("bash.exe")) + .filter(|bash| bash.is_file()) + }); + GIT_BASH.as_deref().unwrap_or(Path::new("bash.exe")) + } else { + Path::new("bash") + } +} + fn write_failure_marker(failure_marker: &Path) { std::fs::write(failure_marker, []).ok(); } @@ -981,4 +999,43 @@ mod tests { assert_eq!(lines, Vec::<&str>::new(), "should be no config variables from files"); assert_eq!(status, 0, "reading the config should succeed"); } + + #[test] + #[cfg(windows)] + fn bash_program_ok_for_platform() { + let path = bash_program(); + assert!(path.is_absolute()); + + let for_version = std::process::Command::new(path) + .arg("--version") + .output() + .expect("can pass it `--version`"); + assert!(for_version.status.success(), "passing `--version` succeeds"); + let version_line = for_version + .stdout + .lines() + .nth(0) + .expect("`--version` output has first line"); + assert!( + version_line.ends_with(b"-pc-msys)"), // On Windows, "-pc-linux-gnu)" would be WSL. + "it is an MSYS bash (such as Git Bash)" + ); + + let for_uname_os = std::process::Command::new(path) + .args(["-c", "uname -o"]) + .output() + .expect("can tell it to run `uname -o`"); + assert!(for_uname_os.status.success(), "telling it to run `uname -o` succeeds"); + assert_eq!( + for_uname_os.stdout.trim_end(), + b"Msys", + "it runs commands in an MSYS environment" + ); + } + + #[test] + #[cfg(not(windows))] + fn bash_program_ok_for_platform() { + assert_eq!(bash_program(), Path::new("bash")); + } } From 1f6a8669a64b15fbe7021c6906f88f5b7c7c142e Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Sun, 1 Dec 2024 09:32:03 -0500 Subject: [PATCH 6/6] Fix an ambiguous `expect` message --- tests/tools/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/tools/src/lib.rs b/tests/tools/src/lib.rs index 6f37ca5ff7a..ad5c8c46fd4 100644 --- a/tests/tools/src/lib.rs +++ b/tests/tools/src/lib.rs @@ -104,7 +104,7 @@ static GIT_CORE_DIR: Lazy = Lazy::new(|| { output .stdout .strip_suffix(b"\n") - .expect("malformed output from `git --exec-path`") + .expect("`git --exec-path` output to be well-formed") .to_os_str() .expect("no invalid UTF-8 in `--exec-path` except as OS allows") .into()