-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Add a configuration option to fetch with git-the-CLI #5914
Conversation
r? @matklad (rust_highfive has picked a reviewer for you, use r? to override) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, just a couple of questions.
tests/testsuite/git.rs
Outdated
|
||
assert_that( | ||
project.cargo("build -v"), | ||
execs().with_status(0).with_stderr(stderr) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the 0
status is redundant 😉
let mut cmd = process("git"); | ||
cmd.arg("fetch") | ||
.arg("--tags") // fetch all tags | ||
.arg("--quiet") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this for feature parity with libgit2
's variant?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed yeah!
refspec: &str, | ||
config: &Config, | ||
) -> CargoResult<()> { | ||
let mut cmd = process("git"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason (either way) to use cargo's process
API over the stdlib's Command
API (like used by the git gc
below)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah it mostly just had to do with how easy it was to deal with the result. The git gc
piece is basically ignoring the result of the process no matter what happens, while here we want to report any failure of git
and Cargo's already got good error handling and UI for all that.
e4acd7e
to
a3388a1
Compare
@bors: r=dwijnand |
📌 Commit a3388a1 has been approved by |
Add a configuration option to fetch with git-the-CLI Currently Cargo always uses `libgit2` to perform all fetches of git repositories, but sometimes this is not sufficient. The `libgit2` library doesn't support all authentication schemes that `git` does and it isn't always quite at feature parity with `git` itself, especially in terms of network configuration. This commit adds a configuration option to Cargo for fetching git repositories with the `git` CLI tool rather than the internal `libgit2`. While this exposes us to changes over time in the `git` CLI it's hopefully a very targeted use case that doesn't change much. The internal `libgit2` library should be sufficient for all other forms of git repository management. (and using `git` for only fetches shouldn't slow us down much) The new configuration option in `.cargo/config` is: [net] git-fetch-with-cli = true which can also be specified with `CARGO_NET_GIT_FETCH_WITH_CLI=true` via an environment variable. Closes #5903
Why didn't @bors merge this yet? |
Bors has been having issues lately. And the solution I've seen used is closing and reopening the PR. |
@bors: retry r+ |
💡 This pull request was already approved, no need to approve it again.
|
📌 Commit a3388a1 has been approved by |
@bors: retry |
⌛ Testing commit a3388a1 with merge f74ff24278fd986b425a160cde831a234199c1f0... |
Currently Cargo always uses `libgit2` to perform all fetches of git repositories, but sometimes this is not sufficient. The `libgit2` library doesn't support all authentication schemes that `git` does and it isn't always quite at feature parity with `git` itself, especially in terms of network configuration. This commit adds a configuration option to Cargo for fetching git repositories with the `git` CLI tool rather than the internal `libgit2`. While this exposes us to changes over time in the `git` CLI it's hopefully a very targeted use case that doesn't change much. The internal `libgit2` library should be sufficient for all other forms of git repository management. (and using `git` for only fetches shouldn't slow us down much) The new configuration option in `.cargo/config` is: [net] git-fetch-with-cli = true which can also be specified with `CARGO_NET_GIT_FETCH_WITH_CLI=true` via an environment variable. Closes rust-lang#5903
a3388a1
to
a0591ee
Compare
@bors: r=dwijnand |
📌 Commit a0591ee has been approved by |
⌛ Testing commit a0591ee with merge 2e9dc07155ea4675b3c532a962f0082731f1ff7f... |
💥 Test timed out |
@bors: retry |
Add a configuration option to fetch with git-the-CLI Currently Cargo always uses `libgit2` to perform all fetches of git repositories, but sometimes this is not sufficient. The `libgit2` library doesn't support all authentication schemes that `git` does and it isn't always quite at feature parity with `git` itself, especially in terms of network configuration. This commit adds a configuration option to Cargo for fetching git repositories with the `git` CLI tool rather than the internal `libgit2`. While this exposes us to changes over time in the `git` CLI it's hopefully a very targeted use case that doesn't change much. The internal `libgit2` library should be sufficient for all other forms of git repository management. (and using `git` for only fetches shouldn't slow us down much) The new configuration option in `.cargo/config` is: [net] git-fetch-with-cli = true which can also be specified with `CARGO_NET_GIT_FETCH_WITH_CLI=true` via an environment variable. Closes #5903
☀️ Test successful - status-appveyor, status-travis |
Currently Cargo always uses
libgit2
to perform all fetches of gitrepositories, but sometimes this is not sufficient. The
libgit2
librarydoesn't support all authentication schemes that
git
does and it isn't alwaysquite at feature parity with
git
itself, especially in terms of networkconfiguration.
This commit adds a configuration option to Cargo for fetching git repositories
with the
git
CLI tool rather than the internallibgit2
. While this exposesus to changes over time in the
git
CLI it's hopefully a very targeted use casethat doesn't change much. The internal
libgit2
library should be sufficientfor all other forms of git repository management. (and using
git
for onlyfetches shouldn't slow us down much)
The new configuration option in
.cargo/config
is:which can also be specified with
CARGO_NET_GIT_FETCH_WITH_CLI=true
via anenvironment variable.
Closes #5903