Skip to content

Commit b1c48e8

Browse files
committed
Test no local scope with empty system config
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).
1 parent 65e00b3 commit b1c48e8

File tree

1 file changed

+41
-2
lines changed

1 file changed

+41
-2
lines changed

gix-path/src/env/git/tests.rs

Lines changed: 41 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -478,9 +478,27 @@ fn exe_info_same_result_with_oversanitized_env() {
478478

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

485+
let _cwd = gix_testtools::set_current_dir(repo).expect("can change to repo dir");
486+
let _env = gix_testtools::Env::new()
487+
.set("GIT_CONFIG_SYSTEM", super::NULL_DEVICE)
488+
.set("GIT_CONFIG_GLOBAL", super::NULL_DEVICE);
489+
490+
let maybe_path = super::exe_info();
491+
assert_eq!(
492+
maybe_path, None,
493+
"Should find no config path if the config would be local (empty system config)"
494+
);
495+
}
496+
497+
#[test]
498+
#[serial]
499+
fn exe_info_never_from_local_scope_nosystem() {
500+
let repo = gix_testtools::scripted_fixture_read_only("local_config.sh").expect("script succeeds");
501+
484502
let _cwd = gix_testtools::set_current_dir(repo).expect("can change to repo dir");
485503
let _env = gix_testtools::Env::new()
486504
.set("GIT_CONFIG_NOSYSTEM", "1")
@@ -489,18 +507,39 @@ fn exe_info_never_from_local_scope() {
489507
let maybe_path = super::exe_info();
490508
assert_eq!(
491509
maybe_path, None,
492-
"Should find no config path if the config would be local"
510+
"Should find no config path if the config would be local (suppressed system config)"
493511
);
494512
}
495513

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

523+
let _cwd = gix_testtools::set_current_dir(&repo).expect("can change to repo dir");
524+
let _env = set_temp_env_vars(&repo)
525+
.set("GIT_CONFIG_SYSTEM", super::NULL_DEVICE)
526+
.set("GIT_CONFIG_GLOBAL", super::NULL_DEVICE);
527+
528+
let maybe_path = super::exe_info();
529+
assert_eq!(
530+
maybe_path, None,
531+
"Should find no config path if the config would be local even in a `/tmp`-like dir (empty system config)"
532+
);
533+
}
534+
535+
#[test]
536+
#[serial]
537+
fn exe_info_never_from_local_scope_even_if_temp_is_here_nosystem() {
538+
let repo = gix_testtools::scripted_fixture_read_only("local_config.sh")
539+
.expect("script succeeds")
540+
.canonicalize()
541+
.expect("repo path is valid and exists");
542+
504543
let _cwd = gix_testtools::set_current_dir(&repo).expect("can change to repo dir");
505544
let _env = set_temp_env_vars(&repo)
506545
.set("GIT_CONFIG_NOSYSTEM", "1")
@@ -509,7 +548,7 @@ fn exe_info_never_from_local_scope_even_if_temp_is_here() {
509548
let maybe_path = super::exe_info();
510549
assert_eq!(
511550
maybe_path, None,
512-
"Should find no config path if the config would be local even in a `/tmp`-like dir"
551+
"Should find no config path if the config would be local even in a `/tmp`-like dir (suppressed system config)"
513552
);
514553
}
515554

0 commit comments

Comments
 (0)