fix(git)!: Default to net.git-fetch-with-cli if git is present - #17329
fix(git)!: Default to net.git-fetch-with-cli if git is present#17329epage wants to merge 3 commits into
Conversation
rustbot
added
A-git
|
r? @weihanglo rustbot has assigned @weihanglo. Use Why was this reviewer chosen?The reviewer was selected based on:
|
A possible upstream fix: libgit2/libgit2#7346 |
| } | ||
|
|
||
| fn is_git_cli_present() -> bool { | ||
| #[tracing::instrument(skip_all)] |
There was a problem hiding this comment.
I think before making it default, we may want to make sure:
- What the minimum required Git CLI version we depend on.
- This doesn't need to be precise, but at least we figure out an approximate.
- What Git CLI v3 would ship and whether it would be hard to keep our compatibility story: https://git-scm.com/docs/BreakingChanges#_git_3_0.
- Looked over the proposed change, refutable and sha256 may require a bit more attention, as old Git may not be able to read the new index format.
- If there is any more defaults we need to make explicit and opt-out, it is better to as early as possible. (e.g.
safe.bareRepositorythough we already setGIT_DIRexplicitly for fetch in git-fetch-with-cli: SetGIT_DIRfor bare repository compatibility #14860).
There was a problem hiding this comment.
What the minimum required Git CLI version we depend on.
Just for curiousity I wanted to know, why can't we simply assume the current version(available on user device) to be the appropriate version and if while doing the action we encounter an error, rust's awesome error handling will explain the error to user and may be add a sugestion to upgrade git CLI?
This way If I am using a old unix system with no sudo privilege (like my college HPC server) and cargo needs git pull that could be done by any version, I will not be blocked! Basically it give highest flexibility.
There was a problem hiding this comment.
Because that may fail previous-successful builds. To provide a good diagnostic we also need to know what version user need to upgrade to.
### What does this PR try to resolve? Git v3 will enforce `safe.bareRepository=explicit`, while older versions don't. See <https://git-scm.com/docs/BreakingChanges/2.55.0#_git_3_0> This ensures `git gc` work with `safe.bareRepository=explicit` by setting an explicit `GIT_DIR` environment variable. This is not a requirement of making CLI fetch the default, but better to have before Git CLI v3 is out. CC rust-lang#17329 ### How to test and review this PR? You can basically repro this manually today with Git 2.38.0 and later (which `safe.bareRepository` first appears). 1. Set `safe.bareRepository=explicit` 2. `cargo fetch` with config `net.git-fetch-with-cli=true` 3. Put a marker file inside `$CARGO_HOME/git/db/<git-dir>/` 4. `cargo update` to force a git fetch 5. Observe the marker is gone, because Cargo reinitialized the entire db. I am not sure if there is a better way to test it than a marker file though. 🤖 **LLM disclosure**: I told Codex to check if Cargo's `git fetch` usage is not compatibility with Git CLI v3, and surprisingly it told me `git gc` is not good. I didn't expect this.
…7400) ### What does this PR try to resolve? Trying to make the output cleaner, particularly on ambiguous refs, in prep for rust-lang#17329 ### How to test and review this PR? Since progress does not have end-to-end tests, I ran ``` cargo new git-dep cd git-dep cargo add cargo --git https://github.com/rust-lang/cargo/ rm -rf ~/.cargo/git CARGO_NET_GIT_FETCH_WITH_CLI=true nargo check ``` Remaining git output is: ``` remote: Enumerating objects: 187021, done. remote: Counting objects: 100% (326/326), done. remote: Compressing objects: 100% (155/155), done. remote: Total 187021 (delta 241), reused 177 (delta 171), pack-reused 186695 (from 3) Receiving objects: 100% (187021/187021), 69.48 MiB | 4.90 MiB/s, done. Resolving deltas: 100% (130194/130194), done. ```
This changes the default for how we fetch to use the git CLI if present for better conformity and performance without breaking people (yet) who don't have it. Part of rust-lang#17227
|
This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
rustbot
removed
the
S-waiting-on-review
4 participants
What does this PR try to resolve?
This changes the default for how we fetch to use the git CLI if present for better conformity and performance without breaking people (yet) who don't have it.
Part of #17227
How to test and review this PR?
I have triaged open issues related to
git-fetch-with-cli.corrupt_git.rsis an interesting case. From what an agent told me that sounds reasonable, we hit an infinite loop in libgit2. We should report that but not feeling it is bad enough to be a blocker for merging this, especially since people are using a sparse index.Future goals:
Future steps:
gitcheck, always defaulting to git-cli