From 85c5e2fea1dbba8b8603bde7209bc8604042c55e Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Fri, 30 Aug 2024 20:41:23 -0400 Subject: [PATCH 01/11] Start testing that we clear system/global scopes for fixtures --- tests/tools/src/lib.rs | 36 ++++++++++++++++++++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/tests/tools/src/lib.rs b/tests/tools/src/lib.rs index d4f1b2766a9..8d8650083a5 100644 --- a/tests/tools/src/lib.rs +++ b/tests/tools/src/lib.rs @@ -868,6 +868,8 @@ impl<'a> Drop for Env<'a> { #[cfg(test)] mod tests { use super::*; + use std::fs::File; + use std::io::Write; #[test] fn parse_version() { @@ -882,4 +884,38 @@ mod tests { fn parse_version_with_trailing_newline() { assert_eq!(git_version_from_bytes(b"git version 2.37.2\n").unwrap(), (2, 37, 2)); } + + fn check_configure_clears_scope(scope_option: &str) { + let temp = tempfile::TempDir::new().expect("can create temp dir"); + #[cfg(windows)] + let names = ["-"]; + #[cfg(not(windows))] + let names = ["-", ":"]; + for name in names { + File::create(temp.path().join(name)) + .expect("can create file") + .write_all(b"[foo]\n\tbar = baz\n") + .expect("can write contents"); + } + let mut cmd = std::process::Command::new("git"); + let args = ["config", scope_option, "foo.bar"].map(String::from); + configure_command(&mut cmd, &args, temp.path()); + let output = cmd.output().expect("can run git"); + let stdout = output.stdout.to_str().expect("valid UTF-8"); + let status = output.status.code().expect("terminated normally"); + assert_eq!(stdout, "", "should be no config variable to display"); + assert_eq!(status, 1, "exit status should indicate config variable is absent"); + + temp.close().expect("Test bug: Should be able to delete everything"); + } + + #[test] + fn configure_command_clears_system_scope() { + check_configure_clears_scope("--system"); + } + + #[test] + fn configure_command_clears_global_scope() { + check_configure_clears_scope("--global"); + } } From d5b61df18c5ab184e242f4e9d5d08730f7cd4fe7 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Fri, 30 Aug 2024 21:36:44 -0400 Subject: [PATCH 02/11] Test that the system/global scopes are really cleared Rather than just not having the configuration variable we add. - Keep testing the current failure mode where the unusually named `never_path` files can actually exist. - Also test for paths for those scopes' configuration files, coming from the environment. - Test that the output of showing the full scope's contents is empty, of all values. Note that this includes the system scope but not all scopes higher than the global scope: the "unknown" scope associated with an Apple Git configuration is deliberately not asserted to be cleared here. (`gix-testtools` possibly intentionally does not clear that scope for fixture scripts.) --- tests/tools/src/lib.rs | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/tests/tools/src/lib.rs b/tests/tools/src/lib.rs index 8d8650083a5..0a2d41e8dd6 100644 --- a/tests/tools/src/lib.rs +++ b/tests/tools/src/lib.rs @@ -885,24 +885,29 @@ mod tests { assert_eq!(git_version_from_bytes(b"git version 2.37.2\n").unwrap(), (2, 37, 2)); } - fn check_configure_clears_scope(scope_option: &str) { + fn check_configure_clears_scope(scope_env_var: &str, scope_option: &str) { let temp = tempfile::TempDir::new().expect("can create temp dir"); + #[cfg(windows)] - let names = ["-"]; + let names = ["config", "-"]; #[cfg(not(windows))] - let names = ["-", ":"]; + let names = ["config", "-", ":"]; for name in names { - File::create(temp.path().join(name)) + File::create_new(temp.path().join(name)) .expect("can create file") .write_all(b"[foo]\n\tbar = baz\n") .expect("can write contents"); } + let mut cmd = std::process::Command::new("git"); - let args = ["config", scope_option, "foo.bar"].map(String::from); + cmd.env(scope_env_var, "config"); // configure_command() should override it. + let args = ["config", "-l", "--show-origin", scope_option].map(String::from); configure_command(&mut cmd, &args, temp.path()); + let output = cmd.output().expect("can run git"); let stdout = output.stdout.to_str().expect("valid UTF-8"); let status = output.status.code().expect("terminated normally"); + assert_eq!(stdout, "", "should be no config variable to display"); assert_eq!(status, 1, "exit status should indicate config variable is absent"); @@ -911,11 +916,11 @@ mod tests { #[test] fn configure_command_clears_system_scope() { - check_configure_clears_scope("--system"); + check_configure_clears_scope("GIT_CONFIG_SYSTEM", "--system"); } #[test] fn configure_command_clears_global_scope() { - check_configure_clears_scope("--global"); + check_configure_clears_scope("GIT_CONFIG_GLOBAL", "--global"); } } From a2dc5d85a2ea61da4b4baddcd183b170763ed610 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Fri, 30 Aug 2024 21:54:10 -0400 Subject: [PATCH 03/11] Fix assertion messages and expected exit status --- tests/tools/src/lib.rs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/tests/tools/src/lib.rs b/tests/tools/src/lib.rs index 0a2d41e8dd6..3ca66d77c3d 100644 --- a/tests/tools/src/lib.rs +++ b/tests/tools/src/lib.rs @@ -907,9 +907,8 @@ mod tests { let output = cmd.output().expect("can run git"); let stdout = output.stdout.to_str().expect("valid UTF-8"); let status = output.status.code().expect("terminated normally"); - - assert_eq!(stdout, "", "should be no config variable to display"); - assert_eq!(status, 1, "exit status should indicate config variable is absent"); + assert_eq!(stdout, "", "should be no config variables to display"); + assert_eq!(status, 0, "reading the config should nonetheless succeed"); temp.close().expect("Test bug: Should be able to delete everything"); } From 7186eed39c40df82de2dd61530b6232c34b91ca9 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Sat, 31 Aug 2024 00:26:22 -0400 Subject: [PATCH 04/11] Refactor the test for readability --- tests/tools/src/lib.rs | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/tests/tools/src/lib.rs b/tests/tools/src/lib.rs index 3ca66d77c3d..6c00e87300e 100644 --- a/tests/tools/src/lib.rs +++ b/tests/tools/src/lib.rs @@ -885,13 +885,16 @@ mod tests { assert_eq!(git_version_from_bytes(b"git version 2.37.2\n").unwrap(), (2, 37, 2)); } - fn check_configure_clears_scope(scope_env_var: &str, scope_option: &str) { - let temp = tempfile::TempDir::new().expect("can create temp dir"); + fn check_configure_clears_scope(scope_env_key: &str, scope_option: &str) { + let scope_env_value = "gitconfig"; #[cfg(windows)] - let names = ["config", "-"]; + let names = [scope_env_value, "-"]; #[cfg(not(windows))] - let names = ["config", "-", ":"]; + let names = [scope_env_value, "-", ":"]; + + let temp = tempfile::TempDir::new().expect("can create temp dir"); + for name in names { File::create_new(temp.path().join(name)) .expect("can create file") @@ -900,7 +903,7 @@ mod tests { } let mut cmd = std::process::Command::new("git"); - cmd.env(scope_env_var, "config"); // configure_command() should override it. + cmd.env(scope_env_key, scope_env_value); // configure_command() should override it. let args = ["config", "-l", "--show-origin", scope_option].map(String::from); configure_command(&mut cmd, &args, temp.path()); From f71d5966c204a2f3d1b48d59bad5b880caab717a Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Sat, 31 Aug 2024 04:38:08 -0400 Subject: [PATCH 05/11] Test on Windows with an actual file called `NUL` This covers a possible concern pertaining to a forthcoming minor gix-testtools bugfix. The null device, accessible as `/dev/null` on a Unix-like system, or as `NUL` (and in some other ways) on Windows, is the best candidate to replace the current value of `:` (on Unix) or `-` (on Windows) for `GIT_CONFIG_SYSTEM` and `GIT_CONFIG_GLOBAL` when test fixture scripts are running. This is because the filenames `:` (on Unix) and `-` (on Windows) do not have any special meaning. They are valid but unusual filenames; files of these names most often do not exist, but can, and when they do, their contents are used the populate the system or global scope for test fixtures, which is not the intended behavior. `git` does not treat them as anything but filenames: a `:` is not treated as a separator because those variables are interpreted as single paths rather than lists of paths, and a `-` is not treated to mean standard input (or any other special data source) because it appears in an environment variable rather than as a command-line argument. But `/dev/null` and `NUL` can exist, too. On Unix, where `/dev/null` should be used, the whole point is that `/dev/null` exists. That absolute path is not treated specially. Instead, the device node for the null device is linked there. On Windows, where the null device is named `NUL`, an entry for it with that name does also sort of exist in the filesystem, accessible via the fully qualified device path `\\.\NUL` as well as by some other fully qualified forms such as `\??\NUL`. However, not all interfaces support all path forms, so accessing it in those ways is not necessarily more robust than accessing it with the relative path `NUL`. Yet it is possible for an actual file, not the null device, to exist with the name `NUL` in an arbitrary directory. Such a file can only be referred to using the UNC syntax with a `\\?\` prefixed path (not to be confused with the `\\.\` device namespace prefix, nor the NT `\??\` prefix). When such a file exists, `NUL` itself still does not refer to it; `NUL` still refers to the null device. The additon to the test here (and refactorings to support it) create an acutal file called `NUL` to demonstrate that changing `-` to `NUL` really does avoid getting a configuration file, regardless of what files exist. --- tests/tools/src/lib.rs | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/tests/tools/src/lib.rs b/tests/tools/src/lib.rs index 6c00e87300e..8f6df69dd35 100644 --- a/tests/tools/src/lib.rs +++ b/tests/tools/src/lib.rs @@ -887,16 +887,18 @@ mod tests { fn check_configure_clears_scope(scope_env_key: &str, scope_option: &str) { let scope_env_value = "gitconfig"; - - #[cfg(windows)] - let names = [scope_env_value, "-"]; - #[cfg(not(windows))] - let names = [scope_env_value, "-", ":"]; - let temp = tempfile::TempDir::new().expect("can create temp dir"); + let dir = temp.path(); + + let paths: &[PathBuf] = if cfg!(windows) { + let unc_literal_nul = dir.canonicalize().expect("directory exists").join("NUL"); + &[dir.join(scope_env_value), dir.join("-"), unc_literal_nul] + } else { + &[dir.join(scope_env_value), dir.join("-"), dir.join(":")] + }; - for name in names { - File::create_new(temp.path().join(name)) + for path in paths { + File::create_new(path) .expect("can create file") .write_all(b"[foo]\n\tbar = baz\n") .expect("can write contents"); @@ -905,7 +907,7 @@ mod tests { let mut cmd = std::process::Command::new("git"); cmd.env(scope_env_key, scope_env_value); // configure_command() should override it. let args = ["config", "-l", "--show-origin", scope_option].map(String::from); - configure_command(&mut cmd, &args, temp.path()); + configure_command(&mut cmd, &args, dir); let output = cmd.output().expect("can run git"); let stdout = output.stdout.to_str().expect("valid UTF-8"); From d7dca27b81e4890cd0c597af275890df3d76b048 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Sat, 31 Aug 2024 05:14:08 -0400 Subject: [PATCH 06/11] Verify that we really write the strangely named test files Ordinarily such a check would not be justified, but the behavior of `canonicalize()` in giving UNC paths is non-obvious. --- tests/tools/src/lib.rs | 35 ++++++++++++++++++++++++++--------- 1 file changed, 26 insertions(+), 9 deletions(-) diff --git a/tests/tools/src/lib.rs b/tests/tools/src/lib.rs index 8f6df69dd35..20a024f6078 100644 --- a/tests/tools/src/lib.rs +++ b/tests/tools/src/lib.rs @@ -869,7 +869,7 @@ impl<'a> Drop for Env<'a> { mod tests { use super::*; use std::fs::File; - use std::io::Write; + use std::io::{Read, Write}; #[test] fn parse_version() { @@ -885,27 +885,44 @@ mod tests { assert_eq!(git_version_from_bytes(b"git version 2.37.2\n").unwrap(), (2, 37, 2)); } - fn check_configure_clears_scope(scope_env_key: &str, scope_option: &str) { - let scope_env_value = "gitconfig"; - let temp = tempfile::TempDir::new().expect("can create temp dir"); - let dir = temp.path(); + const SCOPE_ENV_VALUE: &str = "gitconfig"; + + fn populate_ad_hoc_config_files(dir: &Path) { + const CONFIG_DATA: &[u8] = b"[foo]\n\tbar = baz\n"; let paths: &[PathBuf] = if cfg!(windows) { let unc_literal_nul = dir.canonicalize().expect("directory exists").join("NUL"); - &[dir.join(scope_env_value), dir.join("-"), unc_literal_nul] + &[dir.join(SCOPE_ENV_VALUE), dir.join("-"), unc_literal_nul] } else { - &[dir.join(scope_env_value), dir.join("-"), dir.join(":")] + &[dir.join(SCOPE_ENV_VALUE), dir.join("-"), dir.join(":")] }; + // Create the files. for path in paths { File::create_new(path) .expect("can create file") - .write_all(b"[foo]\n\tbar = baz\n") + .write_all(CONFIG_DATA) .expect("can write contents"); } + // Verify the files. This is mostly to show we really made a `\\?\...\NUL` on Windows. + for path in paths { + let mut buf = Vec::with_capacity(CONFIG_DATA.len()); + File::open(path) + .expect("the file really exists") + .read_to_end(&mut buf) + .expect("can read contents"); + assert_eq!(buf, CONFIG_DATA, "File {:?} should be created", path); + } + } + + fn check_configure_clears_scope(scope_env_key: &str, scope_option: &str) { + let temp = tempfile::TempDir::new().expect("can create temp dir"); + let dir = temp.path(); + populate_ad_hoc_config_files(dir); + let mut cmd = std::process::Command::new("git"); - cmd.env(scope_env_key, scope_env_value); // configure_command() should override it. + cmd.env(scope_env_key, SCOPE_ENV_VALUE); // configure_command() should override it. let args = ["config", "-l", "--show-origin", scope_option].map(String::from); configure_command(&mut cmd, &args, dir); From 3cf9fc12cb8ebb9bf04e4f5bd2aee884c18d672f Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Sat, 31 Aug 2024 05:30:47 -0400 Subject: [PATCH 07/11] fix: Omit system/global config in fixtures regardless of contents This uses the null device, `/dev/null` on Unix-like systems and `NUL` on Windows, as the value of `GIT_CONFIG_SYSTEM` and `GIT_CONFIG_GLOBAL` when `gix-testtols` runs test fixture shell scripts. `/dev/null` is explicitly recommended for this purpose, when setting those environment variables for the purpose of preventing configuration files from being read, in the Git documentation: - https://git-scm.com/docs/git#Documentation/git.txt-codeGITCONFIGGLOBALcode On Windows, `NUL` is an analogue of `/dev/null`. Even in the unusual scenario that a `\\?\` prefixed UNC path is used to create an actual file named `NUL` in the directory the fixture script operates in, the relative path `NUL` still resolves to the null device and not to that file. The previous behavior was to use a value of `:` on Unix-like systems or `-` on Windows. But these were really just unusual but valid paths, such that files of those names could exist in any location. `git` furthermore treats them as paths: a `:` is not special in these environment variables because they hold a single path rather than a list of paths, and a `-` is not special (for example, it does not specify stdin) because it appears in an environment variable rather than a command-line argument. While `:` and `-` are unusual filenames, this code is used in testing, including of edge cases where unusual files may be used. So this change may make the test tools slightly more robust. --- tests/tools/src/lib.rs | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/tests/tools/src/lib.rs b/tests/tools/src/lib.rs index 20a024f6078..b20eb322826 100644 --- a/tests/tools/src/lib.rs +++ b/tests/tools/src/lib.rs @@ -583,12 +583,16 @@ fn scripted_fixture_read_only_with_args_inner( Ok(script_result_directory) } +#[cfg(windows)] +const NULL_DEVICE: &str = "NUL"; +#[cfg(not(windows))] +const NULL_DEVICE: &str = "/dev/null"; + fn configure_command<'a>( cmd: &'a mut std::process::Command, args: &[String], script_result_directory: &Path, ) -> &'a mut std::process::Command { - let never_path = if cfg!(windows) { "-" } else { ":" }; let mut msys_for_git_bash_on_windows = std::env::var("MSYS").unwrap_or_default(); msys_for_git_bash_on_windows.push_str(" winsymlinks:nativestrict"); cmd.args(args) @@ -599,8 +603,8 @@ fn configure_command<'a>( .env_remove("GIT_ASKPASS") .env_remove("SSH_ASKPASS") .env("MSYS", msys_for_git_bash_on_windows) - .env("GIT_CONFIG_SYSTEM", never_path) - .env("GIT_CONFIG_GLOBAL", never_path) + .env("GIT_CONFIG_SYSTEM", NULL_DEVICE) + .env("GIT_CONFIG_GLOBAL", NULL_DEVICE) .env("GIT_TERMINAL_PROMPT", "false") .env("GIT_AUTHOR_DATE", "2000-01-01 00:00:00 +0000") .env("GIT_AUTHOR_EMAIL", "author@example.com") From 2d7abaf8a816ecc3ec8a006223d4b636eab7a1b6 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Sat, 31 Aug 2024 06:01:20 -0400 Subject: [PATCH 08/11] Thanks clippy --- tests/tools/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/tools/src/lib.rs b/tests/tools/src/lib.rs index b20eb322826..ddd0e37b5bd 100644 --- a/tests/tools/src/lib.rs +++ b/tests/tools/src/lib.rs @@ -916,7 +916,7 @@ mod tests { .expect("the file really exists") .read_to_end(&mut buf) .expect("can read contents"); - assert_eq!(buf, CONFIG_DATA, "File {:?} should be created", path); + assert_eq!(buf, CONFIG_DATA, "File {path:?} should be created"); } } From 40ac226e6d44a05c8731e4eaf647bb5d6a9dda79 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Sat, 31 Aug 2024 06:08:37 -0400 Subject: [PATCH 09/11] Avoid `File::create_new` for compatibility with project MSRV Besides the style change done in the previous commit, `clippy` also identified `File::create_new` is newer than we may wish to rely on: error: current MSRV (Minimum Supported Rust Version) is `1.76.0` but this item is stable since `1.77.0` --> tests/tools/src/lib.rs:906:13 | 906 | File::create_new(path) | ^^^^^^^^^^^^^^^^ This changes that code to use `OpenOptions` instead. This is simlar to the suggested equivalent code in the `File::create_new` documentation: File::options().read(true).write(true).create_new(true).open(...) But the approach used here differs in that `read(true)` is removed, since this call is only creating and writing, not reading. --- tests/tools/src/lib.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/tests/tools/src/lib.rs b/tests/tools/src/lib.rs index ddd0e37b5bd..884870bac67 100644 --- a/tests/tools/src/lib.rs +++ b/tests/tools/src/lib.rs @@ -903,7 +903,10 @@ mod tests { // Create the files. for path in paths { - File::create_new(path) + File::options() + .write(true) + .create_new(true) + .open(path) .expect("can create file") .write_all(CONFIG_DATA) .expect("can write contents"); From 15bb2e36b17b57ae08d263b8e550e4655aad74c8 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Sat, 31 Aug 2024 06:13:11 -0400 Subject: [PATCH 10/11] Don't assert that `tempfile::TempDir` cleans up It still does clean up (that is done by RAII; explicitly calling `close()` is just to be able to identify and handle errors). The assertion that it does is not part of what the test is testing, nor is it critical to the correctness of the test itself. I had put it there to catch bugs that might arise during development of the test itself. (In some libraries/frameworks, a recursive deletion on Windows that involves files that need to be accessed with `\\?\` paths does not automatically succeed.) --- tests/tools/src/lib.rs | 2 -- 1 file changed, 2 deletions(-) diff --git a/tests/tools/src/lib.rs b/tests/tools/src/lib.rs index 884870bac67..55a833f9faa 100644 --- a/tests/tools/src/lib.rs +++ b/tests/tools/src/lib.rs @@ -938,8 +938,6 @@ mod tests { let status = output.status.code().expect("terminated normally"); assert_eq!(stdout, "", "should be no config variables to display"); assert_eq!(status, 0, "reading the config should nonetheless succeed"); - - temp.close().expect("Test bug: Should be able to delete everything"); } #[test] From 50fcd7eb2dee0a8f57b1ffcf01868379571c1afb Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Sat, 31 Aug 2024 17:25:52 +0200 Subject: [PATCH 11/11] minor refactors --- tests/tools/src/lib.rs | 16 ++-------------- 1 file changed, 2 insertions(+), 14 deletions(-) diff --git a/tests/tools/src/lib.rs b/tests/tools/src/lib.rs index 55a833f9faa..e190a1ac58b 100644 --- a/tests/tools/src/lib.rs +++ b/tests/tools/src/lib.rs @@ -872,8 +872,6 @@ impl<'a> Drop for Env<'a> { #[cfg(test)] mod tests { use super::*; - use std::fs::File; - use std::io::{Read, Write}; #[test] fn parse_version() { @@ -903,22 +901,12 @@ mod tests { // Create the files. for path in paths { - File::options() - .write(true) - .create_new(true) - .open(path) - .expect("can create file") - .write_all(CONFIG_DATA) - .expect("can write contents"); + std::fs::write(path, CONFIG_DATA).expect("can write contents"); } // Verify the files. This is mostly to show we really made a `\\?\...\NUL` on Windows. for path in paths { - let mut buf = Vec::with_capacity(CONFIG_DATA.len()); - File::open(path) - .expect("the file really exists") - .read_to_end(&mut buf) - .expect("can read contents"); + let buf = std::fs::read(path).expect("the file really exists"); assert_eq!(buf, CONFIG_DATA, "File {path:?} should be created"); } }