From d576b321008d7e19180cfcd6a8d132352600bc91 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Sun, 1 Sep 2024 06:11:12 -0400 Subject: [PATCH 1/3] Test that env for fixture scripts has only command-scope config It doesn't have to have any configuration variables set in the command scope, though in practice it always should (with versions of `git` with which `gix-testtools` is compatible) because we are setting them with `GIT_CONFIG_{COUNT,KEY,VALUE}`, which, like `-c`, sets configuration variables in the command scope. But it must not have any configuration variables set in other scopes. Of course, actual fixture scripts, when run, most often create Git repositories, in which case there will in practice almost always be local scope configuration for the script. The main point here is to make sure we are omitting variables from the global and system scopes, as we have already been doing (and testing for), and also omitting them from the other high scopes, such as the "unknown" scope associated with an Apple Git installation that is higher than the system scope and unaffected by `GIT_CONFIG_SYSTEM` but affected by `GIT_CONFIG_NOSYSTEM`. Like the tests that preceded it, this test creates a new empty temporary directory to set as the CWD of the test `git` command configured by `configure_command`. As such, there should be no local scope configuration, because this directory should be a subdirectory of a system `/tmp`-like directory. A `/tmp`-like directory can technically be a Git repository, and can even contribute configuration if the repository is owned by the current user or something is keeping `safe.directory` protections from excluding it. When the goal is to *get* configuration from scopes higher than the local scope, it is worth taking steps to prevent this (which for `gix-path` is achieved by the combination of #1523 and #1567). But in the test suite, temporary directories should preferably be in locations that are only `git` repositories (owned by the curent user) in the unusual situation that this is desired, and supporting tooling such as `git` should be at recent enough versions to really support the usage that the test suite makes of it. Furthermore, while it is possible to clear the local scope in this new test -- such as by using, as the command's CWD, a subdirectory of a directory that is, in the command's environment, prepended to `GIT_CEILING_DIRECTORIES` -- this would tend to hide problems in the actual `gix-testtools` code that this is testing. If any such measure needs to be taken, it would be better done in code that uses `configure_command` (or in `configure_command` itself it it is widely needed and generally acceptable), rather than in tests of `configure_command`. For these reasons, the test is, at least for now, deliberately written in such a way that it will fail if the directory we get from `tempfile::TempDir::new()` is somehow a Git repository. This commit consolidates the two preceding test cases into this one. So now there is a single test that `configure-command` both: - Excludes global and system scope config, as it already did. - Also excludes other high-scoped config, which it doesn't yet do. Thus, this new test is expected to fail on most macOS systems (where `git` is Apple Git and the installation-associated "unknown" scope configuration file is nonempty), until that is fixed. --- tests/tools/src/lib.rs | 38 ++++++++++++++++---------------------- 1 file changed, 16 insertions(+), 22 deletions(-) diff --git a/tests/tools/src/lib.rs b/tests/tools/src/lib.rs index e190a1ac58..98e1a7c5e1 100644 --- a/tests/tools/src/lib.rs +++ b/tests/tools/src/lib.rs @@ -898,43 +898,37 @@ mod tests { } else { &[dir.join(SCOPE_ENV_VALUE), dir.join("-"), dir.join(":")] }; - // Create the files. for path in paths { 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 buf = std::fs::read(path).expect("the file really exists"); - assert_eq!(buf, CONFIG_DATA, "File {path:?} should be created"); + assert_eq!(buf, CONFIG_DATA, "{path:?} should be a config file"); } } - fn check_configure_clears_scope(scope_env_key: &str, scope_option: &str) { + #[test] + fn configure_command_clears_external_config() { let temp = tempfile::TempDir::new().expect("can create temp dir"); - let dir = temp.path(); - populate_ad_hoc_config_files(dir); + populate_ad_hoc_config_files(temp.path()); 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, dir); + let args = ["config", "-l", "--show-origin"].map(String::from); + cmd.env("GIT_CONFIG_SYSTEM", SCOPE_ENV_VALUE); + cmd.env("GIT_CONFIG_GLOBAL", SCOPE_ENV_VALUE); + configure_command(&mut cmd, &args, temp.path()); let output = cmd.output().expect("can run git"); - let stdout = output.stdout.to_str().expect("valid UTF-8"); + let lines: Vec<_> = output.stdout + .to_str() + .expect("valid UTF-8") + .lines() + .filter(|line| !line.starts_with("command line:\t")) + .collect(); 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"); - } - - #[test] - fn configure_command_clears_system_scope() { - check_configure_clears_scope("GIT_CONFIG_SYSTEM", "--system"); - } - - #[test] - fn configure_command_clears_global_scope() { - check_configure_clears_scope("GIT_CONFIG_GLOBAL", "--global"); + assert_eq!(lines, Vec::<&str>::new(), "should be no config variables from files"); + assert_eq!(status, 0, "reading the config should succeed"); } } From a879d2214ae40be7692fa00360c8151bb8e2e88e Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Sun, 1 Sep 2024 06:37:16 -0400 Subject: [PATCH 2/3] fix: Omit other high-scoped config in fixtures In addition to keeping fixture scripts from receiving global and system scope Git configuration variables, as was already done, this also omits configuration variables from high scopes similar to or above the system scope, associated with the Git installation but separate from the system scope. The main and possibly only case where this happens is the "unknown" scope associated with an Apple Git installation on macOS. This is a file usually located under `/Library` or `/Applications`. This is done by using `GIT_CONFIG_NOSYSTEM`, which suppresses both the system scope and this separate "unknown" scope, instead of by settng `GIT_CONFIG_SYSTEM` to a path like `/dev/null`. The latter approach continues to be used to omit global scope config via `GIT_CONFIG_GLOBAL` (as `git` recognized no `GIT_CONFIG_NOGLOBAL`). --- 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 98e1a7c5e1..e122807fea 100644 --- a/tests/tools/src/lib.rs +++ b/tests/tools/src/lib.rs @@ -603,7 +603,7 @@ fn configure_command<'a>( .env_remove("GIT_ASKPASS") .env_remove("SSH_ASKPASS") .env("MSYS", msys_for_git_bash_on_windows) - .env("GIT_CONFIG_SYSTEM", NULL_DEVICE) + .env("GIT_CONFIG_NOSYSTEM", "1") .env("GIT_CONFIG_GLOBAL", NULL_DEVICE) .env("GIT_TERMINAL_PROMPT", "false") .env("GIT_AUTHOR_DATE", "2000-01-01 00:00:00 +0000") From 91e065cbbaad4454f9116d43e5a43a0d20bfd866 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Sun, 1 Sep 2024 07:04:40 -0400 Subject: [PATCH 3/3] Run `cargo fmt` --- tests/tools/src/lib.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/tools/src/lib.rs b/tests/tools/src/lib.rs index e122807fea..b3c0102eab 100644 --- a/tests/tools/src/lib.rs +++ b/tests/tools/src/lib.rs @@ -921,7 +921,8 @@ mod tests { configure_command(&mut cmd, &args, temp.path()); let output = cmd.output().expect("can run git"); - let lines: Vec<_> = output.stdout + let lines: Vec<_> = output + .stdout .to_str() .expect("valid UTF-8") .lines()