GitoxideLabs / gitoxide

An idiomatic, lean, fast & safe pure Rust implementation of Git
Apache License 2.0
8.91k stars 303 forks source link

run `git config -l` in temp dir when looking up system config #1523

Closed martinvonz closed 2 months ago

martinvonz commented 2 months ago

Commit a9a3545db2b2 made gix::config::File::from_globals() try to find the system config (typically /etc/config) by subprocessing to git config -l --show-origin. That command takes about 500 ms when run in certain directories in our VFS at Google. Since we don't need anything but the system config, let's run the command in the system's temporary directory instead.

I also added --system for good measure. I've confirmed that just adding --system is not enough, however - git config -l --system --show-origin is still slow in our VFS.

EliahKagan commented 2 months ago

The nature of the macOS failure is interesting:

~> git config -l --show-origin --show-scope
unknown file:/Library/Developer/CommandLineTools/usr/share/git-core/gitconfig   credential.helper=osxkeychain
unknown file:/Library/Developer/CommandLineTools/usr/share/git-core/gitconfig   init.defaultbranch=main
global  file:/Users/ek/.gitconfig       user.name=Eliah Kagan
global  file:/Users/ek/.gitconfig       user.email=degeneracypressure@gmail.com
global  file:/Users/ek/.gitconfig       core.precomposeunicode=false

It has the usual value of /etc/gitconfig as the path of its system configuration file, which usually does not exist, and it has its installation configuration in a separate file (whose scope is shown as unknown).

I cannot create /etc/gitconfig on this system, however, so I don't know which has precedence when both exist.

martinvonz commented 2 months ago

With that said, since --system doesn't make a performance difference and fails on MacOS (see below), I will have to remove that before merging.

It works on my Mac git config -l --system but that seems to be because we have a custom git at Google.

Actually, --system was not what I thought it was anyway, so I'm glad you're removing that! :) On my Linux machine, --system is /usr/share/git-core/config, while the first ("installation") path in git config -l --show-origin is /etc/gitconfig.

Byron commented 2 months ago

Sorry for the early merge - apparently the "Require status checks before merging" option does not like I thought require all checks to pass (what I want), but is a no-op unless a check is chosen. Now I would have to select all checks and keep them in sync with what's actually there, which seems inconvenient at best.

Screenshot 2024-08-15 at 17 27 38

And I so wanted to have auto-merge 😭.

martinvonz commented 2 months ago

Thanks for the quick review. I hope the lack of GitHub checks didn't lead to anything getting broken (doesn't look like it AFAICT).

NobodyXu commented 1 month ago

@Byron you can add a dummy job "test-pass":

https://github.com/cargo-bins/cargo-binstall/blob/c4abcb90a5fac18f5f8744cbf7bd11b00a37ca41/.github/workflows/ci.yml#L372

EliahKagan commented 1 month ago

This pull request, in addition to improving performance, fixed the vulnerability CVE-2024-45305 (RUSTSEC-2024-0367, https://github.com/Byron/gitoxide/security/advisories/GHSA-v26r-4c9c-h3j6).

(It may make sense to edit the PR description to mention this, though either way should be fine because this comment can itself be found by searching. Further related hardening, as well as a fix for a separate vulnerability, is included in #1567, but the fix for CVE-2024-45305 is here, and the changes there build on this.)