From c2020848f1be737c06669aeb977383559077b446 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Wed, 11 Sep 2024 02:40:05 -0400 Subject: [PATCH 1/3] Broaden `args` param of `configure_command` This makes the private `gix_testtools::configure_command` function generic (in type as well as lifetime) to accept anything as `args` that the `args` method of `std::process::Command` accepts. This also uses the broader parameter type to simplify a test. --- tests/tools/src/lib.rs | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/tests/tools/src/lib.rs b/tests/tools/src/lib.rs index d8b5bd48201..6d156f66697 100644 --- a/tests/tools/src/lib.rs +++ b/tests/tools/src/lib.rs @@ -11,7 +11,7 @@ use std::{ collections::BTreeMap, env, - ffi::OsString, + ffi::{OsStr, OsString}, io::Read, path::{Path, PathBuf}, str::FromStr, @@ -589,11 +589,15 @@ const NULL_DEVICE: &str = "NUL"; #[cfg(not(windows))] const NULL_DEVICE: &str = "/dev/null"; -fn configure_command<'a>( +fn configure_command<'a, I, S>( cmd: &'a mut std::process::Command, - args: &[String], + args: I, script_result_directory: &Path, -) -> &'a mut std::process::Command { +) -> &'a mut std::process::Command +where + I: IntoIterator, + S: AsRef, +{ let mut msys_for_git_bash_on_windows = env::var_os("MSYS").unwrap_or_default(); msys_for_git_bash_on_windows.push(" winsymlinks:nativestrict"); cmd.args(args) @@ -925,10 +929,9 @@ mod tests { populate_ad_hoc_config_files(temp.path()); let mut cmd = std::process::Command::new("git"); - let args = ["config", "-l", "--show-origin"].map(String::from); cmd.env("GIT_CONFIG_SYSTEM", SCOPE_ENV_VALUE); cmd.env("GIT_CONFIG_GLOBAL", SCOPE_ENV_VALUE); - configure_command(&mut cmd, &args, temp.path()); + configure_command(&mut cmd, ["config", "-l", "--show-origin"], temp.path()); let output = cmd.output().expect("can run git"); let lines: Vec<_> = output From 0e1e6a9bba4390906326c0a074f082e93b8345fe Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Wed, 11 Sep 2024 05:00:06 -0400 Subject: [PATCH 2/3] Use more compact notation I'm not sure if this is better, but the new genericity distracts less from what the function is really about when prsented this way. --- tests/tools/src/lib.rs | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/tests/tools/src/lib.rs b/tests/tools/src/lib.rs index 6d156f66697..231a87b5eaa 100644 --- a/tests/tools/src/lib.rs +++ b/tests/tools/src/lib.rs @@ -589,15 +589,11 @@ const NULL_DEVICE: &str = "NUL"; #[cfg(not(windows))] const NULL_DEVICE: &str = "/dev/null"; -fn configure_command<'a, I, S>( +fn configure_command<'a, I: IntoIterator, S: AsRef>( cmd: &'a mut std::process::Command, args: I, script_result_directory: &Path, -) -> &'a mut std::process::Command -where - I: IntoIterator, - S: AsRef, -{ +) -> &'a mut std::process::Command { let mut msys_for_git_bash_on_windows = env::var_os("MSYS").unwrap_or_default(); msys_for_git_bash_on_windows.push(" winsymlinks:nativestrict"); cmd.args(args) From 4a25ef5030a868164eaf12ffe9603fbcc1a89d01 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Wed, 11 Sep 2024 05:09:16 -0400 Subject: [PATCH 3/3] Note that how we set `MSYS` ignores `env`/`env_remove` calls This problem can be avoided by a design that doesn't create the assumption that such calls would have an effect, or that prevents them from being made (e.g. a builder funtion), or by accounting for and honoring them (which may involve reimplementing a fragment of the Windows API based case-folding that `std::process::Command` uses). But none of these approaches really seems overall better than what we are doing now. So for now, keep the design but note this one confusing spot. This is a private function, so I've just commented by where the relevant logic is, rather than using a `///` comment. --- tests/tools/src/lib.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tests/tools/src/lib.rs b/tests/tools/src/lib.rs index 231a87b5eaa..47d68e3f979 100644 --- a/tests/tools/src/lib.rs +++ b/tests/tools/src/lib.rs @@ -594,6 +594,9 @@ fn configure_command<'a, I: IntoIterator, S: AsRef>( args: I, script_result_directory: &Path, ) -> &'a mut std::process::Command { + // For simplicity, we extend the `MSYS` variable from from our own environment. This disregards + // state from any prior `cmd.env("MSYS")` or `cmd.env_remove("MSYS")` calls. Such calls should + // either be avoided, or made after this function returns (but before spawning the command). let mut msys_for_git_bash_on_windows = env::var_os("MSYS").unwrap_or_default(); msys_for_git_bash_on_windows.push(" winsymlinks:nativestrict"); cmd.args(args)