From 01a412feb5abf201a7eb98901d81f73a83a820e6 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Fri, 6 Sep 2024 00:03:16 -0400 Subject: [PATCH 1/4] Unset other `GIT_DIR` related env vars in `git config -l` This has the `git config -l ...` invocation `gix-path` uses to find a configuration file to treat as associated with the `git` installation remove more environment variables related to `GIT_DIR` besides `GIT_COMMON_DIR`. The new removals probably don't make a difference, because they are only used if `GIT_DIR` is either unset or set to a location that actually has a Git repository (that `git` is willing to use). Unsetting these others is thus more to make clear that they have no effect than to prevent them from having an effect, though various versions and downstream releases of `git` exist, so it's narrowly possible that this is also avoiding a problem somewhere, I guess. This change is motivated by my realization in 9d4dd12 (#1581) that it could be slightly beneficial. Other differences between the variables unset here and in `gix-testtools` remain due to the differences described in that commit message. --- gix-path/src/env/git/mod.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/gix-path/src/env/git/mod.rs b/gix-path/src/env/git/mod.rs index f32f06eef2f..f04cc17ba3b 100644 --- a/gix-path/src/env/git/mod.rs +++ b/gix-path/src/env/git/mod.rs @@ -140,8 +140,10 @@ fn git_cmd(executable: PathBuf) -> Command { // scope. Although `GIT_CONFIG_NOSYSTEM` will suppress this as well, passing --system omits it. cmd.args(["config", "-lz", "--show-origin", "--name-only"]) .current_dir(cwd) - .env_remove("GIT_COMMON_DIR") // We are setting `GIT_DIR`. .env_remove("GIT_DISCOVERY_ACROSS_FILESYSTEM") + .env_remove("GIT_OBJECT_DIRECTORY") + .env_remove("GIT_ALTERNATE_OBJECT_DIRECTORIES") + .env_remove("GIT_COMMON_DIR") .env("GIT_DIR", NULL_DEVICE) // Avoid getting local-scope config. .env("GIT_WORK_TREE", NULL_DEVICE) // Avoid confusion when debugging. .stdin(Stdio::null()) From 340ff371d181686afd80582f7d5c8f340aabd24c Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Fri, 6 Sep 2024 01:05:26 -0400 Subject: [PATCH 2/4] Start testing that installation config is not from `GIT_CONFIG` These tests should not pass yet, because we currently do allow `GIT_CONFIG` to be read. Although the optimization that reads `EXEPATH` will sometimes prevent it from being used, it will usually be used, becuase `EXEPATH` is usually not set on Windows (and outside of Windows, that optimization is inapplicable). Furthermore, the tests in `gix-path/src/env/git/tests.rs` bypass that optimization. Two of the three new tests rightly fail. But one of them, `never_from_git_config_env_var`, wrongly passes, due to a big in the test, described in the comments there. --- gix-path/src/env/git/tests.rs | 48 +++++++++++++++++++++++++++++++++++ 1 file changed, 48 insertions(+) diff --git a/gix-path/src/env/git/tests.rs b/gix-path/src/env/git/tests.rs index e02fdb349f0..1e7de260529 100644 --- a/gix-path/src/env/git/tests.rs +++ b/gix-path/src/env/git/tests.rs @@ -453,6 +453,13 @@ mod exe_info { check_exe_info(); } + #[test] + #[serial] + fn tolerates_git_config_env_var() { + let _env = gix_testtools::Env::new().set("GIT_CONFIG", NULL_DEVICE); + check_exe_info(); + } + #[test] #[serial] fn same_result_with_broken_temp() { @@ -480,6 +487,19 @@ mod exe_info { assert_eq!(with_unmodified_env, with_oversanitized_env); } + #[test] + #[serial] + fn same_result_with_git_config_env_var() { + let with_unmodified_env = exe_info(); + + let with_git_config_env_var = { + let _env = gix_testtools::Env::new().set("GIT_CONFIG", NULL_DEVICE); + exe_info() + }; + + assert_eq!(with_unmodified_env, with_git_config_env_var); + } + #[test] #[serial] #[cfg(not(target_os = "macos"))] // Assumes no higher "unknown" scope. The `nosystem` case works. @@ -556,6 +576,34 @@ mod exe_info { ); } + #[test] + #[serial] + fn never_from_git_config_env_var() { + let repo = gix_testtools::scripted_fixture_read_only("local_config.sh").expect("script succeeds"); + + // FIXME: exe_info() changes directory, so this is not seen, which results in this test + // wrongly passing even if exe_info() does not really ensure GIT_CONFIG is ignore. So + // either make this path an absolute non-UNC path (non-UNC to ensure all versions of Git + // accept it) or use a newly created temporary file rather than the fixture. + let config_path = repo + .join(".git") + .join("config") + .to_str() + .expect("valid UTF-8") + .to_owned(); + + let _env = gix_testtools::Env::new() + .set("GIT_CONFIG_NOSYSTEM", "1") + .set("GIT_CONFIG_GLOBAL", NULL_DEVICE) + .set("GIT_CONFIG", config_path); + + let maybe_path = exe_info(); + assert_eq!( + maybe_path, None, + "Should find no config path from GIT_CONFIG (even if nonempty)" + ); + } + #[test] fn first_file_from_config_with_origin() { let macos = From f8e38d0575e7e898133aa471539ad79b7508c88e Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Fri, 6 Sep 2024 01:34:05 -0400 Subject: [PATCH 3/4] Fix the `never_from_git_config_env_var` test So it fails too, as expected. --- gix-path/src/env/git/tests.rs | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/gix-path/src/env/git/tests.rs b/gix-path/src/env/git/tests.rs index 1e7de260529..0b2bf195af9 100644 --- a/gix-path/src/env/git/tests.rs +++ b/gix-path/src/env/git/tests.rs @@ -581,11 +581,10 @@ mod exe_info { fn never_from_git_config_env_var() { let repo = gix_testtools::scripted_fixture_read_only("local_config.sh").expect("script succeeds"); - // FIXME: exe_info() changes directory, so this is not seen, which results in this test - // wrongly passing even if exe_info() does not really ensure GIT_CONFIG is ignore. So - // either make this path an absolute non-UNC path (non-UNC to ensure all versions of Git - // accept it) or use a newly created temporary file rather than the fixture. - let config_path = repo + // Get an absolute path to a config file that is non-UNC if possible so any Git accepts it. + let config_path = std::env::current_dir() + .expect("got CWD") + .join(repo) .join(".git") .join("config") .to_str() From eb72d31a64b5041170e5903c328b246dab522067 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Fri, 6 Sep 2024 01:52:31 -0400 Subject: [PATCH 4/4] fix: Don't read "installation" config from `GIT_CONFIG` When `gix-path` runs `git config -l ...` to obtain the path of a configuration file to regard as being associated with the `git` installation itself, it now no longer allows `GIT_CONFIG` to affect this path. Previously, setting `GIT_CONFIG` would treat it as the path of a `GitInstallation` configuration file. Although it is possible that this may have been used intentionally, it was never documented, did not work reliably on all platforms, and carried significant disadvantages. This is most likely a non-breaking change. The disadvantages of treating a path in `GIT_CONFIG` as the path to a configuration file associated with the `git` installation were: - For `git`, the `GIT_CONFIG` environment variable *only* affects `git config` commands, and not other commands that use configuration variables (which are most `git` commands). But when `gix-path` would obtain a path from `git config -l ...` that came from `GIT_CONFIG`, that configuration file would be used anywhere, and not only `gix config` commands. - Even for `gix config`, it is not at all clear that the `GIT_CONFIG` environment variable ought to be honored, because `gix config` has a different interface from and is not meant to be a drop-in replacement for `git config`, and because this environment variable is only even supported by `git config` for historical reasons. (In `git config`, one should typically use `--file` instead.) - The installation path is generally the path of a high-scoped configuration file, usually the system scope or, with Apple Git on macOS, the "unknown" scope even higher than that (of a file located under `/Library` or `/Applications`). In contrast, the `GIT_CONFIG` environment variable is really command scope, since it is an alternative way of achieving the same goal as `--file` when running `git config`, which `git` supports only for backward compatibility. - On Windows, when `EXEPATH` is set in a way that indentifies a Git for Windows installation directory, which is typically the case in a Git Bash environment (and not otherwise), `GIT_CONFIG` is not used by the public `gix-path` functions that ordinarily get information from calling `git config -l ...`, because this call is not performed and instead the location of a configuration file associated with the installation is inferred from that value. Although this also applies to some other ways the environment affects the behavior of `git config -l ...`, it is much stranger for `GIT_CONFIG`, because whether `GIT_CONFIG` should be used does not intuitively seem like it should be related to what other sources of information are available; the semantics of `GIT_CONFIG` for `git` are to be ignored by most commands, but to always be used commands (`git config`) that do not ignore it. - The effect on finding other files associated with the installation, such as a gitattributes file, may be especially hard to reason about for `GIT_CONFIG`. Such paths are sometimes inferred from the path that `gix-path` reports. In particular, this is how `installation_config_prefix()` works. This change only prevents `GIT_CONFIG` from specifying an installation config file or suppressing the search for it. This does not affect how other `GIT_CONFIG_*` environment variables are used. --- gix-path/src/env/git/mod.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/gix-path/src/env/git/mod.rs b/gix-path/src/env/git/mod.rs index f04cc17ba3b..03d7a000aed 100644 --- a/gix-path/src/env/git/mod.rs +++ b/gix-path/src/env/git/mod.rs @@ -140,6 +140,7 @@ fn git_cmd(executable: PathBuf) -> Command { // scope. Although `GIT_CONFIG_NOSYSTEM` will suppress this as well, passing --system omits it. cmd.args(["config", "-lz", "--show-origin", "--name-only"]) .current_dir(cwd) + .env_remove("GIT_CONFIG") .env_remove("GIT_DISCOVERY_ACROSS_FILESYSTEM") .env_remove("GIT_OBJECT_DIRECTORY") .env_remove("GIT_ALTERNATE_OBJECT_DIRECTORIES")