Skip to content

Find git installation-level configuration more robustly #1567

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 39 commits into from
Aug 31, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
39 commits
Select commit Hold shift + click to select a range
9df57aa
Comment Git version compatibility for EXE_INFO
EliahKagan Aug 24, 2024
650a1b5
fix: Parse installation config path more robustly
EliahKagan Aug 24, 2024
de2f35f
Extract git_cmd helper for EXE_INFO
EliahKagan Aug 24, 2024
ccd0401
Reorder gix_path::env::git tests to match order in code
EliahKagan Aug 24, 2024
1ee98bf
Make EXE_INFO testable and add a basic test for it
EliahKagan Aug 24, 2024
5a300e6
Test that EXE_INFO never has local scope config
EliahKagan Aug 24, 2024
fd065ac
Add generated archive for local_config.sh
EliahKagan Aug 24, 2024
49e0715
Fix EXE_INFO no local scope test for macOS
EliahKagan Aug 24, 2024
65d5151
Code formatting
EliahKagan Aug 24, 2024
287f267
Test EXE_INFO no local config even if temp dir is a repo
EliahKagan Aug 26, 2024
744bb38
Fix bug in new test where temp dir should be a repo
EliahKagan Aug 26, 2024
15cec4e
Check that the test affects `env::temp_dir()` as desired
EliahKagan Aug 26, 2024
7280a2d
fix: More robustly ensure "installation" config is not local
EliahKagan Aug 26, 2024
5723077
Set GIT_WORK_TREE along with GIT_DIR, to avoid confusion
EliahKagan Aug 26, 2024
9641660
Slightly improve quality of test failure messages
EliahKagan Aug 26, 2024
60465a5
Test EXE_INFO no local config even if temp dir doesn't exist
EliahKagan Aug 26, 2024
703f882
Clarify assert and expect messages
EliahKagan Aug 26, 2024
79af259
Check `env::temp_dir()` in both tests that set temp vars
EliahKagan Aug 26, 2024
5c1b4c0
Adjust some test code for clarity
EliahKagan Aug 26, 2024
56dab13
Maybe slightly decrease risk of test precondition check failure
EliahKagan Aug 26, 2024
e60540f
Extract nonexistent directory logic to a test helper struct
EliahKagan Aug 26, 2024
c80d562
Add another broken temp test
EliahKagan Aug 26, 2024
15e7b67
Fix a test name for consistency
EliahKagan Aug 26, 2024
f35e44c
Explain why we don't just use `--show-scope`
EliahKagan Aug 26, 2024
29c6cca
Explain why we don't just use `--system`
EliahKagan Aug 26, 2024
f70b904
fix: Don't require usable temp dir to get installation config
EliahKagan Aug 26, 2024
8472447
Fix unused import on non-Windows systems
EliahKagan Aug 28, 2024
ab0dcc1
Refactor for readability; clarify comments
EliahKagan Aug 28, 2024
1305114
Fix `os::windows` error on non-Windows
EliahKagan Aug 28, 2024
598c487
Small clarity tweaks
EliahKagan Aug 28, 2024
7fa5e35
Explain why we run `git` from a different directory
EliahKagan Aug 29, 2024
b827813
Simplify the new comments
EliahKagan Aug 29, 2024
8f6d39d
Unset a couple env vars just in case
EliahKagan Aug 29, 2024
4e936bc
Fix misstatement of Windows directory rationale
EliahKagan Aug 30, 2024
073e277
Explore also setting a ceiling directory
EliahKagan Aug 30, 2024
2bce0d2
Don't set/change ceiling directories
EliahKagan Aug 30, 2024
6160a83
Test no local scope with empty system config
EliahKagan Aug 30, 2024
5200184
Clarify comment about where we run `git` from
EliahKagan Aug 30, 2024
5ac5f74
improve structure of `exe_info` tests
Byron Aug 31, 2024
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 2 additions & 1 deletion gix-path/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,8 @@ once_cell = "1.17.1"
home = "0.5.5"

[dev-dependencies]
tempfile = "3.3.0"
gix-testtools = { path = "../tests/tools" }
serial_test = { version = "3.1.0", default-features = false }

[target.'cfg(windows)'.dev-dependencies]
known-folders = "1.1.0"
Expand Down
79 changes: 57 additions & 22 deletions gix-path/src/env/git/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -80,21 +80,14 @@ pub(super) static EXE_NAME: &str = "git";
/// Invoke the git executable to obtain the origin configuration, which is cached and returned.
///
/// The git executable is the one found in PATH or an alternative location.
pub(super) static EXE_INFO: Lazy<Option<BString>> = Lazy::new(|| {
let git_cmd = |executable: PathBuf| {
let mut cmd = Command::new(executable);
#[cfg(windows)]
{
use std::os::windows::process::CommandExt;
const CREATE_NO_WINDOW: u32 = 0x08000000;
cmd.creation_flags(CREATE_NO_WINDOW);
}
cmd.args(["config", "-l", "--show-origin"])
.current_dir(env::temp_dir())
.stdin(Stdio::null())
.stderr(Stdio::null());
cmd
};
pub(super) static EXE_INFO: Lazy<Option<BString>> = Lazy::new(exe_info);

#[cfg(windows)]
static NULL_DEVICE: &str = "NUL";
#[cfg(not(windows))]
static NULL_DEVICE: &str = "/dev/null";

fn exe_info() -> Option<BString> {
let mut cmd = git_cmd(EXE_NAME.into());
gix_trace::debug!(cmd = ?cmd, "invoking git for installation config path");
let cmd_output = match cmd.output() {
Expand All @@ -112,7 +105,55 @@ pub(super) static EXE_INFO: Lazy<Option<BString>> = Lazy::new(|| {
};

first_file_from_config_with_origin(cmd_output.as_slice().into()).map(ToOwned::to_owned)
});
}

fn git_cmd(executable: PathBuf) -> Command {
let mut cmd = Command::new(executable);
#[cfg(windows)]
{
use std::os::windows::process::CommandExt;
const CREATE_NO_WINDOW: u32 = 0x08000000;
cmd.creation_flags(CREATE_NO_WINDOW);
}
// We will try to run `git` from a location fairly high in the filesystem, in the hope it may
// be faster if we are deeply nested, on a slow disk, or in a directory that has been deleted.
let cwd = if cfg!(windows) {
// We try the Windows directory (usually `C:\Windows`) first. It is given by `SystemRoot`,
// except in rare cases where our own parent has not passed down that environment variable.
env::var_os("SystemRoot")
.or_else(|| env::var_os("windir"))
.map(PathBuf::from)
.filter(|p| p.is_absolute())
.unwrap_or_else(env::temp_dir)
} else {
"/".into()
};
// Git 2.8.0 and higher support --show-origin. The -l, -z, and --name-only options were
// supported even before that. In contrast, --show-scope was introduced later, in Git 2.26.0.
// Low versions of Git are still sometimes used, and this is sometimes reasonable because
// downstream distributions often backport security patches without adding most new features.
// So for now, we forgo the convenience of --show-scope for greater backward compatibility.
//
// Separately from that, we can't use --system here, because scopes treated higher than the
// system scope are possible. This commonly happens on macOS with Apple Git, where the config
// file under `/Library` is shown as an "unknown" scope but takes precedence over the system
// 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("GIT_DIR", NULL_DEVICE) // Avoid getting local-scope config.
.env("GIT_WORK_TREE", NULL_DEVICE) // Avoid confusion when debugging.
.stdin(Stdio::null())
.stderr(Stdio::null());
cmd
}

fn first_file_from_config_with_origin(source: &BStr) -> Option<&BStr> {
let file = source.strip_prefix(b"file:")?;
let end_pos = file.find_byte(b'\0')?;
file[..end_pos].as_bstr().into()
}

/// Try to find the file that contains git configuration coming with the git installation.
///
Expand All @@ -135,12 +176,6 @@ pub(super) fn install_config_path() -> Option<&'static BStr> {
PATH.as_ref().map(AsRef::as_ref)
}

fn first_file_from_config_with_origin(source: &BStr) -> Option<&BStr> {
let file = source.strip_prefix(b"file:")?;
let end_pos = file.find_byte(b'\t')?;
file[..end_pos].trim_with(|c| c == '"').as_bstr().into()
}

/// Given `config_path` as obtained from `install_config_path()`, return the path of the git installation base.
pub(super) fn config_to_base_path(config_path: &Path) -> &Path {
config_path
Expand Down
Loading
Loading