Skip to content

Commit

Permalink
Test no local scope with empty system config
Browse files Browse the repository at this point in the history
This is in addition to testing it with suppressed system config,
which was already being done.

The `exe_info_never_from_local_scope*` tests are now back to the
approach they originally used, setting `GIT_CONFIG_SYSTEM` rather
than `GIT_CONFIG_NOSYSTEM`, and new tests (or, rather, the previous
ones, renamed) set `GIT_CONFIG_NOSYSTEM`.

The reason setting `GIT_CONFIG_SYSTEM` to paths like `/dev/null`
had been replaced by setting `GIT_CONFIG_NOSYSTEM` to `1` was to
make the tests work on macOS, where it is harder for an end-to-end
test to allow a system-scope configuration to be used but arrange
for it to be empty. This is harder on macOS because of the
"unknown" scope of configuration variables from a file belonging to
the installation, often in `/Library`, which is higher than the
system scope and can be suppressed via `GIT_CONFIG_NOSYSTEM`, but
which is not suppressed by setting `GIT_CONFIG_SYSTEM` to an empty
file.

The tests, brought back here, that set `GIT_CONFIG_SYSTEM` while
leaving `GIT_CONFIG_NOSYSTEM` unset, are thus skipped on macOS.
In part to allow the important functionality in play to be *mostly*
tested even on macOS, and in part because it is an important case
unto itself, this also preserves (renamed) tests that set
`GIT_CONFIG_NOSYSTEM`.

The reason to restore testing of the `GIT_CONFIG_SYSTEM` approach
is threefold:

- It is not obvious by reading the tests themselves that setting
  `GIT_CONFIG_NOSYSTEM` covers cases where system scope config is
  allowed but happens to provide no configuration variables.

- Although the implementation does not currently special-case
  `GIT_CONFIG_NOSYSTEM` as a shortcut to elide the `git config -l`
  subprocess invocation, this may be done in the future.

  (I believe the main reason it is not done now is that we
  currently allow global scope configuration to be treated as
  belonging to the installation if no higher scope config is
  available, and it is not obvious that this behavior will be
  preserved.)

- Even if we get configuration we consider to be associated with
  the `git` installation that does not come from the system scope,
  suppressing the system scope with `GIT_CONFIG_NOSYSTEM` often
  causes it not to be used. That environment variable is checked in
  `gix_config::types::Source::storage_location()`, suppressing both
  the `GitInstallation` and `System` cases.

  Therefore, while local scope configuration leaking through would
  carry some risk even when `GIT_CONFIG_NOSYSTEM` is turned on, the
  significance of this is unclear, and it is not the main concern.
  What most needs to be avoided is misattribuitng local scope
  configuration as being associated with the `git` installation
  when configuration associated with the `git` installation is
  actually intended to be used.

  So there should be test cases that cover that, so it is clear
  that it is covered, and so it is clear from reading the test
  suite that these tests really are related to a realistic problem.

The assertion messages are edited to distinguish between system
scope configuration being suppressed versus being empty (no vars).
  • Loading branch information
EliahKagan committed Aug 30, 2024
1 parent 2bce0d2 commit 6160a83
Showing 1 changed file with 41 additions and 2 deletions.
43 changes: 41 additions & 2 deletions gix-path/src/env/git/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -478,9 +478,27 @@ fn exe_info_same_result_with_oversanitized_env() {

#[test]
#[serial]
#[cfg(not(target_os = "macos"))] // Assumes no higher "unknown" scope. The `nosystem` case works.
fn exe_info_never_from_local_scope() {
let repo = gix_testtools::scripted_fixture_read_only("local_config.sh").expect("script succeeds");

let _cwd = gix_testtools::set_current_dir(repo).expect("can change to repo dir");
let _env = gix_testtools::Env::new()
.set("GIT_CONFIG_SYSTEM", super::NULL_DEVICE)
.set("GIT_CONFIG_GLOBAL", super::NULL_DEVICE);

let maybe_path = super::exe_info();
assert_eq!(
maybe_path, None,
"Should find no config path if the config would be local (empty system config)"
);
}

#[test]
#[serial]
fn exe_info_never_from_local_scope_nosystem() {
let repo = gix_testtools::scripted_fixture_read_only("local_config.sh").expect("script succeeds");

let _cwd = gix_testtools::set_current_dir(repo).expect("can change to repo dir");
let _env = gix_testtools::Env::new()
.set("GIT_CONFIG_NOSYSTEM", "1")
Expand All @@ -489,18 +507,39 @@ fn exe_info_never_from_local_scope() {
let maybe_path = super::exe_info();
assert_eq!(
maybe_path, None,
"Should find no config path if the config would be local"
"Should find no config path if the config would be local (suppressed system config)"
);
}

#[test]
#[serial]
#[cfg(not(target_os = "macos"))] // Assumes no higher "unknown" scope. The `nosystem` case works.
fn exe_info_never_from_local_scope_even_if_temp_is_here() {
let repo = gix_testtools::scripted_fixture_read_only("local_config.sh")
.expect("script succeeds")
.canonicalize()
.expect("repo path is valid and exists");

let _cwd = gix_testtools::set_current_dir(&repo).expect("can change to repo dir");
let _env = set_temp_env_vars(&repo)
.set("GIT_CONFIG_SYSTEM", super::NULL_DEVICE)
.set("GIT_CONFIG_GLOBAL", super::NULL_DEVICE);

let maybe_path = super::exe_info();
assert_eq!(
maybe_path, None,
"Should find no config path if the config would be local even in a `/tmp`-like dir (empty system config)"
);
}

#[test]
#[serial]
fn exe_info_never_from_local_scope_even_if_temp_is_here_nosystem() {
let repo = gix_testtools::scripted_fixture_read_only("local_config.sh")
.expect("script succeeds")
.canonicalize()
.expect("repo path is valid and exists");

let _cwd = gix_testtools::set_current_dir(&repo).expect("can change to repo dir");
let _env = set_temp_env_vars(&repo)
.set("GIT_CONFIG_NOSYSTEM", "1")
Expand All @@ -509,7 +548,7 @@ fn exe_info_never_from_local_scope_even_if_temp_is_here() {
let maybe_path = super::exe_info();
assert_eq!(
maybe_path, None,
"Should find no config path if the config would be local even in a `/tmp`-like dir"
"Should find no config path if the config would be local even in a `/tmp`-like dir (suppressed system config)"
);
}

Expand Down

0 comments on commit 6160a83

Please sign in to comment.