From 9eaa0d90470192104def649d0e317b0b55f0d7a6 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Thu, 11 Jul 2024 18:17:50 -0400 Subject: [PATCH 01/34] Make ALTERNATIVE_LOCATIONS dynamic and OsString-based This is to support a forthcoming fix where the paths will no longer be hard-coded. --- gix-path/src/env/git.rs | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/gix-path/src/env/git.rs b/gix-path/src/env/git.rs index 59996827961..dcb7f6fef9c 100644 --- a/gix-path/src/env/git.rs +++ b/gix-path/src/env/git.rs @@ -1,5 +1,6 @@ use std::path::PathBuf; use std::{ + ffi::OsString, path::Path, process::{Command, Stdio}, }; @@ -8,12 +9,14 @@ use bstr::{BStr, BString, ByteSlice}; /// Other places to find Git in. #[cfg(windows)] -pub(super) static ALTERNATIVE_LOCATIONS: &[&str] = &[ - "C:/Program Files/Git/mingw64/bin", - "C:/Program Files (x86)/Git/mingw32/bin", -]; +pub(super) static ALTERNATIVE_LOCATIONS: once_cell::sync::Lazy> = once_cell::sync::Lazy::new(|| { + vec![ + "C:/Program Files/Git/mingw64/bin".into(), + "C:/Program Files (x86)/Git/mingw32/bin".into(), + ] +}); #[cfg(not(windows))] -pub(super) static ALTERNATIVE_LOCATIONS: &[&str] = &[]; +pub(super) static ALTERNATIVE_LOCATIONS: Lazy> = Lazy::new(|| vec![]); #[cfg(windows)] pub(super) static EXE_NAME: &str = "git.exe"; @@ -35,7 +38,7 @@ pub(super) static EXE_INFO: once_cell::sync::Lazy> = once_cell:: Ok(out) => out.stdout, #[cfg(windows)] Err(err) if err.kind() == std::io::ErrorKind::NotFound => { - let executable = ALTERNATIVE_LOCATIONS.into_iter().find_map(|prefix| { + let executable = ALTERNATIVE_LOCATIONS.iter().find_map(|prefix| { let candidate = Path::new(prefix).join(EXE_NAME); candidate.is_file().then_some(candidate) })?; From 98b3d901bd770cc8cf178fce52695306ca48f403 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Thu, 11 Jul 2024 18:46:27 -0400 Subject: [PATCH 02/34] Restyle just-changed and related code for clarity --- gix-path/src/env/git.rs | 16 +++++++--------- gix-path/src/env/mod.rs | 14 +++++++------- 2 files changed, 14 insertions(+), 16 deletions(-) diff --git a/gix-path/src/env/git.rs b/gix-path/src/env/git.rs index dcb7f6fef9c..d6c446a74ab 100644 --- a/gix-path/src/env/git.rs +++ b/gix-path/src/env/git.rs @@ -1,15 +1,13 @@ -use std::path::PathBuf; -use std::{ - ffi::OsString, - path::Path, - process::{Command, Stdio}, -}; +use std::ffi::OsString; +use std::path::{Path, PathBuf}; +use std::process::{Command, Stdio}; use bstr::{BStr, BString, ByteSlice}; +use once_cell::sync::Lazy; /// Other places to find Git in. #[cfg(windows)] -pub(super) static ALTERNATIVE_LOCATIONS: once_cell::sync::Lazy> = once_cell::sync::Lazy::new(|| { +pub(super) static ALTERNATIVE_LOCATIONS: Lazy> = Lazy::new(|| { vec![ "C:/Program Files/Git/mingw64/bin".into(), "C:/Program Files (x86)/Git/mingw32/bin".into(), @@ -24,7 +22,7 @@ pub(super) static EXE_NAME: &str = "git.exe"; pub(super) static EXE_NAME: &str = "git"; /// Invoke the git executable in PATH to obtain the origin configuration, which is cached and returned. -pub(super) static EXE_INFO: once_cell::sync::Lazy> = once_cell::sync::Lazy::new(|| { +pub(super) static EXE_INFO: Lazy> = Lazy::new(|| { let git_cmd = |executable: PathBuf| { let mut cmd = Command::new(executable); cmd.args(["config", "-l", "--show-origin"]) @@ -55,7 +53,7 @@ pub(super) static EXE_INFO: once_cell::sync::Lazy> = once_cell:: /// if no `git` executable was found or there were other errors during execution. pub(super) fn install_config_path() -> Option<&'static BStr> { let _span = gix_trace::detail!("gix_path::git::install_config_path()"); - static PATH: once_cell::sync::Lazy> = once_cell::sync::Lazy::new(|| { + static PATH: Lazy> = Lazy::new(|| { // Shortcut: in Msys shells this variable is set which allows to deduce the installation directory, // so we can save the `git` invocation. #[cfg(windows)] diff --git a/gix-path/src/env/mod.rs b/gix-path/src/env/mod.rs index 06baff53db2..0e528058e9b 100644 --- a/gix-path/src/env/mod.rs +++ b/gix-path/src/env/mod.rs @@ -1,10 +1,10 @@ -use std::{ - ffi::OsString, - path::{Path, PathBuf}, -}; +use std::ffi::OsString; +use std::path::{Path, PathBuf}; -use crate::env::git::EXE_NAME; use bstr::{BString, ByteSlice}; +use once_cell::sync::Lazy; + +use crate::env::git::EXE_NAME; mod git; @@ -37,7 +37,7 @@ 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. - static EXECUTABLE_PATH: once_cell::sync::Lazy> = once_cell::sync::Lazy::new(|| { + static EXECUTABLE_PATH: Lazy> = Lazy::new(|| { std::env::split_paths(&std::env::var_os("PATH")?) .chain(git::ALTERNATIVE_LOCATIONS.iter().map(Into::into)) .find_map(|prefix| { @@ -98,7 +98,7 @@ pub fn xdg_config(file: &str, env_var: &mut dyn FnMut(&str) -> Option) /// wasn't built with a well-known directory structure or environment. pub fn system_prefix() -> Option<&'static Path> { if cfg!(windows) { - static PREFIX: once_cell::sync::Lazy> = once_cell::sync::Lazy::new(|| { + static PREFIX: Lazy> = Lazy::new(|| { if let Some(root) = std::env::var_os("EXEPATH").map(PathBuf::from) { for candidate in ["mingw64", "mingw32"] { let candidate = root.join(candidate); From 6af59cab05183472a1843e867bce894b8c07557c Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Thu, 11 Jul 2024 19:04:06 -0400 Subject: [PATCH 03/34] Add dev dependencies for soon to be added unit test + Let one of the already affected `Cargo.lock` files pick up the (unrelated to this) `portable-atomic` dependency from 3639678. --- Cargo.lock | 152 ++++++++++++++++++++++++++++++++++++-------- gix-path/Cargo.toml | 5 ++ 2 files changed, 131 insertions(+), 26 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index b9d06485ebf..aa20bb07a24 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2219,9 +2219,12 @@ dependencies = [ "bstr", "gix-trace 0.1.9", "home", + "known-folders", "once_cell", "tempfile", "thiserror", + "windows 0.58.0", + "winreg 0.52.0", ] [[package]] @@ -2472,6 +2475,7 @@ dependencies = [ "gix-path 0.10.8", "gix-pathspec", "gix-worktree 0.34.0", + "portable-atomic", "thiserror", ] @@ -3204,6 +3208,15 @@ dependencies = [ "rayon", ] +[[package]] +name = "known-folders" +version = "1.1.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "4397c789f2709d23cfcb703b316e0766a8d4b17db2d47b0ab096ef6047cae1d8" +dependencies = [ + "windows-sys 0.52.0", +] + [[package]] name = "kstring" version = "2.0.0" @@ -3747,6 +3760,12 @@ dependencies = [ "windows-sys 0.52.0", ] +[[package]] +name = "portable-atomic" +version = "1.6.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "7170ef9988bc169ba16dd36a7fa041e5c4cbeb6a35b76d4c03daded371eae7c0" + [[package]] name = "powerfmt" version = "0.2.0" @@ -3946,7 +3965,7 @@ dependencies = [ "wasm-bindgen-futures", "web-sys", "webpki-roots", - "winreg", + "winreg 0.50.0", ] [[package]] @@ -5109,10 +5128,20 @@ version = "0.51.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "ca229916c5ee38c2f2bc1e9d8f04df975b4bd93f9955dc69fabb5d91270045c9" dependencies = [ - "windows-core", + "windows-core 0.51.1", "windows-targets 0.48.5", ] +[[package]] +name = "windows" +version = "0.58.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "dd04d41d93c4992d421894c18c8b43496aa748dd4c081bac0dc93eb0489272b6" +dependencies = [ + "windows-core 0.58.0", + "windows-targets 0.52.6", +] + [[package]] name = "windows-core" version = "0.51.1" @@ -5122,6 +5151,60 @@ dependencies = [ "windows-targets 0.48.5", ] +[[package]] +name = "windows-core" +version = "0.58.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "6ba6d44ec8c2591c134257ce647b7ea6b20335bf6379a27dac5f1641fcf59f99" +dependencies = [ + "windows-implement", + "windows-interface", + "windows-result", + "windows-strings", + "windows-targets 0.52.6", +] + +[[package]] +name = "windows-implement" +version = "0.58.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "2bbd5b46c938e506ecbce286b6628a02171d56153ba733b6c741fc627ec9579b" +dependencies = [ + "proc-macro2", + "quote", + "syn 2.0.47", +] + +[[package]] +name = "windows-interface" +version = "0.58.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "053c4c462dc91d3b1504c6fe5a726dd15e216ba718e84a0e46a88fbe5ded3515" +dependencies = [ + "proc-macro2", + "quote", + "syn 2.0.47", +] + +[[package]] +name = "windows-result" +version = "0.2.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "1d1043d8214f791817bab27572aaa8af63732e11bf84aa21a45a78d6c317ae0e" +dependencies = [ + "windows-targets 0.52.6", +] + +[[package]] +name = "windows-strings" +version = "0.1.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "4cd9b125c486025df0eabcb585e62173c6c9eddcec5d117d3b6e8c30e2ee4d10" +dependencies = [ + "windows-result", + "windows-targets 0.52.6", +] + [[package]] name = "windows-sys" version = "0.48.0" @@ -5137,7 +5220,7 @@ version = "0.52.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "282be5f36a8ce781fad8c8ae18fa3f9beff57ec1b52cb3de0789201425d9a33d" dependencies = [ - "windows-targets 0.52.0", + "windows-targets 0.52.6", ] [[package]] @@ -5172,17 +5255,18 @@ dependencies = [ [[package]] name = "windows-targets" -version = "0.52.0" +version = "0.52.6" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "8a18201040b24831fbb9e4eb208f8892e1f50a37feb53cc7ff887feb8f50e7cd" +checksum = "9b724f72796e036ab90c1021d4780d4d3d648aca59e491e6b98e725b84e99973" dependencies = [ - "windows_aarch64_gnullvm 0.52.0", - "windows_aarch64_msvc 0.52.0", - "windows_i686_gnu 0.52.0", - "windows_i686_msvc 0.52.0", - "windows_x86_64_gnu 0.52.0", - "windows_x86_64_gnullvm 0.52.0", - "windows_x86_64_msvc 0.52.0", + "windows_aarch64_gnullvm 0.52.6", + "windows_aarch64_msvc 0.52.6", + "windows_i686_gnu 0.52.6", + "windows_i686_gnullvm", + "windows_i686_msvc 0.52.6", + "windows_x86_64_gnu 0.52.6", + "windows_x86_64_gnullvm 0.52.6", + "windows_x86_64_msvc 0.52.6", ] [[package]] @@ -5199,9 +5283,9 @@ checksum = "2b38e32f0abccf9987a4e3079dfb67dcd799fb61361e53e2882c3cbaf0d905d8" [[package]] name = "windows_aarch64_gnullvm" -version = "0.52.0" +version = "0.52.6" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "cb7764e35d4db8a7921e09562a0304bf2f93e0a51bfccee0bd0bb0b666b015ea" +checksum = "32a4622180e7a0ec044bb555404c800bc9fd9ec262ec147edd5989ccd0c02cd3" [[package]] name = "windows_aarch64_msvc" @@ -5217,9 +5301,9 @@ checksum = "dc35310971f3b2dbbf3f0690a219f40e2d9afcf64f9ab7cc1be722937c26b4bc" [[package]] name = "windows_aarch64_msvc" -version = "0.52.0" +version = "0.52.6" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "bbaa0368d4f1d2aaefc55b6fcfee13f41544ddf36801e793edbbfd7d7df075ef" +checksum = "09ec2a7bb152e2252b53fa7803150007879548bc709c039df7627cabbd05d469" [[package]] name = "windows_i686_gnu" @@ -5235,9 +5319,15 @@ checksum = "a75915e7def60c94dcef72200b9a8e58e5091744960da64ec734a6c6e9b3743e" [[package]] name = "windows_i686_gnu" -version = "0.52.0" +version = "0.52.6" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "a28637cb1fa3560a16915793afb20081aba2c92ee8af57b4d5f28e4b3e7df313" +checksum = "8e9b5ad5ab802e97eb8e295ac6720e509ee4c243f69d781394014ebfe8bbfa0b" + +[[package]] +name = "windows_i686_gnullvm" +version = "0.52.6" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "0eee52d38c090b3caa76c563b86c3a4bd71ef1a819287c19d586d7334ae8ed66" [[package]] name = "windows_i686_msvc" @@ -5253,9 +5343,9 @@ checksum = "8f55c233f70c4b27f66c523580f78f1004e8b5a8b659e05a4eb49d4166cca406" [[package]] name = "windows_i686_msvc" -version = "0.52.0" +version = "0.52.6" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "ffe5e8e31046ce6230cc7215707b816e339ff4d4d67c65dffa206fd0f7aa7b9a" +checksum = "240948bc05c5e7c6dabba28bf89d89ffce3e303022809e73deaefe4f6ec56c66" [[package]] name = "windows_x86_64_gnu" @@ -5271,9 +5361,9 @@ checksum = "53d40abd2583d23e4718fddf1ebec84dbff8381c07cae67ff7768bbf19c6718e" [[package]] name = "windows_x86_64_gnu" -version = "0.52.0" +version = "0.52.6" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "3d6fa32db2bc4a2f5abeacf2b69f7992cd09dca97498da74a151a3132c26befd" +checksum = "147a5c80aabfbf0c7d901cb5895d1de30ef2907eb21fbbab29ca94c5b08b1a78" [[package]] name = "windows_x86_64_gnullvm" @@ -5289,9 +5379,9 @@ checksum = "0b7b52767868a23d5bab768e390dc5f5c55825b6d30b86c844ff2dc7414044cc" [[package]] name = "windows_x86_64_gnullvm" -version = "0.52.0" +version = "0.52.6" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "1a657e1e9d3f514745a572a6846d3c7aa7dbe1658c056ed9c3344c4109a6949e" +checksum = "24d5b23dc417412679681396f2b49f3de8c1473deb516bd34410872eff51ed0d" [[package]] name = "windows_x86_64_msvc" @@ -5307,9 +5397,9 @@ checksum = "ed94fce61571a4006852b7389a063ab983c02eb1bb37b47f8272ce92d06d9538" [[package]] name = "windows_x86_64_msvc" -version = "0.52.0" +version = "0.52.6" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "dff9641d1cd4be8d1a070daf9e3773c5f67e78b4d9d42263020c057706765c04" +checksum = "589f6da84c646204747d1270a2a5661ea66ed1cced2631d546fdfb155959f9ec" [[package]] name = "winnow" @@ -5339,6 +5429,16 @@ dependencies = [ "windows-sys 0.48.0", ] +[[package]] +name = "winreg" +version = "0.52.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "a277a57398d4bfa075df44f501a17cfdf8542d224f0d36095a2adc7aee4ef0a5" +dependencies = [ + "cfg-if", + "windows-sys 0.48.0", +] + [[package]] name = "xattr" version = "1.2.0" diff --git a/gix-path/Cargo.toml b/gix-path/Cargo.toml index fb942e41965..bc084e8341e 100644 --- a/gix-path/Cargo.toml +++ b/gix-path/Cargo.toml @@ -23,3 +23,8 @@ home = "0.5.5" [dev-dependencies] tempfile = "3.3.0" + +[target.'cfg(windows)'.dev-dependencies] +known-folders = "1.1.0" +windows = { version = "0.58.0", features = ["Win32_System_Threading"] } +winreg = "0.52.0" From edc1351d66f9ae1482193228d00a2b555a1ee979 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Thu, 11 Jul 2024 21:46:27 -0400 Subject: [PATCH 04/34] Start on unit test for ALTERNATIVE_LOCATIONS --- gix-path/src/env/git.rs | 188 ++++++++++++++++++++++++++++++++++++++++ gix-path/src/lib.rs | 2 +- 2 files changed, 189 insertions(+), 1 deletion(-) diff --git a/gix-path/src/env/git.rs b/gix-path/src/env/git.rs index d6c446a74ab..0d47a2d44e0 100644 --- a/gix-path/src/env/git.rs +++ b/gix-path/src/env/git.rs @@ -98,6 +98,7 @@ mod tests { assert_eq!(super::config_to_base_path(Path::new(input)), Path::new(expected)); } } + #[test] fn first_file_from_config_with_origin() { let macos = "file:/Applications/Xcode.app/Contents/Developer/usr/share/git-core/gitconfig credential.helper=osxkeychain\nfile:/Users/byron/.gitconfig push.default=simple\n"; @@ -127,4 +128,191 @@ mod tests { ); } } + + #[cfg(windows)] + use { + known_folders::{get_known_folder_path, KnownFolder}, + std::ffi::{OsStr, OsString}, + std::path::PathBuf, + windows::core::Result as WindowsResult, + windows::Win32::Foundation::BOOL, + windows::Win32::System::Threading::{GetCurrentProcess, IsWow64Process}, + winreg::enums::{HKEY_LOCAL_MACHINE, KEY_QUERY_VALUE}, + winreg::RegKey, + }; + + #[cfg(windows)] + trait Current: Sized { + fn current() -> WindowsResult; + } + + #[cfg(windows)] + enum PlatformArchitecture { + Is32on32, + Is32on64, + Is64on64, + } + + #[cfg(windows)] + impl Current for PlatformArchitecture { + fn current() -> WindowsResult { + // Ordinarily, we would check the target pointer width first to avoid doing extra work, + // because if this is a 64-bit executable then the operating system is 64-bit. But this + // is for the test suite, and doing it this way allows problems to be caught earlier if + // a change made on a 64-bit development machine breaks the IsWow64Process() call. + let mut wow64process = BOOL::default(); + unsafe { IsWow64Process(GetCurrentProcess(), &mut wow64process)? }; + + let platform_architecture = if wow64process.as_bool() { + Self::Is32on64 + } else if cfg!(target_pointer_width = "32") { + Self::Is32on32 + } else { + assert!(cfg!(target_pointer_width = "64")); + Self::Is64on64 + }; + Ok(platform_architecture) + } + } + + #[cfg(windows)] + fn ends_with_case_insensitive(text: &OsStr, suffix: &str) -> Option { + Some(text.to_str()?.to_lowercase().ends_with(&suffix.to_lowercase())) + } + + #[cfg(windows)] + struct PathsByRole { + /// The program files directory relative to what architecture this program was built for. + pf_current: PathBuf, + + /// The x86 program files directory regardless of the architecture of the program. + /// + /// If Rust gains Windows targets like ARMv7 where this is unavailable, this could fail. + pf_x86: PathBuf, + + /// The 64-bit program files directory if there is one. + /// + /// This is present on x64 and also ARM64 systems. On an ARM64 system, ARM64 and AMD64 + /// programs use the same program files directory while 32-bit x86 and ARM programs use two + /// others. Only a 32-bit has no 64-bit program files directory. + maybe_pf_64bit: Result, + // While + // KnownFolder::ProgramFilesX64 exists, it's unfortunately unavailable to 32-bit + // processes; see + // https://learn.microsoft.com/en-us/windows/win32/shell/knownfolderid#remarks. + } + + impl PathsByRole { + /// Gets the three common kinds of global program files paths without environment variables. + /// + /// The idea here is to obtain this information, which the `alternative_locations()` unit + /// test uses to learn the expected alternative locations, without duplicating *any* of the + /// approach used for `ALTERNATIVE_LOCATIONS`, so it can be used to test that. The approach + /// here is also be more reliable than using environment variables, but it is a bit more + /// complex, and it requires either additional dependencies or the use of unsafe code. + /// + /// This obtains `pf_current` and `pf_x86` by the [known folders][known-folders] system, + /// and `maybe_pf_64bit` from the registry since the corresponding known folder is not + /// available to 32-bit processes, per the [KNOWNFOLDDERID][knownfolderid] documentation. + /// + /// If in the future the implementation of `ALTERNATIVE_LOCATIONS` uses these techniques, + /// then this function can be changed to use environment variables and renamed accordingly. + /// + /// [known-folders]: https://learn.microsoft.com/en-us/windows/win32/shell/known-folders + /// [knownfolderid]: https://learn.microsoft.com/en-us/windows/win32/shell/knownfolderid#remarks + fn obtain_envlessly() -> Self { + let pf_current = get_known_folder_path(KnownFolder::ProgramFiles) + .expect("The process architecture specific program files folder is always available."); + + let pf_x86 = get_known_folder_path(KnownFolder::ProgramFilesX86) + .expect("The x86 program files folder will in practice always be available."); + + let maybe_pf_64bit = RegKey::predef(HKEY_LOCAL_MACHINE) + .open_subkey_with_flags(r#"SOFTWARE\Microsoft\Windows\CurrentVersion"#, KEY_QUERY_VALUE) + .expect("The `CurrentVersion` key exists and allows reading.") + .get_value::("ProgramW6432Dir") + .map(PathBuf::from); + + Self { + pf_current, + pf_x86, + maybe_pf_64bit, + } + } + + /// Checks that the paths we got for testing are reasonable. + /// + /// This checks that `obtain_envlessly()` returned paths that are likely to be correct and + /// that satisfy the most important properties based on the current system and process. + fn validate(self) -> Self { + let PathsByRole { + pf_current, + pf_x86, + maybe_pf_64bit, + } = self; + + match PlatformArchitecture::current().expect("Process and system 'bitness' should be available.") { + PlatformArchitecture::Is32on32 => { + assert_eq!( + pf_current.as_os_str(), + pf_x86.as_os_str(), + "Ours is identical to 32-bit path." + ); + for arch_suffix in [" (x86)", " (Arm)"] { + let has_arch_suffix = ends_with_case_insensitive(pf_current.as_os_str(), arch_suffix) + .expect("Assume the test systems key dirs are valid Unicode."); + assert!( + !has_arch_suffix, + "32-bit program files on 32-bit system has unadorned name." + ); + } + maybe_pf_64bit + .as_ref() + .expect_err("A 32-bit system has no 64-bit program files directory."); + } + PlatformArchitecture::Is32on64 => { + assert_eq!( + pf_current.as_os_str(), + pf_x86.as_os_str(), + "Ours is identical to 32-bit path." + ); + let pf_64bit = maybe_pf_64bit.as_ref().expect("64-bit program files exists."); + assert_ne!(&pf_x86, pf_64bit, "32-bit and 64-bit program files directories differ."); + } + PlatformArchitecture::Is64on64 => { + let pf_64bit = maybe_pf_64bit.as_ref().expect("64-bit program files exists."); + assert_eq!( + pf_current.as_os_str(), + pf_64bit.as_os_str(), + "Ours is identical to 64-bit path." + ); + assert_ne!(&pf_x86, pf_64bit, "32-bit and 64-bit program files directories differ."); + } + } + + Self { + pf_current, + pf_x86, + maybe_pf_64bit, + } + } + } + + #[test] + #[cfg(windows)] + fn alternative_locations() { + let PathsByRole { + pf_current, + pf_x86, + maybe_pf_64bit, + } = PathsByRole::obtain_envlessly().validate(); + + // FIXME: Assert the relationships between the above values and ALTERNATIVE_LOCATIONS contents! + } + + #[test] + #[cfg(not(windows))] + fn alternative_locations() { + assert!(super::ALTERNATIVE_LOCATIONS.is_empty()); + } } diff --git a/gix-path/src/lib.rs b/gix-path/src/lib.rs index 7f913042fac..c433e48d9f8 100644 --- a/gix-path/src/lib.rs +++ b/gix-path/src/lib.rs @@ -47,7 +47,7 @@ //! ever get into a code-path which does panic though. //! #![deny(missing_docs, rust_2018_idioms)] -#![forbid(unsafe_code)] +#![cfg_attr(not(test), forbid(unsafe_code))] /// A dummy type to represent path specs and help finding all spots that take path specs once it is implemented. mod convert; From 5b206bce8149349f9b4390ca04c1304def225dff Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Fri, 12 Jul 2024 05:34:34 -0400 Subject: [PATCH 05/34] Make ALTERNATIVE_LOCATIONS items PathBuf Instead of directly OsString. This is because these locations are paths. Representing them this way has the further advantage of allowing some code to be slightly simplified. --- gix-path/src/env/git.rs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/gix-path/src/env/git.rs b/gix-path/src/env/git.rs index 0d47a2d44e0..b61ae19008a 100644 --- a/gix-path/src/env/git.rs +++ b/gix-path/src/env/git.rs @@ -1,4 +1,3 @@ -use std::ffi::OsString; use std::path::{Path, PathBuf}; use std::process::{Command, Stdio}; @@ -7,14 +6,14 @@ use once_cell::sync::Lazy; /// Other places to find Git in. #[cfg(windows)] -pub(super) static ALTERNATIVE_LOCATIONS: Lazy> = Lazy::new(|| { +pub(super) static ALTERNATIVE_LOCATIONS: Lazy> = Lazy::new(|| { vec![ "C:/Program Files/Git/mingw64/bin".into(), "C:/Program Files (x86)/Git/mingw32/bin".into(), ] }); #[cfg(not(windows))] -pub(super) static ALTERNATIVE_LOCATIONS: Lazy> = Lazy::new(|| vec![]); +pub(super) static ALTERNATIVE_LOCATIONS: Lazy> = Lazy::new(|| vec![]); #[cfg(windows)] pub(super) static EXE_NAME: &str = "git.exe"; From 99a8eb3748db9f50f11491ce3c9ebb197e70a942 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Fri, 12 Jul 2024 05:39:41 -0400 Subject: [PATCH 06/34] Do a minor simplification now that it's PathBuf The code still worked with the extra step there, but now it's unnecessary. --- gix-path/src/env/git.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gix-path/src/env/git.rs b/gix-path/src/env/git.rs index b61ae19008a..d3d24b19ac0 100644 --- a/gix-path/src/env/git.rs +++ b/gix-path/src/env/git.rs @@ -36,7 +36,7 @@ pub(super) static EXE_INFO: Lazy> = Lazy::new(|| { #[cfg(windows)] Err(err) if err.kind() == std::io::ErrorKind::NotFound => { let executable = ALTERNATIVE_LOCATIONS.iter().find_map(|prefix| { - let candidate = Path::new(prefix).join(EXE_NAME); + let candidate = prefix.join(EXE_NAME); candidate.is_file().then_some(candidate) })?; gix_trace::debug!(cmd = ?cmd, "invoking git for installation config path in alternate location"); From a5a5342c7f94e3e53e2a4969beee81fadc8592ef Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Fri, 12 Jul 2024 06:17:43 -0400 Subject: [PATCH 07/34] Revise obtain_envlessly() doc comment for clarity --- gix-path/src/env/git.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/gix-path/src/env/git.rs b/gix-path/src/env/git.rs index d3d24b19ac0..a1c01811be3 100644 --- a/gix-path/src/env/git.rs +++ b/gix-path/src/env/git.rs @@ -207,12 +207,12 @@ mod tests { /// The idea here is to obtain this information, which the `alternative_locations()` unit /// test uses to learn the expected alternative locations, without duplicating *any* of the /// approach used for `ALTERNATIVE_LOCATIONS`, so it can be used to test that. The approach - /// here is also be more reliable than using environment variables, but it is a bit more + /// here is also more reliable than using environment variables, but it is a bit more /// complex, and it requires either additional dependencies or the use of unsafe code. /// - /// This obtains `pf_current` and `pf_x86` by the [known folders][known-folders] system, - /// and `maybe_pf_64bit` from the registry since the corresponding known folder is not - /// available to 32-bit processes, per the [KNOWNFOLDDERID][knownfolderid] documentation. + /// This gets `pf_current` and `pf_x86` by the [known folders][known-folders] system. But + /// it gets `maybe_pf_64bit` from the registry, as the corresponding known folder is not + /// available to 32-bit processes. See the [`KNOWNFOLDDERID`][knownfolderid] documentation. /// /// If in the future the implementation of `ALTERNATIVE_LOCATIONS` uses these techniques, /// then this function can be changed to use environment variables and renamed accordingly. From 5df0cf581e7bdcacaf6e630a3653ac449e01e30b Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Fri, 12 Jul 2024 06:23:45 -0400 Subject: [PATCH 08/34] Improve expect and assert messages in PathsByRole::validate() --- gix-path/src/env/git.rs | 28 +++++++++++++++++++--------- 1 file changed, 19 insertions(+), 9 deletions(-) diff --git a/gix-path/src/env/git.rs b/gix-path/src/env/git.rs index a1c01811be3..2a8b467c354 100644 --- a/gix-path/src/env/git.rs +++ b/gix-path/src/env/git.rs @@ -255,14 +255,14 @@ mod tests { assert_eq!( pf_current.as_os_str(), pf_x86.as_os_str(), - "Ours is identical to 32-bit path." + "Our program files path is exactly identical to the 32-bit one.", ); for arch_suffix in [" (x86)", " (Arm)"] { let has_arch_suffix = ends_with_case_insensitive(pf_current.as_os_str(), arch_suffix) - .expect("Assume the test systems key dirs are valid Unicode."); + .expect("Assume the test system's important directories are valid Unicode."); assert!( !has_arch_suffix, - "32-bit program files on 32-bit system has unadorned name." + "The 32-bit program files directory name on a 32-bit system mentions no architecture.", ); } maybe_pf_64bit @@ -273,19 +273,29 @@ mod tests { assert_eq!( pf_current.as_os_str(), pf_x86.as_os_str(), - "Ours is identical to 32-bit path." + "Our program files path is exactly identical to the 32-bit one.", + ); + let pf_64bit = maybe_pf_64bit + .as_ref() + .expect("The 64-bit program files directory exists."); + assert_ne!( + &pf_x86, pf_64bit, + "The 32-bit and 64-bit program files directories have different locations.", ); - let pf_64bit = maybe_pf_64bit.as_ref().expect("64-bit program files exists."); - assert_ne!(&pf_x86, pf_64bit, "32-bit and 64-bit program files directories differ."); } PlatformArchitecture::Is64on64 => { - let pf_64bit = maybe_pf_64bit.as_ref().expect("64-bit program files exists."); + let pf_64bit = maybe_pf_64bit + .as_ref() + .expect("The 64-bit program files directory exists."); assert_eq!( pf_current.as_os_str(), pf_64bit.as_os_str(), - "Ours is identical to 64-bit path." + "Our program files path is exactly identical to the 64-bit one.", + ); + assert_ne!( + &pf_x86, pf_64bit, + "The 32-bit and 64-bit program files directories have different locations.", ); - assert_ne!(&pf_x86, pf_64bit, "32-bit and 64-bit program files directories differ."); } } From 518fd2709ea3606da07a7fa9fb634d19df3425d5 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Fri, 12 Jul 2024 13:57:03 -0400 Subject: [PATCH 09/34] Remove extra comment left over from before --- gix-path/src/env/git.rs | 4 ---- 1 file changed, 4 deletions(-) diff --git a/gix-path/src/env/git.rs b/gix-path/src/env/git.rs index 2a8b467c354..418eecbf6c2 100644 --- a/gix-path/src/env/git.rs +++ b/gix-path/src/env/git.rs @@ -195,10 +195,6 @@ mod tests { /// programs use the same program files directory while 32-bit x86 and ARM programs use two /// others. Only a 32-bit has no 64-bit program files directory. maybe_pf_64bit: Result, - // While - // KnownFolder::ProgramFilesX64 exists, it's unfortunately unavailable to 32-bit - // processes; see - // https://learn.microsoft.com/en-us/windows/win32/shell/knownfolderid#remarks. } impl PathsByRole { From 5701145bafae708f9c43e2c6947c11037f1349e0 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Fri, 12 Jul 2024 16:23:17 -0400 Subject: [PATCH 10/34] Make PathsByRole::maybe_pf_64bit an Option The `PathsByRole` test helper struct's `maybe_pf_64bit` member was previously `Result`. But construction, which is by `PathsByRole::obtain_envlessly()`, already asserted the expected existence of the other two paths. It should likewise assert that if the guaranteed-64-bit program files path cannot be obtained from the registry, then this is due to it being absent, rather than some other error. Checking this in `obtain_envlessly()` makes it reasonable for `maybe_pf_64bit` to be an `Option`, as its name would suggest it should be. This in turn makes clear that all the validation and assertions for this "reference value" have already been done. Thus `PathsByRole::validate()` can continue omitting validation for the error type, validation it should not perform because the point of it is do checks that are agnostic of how the paths were obtained. This also makes `PathsByRole` cloneable, which is now possible because it no longer holds on to a `std::io::Error` with its non-cloneable dynamic error information, and adds some other missing traits on test helper types. --- gix-path/src/env/git.rs | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/gix-path/src/env/git.rs b/gix-path/src/env/git.rs index 418eecbf6c2..144b4a73fd3 100644 --- a/gix-path/src/env/git.rs +++ b/gix-path/src/env/git.rs @@ -132,6 +132,7 @@ mod tests { use { known_folders::{get_known_folder_path, KnownFolder}, std::ffi::{OsStr, OsString}, + std::io::ErrorKind, std::path::PathBuf, windows::core::Result as WindowsResult, windows::Win32::Foundation::BOOL, @@ -146,6 +147,7 @@ mod tests { } #[cfg(windows)] + #[derive(Clone, Copy, Debug)] enum PlatformArchitecture { Is32on32, Is32on64, @@ -180,6 +182,7 @@ mod tests { } #[cfg(windows)] + #[derive(Clone, Debug)] struct PathsByRole { /// The program files directory relative to what architecture this program was built for. pf_current: PathBuf, @@ -194,7 +197,7 @@ mod tests { /// This is present on x64 and also ARM64 systems. On an ARM64 system, ARM64 and AMD64 /// programs use the same program files directory while 32-bit x86 and ARM programs use two /// others. Only a 32-bit has no 64-bit program files directory. - maybe_pf_64bit: Result, + maybe_pf_64bit: Option, } impl PathsByRole { @@ -226,7 +229,12 @@ mod tests { .open_subkey_with_flags(r#"SOFTWARE\Microsoft\Windows\CurrentVersion"#, KEY_QUERY_VALUE) .expect("The `CurrentVersion` key exists and allows reading.") .get_value::("ProgramW6432Dir") - .map(PathBuf::from); + .map(PathBuf::from) + .map_err(|error| { + assert_eq!(error.kind(), ErrorKind::NotFound); + error + }) + .ok(); Self { pf_current, @@ -261,9 +269,10 @@ mod tests { "The 32-bit program files directory name on a 32-bit system mentions no architecture.", ); } - maybe_pf_64bit - .as_ref() - .expect_err("A 32-bit system has no 64-bit program files directory."); + assert_eq!( + maybe_pf_64bit, None, + "A 32-bit system has no 64-bit program files directory.", + ); } PlatformArchitecture::Is32on64 => { assert_eq!( From 95708dd754482db9afe4ec417f6d5ebdd35bc539 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Fri, 12 Jul 2024 17:06:23 -0400 Subject: [PATCH 11/34] Clarify what pf_current represents --- gix-path/src/env/git.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gix-path/src/env/git.rs b/gix-path/src/env/git.rs index 144b4a73fd3..586faf33639 100644 --- a/gix-path/src/env/git.rs +++ b/gix-path/src/env/git.rs @@ -184,7 +184,7 @@ mod tests { #[cfg(windows)] #[derive(Clone, Debug)] struct PathsByRole { - /// The program files directory relative to what architecture this program was built for. + /// The program files directory used for whatever architecture this program was built for. pf_current: PathBuf, /// The x86 program files directory regardless of the architecture of the program. From 3dd1d1fd4e9f98d842ad70ca181160a51b87e844 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Fri, 12 Jul 2024 18:10:05 -0400 Subject: [PATCH 12/34] Rename PathsByRole to ProgramFilesPaths + Remove the "pf" parts of its fields. + Refactor usage now that unpacking isn't needed to refer to the values intuitively. + Write a fragment of the unit test that uses it. This was part of checking that the new names make sense, but these particular assertions may not all be kept (and may drastically change form). This is because, even if the process architecture specific path is something we want to list first, that is a change to the implementation that is separate from the first and main fix that this test is being introduced to provide coverage for. --- gix-path/src/env/git.rs | 67 +++++++++++++++++++---------------------- 1 file changed, 31 insertions(+), 36 deletions(-) diff --git a/gix-path/src/env/git.rs b/gix-path/src/env/git.rs index 586faf33639..cb803e306d9 100644 --- a/gix-path/src/env/git.rs +++ b/gix-path/src/env/git.rs @@ -181,26 +181,27 @@ mod tests { Some(text.to_str()?.to_lowercase().ends_with(&suffix.to_lowercase())) } + /// The common global program files paths on this system, by process and system architecture. #[cfg(windows)] #[derive(Clone, Debug)] - struct PathsByRole { + struct ProgramFilesPaths { /// The program files directory used for whatever architecture this program was built for. - pf_current: PathBuf, + current: PathBuf, /// The x86 program files directory regardless of the architecture of the program. /// /// If Rust gains Windows targets like ARMv7 where this is unavailable, this could fail. - pf_x86: PathBuf, + x86: PathBuf, /// The 64-bit program files directory if there is one. /// /// This is present on x64 and also ARM64 systems. On an ARM64 system, ARM64 and AMD64 /// programs use the same program files directory while 32-bit x86 and ARM programs use two /// others. Only a 32-bit has no 64-bit program files directory. - maybe_pf_64bit: Option, + maybe_64bit: Option, } - impl PathsByRole { + impl ProgramFilesPaths { /// Gets the three common kinds of global program files paths without environment variables. /// /// The idea here is to obtain this information, which the `alternative_locations()` unit @@ -237,9 +238,9 @@ mod tests { .ok(); Self { - pf_current, - pf_x86, - maybe_pf_64bit, + current: pf_current, + x86: pf_x86, + maybe_64bit: maybe_pf_64bit, } } @@ -248,21 +249,15 @@ mod tests { /// This checks that `obtain_envlessly()` returned paths that are likely to be correct and /// that satisfy the most important properties based on the current system and process. fn validate(self) -> Self { - let PathsByRole { - pf_current, - pf_x86, - maybe_pf_64bit, - } = self; - match PlatformArchitecture::current().expect("Process and system 'bitness' should be available.") { PlatformArchitecture::Is32on32 => { assert_eq!( - pf_current.as_os_str(), - pf_x86.as_os_str(), + self.current.as_os_str(), + self.x86.as_os_str(), "Our program files path is exactly identical to the 32-bit one.", ); for arch_suffix in [" (x86)", " (Arm)"] { - let has_arch_suffix = ends_with_case_insensitive(pf_current.as_os_str(), arch_suffix) + let has_arch_suffix = ends_with_case_insensitive(self.current.as_os_str(), arch_suffix) .expect("Assume the test system's important directories are valid Unicode."); assert!( !has_arch_suffix, @@ -270,58 +265,58 @@ mod tests { ); } assert_eq!( - maybe_pf_64bit, None, + self.maybe_64bit, None, "A 32-bit system has no 64-bit program files directory.", ); } PlatformArchitecture::Is32on64 => { assert_eq!( - pf_current.as_os_str(), - pf_x86.as_os_str(), + self.current.as_os_str(), + self.x86.as_os_str(), "Our program files path is exactly identical to the 32-bit one.", ); - let pf_64bit = maybe_pf_64bit + let pf_64bit = self + .maybe_64bit .as_ref() .expect("The 64-bit program files directory exists."); assert_ne!( - &pf_x86, pf_64bit, + &self.x86, pf_64bit, "The 32-bit and 64-bit program files directories have different locations.", ); } PlatformArchitecture::Is64on64 => { - let pf_64bit = maybe_pf_64bit + let pf_64bit = self + .maybe_64bit .as_ref() .expect("The 64-bit program files directory exists."); assert_eq!( - pf_current.as_os_str(), + self.current.as_os_str(), pf_64bit.as_os_str(), "Our program files path is exactly identical to the 64-bit one.", ); assert_ne!( - &pf_x86, pf_64bit, + &self.x86, pf_64bit, "The 32-bit and 64-bit program files directories have different locations.", ); } } - Self { - pf_current, - pf_x86, - maybe_pf_64bit, - } + self } } #[test] #[cfg(windows)] fn alternative_locations() { - let PathsByRole { - pf_current, - pf_x86, - maybe_pf_64bit, - } = PathsByRole::obtain_envlessly().validate(); + let pf = ProgramFilesPaths::obtain_envlessly().validate(); + + let primary_suffix = super::ALTERNATIVE_LOCATIONS + .get(0) + .expect("It gave at least one path (assuming normal conditions).") + .strip_prefix(pf.current) + .expect("It gave a process architecture specific directory and listed it first."); - // FIXME: Assert the relationships between the above values and ALTERNATIVE_LOCATIONS contents! + // FIXME: Assert the other relationships between pf values and ALTERNATIVE_LOCATIONS contents! } #[test] From dd1e5c8d30fb1326a96172e886fb03c3533453c8 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Fri, 12 Jul 2024 20:10:48 -0400 Subject: [PATCH 13/34] Assert `ALTERNATIVE_LOCATIONS` count and prefixes And extract the corresponding suffixes, to be used in some of the subsequent assertions. --- gix-path/src/env/git.rs | 48 +++++++++++++++++++++++++++++++++++------ 1 file changed, 42 insertions(+), 6 deletions(-) diff --git a/gix-path/src/env/git.rs b/gix-path/src/env/git.rs index cb803e306d9..23caef5ce7e 100644 --- a/gix-path/src/env/git.rs +++ b/gix-path/src/env/git.rs @@ -305,16 +305,52 @@ mod tests { } } + /// Paths relative to process architecture specific program files directories. + #[cfg(windows)] + #[derive(Clone, Debug)] + struct GitBinSuffixes<'a> { + x86: &'a Path, + maybe_64bit: Option<&'a Path>, + } + + #[cfg(windows)] + impl<'a> GitBinSuffixes<'a> { + /// Assert that `ALTERNATIVE_LOCATIONS` has the given prefixes, and extract the suffixes. + fn assert_from(pf: &'a ProgramFilesPaths) -> Self { + match super::ALTERNATIVE_LOCATIONS.as_slice() { + [primary, secondary] => { + let prefix_64bit = pf + .maybe_64bit + .as_ref() + .expect("It gives two paths only if one can be 64-bit."); + let got_64bit = primary + .strip_prefix(prefix_64bit) + .expect("It gives the 64-bit path and lists it first."); + let x86 = secondary + .strip_prefix(pf.x86.as_path()) + .expect("It gives the 32-bit path and lists it second."); + Self { + x86, + maybe_64bit: Some(got_64bit), + } + } + [only] => { + assert_eq!(pf.maybe_64bit, None, "It gives one path only if none can be 64-bit."); + Self { + x86: only, + maybe_64bit: None, + } + } + other => panic!("Got length {}, expected 1 or 2.", other.len()), + } + } + } + #[test] #[cfg(windows)] fn alternative_locations() { let pf = ProgramFilesPaths::obtain_envlessly().validate(); - - let primary_suffix = super::ALTERNATIVE_LOCATIONS - .get(0) - .expect("It gave at least one path (assuming normal conditions).") - .strip_prefix(pf.current) - .expect("It gave a process architecture specific directory and listed it first."); + let suffixes = GitBinSuffixes::assert_from(&pf); // FIXME: Assert the other relationships between pf values and ALTERNATIVE_LOCATIONS contents! } From 5258f7ab1ec02a1f64e2fa4e3e1bf646012a4dea Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Fri, 12 Jul 2024 20:32:53 -0400 Subject: [PATCH 14/34] Assert that the corresponding suffixes are correct + Rename some variables for clarity. + Comment about the possibility of a future clangarm64/bin subdirectory to check under (the same) 64-bit program files. --- gix-path/src/env/git.rs | 26 +++++++++++++++++++++----- 1 file changed, 21 insertions(+), 5 deletions(-) diff --git a/gix-path/src/env/git.rs b/gix-path/src/env/git.rs index 23caef5ce7e..780d8230489 100644 --- a/gix-path/src/env/git.rs +++ b/gix-path/src/env/git.rs @@ -323,15 +323,15 @@ mod tests { .maybe_64bit .as_ref() .expect("It gives two paths only if one can be 64-bit."); - let got_64bit = primary + let suffix_64bit = primary .strip_prefix(prefix_64bit) .expect("It gives the 64-bit path and lists it first."); - let x86 = secondary + let suffix_x86 = secondary .strip_prefix(pf.x86.as_path()) .expect("It gives the 32-bit path and lists it second."); Self { - x86, - maybe_64bit: Some(got_64bit), + x86: suffix_x86, + maybe_64bit: Some(suffix_64bit), } } [only] => { @@ -344,13 +344,29 @@ mod tests { other => panic!("Got length {}, expected 1 or 2.", other.len()), } } + + /// Assert that the suffixes are the common per-architecture Git install locations. + fn validate(&self) { + assert_eq!(self.x86, Path::new("Git/mingw32/bin")); + + if let Some(suffix_64bit) = self.maybe_64bit { + // When Git for Windows releases ARM64 builds, there will be another 64-bit suffix, + // likely clangarm64. In that case, this and other assertions will need updating, + // as there will be two separate paths to check under the same 64-bit program files + // directory. (See the definition of ProgramFilesPaths::maybe_64bit for details.) + assert_eq!(suffix_64bit, Path::new("Git/mingw64/bin")); + } + } } #[test] #[cfg(windows)] fn alternative_locations() { + // Obtain program files directory paths by other means and check that they seem correct. let pf = ProgramFilesPaths::obtain_envlessly().validate(); - let suffixes = GitBinSuffixes::assert_from(&pf); + + // Check that `ALTERNATIVE_LOCATIONS` correspond to them, with the correct subdirectories. + GitBinSuffixes::assert_from(&pf).validate(); // FIXME: Assert the other relationships between pf values and ALTERNATIVE_LOCATIONS contents! } From 671c47609f9726d629ab34d93630dcb882ceea94 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Fri, 12 Jul 2024 21:45:17 -0400 Subject: [PATCH 15/34] Adjust GitBinSuffixes so uses in the test method are clearer --- gix-path/src/env/git.rs | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/gix-path/src/env/git.rs b/gix-path/src/env/git.rs index 780d8230489..a379d4c662e 100644 --- a/gix-path/src/env/git.rs +++ b/gix-path/src/env/git.rs @@ -315,9 +315,9 @@ mod tests { #[cfg(windows)] impl<'a> GitBinSuffixes<'a> { - /// Assert that `ALTERNATIVE_LOCATIONS` has the given prefixes, and extract the suffixes. - fn assert_from(pf: &'a ProgramFilesPaths) -> Self { - match super::ALTERNATIVE_LOCATIONS.as_slice() { + /// Assert that `locations` has the given prefixes, and extract the suffixes. + fn assert_from(pf: &'a ProgramFilesPaths, locations: &'static [PathBuf]) -> Self { + match locations { [primary, secondary] => { let prefix_64bit = pf .maybe_64bit @@ -346,7 +346,7 @@ mod tests { } /// Assert that the suffixes are the common per-architecture Git install locations. - fn validate(&self) { + fn assert_architectures(&self) { assert_eq!(self.x86, Path::new("Git/mingw32/bin")); if let Some(suffix_64bit) = self.maybe_64bit { @@ -362,11 +362,13 @@ mod tests { #[test] #[cfg(windows)] fn alternative_locations() { + let locations = super::ALTERNATIVE_LOCATIONS.as_slice(); + // Obtain program files directory paths by other means and check that they seem correct. let pf = ProgramFilesPaths::obtain_envlessly().validate(); // Check that `ALTERNATIVE_LOCATIONS` correspond to them, with the correct subdirectories. - GitBinSuffixes::assert_from(&pf).validate(); + GitBinSuffixes::assert_from(&pf, locations).assert_architectures(); // FIXME: Assert the other relationships between pf values and ALTERNATIVE_LOCATIONS contents! } From e9eabeb58c33ebed05d2ae72842da5898badcc3b Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Sat, 13 Jul 2024 04:52:37 -0400 Subject: [PATCH 16/34] Narrow assert_architectures fixme to just what's left --- gix-path/src/env/git.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gix-path/src/env/git.rs b/gix-path/src/env/git.rs index a379d4c662e..3b69a2e3ed2 100644 --- a/gix-path/src/env/git.rs +++ b/gix-path/src/env/git.rs @@ -370,7 +370,7 @@ mod tests { // Check that `ALTERNATIVE_LOCATIONS` correspond to them, with the correct subdirectories. GitBinSuffixes::assert_from(&pf, locations).assert_architectures(); - // FIXME: Assert the other relationships between pf values and ALTERNATIVE_LOCATIONS contents! + // FIXME: Assert that the directory separators are `/` in the underlying `OsString`s. } #[test] From 167dc14f917c23104e01ab591bb00ccd26ba1c11 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Sat, 13 Jul 2024 02:12:05 -0400 Subject: [PATCH 17/34] Start implementing a helper for ALTERNATIVE_LOCATIONS --- gix-path/src/env/git.rs | 86 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 86 insertions(+) diff --git a/gix-path/src/env/git.rs b/gix-path/src/env/git.rs index 3b69a2e3ed2..a4f537c0e33 100644 --- a/gix-path/src/env/git.rs +++ b/gix-path/src/env/git.rs @@ -15,6 +15,92 @@ pub(super) static ALTERNATIVE_LOCATIONS: Lazy> = Lazy::new(|| { #[cfg(not(windows))] pub(super) static ALTERNATIVE_LOCATIONS: Lazy> = Lazy::new(|| vec![]); +#[cfg(any(windows, test))] // So this can always be tested. +fn alternative_locations_from_env(var_os_func: F) -> Vec +where + F: Fn(&str) -> Option, +{ + // FIXME: Define pairs of the environment variable name and the path suffix to apply, or + // something, because right now this new function is totally wrong because it returns the + // program files directory paths instead of the Git for Windows bin directory paths under them. + // + // But I am not really sure what this should do to handle the ProgramFiles environment variable + // and figure out if it is 64-bit or 32-bit -- which we need in order to know whether to append + // `Git/mingw64/bin` or `Git/mingw32/bin`. We do need to look at the ProgramFiles environment + // variable in addition to the other two preferable architecture-specific ones (for which the + // mingw64 vs. mingw32 decision is easy), at least some of the time, for two reasons. + // + // First, we may be on a 32-bit system. Then we do not have either of the other two variables. + // + // Second, the others may also absent on a 64-bit system, if a parent process whittles down the + // variables to pass to this child process, out of an erroneous belief about which ones are + // needed to provide access to the program files directory. + // + // On a 64-bit system, the way a child process inherits its ProgramFiles variable to be for its + // own architecture -- since after all its parent's architecture could be different -- is: + // + // - The value of a 64-bit child's ProgramFiles variable comes from whatever the parent passed + // down as ProgramW6432, if that variable was passed down. + // + // - The value of a 32-bit child's ProgramFiles variable comes from whatever the parent passed + // down as ProgramFiles(x86), if that variable was passed down. + // + // - If the variable corresponding to the child's architecture was not passed down by the + // parent, but ProgramFiles was passed down, then the child receives that as the value of + // its ProgramFiles variable. Only in this case does ProgramFiles come from ProgramFiles. + // + // The problem -- besides that those rules are not well known, and parent processes that seek + // to pass down minimal environments often do not take heed of them, such that a child process + // will get the wrong architecture's value for its ProgramFiles environment variable -- is that + // the point of this function is to make use of environment variables in order to avoid: + // + // - Calling Windows API functions explicitly, even via the higher level `windows` crate. + // - Accessing the Windows registry, even through the very widely used `winreg` crate. + // - Adding any significant new production dependencies, such as the `known-folders` crate. + // + // Possible solutions: + // + // 1. Abandon at least one of those goals. + // 2. Check both `mingw*` subdirectories regardless of which program files Git is in. + // 3. Inspect the names for substrings like ` (x86)` in an expected position. + // 4. Assume the value of `ProgramFiles` is correct, i.e., is for the process's architecture. + // + // I think (4) is the way to go, at least until (1) is assessed. With (2), we would be checking + // paths that there is no specific reason to think have *working* Git executables...though this + // does have the advantage that its logic would be the same as would be needed in the local + // program files directory (usually `%LocalAppData$/Programs`) if we ever add that. With (3), + // the risk of getting it wrong is low, but the logic is more complex, and we lose the + // simplicity of getting the paths from outside rather than applying assumptions about them. + // + // With (4), we take the ProgramFiles environment variable at its word. This is good not just + // for abstract correctness, but also if the parent modifies these variables intentionally on a + // 64-bit system. A parent process can't reasonably expect this to be followed, because a child + // process may use another mechanism such as known folders. However, following it, when we are + // using environment variables already, satisfies a weaker expectation that the environment + // value *or* actual value (obtainable via known folders or the registry), rather than some + // third value, is used. (4) is also a simple way to do the right thing on a 32-bit system. + let suffix_x86 = + let rules = [ + + ]; + + let names = [ + "ProgramW6432", // 64-bit path from a 32-bit or 64-bit process on a 64-bit system. + "ProgramFiles(x86)", // 32-bit path from a 32-bit or 64-bit process on a 64-bit system. + "ProgramFiles", // 32-bit path on 32-bit system. Or if the parent cleared the others. + ]; + + let mut locations = vec![]; + + for path in names.into_iter().filter_map(var_os_func).map(PathBuf::from) { + if !locations.contains(&path) { + locations.push(path); + } + } + + locations +} + #[cfg(windows)] pub(super) static EXE_NAME: &str = "git.exe"; #[cfg(not(windows))] From c486a7d8b2ad669e9d6daaee53a151b0dea4920b Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Sat, 13 Jul 2024 15:06:45 -0400 Subject: [PATCH 18/34] Fix the helper to join the `mingw*/bin` subdirs --- gix-path/src/env/git.rs | 56 +++++++++++++++++++++++++++++------------ 1 file changed, 40 insertions(+), 16 deletions(-) diff --git a/gix-path/src/env/git.rs b/gix-path/src/env/git.rs index a4f537c0e33..baa8ebf08ab 100644 --- a/gix-path/src/env/git.rs +++ b/gix-path/src/env/git.rs @@ -20,12 +20,8 @@ fn alternative_locations_from_env(var_os_func: F) -> Vec where F: Fn(&str) -> Option, { - // FIXME: Define pairs of the environment variable name and the path suffix to apply, or - // something, because right now this new function is totally wrong because it returns the - // program files directory paths instead of the Git for Windows bin directory paths under them. - // - // But I am not really sure what this should do to handle the ProgramFiles environment variable - // and figure out if it is 64-bit or 32-bit -- which we need in order to know whether to append + // I am not really sure what this should do to handle the ProgramFiles environment variable and + // figure out if it is 64-bit or 32-bit -- which we need in order to know whether to append // `Git/mingw64/bin` or `Git/mingw32/bin`. We do need to look at the ProgramFiles environment // variable in addition to the other two preferable architecture-specific ones (for which the // mingw64 vs. mingw32 decision is easy), at least some of the time, for two reasons. @@ -79,22 +75,50 @@ where // using environment variables already, satisfies a weaker expectation that the environment // value *or* actual value (obtainable via known folders or the registry), rather than some // third value, is used. (4) is also a simple way to do the right thing on a 32-bit system. - let suffix_x86 = - let rules = [ - ]; + // Should give a 64-bit program files path from a 32-bit or 64-bit process on a 64-bit system. + let varname_64bit = "ProgramW6432"; + + // Should give a 32-bit program files path from a 32-bit or 64-bit process on a 64-bit system. + // This variable is x86-specific, but neither Git nor Rust target 32-bit ARM on Windows. + let varname_x86 = "ProgramFiles(x86)"; + + // Should give a 32-bit program files path on a 32-bit system. We also need to check it on a + // 64-bit system. [FIXME: Somehow briefly explain why we still need to do that.] + let varname_current = "ProgramFiles"; + + // 64-bit relative bin dir. So far, this is always mingw64, not ucrt64, clang64, or clangarm64. + let suffix_64 = Path::new("bin/mingw64"); - let names = [ - "ProgramW6432", // 64-bit path from a 32-bit or 64-bit process on a 64-bit system. - "ProgramFiles(x86)", // 32-bit path from a 32-bit or 64-bit process on a 64-bit system. - "ProgramFiles", // 32-bit path on 32-bit system. Or if the parent cleared the others. + // 32-bit relative bin dir. This is always mingw32, not clang32. + let suffix_32 = Path::new("bin/mingw32"); + + // Whichever of the 64-bit or 32-bit relative bin better matches this process's architecture. + // Unlike the system architecture, the process architecture is always known at compile time. + #[cfg(target_pointer_width = "64")] + let suffix_current = suffix_64; + #[cfg(target_pointer_width = "32")] + let suffix_current = suffix_32; + + let rules = [ + (varname_64bit, suffix_64), + (varname_x86, suffix_32), + (varname_current, suffix_current), ]; let mut locations = vec![]; - for path in names.into_iter().filter_map(var_os_func).map(PathBuf::from) { - if !locations.contains(&path) { - locations.push(path); + for (name, suffix) in rules { + if let Some(value) = var_os_func(name) { + let pf = Path::new(&value); + if pf.is_relative() { + continue; + }; + let components = pf.iter().chain(suffix.iter()); + let location = PathBuf::from_iter(components); + if !locations.contains(&location) { + locations.push(location); + } } } From df175bc1a8d11a72dd059464a6553f675570dddd Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Sat, 13 Jul 2024 16:59:49 -0400 Subject: [PATCH 19/34] Rename helper; condense huge comment and move into position --- gix-path/src/env/git.rs | 72 +++++++---------------------------------- 1 file changed, 11 insertions(+), 61 deletions(-) diff --git a/gix-path/src/env/git.rs b/gix-path/src/env/git.rs index baa8ebf08ab..6f21965556f 100644 --- a/gix-path/src/env/git.rs +++ b/gix-path/src/env/git.rs @@ -15,67 +15,11 @@ pub(super) static ALTERNATIVE_LOCATIONS: Lazy> = Lazy::new(|| { #[cfg(not(windows))] pub(super) static ALTERNATIVE_LOCATIONS: Lazy> = Lazy::new(|| vec![]); -#[cfg(any(windows, test))] // So this can always be tested. -fn alternative_locations_from_env(var_os_func: F) -> Vec +#[cfg(any(windows, test))] +fn alternative_windows_locations_from_environment(var_os_func: F) -> Vec where F: Fn(&str) -> Option, { - // I am not really sure what this should do to handle the ProgramFiles environment variable and - // figure out if it is 64-bit or 32-bit -- which we need in order to know whether to append - // `Git/mingw64/bin` or `Git/mingw32/bin`. We do need to look at the ProgramFiles environment - // variable in addition to the other two preferable architecture-specific ones (for which the - // mingw64 vs. mingw32 decision is easy), at least some of the time, for two reasons. - // - // First, we may be on a 32-bit system. Then we do not have either of the other two variables. - // - // Second, the others may also absent on a 64-bit system, if a parent process whittles down the - // variables to pass to this child process, out of an erroneous belief about which ones are - // needed to provide access to the program files directory. - // - // On a 64-bit system, the way a child process inherits its ProgramFiles variable to be for its - // own architecture -- since after all its parent's architecture could be different -- is: - // - // - The value of a 64-bit child's ProgramFiles variable comes from whatever the parent passed - // down as ProgramW6432, if that variable was passed down. - // - // - The value of a 32-bit child's ProgramFiles variable comes from whatever the parent passed - // down as ProgramFiles(x86), if that variable was passed down. - // - // - If the variable corresponding to the child's architecture was not passed down by the - // parent, but ProgramFiles was passed down, then the child receives that as the value of - // its ProgramFiles variable. Only in this case does ProgramFiles come from ProgramFiles. - // - // The problem -- besides that those rules are not well known, and parent processes that seek - // to pass down minimal environments often do not take heed of them, such that a child process - // will get the wrong architecture's value for its ProgramFiles environment variable -- is that - // the point of this function is to make use of environment variables in order to avoid: - // - // - Calling Windows API functions explicitly, even via the higher level `windows` crate. - // - Accessing the Windows registry, even through the very widely used `winreg` crate. - // - Adding any significant new production dependencies, such as the `known-folders` crate. - // - // Possible solutions: - // - // 1. Abandon at least one of those goals. - // 2. Check both `mingw*` subdirectories regardless of which program files Git is in. - // 3. Inspect the names for substrings like ` (x86)` in an expected position. - // 4. Assume the value of `ProgramFiles` is correct, i.e., is for the process's architecture. - // - // I think (4) is the way to go, at least until (1) is assessed. With (2), we would be checking - // paths that there is no specific reason to think have *working* Git executables...though this - // does have the advantage that its logic would be the same as would be needed in the local - // program files directory (usually `%LocalAppData$/Programs`) if we ever add that. With (3), - // the risk of getting it wrong is low, but the logic is more complex, and we lose the - // simplicity of getting the paths from outside rather than applying assumptions about them. - // - // With (4), we take the ProgramFiles environment variable at its word. This is good not just - // for abstract correctness, but also if the parent modifies these variables intentionally on a - // 64-bit system. A parent process can't reasonably expect this to be followed, because a child - // process may use another mechanism such as known folders. However, following it, when we are - // using environment variables already, satisfies a weaker expectation that the environment - // value *or* actual value (obtainable via known folders or the registry), rather than some - // third value, is used. (4) is also a simple way to do the right thing on a 32-bit system. - // Should give a 64-bit program files path from a 32-bit or 64-bit process on a 64-bit system. let varname_64bit = "ProgramW6432"; @@ -83,14 +27,20 @@ where // This variable is x86-specific, but neither Git nor Rust target 32-bit ARM on Windows. let varname_x86 = "ProgramFiles(x86)"; - // Should give a 32-bit program files path on a 32-bit system. We also need to check it on a - // 64-bit system. [FIXME: Somehow briefly explain why we still need to do that.] + // Should give a 32-bit program files path on a 32-bit system. We also check this on a 64-bit + // system, even though it *should* equal the process architecture specific variable, so that we + // cover the case of a parent process that passes down an overly sanitized environment that + // lacks the architecture-specific variable. On a 64-bit system, because parent and child + // processes' architectures can be different, Windows sets the child's ProgramFiles variable + // from the ProgramW6432 or ProgramFiles(x86) variable applicable to the child's architecture. + // Only if the parent does not pass that down is the passed-down ProgramFiles variable even + // used. But this behavior is not well known, so that situation does sometimes happen. let varname_current = "ProgramFiles"; // 64-bit relative bin dir. So far, this is always mingw64, not ucrt64, clang64, or clangarm64. let suffix_64 = Path::new("bin/mingw64"); - // 32-bit relative bin dir. This is always mingw32, not clang32. + // 32-bit relative bin dir. So far, this is always mingw32, not clang32. let suffix_32 = Path::new("bin/mingw32"); // Whichever of the 64-bit or 32-bit relative bin better matches this process's architecture. From 00127a7195cd9552ec9ee7377872b5ae32a87408 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Sat, 13 Jul 2024 17:01:31 -0400 Subject: [PATCH 20/34] Remove a fixme that should no longer be done Add a couple explanatory comments in the implementation, one of which relates to (the absence of) this feature. --- gix-path/src/env/git.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/gix-path/src/env/git.rs b/gix-path/src/env/git.rs index 6f21965556f..01cfa62734c 100644 --- a/gix-path/src/env/git.rs +++ b/gix-path/src/env/git.rs @@ -62,8 +62,10 @@ where if let Some(value) = var_os_func(name) { let pf = Path::new(&value); if pf.is_relative() { + // This should never happen, but if it does then we should not use the path. continue; }; + // Chain components to weakly normalize the path, mostly just for its separators. let components = pf.iter().chain(suffix.iter()); let location = PathBuf::from_iter(components); if !locations.contains(&location) { @@ -429,8 +431,6 @@ mod tests { // Check that `ALTERNATIVE_LOCATIONS` correspond to them, with the correct subdirectories. GitBinSuffixes::assert_from(&pf, locations).assert_architectures(); - - // FIXME: Assert that the directory separators are `/` in the underlying `OsString`s. } #[test] From 1f0c3bf87905ead954646c045c846422cffcf67c Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Sat, 13 Jul 2024 17:15:48 -0400 Subject: [PATCH 21/34] Add the missing `Git` component in the suffixes --- gix-path/src/env/git.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/gix-path/src/env/git.rs b/gix-path/src/env/git.rs index 01cfa62734c..98058837e1b 100644 --- a/gix-path/src/env/git.rs +++ b/gix-path/src/env/git.rs @@ -38,10 +38,10 @@ where let varname_current = "ProgramFiles"; // 64-bit relative bin dir. So far, this is always mingw64, not ucrt64, clang64, or clangarm64. - let suffix_64 = Path::new("bin/mingw64"); + let suffix_64 = Path::new("Git/bin/mingw64"); // 32-bit relative bin dir. So far, this is always mingw32, not clang32. - let suffix_32 = Path::new("bin/mingw32"); + let suffix_32 = Path::new("Git/bin/mingw32"); // Whichever of the 64-bit or 32-bit relative bin better matches this process's architecture. // Unlike the system architecture, the process architecture is always known at compile time. From c8b2eb35f880d1257318327bc6980f753a45e3b6 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Sat, 13 Jul 2024 18:07:22 -0400 Subject: [PATCH 22/34] Simplify path construction and config; improve comments This stops attempting to make the Windows-specific helper function semi-portable. I had done that with the idea tha it could be tested on all platforms so as to gain better coverage in practice. But it doesn't really make sense to do this because it keeps the native path separator `\` from being used even where it is now clearer to use it, and because such tests would not be able to use common values such as `C:\Program Files` because a path that starts with `C:\`, while absolute on Windows, is relative on Unix-like systems. --- gix-path/src/env/git.rs | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/gix-path/src/env/git.rs b/gix-path/src/env/git.rs index 98058837e1b..b01ced55822 100644 --- a/gix-path/src/env/git.rs +++ b/gix-path/src/env/git.rs @@ -15,7 +15,7 @@ pub(super) static ALTERNATIVE_LOCATIONS: Lazy> = Lazy::new(|| { #[cfg(not(windows))] pub(super) static ALTERNATIVE_LOCATIONS: Lazy> = Lazy::new(|| vec![]); -#[cfg(any(windows, test))] +#[cfg(windows)] fn alternative_windows_locations_from_environment(var_os_func: F) -> Vec where F: Fn(&str) -> Option, @@ -38,10 +38,10 @@ where let varname_current = "ProgramFiles"; // 64-bit relative bin dir. So far, this is always mingw64, not ucrt64, clang64, or clangarm64. - let suffix_64 = Path::new("Git/bin/mingw64"); + let suffix_64 = Path::new(r"Git\bin\mingw64"); // 32-bit relative bin dir. So far, this is always mingw32, not clang32. - let suffix_32 = Path::new("Git/bin/mingw32"); + let suffix_32 = Path::new(r"Git\bin\mingw32"); // Whichever of the 64-bit or 32-bit relative bin better matches this process's architecture. // Unlike the system architecture, the process architecture is always known at compile time. @@ -62,12 +62,11 @@ where if let Some(value) = var_os_func(name) { let pf = Path::new(&value); if pf.is_relative() { - // This should never happen, but if it does then we should not use the path. + // This shouldn't happen, but if it does then don't use the path. This is mainly in + // case we are accidentally invoked with the environment variable set but empty. continue; }; - // Chain components to weakly normalize the path, mostly just for its separators. - let components = pf.iter().chain(suffix.iter()); - let location = PathBuf::from_iter(components); + let location = pf.join(suffix); if !locations.contains(&location) { locations.push(location); } @@ -289,7 +288,7 @@ mod tests { .expect("The x86 program files folder will in practice always be available."); let maybe_pf_64bit = RegKey::predef(HKEY_LOCAL_MACHINE) - .open_subkey_with_flags(r#"SOFTWARE\Microsoft\Windows\CurrentVersion"#, KEY_QUERY_VALUE) + .open_subkey_with_flags(r"SOFTWARE\Microsoft\Windows\CurrentVersion", KEY_QUERY_VALUE) .expect("The `CurrentVersion` key exists and allows reading.") .get_value::("ProgramW6432Dir") .map(PathBuf::from) From 4d9853549fa4750de35f8a5b4306ccb105dfff0e Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Sat, 13 Jul 2024 18:15:32 -0400 Subject: [PATCH 23/34] Reorder statements in alternative_locations test for clarity --- gix-path/src/env/git.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/gix-path/src/env/git.rs b/gix-path/src/env/git.rs index b01ced55822..f715fe211dc 100644 --- a/gix-path/src/env/git.rs +++ b/gix-path/src/env/git.rs @@ -423,12 +423,11 @@ mod tests { #[test] #[cfg(windows)] fn alternative_locations() { - let locations = super::ALTERNATIVE_LOCATIONS.as_slice(); - // Obtain program files directory paths by other means and check that they seem correct. let pf = ProgramFilesPaths::obtain_envlessly().validate(); // Check that `ALTERNATIVE_LOCATIONS` correspond to them, with the correct subdirectories. + let locations = super::ALTERNATIVE_LOCATIONS.as_slice(); GitBinSuffixes::assert_from(&pf, locations).assert_architectures(); } From de7c49fbd61ae518db0eb0ad5ec862569e6dae61 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Sat, 13 Jul 2024 19:31:05 -0400 Subject: [PATCH 24/34] Call fewer different things "suffixes" + Adjust wording of some related comments to further decrease confusion. + Put more details in the assert_from panic message when the list of paths has the wrong length, since we would want to know what's going on. --- gix-path/src/env/git.rs | 24 +++++++++++++----------- 1 file changed, 13 insertions(+), 11 deletions(-) diff --git a/gix-path/src/env/git.rs b/gix-path/src/env/git.rs index f715fe211dc..c1c76cffb97 100644 --- a/gix-path/src/env/git.rs +++ b/gix-path/src/env/git.rs @@ -238,8 +238,10 @@ mod tests { } #[cfg(windows)] - fn ends_with_case_insensitive(text: &OsStr, suffix: &str) -> Option { - Some(text.to_str()?.to_lowercase().ends_with(&suffix.to_lowercase())) + fn ends_with_case_insensitive(full_text: &OsStr, literal_pattern: &str) -> Option { + let folded_text = full_text.to_str()?.to_lowercase(); + let folded_pattern = literal_pattern.to_lowercase(); + Some(folded_text.ends_with(&folded_pattern)) } /// The common global program files paths on this system, by process and system architecture. @@ -317,11 +319,11 @@ mod tests { self.x86.as_os_str(), "Our program files path is exactly identical to the 32-bit one.", ); - for arch_suffix in [" (x86)", " (Arm)"] { - let has_arch_suffix = ends_with_case_insensitive(self.current.as_os_str(), arch_suffix) + for trailing_arch in [" (x86)", " (Arm)"] { + let is_adorned = ends_with_case_insensitive(self.current.as_os_str(), trailing_arch) .expect("Assume the test system's important directories are valid Unicode."); assert!( - !has_arch_suffix, + !is_adorned, "The 32-bit program files directory name on a 32-bit system mentions no architecture.", ); } @@ -369,14 +371,14 @@ mod tests { /// Paths relative to process architecture specific program files directories. #[cfg(windows)] #[derive(Clone, Debug)] - struct GitBinSuffixes<'a> { + struct RelativeGitBinPaths<'a> { x86: &'a Path, maybe_64bit: Option<&'a Path>, } #[cfg(windows)] - impl<'a> GitBinSuffixes<'a> { - /// Assert that `locations` has the given prefixes, and extract the suffixes. + impl<'a> RelativeGitBinPaths<'a> { + /// Assert that `locations` has the given path prefixes, and extract the suffixes. fn assert_from(pf: &'a ProgramFilesPaths, locations: &'static [PathBuf]) -> Self { match locations { [primary, secondary] => { @@ -402,11 +404,11 @@ mod tests { maybe_64bit: None, } } - other => panic!("Got length {}, expected 1 or 2.", other.len()), + other => panic!("{:?} has length {}, expected 1 or 2.", other, other.len()), } } - /// Assert that the suffixes are the common per-architecture Git install locations. + /// Assert that the suffixes (relative subdirectories) are the common per-architecture Git install locations. fn assert_architectures(&self) { assert_eq!(self.x86, Path::new("Git/mingw32/bin")); @@ -428,7 +430,7 @@ mod tests { // Check that `ALTERNATIVE_LOCATIONS` correspond to them, with the correct subdirectories. let locations = super::ALTERNATIVE_LOCATIONS.as_slice(); - GitBinSuffixes::assert_from(&pf, locations).assert_architectures(); + RelativeGitBinPaths::assert_from(&pf, locations).assert_architectures(); } #[test] From e990bcd48212db0d5b39c1a2c5b3cf1aa22faa6d Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Sat, 13 Jul 2024 23:19:59 -0400 Subject: [PATCH 25/34] Rename helper, write its tests (and their helpers), fix a bug The new name is locations_under_program_files. The bug was in the order of the `bin` and `mingw*` components. --- gix-path/src/env/git.rs | 128 ++++++++++++++++++++++++++++++++++++++-- 1 file changed, 123 insertions(+), 5 deletions(-) diff --git a/gix-path/src/env/git.rs b/gix-path/src/env/git.rs index c1c76cffb97..a513b7ddd50 100644 --- a/gix-path/src/env/git.rs +++ b/gix-path/src/env/git.rs @@ -16,7 +16,7 @@ pub(super) static ALTERNATIVE_LOCATIONS: Lazy> = Lazy::new(|| { pub(super) static ALTERNATIVE_LOCATIONS: Lazy> = Lazy::new(|| vec![]); #[cfg(windows)] -fn alternative_windows_locations_from_environment(var_os_func: F) -> Vec +fn locations_under_program_files(var_os_func: F) -> Vec where F: Fn(&str) -> Option, { @@ -28,8 +28,8 @@ where let varname_x86 = "ProgramFiles(x86)"; // Should give a 32-bit program files path on a 32-bit system. We also check this on a 64-bit - // system, even though it *should* equal the process architecture specific variable, so that we - // cover the case of a parent process that passes down an overly sanitized environment that + // system, even though it *should* equal the process's architecture specific variable, so that + // we cover the case of a parent process that passes down an overly sanitized environment that // lacks the architecture-specific variable. On a 64-bit system, because parent and child // processes' architectures can be different, Windows sets the child's ProgramFiles variable // from the ProgramW6432 or ProgramFiles(x86) variable applicable to the child's architecture. @@ -38,10 +38,10 @@ where let varname_current = "ProgramFiles"; // 64-bit relative bin dir. So far, this is always mingw64, not ucrt64, clang64, or clangarm64. - let suffix_64 = Path::new(r"Git\bin\mingw64"); + let suffix_64 = Path::new(r"Git\mingw64\bin"); // 32-bit relative bin dir. So far, this is always mingw32, not clang32. - let suffix_32 = Path::new(r"Git\bin\mingw32"); + let suffix_32 = Path::new(r"Git\mingw32\bin"); // Whichever of the 64-bit or 32-bit relative bin better matches this process's architecture. // Unlike the system architecture, the process architecture is always known at compile time. @@ -189,6 +189,124 @@ mod tests { } } + #[cfg(windows)] + macro_rules! var_os_stub { + { $($name:expr => $value:expr),* $(,)? } => { + |key| { + match key { + $( + $name => Some(OsString::from($value)), + )* + _ => None, + } + } + }; + } + + #[cfg(windows)] + macro_rules! locations_from { + ($($name:expr => $value:expr),* $(,)?) => { + super::locations_under_program_files(var_os_stub! { + $( + $name => $value, + )* + }) + } + } + + #[cfg(windows)] + macro_rules! pathbuf_vec { + [$($path:expr),* $(,)?] => { + vec![$( + PathBuf::from($path), + )*] + } + } + + #[test] + #[cfg(windows)] + fn locations_under_program_files_ordinary() { + assert_eq!( + locations_from!( + "ProgramFiles" => r"C:\Program Files", + ), + if cfg!(target_pointer_width = "64") { + pathbuf_vec![r"C:\Program Files\Git\mingw64\bin"] + } else { + pathbuf_vec![r"C:\Program Files\Git\mingw32\bin"] + }, + ); + assert_eq!( + locations_from!( + "ProgramFiles" => { + if cfg!(target_pointer_width = "64") { + r"C:\Program Files" + } else { + r"C:\Program Files (x86)" + } + }, + "ProgramFiles(x86)" => r"C:\Program Files (x86)", + "ProgramW6432" => r"C:\Program Files", + ), + pathbuf_vec![ + r"C:\Program Files\Git\mingw64\bin", + r"C:\Program Files (x86)\Git\mingw32\bin", + ], + ); + assert_eq!(locations_from!(), Vec::::new()); + } + + #[test] + #[cfg(windows)] + fn locations_under_program_files_strange() { + assert_eq!( + locations_from!( + "ProgramFiles" => r"X:\cur\rent", + "ProgramFiles(x86)" => r"Y:\nar\row", + "ProgramW6432" => r"Z:\wi\de", + ), + pathbuf_vec![ + r"Z:\wi\de\Git\mingw64\bin", + r"Y:\nar\row\Git\mingw32\bin", + if cfg!(target_pointer_width = "64") { + r"X:\cur\rent\Git\mingw64\bin" + } else { + r"X:\cur\rent\Git\mingw32\bin" + }, + ], + ); + assert_eq!( + locations_from!( + "ProgramW6432" => r"Z:\wi\de", + ), + pathbuf_vec![r"Z:\wi\de\Git\mingw64\bin"], + ); + assert_eq!( + locations_from!( + "ProgramFiles" => r"Z:/wi//de/", + "ProgramFiles(x86)" => r"Y:/\nar\/row", + "ProgramW6432" => r"Z:\wi\.\de", + ), + if cfg!(target_pointer_width = "64") { + pathbuf_vec![r"Z:\wi\de\Git\mingw64\bin", r"Y:\nar\row\Git\mingw32\bin"] + } else { + pathbuf_vec![ + r"Z:\wi\de\Git\mingw64\bin", + r"Y:\nar\row\Git\mingw32\bin", + r"Z:\wi\de\Git\mingw32\bin", + ] + }, + ); + assert_eq!( + locations_from!( + "ProgramFiles" => r"foo\bar", + "ProgramFiles(x86)" => r"\\host\share\subdir", + "ProgramW6432" => r"", + ), + pathbuf_vec![r"\\host\share\subdir\Git\mingw32\bin"], + ); + } + #[cfg(windows)] use { known_folders::{get_known_folder_path, KnownFolder}, From d254e62819b66e5385b633f0670f8456ad2d0b6e Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Sun, 14 Jul 2024 06:40:09 -0400 Subject: [PATCH 26/34] Put the new tests in their own nested mod The module is in the same file, nested inside the `tests` module written there (which this test code had resided directly inside). It is currently named `loc`. This allows the repeated `#[cfg(windows)]` to be eliminated, allows the imports to be presented readably and to be grouped without `cargo fmt` undoing the grouping, and makes clear that these tests are all related to each other more than to the preexsting tests. But I am not sure if this is the best approach to organizing them. Maybe the test code for `locations_under_program_files`, which is a helper function meant to be used only in this module and only to define `ALTERNATIVE_LOCATIONS`, could remain in this file, while the other tests of `ALTERNATIVE_LOCATIONS` could be moved to another file. `ALTERNATIVE_LOCATIONS` has `pub(super)` visibility, so it's not part of the interface of `gix_path::env`, but it could perhaps have tests in another file directly or indirectly inside `gix-path/src/env`. --- gix-path/src/env/git.rs | 632 ++++++++++++++++++++-------------------- 1 file changed, 311 insertions(+), 321 deletions(-) diff --git a/gix-path/src/env/git.rs b/gix-path/src/env/git.rs index a513b7ddd50..b3b675baed1 100644 --- a/gix-path/src/env/git.rs +++ b/gix-path/src/env/git.rs @@ -190,370 +190,360 @@ mod tests { } #[cfg(windows)] - macro_rules! var_os_stub { - { $($name:expr => $value:expr),* $(,)? } => { - |key| { - match key { + mod loc { + macro_rules! var_os_stub { + { $($name:expr => $value:expr),* $(,)? } => { + |key| { + match key { + $( + $name => Some(OsString::from($value)), + )* + _ => None, + } + } + }; + } + + macro_rules! locations_from { + ($($name:expr => $value:expr),* $(,)?) => { + super::super::locations_under_program_files(var_os_stub! { $( - $name => Some(OsString::from($value)), + $name => $value, )* - _ => None, - } + }) } - }; - } - - #[cfg(windows)] - macro_rules! locations_from { - ($($name:expr => $value:expr),* $(,)?) => { - super::locations_under_program_files(var_os_stub! { - $( - $name => $value, - )* - }) } - } - #[cfg(windows)] - macro_rules! pathbuf_vec { - [$($path:expr),* $(,)?] => { - vec![$( - PathBuf::from($path), - )*] + macro_rules! pathbuf_vec { + [$($path:expr),* $(,)?] => { + vec![$( + PathBuf::from($path), + )*] + } } - } - - #[test] - #[cfg(windows)] - fn locations_under_program_files_ordinary() { - assert_eq!( - locations_from!( - "ProgramFiles" => r"C:\Program Files", - ), - if cfg!(target_pointer_width = "64") { - pathbuf_vec![r"C:\Program Files\Git\mingw64\bin"] - } else { - pathbuf_vec![r"C:\Program Files\Git\mingw32\bin"] - }, - ); - assert_eq!( - locations_from!( - "ProgramFiles" => { - if cfg!(target_pointer_width = "64") { - r"C:\Program Files" - } else { - r"C:\Program Files (x86)" - } - }, - "ProgramFiles(x86)" => r"C:\Program Files (x86)", - "ProgramW6432" => r"C:\Program Files", - ), - pathbuf_vec![ - r"C:\Program Files\Git\mingw64\bin", - r"C:\Program Files (x86)\Git\mingw32\bin", - ], - ); - assert_eq!(locations_from!(), Vec::::new()); - } - #[test] - #[cfg(windows)] - fn locations_under_program_files_strange() { - assert_eq!( - locations_from!( - "ProgramFiles" => r"X:\cur\rent", - "ProgramFiles(x86)" => r"Y:\nar\row", - "ProgramW6432" => r"Z:\wi\de", - ), - pathbuf_vec![ - r"Z:\wi\de\Git\mingw64\bin", - r"Y:\nar\row\Git\mingw32\bin", + #[test] + fn locations_under_program_files_ordinary() { + assert_eq!( + locations_from!( + "ProgramFiles" => r"C:\Program Files", + ), if cfg!(target_pointer_width = "64") { - r"X:\cur\rent\Git\mingw64\bin" + pathbuf_vec![r"C:\Program Files\Git\mingw64\bin"] } else { - r"X:\cur\rent\Git\mingw32\bin" + pathbuf_vec![r"C:\Program Files\Git\mingw32\bin"] }, - ], - ); - assert_eq!( - locations_from!( - "ProgramW6432" => r"Z:\wi\de", - ), - pathbuf_vec![r"Z:\wi\de\Git\mingw64\bin"], - ); - assert_eq!( - locations_from!( - "ProgramFiles" => r"Z:/wi//de/", - "ProgramFiles(x86)" => r"Y:/\nar\/row", - "ProgramW6432" => r"Z:\wi\.\de", - ), - if cfg!(target_pointer_width = "64") { - pathbuf_vec![r"Z:\wi\de\Git\mingw64\bin", r"Y:\nar\row\Git\mingw32\bin"] - } else { + ); + assert_eq!( + locations_from!( + "ProgramFiles" => { + if cfg!(target_pointer_width = "64") { + r"C:\Program Files" + } else { + r"C:\Program Files (x86)" + } + }, + "ProgramFiles(x86)" => r"C:\Program Files (x86)", + "ProgramW6432" => r"C:\Program Files", + ), + pathbuf_vec![ + r"C:\Program Files\Git\mingw64\bin", + r"C:\Program Files (x86)\Git\mingw32\bin", + ], + ); + assert_eq!(locations_from!(), Vec::::new()); + } + + #[test] + fn locations_under_program_files_strange() { + assert_eq!( + locations_from!( + "ProgramFiles" => r"X:\cur\rent", + "ProgramFiles(x86)" => r"Y:\nar\row", + "ProgramW6432" => r"Z:\wi\de", + ), pathbuf_vec![ r"Z:\wi\de\Git\mingw64\bin", r"Y:\nar\row\Git\mingw32\bin", - r"Z:\wi\de\Git\mingw32\bin", - ] - }, - ); - assert_eq!( - locations_from!( - "ProgramFiles" => r"foo\bar", - "ProgramFiles(x86)" => r"\\host\share\subdir", - "ProgramW6432" => r"", - ), - pathbuf_vec![r"\\host\share\subdir\Git\mingw32\bin"], - ); - } + if cfg!(target_pointer_width = "64") { + r"X:\cur\rent\Git\mingw64\bin" + } else { + r"X:\cur\rent\Git\mingw32\bin" + }, + ], + ); + assert_eq!( + locations_from!( + "ProgramW6432" => r"Z:\wi\de", + ), + pathbuf_vec![r"Z:\wi\de\Git\mingw64\bin"], + ); + assert_eq!( + locations_from!( + "ProgramFiles" => r"Z:/wi//de/", + "ProgramFiles(x86)" => r"Y:/\nar\/row", + "ProgramW6432" => r"Z:\wi\.\de", + ), + if cfg!(target_pointer_width = "64") { + pathbuf_vec![r"Z:\wi\de\Git\mingw64\bin", r"Y:\nar\row\Git\mingw32\bin"] + } else { + pathbuf_vec![ + r"Z:\wi\de\Git\mingw64\bin", + r"Y:\nar\row\Git\mingw32\bin", + r"Z:\wi\de\Git\mingw32\bin", + ] + }, + ); + assert_eq!( + locations_from!( + "ProgramFiles" => r"foo\bar", + "ProgramFiles(x86)" => r"\\host\share\subdir", + "ProgramW6432" => r"", + ), + pathbuf_vec![r"\\host\share\subdir\Git\mingw32\bin"], + ); + } - #[cfg(windows)] - use { - known_folders::{get_known_folder_path, KnownFolder}, - std::ffi::{OsStr, OsString}, - std::io::ErrorKind, - std::path::PathBuf, - windows::core::Result as WindowsResult, - windows::Win32::Foundation::BOOL, - windows::Win32::System::Threading::{GetCurrentProcess, IsWow64Process}, - winreg::enums::{HKEY_LOCAL_MACHINE, KEY_QUERY_VALUE}, - winreg::RegKey, - }; + use std::ffi::{OsStr, OsString}; + use std::io::ErrorKind; + use std::path::{Path, PathBuf}; - #[cfg(windows)] - trait Current: Sized { - fn current() -> WindowsResult; - } + use known_folders::{get_known_folder_path, KnownFolder}; + use windows::core::Result as WindowsResult; + use windows::Win32::Foundation::BOOL; + use windows::Win32::System::Threading::{GetCurrentProcess, IsWow64Process}; + use winreg::enums::{HKEY_LOCAL_MACHINE, KEY_QUERY_VALUE}; + use winreg::RegKey; - #[cfg(windows)] - #[derive(Clone, Copy, Debug)] - enum PlatformArchitecture { - Is32on32, - Is32on64, - Is64on64, - } + trait Current: Sized { + fn current() -> WindowsResult; + } - #[cfg(windows)] - impl Current for PlatformArchitecture { - fn current() -> WindowsResult { - // Ordinarily, we would check the target pointer width first to avoid doing extra work, - // because if this is a 64-bit executable then the operating system is 64-bit. But this - // is for the test suite, and doing it this way allows problems to be caught earlier if - // a change made on a 64-bit development machine breaks the IsWow64Process() call. - let mut wow64process = BOOL::default(); - unsafe { IsWow64Process(GetCurrentProcess(), &mut wow64process)? }; - - let platform_architecture = if wow64process.as_bool() { - Self::Is32on64 - } else if cfg!(target_pointer_width = "32") { - Self::Is32on32 - } else { - assert!(cfg!(target_pointer_width = "64")); - Self::Is64on64 - }; - Ok(platform_architecture) + #[derive(Clone, Copy, Debug)] + enum PlatformArchitecture { + Is32on32, + Is32on64, + Is64on64, } - } - #[cfg(windows)] - fn ends_with_case_insensitive(full_text: &OsStr, literal_pattern: &str) -> Option { - let folded_text = full_text.to_str()?.to_lowercase(); - let folded_pattern = literal_pattern.to_lowercase(); - Some(folded_text.ends_with(&folded_pattern)) - } + impl Current for PlatformArchitecture { + fn current() -> WindowsResult { + // Ordinarily, we would check the target pointer width first to avoid doing extra work, + // because if this is a 64-bit executable then the operating system is 64-bit. But this + // is for the test suite, and doing it this way allows problems to be caught earlier if + // a change made on a 64-bit development machine breaks the IsWow64Process() call. + let mut wow64process = BOOL::default(); + unsafe { IsWow64Process(GetCurrentProcess(), &mut wow64process)? }; + + let platform_architecture = if wow64process.as_bool() { + Self::Is32on64 + } else if cfg!(target_pointer_width = "32") { + Self::Is32on32 + } else { + assert!(cfg!(target_pointer_width = "64")); + Self::Is64on64 + }; + Ok(platform_architecture) + } + } - /// The common global program files paths on this system, by process and system architecture. - #[cfg(windows)] - #[derive(Clone, Debug)] - struct ProgramFilesPaths { - /// The program files directory used for whatever architecture this program was built for. - current: PathBuf, - - /// The x86 program files directory regardless of the architecture of the program. - /// - /// If Rust gains Windows targets like ARMv7 where this is unavailable, this could fail. - x86: PathBuf, - - /// The 64-bit program files directory if there is one. - /// - /// This is present on x64 and also ARM64 systems. On an ARM64 system, ARM64 and AMD64 - /// programs use the same program files directory while 32-bit x86 and ARM programs use two - /// others. Only a 32-bit has no 64-bit program files directory. - maybe_64bit: Option, - } + fn ends_with_case_insensitive(full_text: &OsStr, literal_pattern: &str) -> Option { + let folded_text = full_text.to_str()?.to_lowercase(); + let folded_pattern = literal_pattern.to_lowercase(); + Some(folded_text.ends_with(&folded_pattern)) + } - impl ProgramFilesPaths { - /// Gets the three common kinds of global program files paths without environment variables. - /// - /// The idea here is to obtain this information, which the `alternative_locations()` unit - /// test uses to learn the expected alternative locations, without duplicating *any* of the - /// approach used for `ALTERNATIVE_LOCATIONS`, so it can be used to test that. The approach - /// here is also more reliable than using environment variables, but it is a bit more - /// complex, and it requires either additional dependencies or the use of unsafe code. - /// - /// This gets `pf_current` and `pf_x86` by the [known folders][known-folders] system. But - /// it gets `maybe_pf_64bit` from the registry, as the corresponding known folder is not - /// available to 32-bit processes. See the [`KNOWNFOLDDERID`][knownfolderid] documentation. - /// - /// If in the future the implementation of `ALTERNATIVE_LOCATIONS` uses these techniques, - /// then this function can be changed to use environment variables and renamed accordingly. - /// - /// [known-folders]: https://learn.microsoft.com/en-us/windows/win32/shell/known-folders - /// [knownfolderid]: https://learn.microsoft.com/en-us/windows/win32/shell/knownfolderid#remarks - fn obtain_envlessly() -> Self { - let pf_current = get_known_folder_path(KnownFolder::ProgramFiles) - .expect("The process architecture specific program files folder is always available."); - - let pf_x86 = get_known_folder_path(KnownFolder::ProgramFilesX86) - .expect("The x86 program files folder will in practice always be available."); - - let maybe_pf_64bit = RegKey::predef(HKEY_LOCAL_MACHINE) - .open_subkey_with_flags(r"SOFTWARE\Microsoft\Windows\CurrentVersion", KEY_QUERY_VALUE) - .expect("The `CurrentVersion` key exists and allows reading.") - .get_value::("ProgramW6432Dir") - .map(PathBuf::from) - .map_err(|error| { - assert_eq!(error.kind(), ErrorKind::NotFound); - error - }) - .ok(); + /// The common global program files paths on this system, by process and system architecture. + #[derive(Clone, Debug)] + struct ProgramFilesPaths { + /// The program files directory used for whatever architecture this program was built for. + current: PathBuf, + + /// The x86 program files directory regardless of the architecture of the program. + /// + /// If Rust gains Windows targets like ARMv7 where this is unavailable, this could fail. + x86: PathBuf, + + /// The 64-bit program files directory if there is one. + /// + /// This is present on x64 and also ARM64 systems. On an ARM64 system, ARM64 and AMD64 + /// programs use the same program files directory while 32-bit x86 and ARM programs use + /// two others. Only a 32-bit has no 64-bit program files directory. + maybe_64bit: Option, + } - Self { - current: pf_current, - x86: pf_x86, - maybe_64bit: maybe_pf_64bit, + impl ProgramFilesPaths { + /// Gets the three common kinds of global program files paths without environment variables. + /// + /// The idea here is to obtain this information, which the `alternative_locations()` unit + /// test uses to learn the expected alternative locations, without duplicating *any* of the + /// approach used for `ALTERNATIVE_LOCATIONS`, so it can be used to test that. The approach + /// here is also more reliable than using environment variables, but it is a bit more + /// complex, and it requires either additional dependencies or the use of unsafe code. + /// + /// This gets `pf_current` and `pf_x86` by the [known folders][known-folders] system. But + /// it gets `maybe_pf_64bit` from the registry, as the corresponding known folder is not + /// available to 32-bit processes. See the [`KNOWNFOLDDERID`][knownfolderid] documentation. + /// + /// If in the future the implementation of `ALTERNATIVE_LOCATIONS` uses these techniques, + /// then this function can be changed to use environment variables and renamed accordingly. + /// + /// [known-folders]: https://learn.microsoft.com/en-us/windows/win32/shell/known-folders + /// [knownfolderid]: https://learn.microsoft.com/en-us/windows/win32/shell/knownfolderid#remarks + fn obtain_envlessly() -> Self { + let pf_current = get_known_folder_path(KnownFolder::ProgramFiles) + .expect("The process architecture specific program files folder is always available."); + + let pf_x86 = get_known_folder_path(KnownFolder::ProgramFilesX86) + .expect("The x86 program files folder will in practice always be available."); + + let maybe_pf_64bit = RegKey::predef(HKEY_LOCAL_MACHINE) + .open_subkey_with_flags(r"SOFTWARE\Microsoft\Windows\CurrentVersion", KEY_QUERY_VALUE) + .expect("The `CurrentVersion` key exists and allows reading.") + .get_value::("ProgramW6432Dir") + .map(PathBuf::from) + .map_err(|error| { + assert_eq!(error.kind(), ErrorKind::NotFound); + error + }) + .ok(); + + Self { + current: pf_current, + x86: pf_x86, + maybe_64bit: maybe_pf_64bit, + } } - } - /// Checks that the paths we got for testing are reasonable. - /// - /// This checks that `obtain_envlessly()` returned paths that are likely to be correct and - /// that satisfy the most important properties based on the current system and process. - fn validate(self) -> Self { - match PlatformArchitecture::current().expect("Process and system 'bitness' should be available.") { - PlatformArchitecture::Is32on32 => { - assert_eq!( - self.current.as_os_str(), - self.x86.as_os_str(), - "Our program files path is exactly identical to the 32-bit one.", - ); - for trailing_arch in [" (x86)", " (Arm)"] { - let is_adorned = ends_with_case_insensitive(self.current.as_os_str(), trailing_arch) - .expect("Assume the test system's important directories are valid Unicode."); - assert!( - !is_adorned, - "The 32-bit program files directory name on a 32-bit system mentions no architecture.", + /// Checks that the paths we got for testing are reasonable. + /// + /// This checks that `obtain_envlessly()` returned paths that are likely to be correct and + /// that satisfy the most important properties based on the current system and process. + fn validate(self) -> Self { + match PlatformArchitecture::current().expect("Process and system 'bitness' should be available.") { + PlatformArchitecture::Is32on32 => { + assert_eq!( + self.current.as_os_str(), + self.x86.as_os_str(), + "Our program files path is exactly identical to the 32-bit one.", + ); + for trailing_arch in [" (x86)", " (Arm)"] { + let is_adorned = ends_with_case_insensitive(self.current.as_os_str(), trailing_arch) + .expect("Assume the test system's important directories are valid Unicode."); + assert!( + !is_adorned, + "The 32-bit program files directory name on a 32-bit system mentions no architecture.", + ); + } + assert_eq!( + self.maybe_64bit, None, + "A 32-bit system has no 64-bit program files directory.", + ); + } + PlatformArchitecture::Is32on64 => { + assert_eq!( + self.current.as_os_str(), + self.x86.as_os_str(), + "Our program files path is exactly identical to the 32-bit one.", + ); + let pf_64bit = self + .maybe_64bit + .as_ref() + .expect("The 64-bit program files directory exists."); + assert_ne!( + &self.x86, pf_64bit, + "The 32-bit and 64-bit program files directories have different locations.", + ); + } + PlatformArchitecture::Is64on64 => { + let pf_64bit = self + .maybe_64bit + .as_ref() + .expect("The 64-bit program files directory exists."); + assert_eq!( + self.current.as_os_str(), + pf_64bit.as_os_str(), + "Our program files path is exactly identical to the 64-bit one.", + ); + assert_ne!( + &self.x86, pf_64bit, + "The 32-bit and 64-bit program files directories have different locations.", ); } - assert_eq!( - self.maybe_64bit, None, - "A 32-bit system has no 64-bit program files directory.", - ); - } - PlatformArchitecture::Is32on64 => { - assert_eq!( - self.current.as_os_str(), - self.x86.as_os_str(), - "Our program files path is exactly identical to the 32-bit one.", - ); - let pf_64bit = self - .maybe_64bit - .as_ref() - .expect("The 64-bit program files directory exists."); - assert_ne!( - &self.x86, pf_64bit, - "The 32-bit and 64-bit program files directories have different locations.", - ); - } - PlatformArchitecture::Is64on64 => { - let pf_64bit = self - .maybe_64bit - .as_ref() - .expect("The 64-bit program files directory exists."); - assert_eq!( - self.current.as_os_str(), - pf_64bit.as_os_str(), - "Our program files path is exactly identical to the 64-bit one.", - ); - assert_ne!( - &self.x86, pf_64bit, - "The 32-bit and 64-bit program files directories have different locations.", - ); } - } - self + self + } } - } - /// Paths relative to process architecture specific program files directories. - #[cfg(windows)] - #[derive(Clone, Debug)] - struct RelativeGitBinPaths<'a> { - x86: &'a Path, - maybe_64bit: Option<&'a Path>, - } + /// Paths relative to process architecture specific program files directories. + #[derive(Clone, Debug)] + struct RelativeGitBinPaths<'a> { + x86: &'a Path, + maybe_64bit: Option<&'a Path>, + } - #[cfg(windows)] - impl<'a> RelativeGitBinPaths<'a> { - /// Assert that `locations` has the given path prefixes, and extract the suffixes. - fn assert_from(pf: &'a ProgramFilesPaths, locations: &'static [PathBuf]) -> Self { - match locations { - [primary, secondary] => { - let prefix_64bit = pf - .maybe_64bit - .as_ref() - .expect("It gives two paths only if one can be 64-bit."); - let suffix_64bit = primary - .strip_prefix(prefix_64bit) - .expect("It gives the 64-bit path and lists it first."); - let suffix_x86 = secondary - .strip_prefix(pf.x86.as_path()) - .expect("It gives the 32-bit path and lists it second."); - Self { - x86: suffix_x86, - maybe_64bit: Some(suffix_64bit), + impl<'a> RelativeGitBinPaths<'a> { + /// Assert that `locations` has the given path prefixes, and extract the suffixes. + fn assert_from(pf: &'a ProgramFilesPaths, locations: &'static [PathBuf]) -> Self { + match locations { + [primary, secondary] => { + let prefix_64bit = pf + .maybe_64bit + .as_ref() + .expect("It gives two paths only if one can be 64-bit."); + let suffix_64bit = primary + .strip_prefix(prefix_64bit) + .expect("It gives the 64-bit path and lists it first."); + let suffix_x86 = secondary + .strip_prefix(pf.x86.as_path()) + .expect("It gives the 32-bit path and lists it second."); + Self { + x86: suffix_x86, + maybe_64bit: Some(suffix_64bit), + } } - } - [only] => { - assert_eq!(pf.maybe_64bit, None, "It gives one path only if none can be 64-bit."); - Self { - x86: only, - maybe_64bit: None, + [only] => { + assert_eq!(pf.maybe_64bit, None, "It gives one path only if none can be 64-bit."); + Self { + x86: only, + maybe_64bit: None, + } } + other => panic!("{:?} has length {}, expected 1 or 2.", other, other.len()), } - other => panic!("{:?} has length {}, expected 1 or 2.", other, other.len()), } - } - /// Assert that the suffixes (relative subdirectories) are the common per-architecture Git install locations. - fn assert_architectures(&self) { - assert_eq!(self.x86, Path::new("Git/mingw32/bin")); + /// Assert that the suffixes (relative subdirectories) are the common per-architecture Git install locations. + fn assert_architectures(&self) { + assert_eq!(self.x86, Path::new("Git/mingw32/bin")); - if let Some(suffix_64bit) = self.maybe_64bit { - // When Git for Windows releases ARM64 builds, there will be another 64-bit suffix, - // likely clangarm64. In that case, this and other assertions will need updating, - // as there will be two separate paths to check under the same 64-bit program files - // directory. (See the definition of ProgramFilesPaths::maybe_64bit for details.) - assert_eq!(suffix_64bit, Path::new("Git/mingw64/bin")); + if let Some(suffix_64bit) = self.maybe_64bit { + // When Git for Windows releases ARM64 builds, there will be another 64-bit suffix, + // likely clangarm64. In that case, this and other assertions will need updating, + // as there will be two separate paths to check under the same 64-bit program files + // directory. (See the definition of ProgramFilesPaths::maybe_64bit for details.) + assert_eq!(suffix_64bit, Path::new("Git/mingw64/bin")); + } } } - } - #[test] - #[cfg(windows)] - fn alternative_locations() { - // Obtain program files directory paths by other means and check that they seem correct. - let pf = ProgramFilesPaths::obtain_envlessly().validate(); + #[test] + fn alternative_locations() { + // Obtain program files directory paths by other means and check that they seem correct. + let pf = ProgramFilesPaths::obtain_envlessly().validate(); - // Check that `ALTERNATIVE_LOCATIONS` correspond to them, with the correct subdirectories. - let locations = super::ALTERNATIVE_LOCATIONS.as_slice(); - RelativeGitBinPaths::assert_from(&pf, locations).assert_architectures(); + // Check that `ALTERNATIVE_LOCATIONS` correspond to them, with the correct subdirectories. + let locations = super::super::ALTERNATIVE_LOCATIONS.as_slice(); + RelativeGitBinPaths::assert_from(&pf, locations).assert_architectures(); + } } - #[test] #[cfg(not(windows))] - fn alternative_locations() { - assert!(super::ALTERNATIVE_LOCATIONS.is_empty()); + mod loc { + #[test] + fn alternative_locations() { + assert!(super::ALTERNATIVE_LOCATIONS.is_empty()); + } } } From 98db88bb81f8b47fe83011aa9ecf1d709b9680c9 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Sun, 14 Jul 2024 14:52:28 -0400 Subject: [PATCH 27/34] Fix test bug generating expected value on 32-bit system --- gix-path/src/env/git.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/gix-path/src/env/git.rs b/gix-path/src/env/git.rs index b3b675baed1..498f2cff141 100644 --- a/gix-path/src/env/git.rs +++ b/gix-path/src/env/git.rs @@ -505,8 +505,11 @@ mod tests { } [only] => { assert_eq!(pf.maybe_64bit, None, "It gives one path only if none can be 64-bit."); + let suffix_x86 = only + .strip_prefix(pf.x86.as_path()) + .expect("The one path it gives is the 32-bit path."); Self { - x86: only, + x86: suffix_x86, maybe_64bit: None, } } From 15235bf7968042da0493d431bbc955d6f9f54188 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Sun, 14 Jul 2024 07:48:55 -0400 Subject: [PATCH 28/34] fix: Don't assume program files folder locations This checks where *program files* directories are located on a Windows system, which are used for a fallback check after `git` has not been found in a `PATH` search (to invoke `git` to find out information such as the location of its system config file). Previously, two hard-coded paths were used. These were correct for the vast majority of 64-bit Windows systems, but were in practice never correct on 32-bit Windows systems. Checking programmatically for the locations should thus enable detection to succeed on more systems and under more circumstances, and avoid other problems. --- gix-path/src/env/git.rs | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/gix-path/src/env/git.rs b/gix-path/src/env/git.rs index 498f2cff141..1ca00acf8fc 100644 --- a/gix-path/src/env/git.rs +++ b/gix-path/src/env/git.rs @@ -6,12 +6,8 @@ use once_cell::sync::Lazy; /// Other places to find Git in. #[cfg(windows)] -pub(super) static ALTERNATIVE_LOCATIONS: Lazy> = Lazy::new(|| { - vec![ - "C:/Program Files/Git/mingw64/bin".into(), - "C:/Program Files (x86)/Git/mingw32/bin".into(), - ] -}); +pub(super) static ALTERNATIVE_LOCATIONS: Lazy> = + Lazy::new(|| locations_under_program_files(|key| std::env::var_os(key))); #[cfg(not(windows))] pub(super) static ALTERNATIVE_LOCATIONS: Lazy> = Lazy::new(|| vec![]); From 8ae54e850463da417dfc87a68d8ca0025f997b7a Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Sun, 14 Jul 2024 18:33:59 -0400 Subject: [PATCH 29/34] Add second `super::` in non-Windows `alternative_locations` test This was needed on both Windows and non-Windows systems ever since I moved the tests from `tests` to `tests::loc`, but I forgot to update it for the non-Windows `alternative_locations` test. --- gix-path/src/env/git.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gix-path/src/env/git.rs b/gix-path/src/env/git.rs index 1ca00acf8fc..1004462b141 100644 --- a/gix-path/src/env/git.rs +++ b/gix-path/src/env/git.rs @@ -542,7 +542,7 @@ mod tests { mod loc { #[test] fn alternative_locations() { - assert!(super::ALTERNATIVE_LOCATIONS.is_empty()); + assert!(super::super::ALTERNATIVE_LOCATIONS.is_empty()); } } } From 464e0a266aa7f3de29750f1c4f77cc4dacf37d91 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Sun, 14 Jul 2024 18:54:01 -0400 Subject: [PATCH 30/34] Thanks clippy --- gix-path/src/env/git.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gix-path/src/env/git.rs b/gix-path/src/env/git.rs index 1004462b141..eb0d7df1e79 100644 --- a/gix-path/src/env/git.rs +++ b/gix-path/src/env/git.rs @@ -9,7 +9,7 @@ use once_cell::sync::Lazy; pub(super) static ALTERNATIVE_LOCATIONS: Lazy> = Lazy::new(|| locations_under_program_files(|key| std::env::var_os(key))); #[cfg(not(windows))] -pub(super) static ALTERNATIVE_LOCATIONS: Lazy> = Lazy::new(|| vec![]); +pub(super) static ALTERNATIVE_LOCATIONS: Lazy> = Lazy::new(Vec::new); #[cfg(windows)] fn locations_under_program_files(var_os_func: F) -> Vec From 76e3b28b8824c00a545faa9079751c8d445c4574 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Sun, 14 Jul 2024 22:31:12 -0400 Subject: [PATCH 31/34] Omit excess punctuation in `expect` messages This removes the periods from the ends of sentences passed as `expect` messages in recently introduced tests and their helpers, because when called on a `Result`, `Some text: Result info` is better than `Some text.: Result info`. Doing this also improves stylistic consistency with other `expect` messages in the project. I didn't make an analogous change to assertion messages, because the awkward `.:` construction does not arise from them. There may be further refinement that can be done, as the new `expect` messages are still worded as assertions and, as such, could be misread as claiming that the situation they decribe wrongly happened, rather than the intended meaning that it wrongly did not happen. This also changes "key" to "registry key" in one of the `expect` messages so that its meaning is clear in output of big test runs. --- gix-path/src/env/git.rs | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/gix-path/src/env/git.rs b/gix-path/src/env/git.rs index eb0d7df1e79..914d19cd14c 100644 --- a/gix-path/src/env/git.rs +++ b/gix-path/src/env/git.rs @@ -388,14 +388,14 @@ mod tests { /// [knownfolderid]: https://learn.microsoft.com/en-us/windows/win32/shell/knownfolderid#remarks fn obtain_envlessly() -> Self { let pf_current = get_known_folder_path(KnownFolder::ProgramFiles) - .expect("The process architecture specific program files folder is always available."); + .expect("The process architecture specific program files folder is always available"); let pf_x86 = get_known_folder_path(KnownFolder::ProgramFilesX86) - .expect("The x86 program files folder will in practice always be available."); + .expect("The x86 program files folder will in practice always be available"); let maybe_pf_64bit = RegKey::predef(HKEY_LOCAL_MACHINE) .open_subkey_with_flags(r"SOFTWARE\Microsoft\Windows\CurrentVersion", KEY_QUERY_VALUE) - .expect("The `CurrentVersion` key exists and allows reading.") + .expect("The `CurrentVersion` registry key exists and allows reading") .get_value::("ProgramW6432Dir") .map(PathBuf::from) .map_err(|error| { @@ -416,7 +416,7 @@ mod tests { /// This checks that `obtain_envlessly()` returned paths that are likely to be correct and /// that satisfy the most important properties based on the current system and process. fn validate(self) -> Self { - match PlatformArchitecture::current().expect("Process and system 'bitness' should be available.") { + match PlatformArchitecture::current().expect("Process and system 'bitness' should be available") { PlatformArchitecture::Is32on32 => { assert_eq!( self.current.as_os_str(), @@ -425,7 +425,7 @@ mod tests { ); for trailing_arch in [" (x86)", " (Arm)"] { let is_adorned = ends_with_case_insensitive(self.current.as_os_str(), trailing_arch) - .expect("Assume the test system's important directories are valid Unicode."); + .expect("Assume the test system's important directories are valid Unicode"); assert!( !is_adorned, "The 32-bit program files directory name on a 32-bit system mentions no architecture.", @@ -445,7 +445,7 @@ mod tests { let pf_64bit = self .maybe_64bit .as_ref() - .expect("The 64-bit program files directory exists."); + .expect("The 64-bit program files directory exists"); assert_ne!( &self.x86, pf_64bit, "The 32-bit and 64-bit program files directories have different locations.", @@ -455,7 +455,7 @@ mod tests { let pf_64bit = self .maybe_64bit .as_ref() - .expect("The 64-bit program files directory exists."); + .expect("The 64-bit program files directory exists"); assert_eq!( self.current.as_os_str(), pf_64bit.as_os_str(), @@ -487,13 +487,13 @@ mod tests { let prefix_64bit = pf .maybe_64bit .as_ref() - .expect("It gives two paths only if one can be 64-bit."); + .expect("It gives two paths only if one can be 64-bit"); let suffix_64bit = primary .strip_prefix(prefix_64bit) - .expect("It gives the 64-bit path and lists it first."); + .expect("It gives the 64-bit path and lists it first"); let suffix_x86 = secondary .strip_prefix(pf.x86.as_path()) - .expect("It gives the 32-bit path and lists it second."); + .expect("It gives the 32-bit path and lists it second"); Self { x86: suffix_x86, maybe_64bit: Some(suffix_64bit), @@ -503,7 +503,7 @@ mod tests { assert_eq!(pf.maybe_64bit, None, "It gives one path only if none can be 64-bit."); let suffix_x86 = only .strip_prefix(pf.x86.as_path()) - .expect("The one path it gives is the 32-bit path."); + .expect("The one path it gives is the 32-bit path"); Self { x86: suffix_x86, maybe_64bit: None, From dea1746b06483796398441de391567f9ac115d21 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Mon, 15 Jul 2024 00:17:59 -0400 Subject: [PATCH 32/34] Update/refine comments and do some minor cleanup * Update a couple of preexisting doc comments for clarity and to avoid implying that the `git` that is found is *always* from PATH, since it may be from an alternative location. * Correct the claim about `EXEPATH` and MSYS, as it is present in Git Bash but not other MSYS environments. (This remains something of an oversimplification, since it is also present in Git Cmd environments, but that shell environment is decreasingly used and probably not essential information for that explanatory comment.) * Hedge claim about what `EXEPATH` gives us, because some ways of running Git Bash cause it to point to the `bin` directory rather than the installation directory, in which case our shortcut use of it will fail. This seems to happen when Git Bash is run by means other than the `git-bash.exe` graphical launcher, even if an attempt is made to start the shell the same way. Traditionally such approaches to running Git Bash may not have been overtly supported by Git for Windows. But the difference seems to be preserved even when the non-`git-bash.exe` way Git Bash is run is through a Windows Terminal profile added by the Git for Windows installer (which recently added that feature). * Remove a couple spurious semicolons I had accidentally added. * Remove unnecessary qualification for an imported name. --- gix-path/src/env/git.rs | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-) diff --git a/gix-path/src/env/git.rs b/gix-path/src/env/git.rs index 914d19cd14c..2190aa0949a 100644 --- a/gix-path/src/env/git.rs +++ b/gix-path/src/env/git.rs @@ -61,7 +61,7 @@ where // This shouldn't happen, but if it does then don't use the path. This is mainly in // case we are accidentally invoked with the environment variable set but empty. continue; - }; + } let location = pf.join(suffix); if !locations.contains(&location) { locations.push(location); @@ -77,7 +77,9 @@ pub(super) static EXE_NAME: &str = "git.exe"; #[cfg(not(windows))] pub(super) static EXE_NAME: &str = "git"; -/// Invoke the git executable in PATH to obtain the origin configuration, which is cached and returned. +/// Invoke the git executable to obtain the origin configuration, which is cached and returned. +/// +/// The git executable is the one found in PATH or an alternative location. pub(super) static EXE_INFO: Lazy> = Lazy::new(|| { let git_cmd = |executable: PathBuf| { let mut cmd = Command::new(executable); @@ -105,15 +107,18 @@ pub(super) static EXE_INFO: Lazy> = Lazy::new(|| { first_file_from_config_with_origin(cmd_output.as_slice().into()).map(ToOwned::to_owned) }); -/// Returns the file that contains git configuration coming with the installation of the `git` file in the current `PATH`, or `None` -/// if no `git` executable was found or there were other errors during execution. +/// Try to find the file that contains git configuration coming with the git installation. +/// +/// This returns the configuration associated with the `git` executable found in the current `PATH` +/// or an alternative location, or `None` if no `git` executable was found or there were other +/// errors during execution. pub(super) fn install_config_path() -> Option<&'static BStr> { let _span = gix_trace::detail!("gix_path::git::install_config_path()"); static PATH: Lazy> = Lazy::new(|| { - // Shortcut: in Msys shells this variable is set which allows to deduce the installation directory, - // so we can save the `git` invocation. + // Shortcut: Specifically in Git for Windows 'Git Bash' shells, this variable is set. It + // may let us deduce the installation directory, so we can save the `git` invocation. #[cfg(windows)] - if let Some(mut exec_path) = std::env::var_os("EXEPATH").map(std::path::PathBuf::from) { + if let Some(mut exec_path) = std::env::var_os("EXEPATH").map(PathBuf::from) { exec_path.push("etc"); exec_path.push("gitconfig"); return crate::os_string_into_bstring(exec_path.into()).ok(); @@ -197,7 +202,7 @@ mod tests { _ => None, } } - }; + } } macro_rules! locations_from { From dd534087fd7829eee1a9b426ee8bcdaca9c352e1 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Mon, 15 Jul 2024 00:56:57 -0400 Subject: [PATCH 33/34] Move `test::loc`'s `use` statements up Since they have the same effect even down where that is not intuitive. --- gix-path/src/env/git.rs | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/gix-path/src/env/git.rs b/gix-path/src/env/git.rs index 2190aa0949a..5550b059e19 100644 --- a/gix-path/src/env/git.rs +++ b/gix-path/src/env/git.rs @@ -192,6 +192,17 @@ mod tests { #[cfg(windows)] mod loc { + use std::ffi::{OsStr, OsString}; + use std::io::ErrorKind; + use std::path::{Path, PathBuf}; + + use known_folders::{get_known_folder_path, KnownFolder}; + use windows::core::Result as WindowsResult; + use windows::Win32::Foundation::BOOL; + use windows::Win32::System::Threading::{GetCurrentProcess, IsWow64Process}; + use winreg::enums::{HKEY_LOCAL_MACHINE, KEY_QUERY_VALUE}; + use winreg::RegKey; + macro_rules! var_os_stub { { $($name:expr => $value:expr),* $(,)? } => { |key| { @@ -305,17 +316,6 @@ mod tests { ); } - use std::ffi::{OsStr, OsString}; - use std::io::ErrorKind; - use std::path::{Path, PathBuf}; - - use known_folders::{get_known_folder_path, KnownFolder}; - use windows::core::Result as WindowsResult; - use windows::Win32::Foundation::BOOL; - use windows::Win32::System::Threading::{GetCurrentProcess, IsWow64Process}; - use winreg::enums::{HKEY_LOCAL_MACHINE, KEY_QUERY_VALUE}; - use winreg::RegKey; - trait Current: Sized { fn current() -> WindowsResult; } From 73ed3402861c88092b9a2cb2502e0f20d8bbcd36 Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Wed, 17 Jul 2024 21:13:10 +0200 Subject: [PATCH 34/34] refactor - place tests in their own module - slightly simplify tests --- gix-path/src/env/git.rs | 553 ---------------------------------- gix-path/src/env/git/mod.rs | 144 +++++++++ gix-path/src/env/git/tests.rs | 403 +++++++++++++++++++++++++ 3 files changed, 547 insertions(+), 553 deletions(-) delete mode 100644 gix-path/src/env/git.rs create mode 100644 gix-path/src/env/git/mod.rs create mode 100644 gix-path/src/env/git/tests.rs diff --git a/gix-path/src/env/git.rs b/gix-path/src/env/git.rs deleted file mode 100644 index 5550b059e19..00000000000 --- a/gix-path/src/env/git.rs +++ /dev/null @@ -1,553 +0,0 @@ -use std::path::{Path, PathBuf}; -use std::process::{Command, Stdio}; - -use bstr::{BStr, BString, ByteSlice}; -use once_cell::sync::Lazy; - -/// Other places to find Git in. -#[cfg(windows)] -pub(super) static ALTERNATIVE_LOCATIONS: Lazy> = - Lazy::new(|| locations_under_program_files(|key| std::env::var_os(key))); -#[cfg(not(windows))] -pub(super) static ALTERNATIVE_LOCATIONS: Lazy> = Lazy::new(Vec::new); - -#[cfg(windows)] -fn locations_under_program_files(var_os_func: F) -> Vec -where - F: Fn(&str) -> Option, -{ - // Should give a 64-bit program files path from a 32-bit or 64-bit process on a 64-bit system. - let varname_64bit = "ProgramW6432"; - - // Should give a 32-bit program files path from a 32-bit or 64-bit process on a 64-bit system. - // This variable is x86-specific, but neither Git nor Rust target 32-bit ARM on Windows. - let varname_x86 = "ProgramFiles(x86)"; - - // Should give a 32-bit program files path on a 32-bit system. We also check this on a 64-bit - // system, even though it *should* equal the process's architecture specific variable, so that - // we cover the case of a parent process that passes down an overly sanitized environment that - // lacks the architecture-specific variable. On a 64-bit system, because parent and child - // processes' architectures can be different, Windows sets the child's ProgramFiles variable - // from the ProgramW6432 or ProgramFiles(x86) variable applicable to the child's architecture. - // Only if the parent does not pass that down is the passed-down ProgramFiles variable even - // used. But this behavior is not well known, so that situation does sometimes happen. - let varname_current = "ProgramFiles"; - - // 64-bit relative bin dir. So far, this is always mingw64, not ucrt64, clang64, or clangarm64. - let suffix_64 = Path::new(r"Git\mingw64\bin"); - - // 32-bit relative bin dir. So far, this is always mingw32, not clang32. - let suffix_32 = Path::new(r"Git\mingw32\bin"); - - // Whichever of the 64-bit or 32-bit relative bin better matches this process's architecture. - // Unlike the system architecture, the process architecture is always known at compile time. - #[cfg(target_pointer_width = "64")] - let suffix_current = suffix_64; - #[cfg(target_pointer_width = "32")] - let suffix_current = suffix_32; - - let rules = [ - (varname_64bit, suffix_64), - (varname_x86, suffix_32), - (varname_current, suffix_current), - ]; - - let mut locations = vec![]; - - for (name, suffix) in rules { - if let Some(value) = var_os_func(name) { - let pf = Path::new(&value); - if pf.is_relative() { - // This shouldn't happen, but if it does then don't use the path. This is mainly in - // case we are accidentally invoked with the environment variable set but empty. - continue; - } - let location = pf.join(suffix); - if !locations.contains(&location) { - locations.push(location); - } - } - } - - locations -} - -#[cfg(windows)] -pub(super) static EXE_NAME: &str = "git.exe"; -#[cfg(not(windows))] -pub(super) static EXE_NAME: &str = "git"; - -/// Invoke the git executable to obtain the origin configuration, which is cached and returned. -/// -/// The git executable is the one found in PATH or an alternative location. -pub(super) static EXE_INFO: Lazy> = Lazy::new(|| { - let git_cmd = |executable: PathBuf| { - let mut cmd = Command::new(executable); - cmd.args(["config", "-l", "--show-origin"]) - .stdin(Stdio::null()) - .stderr(Stdio::null()); - cmd - }; - let mut cmd = git_cmd(EXE_NAME.into()); - gix_trace::debug!(cmd = ?cmd, "invoking git for installation config path"); - let cmd_output = match cmd.output() { - Ok(out) => out.stdout, - #[cfg(windows)] - Err(err) if err.kind() == std::io::ErrorKind::NotFound => { - let executable = ALTERNATIVE_LOCATIONS.iter().find_map(|prefix| { - let candidate = prefix.join(EXE_NAME); - candidate.is_file().then_some(candidate) - })?; - gix_trace::debug!(cmd = ?cmd, "invoking git for installation config path in alternate location"); - git_cmd(executable).output().ok()?.stdout - } - Err(_) => return None, - }; - - first_file_from_config_with_origin(cmd_output.as_slice().into()).map(ToOwned::to_owned) -}); - -/// Try to find the file that contains git configuration coming with the git installation. -/// -/// This returns the configuration associated with the `git` executable found in the current `PATH` -/// or an alternative location, or `None` if no `git` executable was found or there were other -/// errors during execution. -pub(super) fn install_config_path() -> Option<&'static BStr> { - let _span = gix_trace::detail!("gix_path::git::install_config_path()"); - static PATH: Lazy> = Lazy::new(|| { - // Shortcut: Specifically in Git for Windows 'Git Bash' shells, this variable is set. It - // may let us deduce the installation directory, so we can save the `git` invocation. - #[cfg(windows)] - if let Some(mut exec_path) = std::env::var_os("EXEPATH").map(PathBuf::from) { - exec_path.push("etc"); - exec_path.push("gitconfig"); - return crate::os_string_into_bstring(exec_path.into()).ok(); - } - EXE_INFO.clone() - }); - PATH.as_ref().map(AsRef::as_ref) -} - -fn first_file_from_config_with_origin(source: &BStr) -> Option<&BStr> { - let file = source.strip_prefix(b"file:")?; - let end_pos = file.find_byte(b'\t')?; - file[..end_pos].trim_with(|c| c == '"').as_bstr().into() -} - -/// Given `config_path` as obtained from `install_config_path()`, return the path of the git installation base. -pub(super) fn config_to_base_path(config_path: &Path) -> &Path { - config_path - .parent() - .expect("config file paths always have a file name to pop") -} - -#[cfg(test)] -mod tests { - use std::path::Path; - - #[test] - fn config_to_base_path() { - for (input, expected) in [ - ( - "/Applications/Xcode.app/Contents/Developer/usr/share/git-core/gitconfig", - "/Applications/Xcode.app/Contents/Developer/usr/share/git-core", - ), - ("C:/git-sdk-64/etc/gitconfig", "C:/git-sdk-64/etc"), - ("C:\\ProgramData/Git/config", "C:\\ProgramData/Git"), - ("C:/Program Files/Git/etc/gitconfig", "C:/Program Files/Git/etc"), - ] { - assert_eq!(super::config_to_base_path(Path::new(input)), Path::new(expected)); - } - } - - #[test] - fn first_file_from_config_with_origin() { - let macos = "file:/Applications/Xcode.app/Contents/Developer/usr/share/git-core/gitconfig credential.helper=osxkeychain\nfile:/Users/byron/.gitconfig push.default=simple\n"; - let win_msys = - "file:C:/git-sdk-64/etc/gitconfig core.symlinks=false\r\nfile:C:/git-sdk-64/etc/gitconfig core.autocrlf=true"; - let win_cmd = "file:C:/Program Files/Git/etc/gitconfig diff.astextplain.textconv=astextplain\r\nfile:C:/Program Files/Git/etc/gitconfig filter.lfs.clean=gix-lfs clean -- %f\r\n"; - let win_msys_old = "file:\"C:\\ProgramData/Git/config\" diff.astextplain.textconv=astextplain\r\nfile:\"C:\\ProgramData/Git/config\" filter.lfs.clean=git-lfs clean -- %f\r\n"; - let linux = "file:/home/parallels/.gitconfig core.excludesfile=~/.gitignore\n"; - let bogus = "something unexpected"; - let empty = ""; - - for (source, expected) in [ - ( - macos, - Some("/Applications/Xcode.app/Contents/Developer/usr/share/git-core/gitconfig"), - ), - (win_msys, Some("C:/git-sdk-64/etc/gitconfig")), - (win_msys_old, Some("C:\\ProgramData/Git/config")), - (win_cmd, Some("C:/Program Files/Git/etc/gitconfig")), - (linux, Some("/home/parallels/.gitconfig")), - (bogus, None), - (empty, None), - ] { - assert_eq!( - super::first_file_from_config_with_origin(source.into()), - expected.map(Into::into) - ); - } - } - - #[cfg(windows)] - mod loc { - use std::ffi::{OsStr, OsString}; - use std::io::ErrorKind; - use std::path::{Path, PathBuf}; - - use known_folders::{get_known_folder_path, KnownFolder}; - use windows::core::Result as WindowsResult; - use windows::Win32::Foundation::BOOL; - use windows::Win32::System::Threading::{GetCurrentProcess, IsWow64Process}; - use winreg::enums::{HKEY_LOCAL_MACHINE, KEY_QUERY_VALUE}; - use winreg::RegKey; - - macro_rules! var_os_stub { - { $($name:expr => $value:expr),* $(,)? } => { - |key| { - match key { - $( - $name => Some(OsString::from($value)), - )* - _ => None, - } - } - } - } - - macro_rules! locations_from { - ($($name:expr => $value:expr),* $(,)?) => { - super::super::locations_under_program_files(var_os_stub! { - $( - $name => $value, - )* - }) - } - } - - macro_rules! pathbuf_vec { - [$($path:expr),* $(,)?] => { - vec![$( - PathBuf::from($path), - )*] - } - } - - #[test] - fn locations_under_program_files_ordinary() { - assert_eq!( - locations_from!( - "ProgramFiles" => r"C:\Program Files", - ), - if cfg!(target_pointer_width = "64") { - pathbuf_vec![r"C:\Program Files\Git\mingw64\bin"] - } else { - pathbuf_vec![r"C:\Program Files\Git\mingw32\bin"] - }, - ); - assert_eq!( - locations_from!( - "ProgramFiles" => { - if cfg!(target_pointer_width = "64") { - r"C:\Program Files" - } else { - r"C:\Program Files (x86)" - } - }, - "ProgramFiles(x86)" => r"C:\Program Files (x86)", - "ProgramW6432" => r"C:\Program Files", - ), - pathbuf_vec![ - r"C:\Program Files\Git\mingw64\bin", - r"C:\Program Files (x86)\Git\mingw32\bin", - ], - ); - assert_eq!(locations_from!(), Vec::::new()); - } - - #[test] - fn locations_under_program_files_strange() { - assert_eq!( - locations_from!( - "ProgramFiles" => r"X:\cur\rent", - "ProgramFiles(x86)" => r"Y:\nar\row", - "ProgramW6432" => r"Z:\wi\de", - ), - pathbuf_vec![ - r"Z:\wi\de\Git\mingw64\bin", - r"Y:\nar\row\Git\mingw32\bin", - if cfg!(target_pointer_width = "64") { - r"X:\cur\rent\Git\mingw64\bin" - } else { - r"X:\cur\rent\Git\mingw32\bin" - }, - ], - ); - assert_eq!( - locations_from!( - "ProgramW6432" => r"Z:\wi\de", - ), - pathbuf_vec![r"Z:\wi\de\Git\mingw64\bin"], - ); - assert_eq!( - locations_from!( - "ProgramFiles" => r"Z:/wi//de/", - "ProgramFiles(x86)" => r"Y:/\nar\/row", - "ProgramW6432" => r"Z:\wi\.\de", - ), - if cfg!(target_pointer_width = "64") { - pathbuf_vec![r"Z:\wi\de\Git\mingw64\bin", r"Y:\nar\row\Git\mingw32\bin"] - } else { - pathbuf_vec![ - r"Z:\wi\de\Git\mingw64\bin", - r"Y:\nar\row\Git\mingw32\bin", - r"Z:\wi\de\Git\mingw32\bin", - ] - }, - ); - assert_eq!( - locations_from!( - "ProgramFiles" => r"foo\bar", - "ProgramFiles(x86)" => r"\\host\share\subdir", - "ProgramW6432" => r"", - ), - pathbuf_vec![r"\\host\share\subdir\Git\mingw32\bin"], - ); - } - - trait Current: Sized { - fn current() -> WindowsResult; - } - - #[derive(Clone, Copy, Debug)] - enum PlatformArchitecture { - Is32on32, - Is32on64, - Is64on64, - } - - impl Current for PlatformArchitecture { - fn current() -> WindowsResult { - // Ordinarily, we would check the target pointer width first to avoid doing extra work, - // because if this is a 64-bit executable then the operating system is 64-bit. But this - // is for the test suite, and doing it this way allows problems to be caught earlier if - // a change made on a 64-bit development machine breaks the IsWow64Process() call. - let mut wow64process = BOOL::default(); - unsafe { IsWow64Process(GetCurrentProcess(), &mut wow64process)? }; - - let platform_architecture = if wow64process.as_bool() { - Self::Is32on64 - } else if cfg!(target_pointer_width = "32") { - Self::Is32on32 - } else { - assert!(cfg!(target_pointer_width = "64")); - Self::Is64on64 - }; - Ok(platform_architecture) - } - } - - fn ends_with_case_insensitive(full_text: &OsStr, literal_pattern: &str) -> Option { - let folded_text = full_text.to_str()?.to_lowercase(); - let folded_pattern = literal_pattern.to_lowercase(); - Some(folded_text.ends_with(&folded_pattern)) - } - - /// The common global program files paths on this system, by process and system architecture. - #[derive(Clone, Debug)] - struct ProgramFilesPaths { - /// The program files directory used for whatever architecture this program was built for. - current: PathBuf, - - /// The x86 program files directory regardless of the architecture of the program. - /// - /// If Rust gains Windows targets like ARMv7 where this is unavailable, this could fail. - x86: PathBuf, - - /// The 64-bit program files directory if there is one. - /// - /// This is present on x64 and also ARM64 systems. On an ARM64 system, ARM64 and AMD64 - /// programs use the same program files directory while 32-bit x86 and ARM programs use - /// two others. Only a 32-bit has no 64-bit program files directory. - maybe_64bit: Option, - } - - impl ProgramFilesPaths { - /// Gets the three common kinds of global program files paths without environment variables. - /// - /// The idea here is to obtain this information, which the `alternative_locations()` unit - /// test uses to learn the expected alternative locations, without duplicating *any* of the - /// approach used for `ALTERNATIVE_LOCATIONS`, so it can be used to test that. The approach - /// here is also more reliable than using environment variables, but it is a bit more - /// complex, and it requires either additional dependencies or the use of unsafe code. - /// - /// This gets `pf_current` and `pf_x86` by the [known folders][known-folders] system. But - /// it gets `maybe_pf_64bit` from the registry, as the corresponding known folder is not - /// available to 32-bit processes. See the [`KNOWNFOLDDERID`][knownfolderid] documentation. - /// - /// If in the future the implementation of `ALTERNATIVE_LOCATIONS` uses these techniques, - /// then this function can be changed to use environment variables and renamed accordingly. - /// - /// [known-folders]: https://learn.microsoft.com/en-us/windows/win32/shell/known-folders - /// [knownfolderid]: https://learn.microsoft.com/en-us/windows/win32/shell/knownfolderid#remarks - fn obtain_envlessly() -> Self { - let pf_current = get_known_folder_path(KnownFolder::ProgramFiles) - .expect("The process architecture specific program files folder is always available"); - - let pf_x86 = get_known_folder_path(KnownFolder::ProgramFilesX86) - .expect("The x86 program files folder will in practice always be available"); - - let maybe_pf_64bit = RegKey::predef(HKEY_LOCAL_MACHINE) - .open_subkey_with_flags(r"SOFTWARE\Microsoft\Windows\CurrentVersion", KEY_QUERY_VALUE) - .expect("The `CurrentVersion` registry key exists and allows reading") - .get_value::("ProgramW6432Dir") - .map(PathBuf::from) - .map_err(|error| { - assert_eq!(error.kind(), ErrorKind::NotFound); - error - }) - .ok(); - - Self { - current: pf_current, - x86: pf_x86, - maybe_64bit: maybe_pf_64bit, - } - } - - /// Checks that the paths we got for testing are reasonable. - /// - /// This checks that `obtain_envlessly()` returned paths that are likely to be correct and - /// that satisfy the most important properties based on the current system and process. - fn validate(self) -> Self { - match PlatformArchitecture::current().expect("Process and system 'bitness' should be available") { - PlatformArchitecture::Is32on32 => { - assert_eq!( - self.current.as_os_str(), - self.x86.as_os_str(), - "Our program files path is exactly identical to the 32-bit one.", - ); - for trailing_arch in [" (x86)", " (Arm)"] { - let is_adorned = ends_with_case_insensitive(self.current.as_os_str(), trailing_arch) - .expect("Assume the test system's important directories are valid Unicode"); - assert!( - !is_adorned, - "The 32-bit program files directory name on a 32-bit system mentions no architecture.", - ); - } - assert_eq!( - self.maybe_64bit, None, - "A 32-bit system has no 64-bit program files directory.", - ); - } - PlatformArchitecture::Is32on64 => { - assert_eq!( - self.current.as_os_str(), - self.x86.as_os_str(), - "Our program files path is exactly identical to the 32-bit one.", - ); - let pf_64bit = self - .maybe_64bit - .as_ref() - .expect("The 64-bit program files directory exists"); - assert_ne!( - &self.x86, pf_64bit, - "The 32-bit and 64-bit program files directories have different locations.", - ); - } - PlatformArchitecture::Is64on64 => { - let pf_64bit = self - .maybe_64bit - .as_ref() - .expect("The 64-bit program files directory exists"); - assert_eq!( - self.current.as_os_str(), - pf_64bit.as_os_str(), - "Our program files path is exactly identical to the 64-bit one.", - ); - assert_ne!( - &self.x86, pf_64bit, - "The 32-bit and 64-bit program files directories have different locations.", - ); - } - } - - self - } - } - - /// Paths relative to process architecture specific program files directories. - #[derive(Clone, Debug)] - struct RelativeGitBinPaths<'a> { - x86: &'a Path, - maybe_64bit: Option<&'a Path>, - } - - impl<'a> RelativeGitBinPaths<'a> { - /// Assert that `locations` has the given path prefixes, and extract the suffixes. - fn assert_from(pf: &'a ProgramFilesPaths, locations: &'static [PathBuf]) -> Self { - match locations { - [primary, secondary] => { - let prefix_64bit = pf - .maybe_64bit - .as_ref() - .expect("It gives two paths only if one can be 64-bit"); - let suffix_64bit = primary - .strip_prefix(prefix_64bit) - .expect("It gives the 64-bit path and lists it first"); - let suffix_x86 = secondary - .strip_prefix(pf.x86.as_path()) - .expect("It gives the 32-bit path and lists it second"); - Self { - x86: suffix_x86, - maybe_64bit: Some(suffix_64bit), - } - } - [only] => { - assert_eq!(pf.maybe_64bit, None, "It gives one path only if none can be 64-bit."); - let suffix_x86 = only - .strip_prefix(pf.x86.as_path()) - .expect("The one path it gives is the 32-bit path"); - Self { - x86: suffix_x86, - maybe_64bit: None, - } - } - other => panic!("{:?} has length {}, expected 1 or 2.", other, other.len()), - } - } - - /// Assert that the suffixes (relative subdirectories) are the common per-architecture Git install locations. - fn assert_architectures(&self) { - assert_eq!(self.x86, Path::new("Git/mingw32/bin")); - - if let Some(suffix_64bit) = self.maybe_64bit { - // When Git for Windows releases ARM64 builds, there will be another 64-bit suffix, - // likely clangarm64. In that case, this and other assertions will need updating, - // as there will be two separate paths to check under the same 64-bit program files - // directory. (See the definition of ProgramFilesPaths::maybe_64bit for details.) - assert_eq!(suffix_64bit, Path::new("Git/mingw64/bin")); - } - } - } - - #[test] - fn alternative_locations() { - // Obtain program files directory paths by other means and check that they seem correct. - let pf = ProgramFilesPaths::obtain_envlessly().validate(); - - // Check that `ALTERNATIVE_LOCATIONS` correspond to them, with the correct subdirectories. - let locations = super::super::ALTERNATIVE_LOCATIONS.as_slice(); - RelativeGitBinPaths::assert_from(&pf, locations).assert_architectures(); - } - } - - #[cfg(not(windows))] - mod loc { - #[test] - fn alternative_locations() { - assert!(super::super::ALTERNATIVE_LOCATIONS.is_empty()); - } - } -} diff --git a/gix-path/src/env/git/mod.rs b/gix-path/src/env/git/mod.rs new file mode 100644 index 00000000000..9ba82d6c027 --- /dev/null +++ b/gix-path/src/env/git/mod.rs @@ -0,0 +1,144 @@ +use std::path::{Path, PathBuf}; +use std::process::{Command, Stdio}; + +use bstr::{BStr, BString, ByteSlice}; +use once_cell::sync::Lazy; + +/// Other places to find Git in. +#[cfg(windows)] +pub(super) static ALTERNATIVE_LOCATIONS: Lazy> = + Lazy::new(|| locations_under_program_files(|key| std::env::var_os(key))); +#[cfg(not(windows))] +pub(super) static ALTERNATIVE_LOCATIONS: Lazy> = Lazy::new(Vec::new); + +#[cfg(windows)] +fn locations_under_program_files(var_os_func: F) -> Vec +where + F: Fn(&str) -> Option, +{ + // Should give a 64-bit program files path from a 32-bit or 64-bit process on a 64-bit system. + let varname_64bit = "ProgramW6432"; + + // Should give a 32-bit program files path from a 32-bit or 64-bit process on a 64-bit system. + // This variable is x86-specific, but neither Git nor Rust target 32-bit ARM on Windows. + let varname_x86 = "ProgramFiles(x86)"; + + // Should give a 32-bit program files path on a 32-bit system. We also check this on a 64-bit + // system, even though it *should* equal the process's architecture specific variable, so that + // we cover the case of a parent process that passes down an overly sanitized environment that + // lacks the architecture-specific variable. On a 64-bit system, because parent and child + // processes' architectures can be different, Windows sets the child's ProgramFiles variable + // from the ProgramW6432 or ProgramFiles(x86) variable applicable to the child's architecture. + // Only if the parent does not pass that down is the passed-down ProgramFiles variable even + // used. But this behavior is not well known, so that situation does sometimes happen. + let varname_current = "ProgramFiles"; + + // 64-bit relative bin dir. So far, this is always mingw64, not ucrt64, clang64, or clangarm64. + let suffix_64 = Path::new(r"Git\mingw64\bin"); + + // 32-bit relative bin dir. So far, this is always mingw32, not clang32. + let suffix_32 = Path::new(r"Git\mingw32\bin"); + + // Whichever of the 64-bit or 32-bit relative bin better matches this process's architecture. + // Unlike the system architecture, the process architecture is always known at compile time. + #[cfg(target_pointer_width = "64")] + let suffix_current = suffix_64; + #[cfg(target_pointer_width = "32")] + let suffix_current = suffix_32; + + let rules = [ + (varname_64bit, suffix_64), + (varname_x86, suffix_32), + (varname_current, suffix_current), + ]; + + let mut locations = vec![]; + + for (name, suffix) in rules { + let Some(pf) = var_os_func(name) else { continue }; + let pf = Path::new(&pf); + if pf.is_relative() { + // This shouldn't happen, but if it does then don't use the path. This is mainly in + // case we are accidentally invoked with the environment variable set but empty. + continue; + } + let location = pf.join(suffix); + if !locations.contains(&location) { + locations.push(location); + } + } + + locations +} + +#[cfg(windows)] +pub(super) static EXE_NAME: &str = "git.exe"; +#[cfg(not(windows))] +pub(super) static EXE_NAME: &str = "git"; + +/// Invoke the git executable to obtain the origin configuration, which is cached and returned. +/// +/// The git executable is the one found in PATH or an alternative location. +pub(super) static EXE_INFO: Lazy> = Lazy::new(|| { + let git_cmd = |executable: PathBuf| { + let mut cmd = Command::new(executable); + cmd.args(["config", "-l", "--show-origin"]) + .stdin(Stdio::null()) + .stderr(Stdio::null()); + cmd + }; + let mut cmd = git_cmd(EXE_NAME.into()); + gix_trace::debug!(cmd = ?cmd, "invoking git for installation config path"); + let cmd_output = match cmd.output() { + Ok(out) => out.stdout, + #[cfg(windows)] + Err(err) if err.kind() == std::io::ErrorKind::NotFound => { + let executable = ALTERNATIVE_LOCATIONS.iter().find_map(|prefix| { + let candidate = prefix.join(EXE_NAME); + candidate.is_file().then_some(candidate) + })?; + gix_trace::debug!(cmd = ?cmd, "invoking git for installation config path in alternate location"); + git_cmd(executable).output().ok()?.stdout + } + Err(_) => return None, + }; + + first_file_from_config_with_origin(cmd_output.as_slice().into()).map(ToOwned::to_owned) +}); + +/// Try to find the file that contains git configuration coming with the git installation. +/// +/// This returns the configuration associated with the `git` executable found in the current `PATH` +/// or an alternative location, or `None` if no `git` executable was found or there were other +/// errors during execution. +pub(super) fn install_config_path() -> Option<&'static BStr> { + let _span = gix_trace::detail!("gix_path::git::install_config_path()"); + static PATH: Lazy> = Lazy::new(|| { + // Shortcut: Specifically in Git for Windows 'Git Bash' shells, this variable is set. It + // may let us deduce the installation directory, so we can save the `git` invocation. + #[cfg(windows)] + if let Some(mut exec_path) = std::env::var_os("EXEPATH").map(PathBuf::from) { + exec_path.push("etc"); + exec_path.push("gitconfig"); + return crate::os_string_into_bstring(exec_path.into()).ok(); + } + EXE_INFO.clone() + }); + PATH.as_ref().map(AsRef::as_ref) +} + +fn first_file_from_config_with_origin(source: &BStr) -> Option<&BStr> { + let file = source.strip_prefix(b"file:")?; + let end_pos = file.find_byte(b'\t')?; + file[..end_pos].trim_with(|c| c == '"').as_bstr().into() +} + +/// Given `config_path` as obtained from `install_config_path()`, return the path of the git installation base. +pub(super) fn config_to_base_path(config_path: &Path) -> &Path { + config_path + .parent() + .expect("config file paths always have a file name to pop") +} + +#[cfg(test)] +mod tests; diff --git a/gix-path/src/env/git/tests.rs b/gix-path/src/env/git/tests.rs new file mode 100644 index 00000000000..033b6f0b983 --- /dev/null +++ b/gix-path/src/env/git/tests.rs @@ -0,0 +1,403 @@ +use std::path::Path; + +#[test] +fn config_to_base_path() { + for (input, expected) in [ + ( + "/Applications/Xcode.app/Contents/Developer/usr/share/git-core/gitconfig", + "/Applications/Xcode.app/Contents/Developer/usr/share/git-core", + ), + ("C:/git-sdk-64/etc/gitconfig", "C:/git-sdk-64/etc"), + ("C:\\ProgramData/Git/config", "C:\\ProgramData/Git"), + ("C:/Program Files/Git/etc/gitconfig", "C:/Program Files/Git/etc"), + ] { + assert_eq!(super::config_to_base_path(Path::new(input)), Path::new(expected)); + } +} + +#[test] +fn first_file_from_config_with_origin() { + let macos = "file:/Applications/Xcode.app/Contents/Developer/usr/share/git-core/gitconfig credential.helper=osxkeychain\nfile:/Users/byron/.gitconfig push.default=simple\n"; + let win_msys = + "file:C:/git-sdk-64/etc/gitconfig core.symlinks=false\r\nfile:C:/git-sdk-64/etc/gitconfig core.autocrlf=true"; + let win_cmd = "file:C:/Program Files/Git/etc/gitconfig diff.astextplain.textconv=astextplain\r\nfile:C:/Program Files/Git/etc/gitconfig filter.lfs.clean=gix-lfs clean -- %f\r\n"; + let win_msys_old = "file:\"C:\\ProgramData/Git/config\" diff.astextplain.textconv=astextplain\r\nfile:\"C:\\ProgramData/Git/config\" filter.lfs.clean=git-lfs clean -- %f\r\n"; + let linux = "file:/home/parallels/.gitconfig core.excludesfile=~/.gitignore\n"; + let bogus = "something unexpected"; + let empty = ""; + + for (source, expected) in [ + ( + macos, + Some("/Applications/Xcode.app/Contents/Developer/usr/share/git-core/gitconfig"), + ), + (win_msys, Some("C:/git-sdk-64/etc/gitconfig")), + (win_msys_old, Some("C:\\ProgramData/Git/config")), + (win_cmd, Some("C:/Program Files/Git/etc/gitconfig")), + (linux, Some("/home/parallels/.gitconfig")), + (bogus, None), + (empty, None), + ] { + assert_eq!( + super::first_file_from_config_with_origin(source.into()), + expected.map(Into::into) + ); + } +} + +#[cfg(windows)] +mod locations { + use std::ffi::{OsStr, OsString}; + use std::io::ErrorKind; + use std::path::{Path, PathBuf}; + + use known_folders::{get_known_folder_path, KnownFolder}; + use windows::core::Result as WindowsResult; + use windows::Win32::Foundation::BOOL; + use windows::Win32::System::Threading::{GetCurrentProcess, IsWow64Process}; + use winreg::enums::{HKEY_LOCAL_MACHINE, KEY_QUERY_VALUE}; + use winreg::RegKey; + + macro_rules! var_os_stub { + { $($name:expr => $value:expr),* $(,)? } => { + |key| { + match key { + $( + $name => Some(OsString::from($value)), + )* + _ => None, + } + } + } + } + + macro_rules! locations_from { + ($($name:expr => $value:expr),* $(,)?) => { + super::super::locations_under_program_files(var_os_stub! { + $( + $name => $value, + )* + }) + } + } + + macro_rules! pathbuf_vec { + [$($path:expr),* $(,)?] => { + vec![$( + PathBuf::from($path), + )*] + } + } + + #[test] + fn locations_under_program_files_ordinary() { + assert_eq!( + locations_from!( + "ProgramFiles" => r"C:\Program Files", + ), + if cfg!(target_pointer_width = "64") { + pathbuf_vec![r"C:\Program Files\Git\mingw64\bin"] + } else { + pathbuf_vec![r"C:\Program Files\Git\mingw32\bin"] + }, + ); + assert_eq!( + locations_from!( + "ProgramFiles" => { + if cfg!(target_pointer_width = "64") { + r"C:\Program Files" + } else { + r"C:\Program Files (x86)" + } + }, + "ProgramFiles(x86)" => r"C:\Program Files (x86)", + "ProgramW6432" => r"C:\Program Files", + ), + pathbuf_vec![ + r"C:\Program Files\Git\mingw64\bin", + r"C:\Program Files (x86)\Git\mingw32\bin", + ], + ); + assert_eq!(locations_from!(), Vec::::new()); + } + + #[test] + fn locations_under_program_files_strange() { + assert_eq!( + locations_from!( + "ProgramFiles" => r"X:\cur\rent", + "ProgramFiles(x86)" => r"Y:\nar\row", + "ProgramW6432" => r"Z:\wi\de", + ), + pathbuf_vec![ + r"Z:\wi\de\Git\mingw64\bin", + r"Y:\nar\row\Git\mingw32\bin", + if cfg!(target_pointer_width = "64") { + r"X:\cur\rent\Git\mingw64\bin" + } else { + r"X:\cur\rent\Git\mingw32\bin" + }, + ], + ); + assert_eq!( + locations_from!( + "ProgramW6432" => r"Z:\wi\de", + ), + pathbuf_vec![r"Z:\wi\de\Git\mingw64\bin"], + ); + assert_eq!( + locations_from!( + "ProgramFiles" => r"Z:/wi//de/", + "ProgramFiles(x86)" => r"Y:/\nar\/row", + "ProgramW6432" => r"Z:\wi\.\de", + ), + if cfg!(target_pointer_width = "64") { + pathbuf_vec![r"Z:\wi\de\Git\mingw64\bin", r"Y:\nar\row\Git\mingw32\bin"] + } else { + pathbuf_vec![ + r"Z:\wi\de\Git\mingw64\bin", + r"Y:\nar\row\Git\mingw32\bin", + r"Z:\wi\de\Git\mingw32\bin", + ] + }, + ); + assert_eq!( + locations_from!( + "ProgramFiles" => r"foo\bar", + "ProgramFiles(x86)" => r"\\host\share\subdir", + "ProgramW6432" => r"", + ), + pathbuf_vec![r"\\host\share\subdir\Git\mingw32\bin"], + ); + } + + #[derive(Clone, Copy, Debug)] + enum PlatformArchitecture { + Is32on32, + Is32on64, + Is64on64, + } + + impl PlatformArchitecture { + fn current() -> WindowsResult { + // Ordinarily, we would check the target pointer width first to avoid doing extra work, + // because if this is a 64-bit executable then the operating system is 64-bit. But this + // is for the test suite, and doing it this way allows problems to be caught earlier if + // a change made on a 64-bit development machine breaks the IsWow64Process() call. + let mut wow64process = BOOL::default(); + unsafe { IsWow64Process(GetCurrentProcess(), &mut wow64process)? }; + + let platform_architecture = if wow64process.as_bool() { + Self::Is32on64 + } else if cfg!(target_pointer_width = "32") { + Self::Is32on32 + } else { + assert!(cfg!(target_pointer_width = "64")); + Self::Is64on64 + }; + Ok(platform_architecture) + } + } + + fn ends_with_case_insensitive(full_text: &OsStr, literal_pattern: &str) -> Option { + let folded_text = full_text.to_str()?.to_lowercase(); + let folded_pattern = literal_pattern.to_lowercase(); + Some(folded_text.ends_with(&folded_pattern)) + } + + /// The common global program files paths on this system, by process and system architecture. + #[derive(Clone, Debug)] + struct ProgramFilesPaths { + /// The program files directory used for whatever architecture this program was built for. + current: PathBuf, + + /// The x86 program files directory regardless of the architecture of the program. + /// + /// If Rust gains Windows targets like ARMv7 where this is unavailable, this could fail. + x86: PathBuf, + + /// The 64-bit program files directory if there is one. + /// + /// This is present on x64 and also ARM64 systems. On an ARM64 system, ARM64 and AMD64 + /// programs use the same program files directory while 32-bit x86 and ARM programs use + /// two others. Only a 32-bit has no 64-bit program files directory. + maybe_64bit: Option, + } + + impl ProgramFilesPaths { + /// Gets the three common kinds of global program files paths without environment variables. + /// + /// The idea here is to obtain this information, which the `alternative_locations()` unit + /// test uses to learn the expected alternative locations, without duplicating *any* of the + /// approach used for `ALTERNATIVE_LOCATIONS`, so it can be used to test that. The approach + /// here is also more reliable than using environment variables, but it is a bit more + /// complex, and it requires either additional dependencies or the use of unsafe code. + /// + /// This gets `pf_current` and `pf_x86` by the [known folders][known-folders] system. But + /// it gets `maybe_pf_64bit` from the registry, as the corresponding known folder is not + /// available to 32-bit processes. See the [`KNOWNFOLDDERID`][knownfolderid] documentation. + /// + /// If in the future the implementation of `ALTERNATIVE_LOCATIONS` uses these techniques, + /// then this function can be changed to use environment variables and renamed accordingly. + /// + /// [known-folders]: https://learn.microsoft.com/en-us/windows/win32/shell/known-folders + /// [knownfolderid]: https://learn.microsoft.com/en-us/windows/win32/shell/knownfolderid#remarks + fn obtain_envlessly() -> Self { + let pf_current = get_known_folder_path(KnownFolder::ProgramFiles) + .expect("The process architecture specific program files folder is always available"); + + let pf_x86 = get_known_folder_path(KnownFolder::ProgramFilesX86) + .expect("The x86 program files folder will in practice always be available"); + + let maybe_pf_64bit = RegKey::predef(HKEY_LOCAL_MACHINE) + .open_subkey_with_flags(r"SOFTWARE\Microsoft\Windows\CurrentVersion", KEY_QUERY_VALUE) + .expect("The `CurrentVersion` registry key exists and allows reading") + .get_value::("ProgramW6432Dir") + .map(PathBuf::from) + .map_err(|error| { + assert_eq!(error.kind(), ErrorKind::NotFound); + error + }) + .ok(); + + Self { + current: pf_current, + x86: pf_x86, + maybe_64bit: maybe_pf_64bit, + } + } + + /// Checks that the paths we got for testing are reasonable. + /// + /// This checks that `obtain_envlessly()` returned paths that are likely to be correct and + /// that satisfy the most important properties based on the current system and process. + fn validated(self) -> Self { + match PlatformArchitecture::current().expect("Process and system 'bitness' should be available") { + PlatformArchitecture::Is32on32 => { + assert_eq!( + self.current.as_os_str(), + self.x86.as_os_str(), + "Our program files path is exactly identical to the 32-bit one.", + ); + for trailing_arch in [" (x86)", " (Arm)"] { + let is_adorned = ends_with_case_insensitive(self.current.as_os_str(), trailing_arch) + .expect("Assume the test system's important directories are valid Unicode"); + assert!( + !is_adorned, + "The 32-bit program files directory name on a 32-bit system mentions no architecture.", + ); + } + assert_eq!( + self.maybe_64bit, None, + "A 32-bit system has no 64-bit program files directory.", + ); + } + PlatformArchitecture::Is32on64 => { + assert_eq!( + self.current.as_os_str(), + self.x86.as_os_str(), + "Our program files path is exactly identical to the 32-bit one.", + ); + let pf_64bit = self + .maybe_64bit + .as_ref() + .expect("The 64-bit program files directory exists"); + assert_ne!( + &self.x86, pf_64bit, + "The 32-bit and 64-bit program files directories have different locations.", + ); + } + PlatformArchitecture::Is64on64 => { + let pf_64bit = self + .maybe_64bit + .as_ref() + .expect("The 64-bit program files directory exists"); + assert_eq!( + self.current.as_os_str(), + pf_64bit.as_os_str(), + "Our program files path is exactly identical to the 64-bit one.", + ); + assert_ne!( + &self.x86, pf_64bit, + "The 32-bit and 64-bit program files directories have different locations.", + ); + } + } + + self + } + } + + /// Paths relative to process architecture specific program files directories. + #[derive(Clone, Debug)] + struct RelativeGitBinPaths<'a> { + x86: &'a Path, + maybe_64bit: Option<&'a Path>, + } + + impl<'a> RelativeGitBinPaths<'a> { + /// Assert that `locations` has the given path prefixes, and extract the suffixes. + fn assert_from(pf: &'a ProgramFilesPaths, locations: &'static [PathBuf]) -> Self { + match locations { + [primary, secondary] => { + let prefix_64bit = pf + .maybe_64bit + .as_ref() + .expect("It gives two paths only if one can be 64-bit"); + let suffix_64bit = primary + .strip_prefix(prefix_64bit) + .expect("It gives the 64-bit path and lists it first"); + let suffix_x86 = secondary + .strip_prefix(pf.x86.as_path()) + .expect("It gives the 32-bit path and lists it second"); + Self { + x86: suffix_x86, + maybe_64bit: Some(suffix_64bit), + } + } + [only] => { + assert_eq!(pf.maybe_64bit, None, "It gives one path only if none can be 64-bit."); + let suffix_x86 = only + .strip_prefix(pf.x86.as_path()) + .expect("The one path it gives is the 32-bit path"); + Self { + x86: suffix_x86, + maybe_64bit: None, + } + } + other => panic!("{:?} has length {}, expected 1 or 2.", other, other.len()), + } + } + + /// Assert that the suffixes (relative subdirectories) are the common per-architecture Git install locations. + fn assert_architectures(&self) { + assert_eq!(self.x86, Path::new("Git/mingw32/bin")); + + if let Some(suffix_64bit) = self.maybe_64bit { + // When Git for Windows releases ARM64 builds, there will be another 64-bit suffix, + // likely clangarm64. In that case, this and other assertions will need updating, + // as there will be two separate paths to check under the same 64-bit program files + // directory. (See the definition of ProgramFilesPaths::maybe_64bit for details.) + assert_eq!(suffix_64bit, Path::new("Git/mingw64/bin")); + } + } + } + + #[test] + fn alternative_locations() { + // Obtain program files directory paths by other means and check that they seem correct. + let pf = ProgramFilesPaths::obtain_envlessly().validated(); + + // Check that `ALTERNATIVE_LOCATIONS` correspond to them, with the correct subdirectories. + let locations = super::super::ALTERNATIVE_LOCATIONS.as_slice(); + RelativeGitBinPaths::assert_from(&pf, locations).assert_architectures(); + } +} + +#[cfg(not(windows))] +mod locations { + #[test] + fn alternative_locations() { + assert!(super::super::ALTERNATIVE_LOCATIONS.is_empty()); + } +}