diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index ea4ebef2a658..d8fe28a8310e 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -85,8 +85,8 @@ jobs: df -h rm -rf target/tmp df -h - - name: gitoxide tests (git-related and enabled tests only) - run: RUSTFLAGS='--cfg test_gitoxide' cargo test git + - name: gitoxide tests (all git-related tests) + run: RUSTFLAGS='--cfg always_test_gitoxide' cargo test git # The testsuite generates a huge amount of data, and fetch-smoke-test was # running out of disk space. - name: Clear test output diff --git a/src/cargo/core/features.rs b/src/cargo/core/features.rs index e21f3c596e07..7d6a12a5412b 100644 --- a/src/cargo/core/features.rs +++ b/src/cargo/core/features.rs @@ -801,6 +801,17 @@ impl GitoxideFeatures { shallow_deps: true, } } + + /// Features we deem safe for everyday use - typically true when all tests pass with them + /// AND they are backwards compatible. + fn safe() -> Self { + GitoxideFeatures { + fetch: true, + shallow_index: false, + checkout: true, + shallow_deps: false, + } + } } fn parse_gitoxide( @@ -880,6 +891,10 @@ impl CliUnstable { for flag in flags { self.add(flag, &mut warnings)?; } + + if self.gitoxide.is_none() && cfg!(always_test_gitoxide) { + self.gitoxide = GitoxideFeatures::safe().into(); + } Ok(warnings) } diff --git a/src/cargo/sources/git/oxide.rs b/src/cargo/sources/git/oxide.rs index d4f1a460282b..27aea5c9b2fd 100644 --- a/src/cargo/sources/git/oxide.rs +++ b/src/cargo/sources/git/oxide.rs @@ -1,9 +1,12 @@ //! This module contains all code sporting `gitoxide` for operations on `git` repositories and it mirrors //! `utils` closely for now. One day it can be renamed into `utils` once `git2` isn't required anymore. +use crate::ops::HttpTimeout; use crate::util::{human_readable_bytes, network, MetricsCounter, Progress}; use crate::{CargoResult, Config}; use git_repository as git; +use git_repository::bstr::ByteSlice; +use std::cell::RefCell; use std::sync::atomic::{AtomicBool, Ordering}; use std::sync::Arc; use std::time::{Duration, Instant}; @@ -14,26 +17,12 @@ pub fn with_retry_and_progress( repo_path: &std::path::Path, config: &Config, cb: &mut (dyn FnMut( - &git::Repository, + &std::path::Path, &AtomicBool, &mut git::progress::tree::Item, - ) -> CargoResult<()> - + Send), + &mut dyn FnMut(&git::bstr::BStr), + ) -> CargoResult<()>), ) -> CargoResult<()> { - let repo = git::open_opts(repo_path, { - let mut opts = git::open::Options::default(); - // We need `git_binary` configuration as well for being able to see credential helpers - // that are configured with the `git` installation itself. - // However, this is slow on windows (~150ms) and most people won't need it as they use the - // standard index which won't ever need authentication. - // TODO: This is certainly something to make configurable, at the very least on windows. - // Maybe it's also something that could be cached, all we need is the path to the configuration file - // which usually doesn't change unless the installation changes. Maybe something keyed by the location of the - // binary along with its fingerprint. - opts.permissions.config = git::permissions::Config::all(); - opts - })?; - let progress_root: Arc = git::progress::tree::root::Options { initial_capacity: 10, message_buffer_capacity: 10, @@ -120,7 +109,103 @@ pub fn with_retry_and_progress( } }); network::with_retry(config, || { - cb(&repo, &git::interrupt::IS_INTERRUPTED, &mut progress) + let mut urls = RefCell::new(Default::default()); + let res = cb( + &repo_path, + &git::interrupt::IS_INTERRUPTED, + &mut progress, + &mut |url| { + *urls.borrow_mut() = Some(url.to_owned()); + }, + ); + amend_authentication_hints(res, urls.get_mut().take()) }) }) } + +fn amend_authentication_hints( + res: CargoResult<()>, + last_url_for_authentication: Option, +) -> CargoResult<()> { + match res { + Ok(()) => Ok(()), + Err(mut err) => { + if let Some(e) = err + .downcast_ref::() + .and_then(|e| match e { + git::remote::fetch::prepare::Error::RefMap( + git::remote::ref_map::Error::Handshake(e), + ) => Some(e), + _ => None, + }) + { + let auth_message = match e { + git::protocol::handshake::Error::Credentials(_) => { + "\n* attempted to find username/password via \ + git's `credential.helper` support, but failed" + .into() + } + git::protocol::handshake::Error::InvalidCredentials { url: _ } => { + "\n* attempted to find username/password via \ + `credential.helper`, but maybe the found \ + credentials were incorrect" + .into() + } + git::protocol::handshake::Error::Transport(_) => { + let msg = concat!("network failure seems to have happened\n", + "if a proxy or similar is necessary `net.git-fetch-with-cli` may help here\n", + "https://doc.rust-lang.org/cargo/reference/config.html#netgit-fetch-with-cli" + ); + return Err(err.context(msg)); + } + _ => None, + }; + if let Some(auth_message) = auth_message { + let mut msg = "failed to authenticate when downloading \ + repository" + .to_string(); + if let Some(url) = last_url_for_authentication { + msg.push_str(": "); + msg.push_str(url.to_str_lossy().as_ref()); + } + msg.push('\n'); + msg.push_str(auth_message); + msg.push_str("\n\n"); + msg.push_str( + "if the git CLI succeeds then `net.git-fetch-with-cli` may help here\n", + ); + msg.push_str( + "https://doc.rust-lang.org/cargo/reference/config.html#netgit-fetch-with-cli", + ); + err = err.context(msg); + } + } + Err(err) + } + } +} + +/// Produce a repository with everything pre-configured according to `config`. Most notably this includes +/// transport configuration. +pub fn open_repo(repo_path: &std::path::Path, config: &Config) -> CargoResult { + git::open_opts(repo_path, { + let mut opts = git::open::Options::default(); + let timeout = HttpTimeout::new(config)?; + + // We need `git_binary` configuration as well for being able to see credential helpers + // that are configured with the `git` installation itself. + // However, this is slow on windows (~150ms) and most people won't need it as they use the + // standard index which won't ever need authentication. + // TODO: This is certainly something to make configurable, at the very least on windows. + // Maybe it's also something that could be cached, all we need is the path to the configuration file + // which usually doesn't change unless the installation changes. Maybe something keyed by the location of the + // binary along with its fingerprint. + opts.permissions.config = git::permissions::Config::all(); + opts.config_overrides([ + format!("gitoxide.http.connectTimeout={}", timeout.dur.as_millis()), + format!("http.lowSpeedLimit={}", timeout.low_speed_limit), + format!("http.lowSpeedTime={}", timeout.dur.as_secs()), + ]) + }) + .map_err(Into::into) +} diff --git a/src/cargo/sources/git/utils.rs b/src/cargo/sources/git/utils.rs index e4cf7c601226..eb1ef54ac64b 100644 --- a/src/cargo/sources/git/utils.rs +++ b/src/cargo/sources/git/utils.rs @@ -776,7 +776,7 @@ pub fn with_fetch_options( pub fn fetch( repo: &mut git2::Repository, - url: &str, + orig_url: &str, reference: &GitReference, config: &Config, ) -> CargoResult<()> { @@ -792,7 +792,7 @@ pub fn fetch( // If we're fetching from GitHub, attempt GitHub's special fast path for // testing if we've already got an up-to-date copy of the repository - let oid_to_fetch = match github_fast_path(repo, url, reference, config) { + let oid_to_fetch = match github_fast_path(repo, orig_url, reference, config) { Ok(FastPathRev::UpToDate) => return Ok(()), Ok(FastPathRev::NeedsFetch(rev)) => Some(rev), Ok(FastPathRev::Indeterminate) => None, @@ -854,20 +854,19 @@ pub fn fetch( // flavors of authentication possible while also still giving us all the // speed and portability of using `libgit2`. if let Some(true) = config.net_config()?.git_fetch_with_cli { - return fetch_with_cli(repo, url, &refspecs, tags, config); + return fetch_with_cli(repo, orig_url, &refspecs, tags, config); } if config .cli_unstable() .gitoxide .map_or(false, |git| git.fetch) { - use git::remote::fetch::Error; use git_repository as git; let git2_repo = repo; oxide::with_retry_and_progress( &git2_repo.path().to_owned(), config, - &mut |repo, should_interrupt, mut progress| { + &mut |repo_path, should_interrupt, mut progress, url_for_authentication| { // The `fetch` operation here may fail spuriously due to a corrupt // repository. It could also fail, however, for a whole slew of other // reasons (aka network related reasons). We want Cargo to automatically @@ -879,19 +878,45 @@ pub fn fetch( // again. If it looks like any other kind of error, or if we've already // blown away the repository, then we want to return the error as-is. let mut repo_reinitialized = false; - let mut repo_storage; - let mut repo = &*repo; loop { - debug!("initiating fetch of {:?} from {}", refspecs, url); - let res = repo - .remote_at(url)? - .with_refspecs( - refspecs.iter().map(|s| s.as_str()), - git::remote::Direction::Fetch, - )? - .connect(git::remote::Direction::Fetch, &mut progress)? - .prepare_fetch(git::remote::ref_map::Options::default())? - .receive(should_interrupt); + let repo = oxide::open_repo(repo_path, config); + let res = match repo { + Ok(repo) => { + debug!("initiating fetch of {:?} from {}", refspecs, orig_url); + ({ + let url_for_authentication = &mut *url_for_authentication; + || -> CargoResult<_> { + let remote = repo.remote_at(orig_url)?.with_refspecs( + refspecs.iter().map(|s| s.as_str()), + git::remote::Direction::Fetch, + )?; + let url = remote + .url(git::remote::Direction::Fetch) + .expect("set at init") + .to_owned(); + let connection = remote + .connect(git::remote::Direction::Fetch, &mut progress)?; + let mut authenticate = + connection.configured_credentials(url)?; + let connection = connection.with_credentials( + move |action: git::protocol::credentials::helper::Action| { + if let Some(url) = + action.context().and_then(|ctx| ctx.url.as_ref().filter(|url| *url != orig_url)) + { + url_for_authentication(url.as_ref()); + } + authenticate(action) + }, + ); + let outcome = connection + .prepare_fetch(git::remote::ref_map::Options::default())? + .receive(should_interrupt)?; + Ok(outcome) + } + })() + } + Err(err) => Err(err), + }; let err = match res { Ok(_) => break, Err(e) => e, @@ -899,14 +924,40 @@ pub fn fetch( debug!("fetch failed: {}", err); if !repo_reinitialized - && matches!( - err, - Error::Configuration { .. } - | Error::IncompatibleObjectHash { .. } - | Error::WritePack(_) - | Error::UpdateRefs(_) - | Error::RemovePackKeepFile { .. } - ) + // We check for errors that could occour if the configuration, refs or odb files are corrupted. + // We don't check for errors related to writing as `gitoxide` is expected to create missing leading + // folder before writing files into it, or else not even open a directory as git repository (which is + // also handled here). + && err.downcast_ref::().map_or(false, |err| { + use git::open::Error; + matches!( + err, + Error::NotARepository { .. }| Error::Config(_) + ) + }) + || err + .downcast_ref::() + .map_or(false, |err| match err { + git::remote::fetch::prepare::Error::RefMap(err) => { + use git::remote::ref_map::Error; + matches!( + err, + Error::ListRefs(_) + | Error::GatherTransportConfig { .. } + | Error::ConfigureCredentials(_) + ) + } + _ => false, + }) + || err + .downcast_ref::() + .map_or(false, |err| { + use git::remote::fetch::Error; + matches!( + err, + Error::Configuration { .. } | Error::RemovePackKeepFile { .. } + ) + }) { repo_reinitialized = true; debug!( @@ -914,9 +965,6 @@ pub fn fetch( and trying again" ); if reinitialize(git2_repo).is_ok() { - repo_storage = - git::open_opts(repo.path(), repo.open_options().to_owned())?; - repo = &repo_storage; continue; } } @@ -927,9 +975,9 @@ pub fn fetch( }, ) } else { - debug!("doing a fetch for {}", url); + debug!("doing a fetch for {}", orig_url); let git_config = git2::Config::open_default()?; - with_fetch_options(&git_config, url, config, &mut |mut opts| { + with_fetch_options(&git_config, orig_url, config, &mut |mut opts| { if tags { opts.download_tags(git2::AutotagOption::All); } @@ -945,9 +993,9 @@ pub fn fetch( // blown away the repository, then we want to return the error as-is. let mut repo_reinitialized = false; loop { - debug!("initiating fetch of {:?} from {}", refspecs, url); + debug!("initiating fetch of {:?} from {}", refspecs, orig_url); let res = repo - .remote_anonymous(url)? + .remote_anonymous(orig_url)? .fetch(&refspecs, Some(&mut opts), None); let err = match res { Ok(()) => break, diff --git a/tests/testsuite/bad_config.rs b/tests/testsuite/bad_config.rs index 590ea57b187a..09ddaedce506 100644 --- a/tests/testsuite/bad_config.rs +++ b/tests/testsuite/bad_config.rs @@ -346,10 +346,31 @@ fn bad_git_dependency() { .file("src/lib.rs", "") .build(); - p.cargo("build -v") - .with_status(101) - .with_stderr( - "\ + let expected_stderr = if cfg!(always_test_gitoxide) { + "\ +[UPDATING] git repository `file:///` +[ERROR] failed to get `foo` as a dependency of package `foo v0.0.0 [..]` + +Caused by: + failed to load source for dependency `foo` + +Caused by: + Unable to update file:/// + +Caused by: + failed to clone into: [..] + +Caused by: + Could not verify that \"file:///\" url [..] + +Caused by: + Could not retrieve metadata of \"/.git\" + +Caused by: +[..] +" + } else { + "\ [UPDATING] git repository `file:///` [ERROR] failed to get `foo` as a dependency of package `foo v0.0.0 [..]` @@ -364,8 +385,11 @@ Caused by: Caused by: [..]'file:///' is not a valid local file URI[..] -", - ) +" + }; + p.cargo("build -v") + .with_status(101) + .with_stderr(expected_stderr) .run(); } diff --git a/tests/testsuite/git.rs b/tests/testsuite/git.rs index cdcbe95848d1..d79326f46df3 100644 --- a/tests/testsuite/git.rs +++ b/tests/testsuite/git.rs @@ -2446,6 +2446,7 @@ fn add_a_git_dep() { } #[cargo_test] +#[cfg_attr(always_test_gitoxide, ignore = "needs auto-tags")] fn two_at_rev_instead_of_tag() { let git = git::new("git", |p| { p.file("Cargo.toml", &basic_manifest("git1", "0.5.0")) diff --git a/tests/testsuite/git_auth.rs b/tests/testsuite/git_auth.rs index 69af128085a0..d37c0cc908c2 100644 --- a/tests/testsuite/git_auth.rs +++ b/tests/testsuite/git_auth.rs @@ -214,7 +214,9 @@ fn https_something_happens() { Caused by: {errmsg} ", - errmsg = if cfg!(windows) { + errmsg = if cfg!(always_test_gitoxide) { + "[..]SSL connect error [..]" + } else if cfg!(windows) { "[..]failed to send request: [..]" } else if cfg!(target_os = "macos") { // macOS is difficult to tests as some builds may use Security.framework, @@ -258,18 +260,27 @@ fn ssh_something_happens() { .file("src/main.rs", "") .build(); + let expected = if cfg!(always_test_gitoxide) { + // Due to the usage of `ssh` and `ssh.exe` respectively, the messages change. + // Maybe this will be adjusted to use `ssh2` to get rid of this dependency. + if cfg!(windows) { + "banner exchange: Connection to 127.0.0.1 [..]" + } else { + "[..]Connection [..] by 127.0.0.1[..]" + } + } else { + "\ +Caused by: + [..]failed to start SSH session: Failed getting banner[..] +" + }; p.cargo("build -v") .with_status(101) .with_stderr_contains(&format!( "[UPDATING] git repository `ssh://{addr}/foo/bar`", addr = addr )) - .with_stderr_contains( - "\ -Caused by: - [..]failed to start SSH session: Failed getting banner[..] -", - ) + .with_stderr_contains(expected) .run(); t.join().ok().unwrap(); } @@ -292,10 +303,38 @@ fn net_err_suggests_fetch_with_cli() { .file("src/lib.rs", "") .build(); - p.cargo("build -v") - .with_status(101) - .with_stderr( - "\ + let expected = if cfg!(always_test_gitoxide) { + "\ +[UPDATING] git repository `ssh://needs-proxy.invalid/git` +ssh: [..] +warning: spurious network error[..] +ssh: [..] +warning: spurious network error[..] +ssh: [..] +[ERROR] failed to get `foo` as a dependency of package `foo v0.0.0 [..]` + +Caused by: + failed to load source for dependency `foo` + +Caused by: + Unable to update ssh://needs-proxy.invalid/git + +Caused by: + failed to clone into: [..] + +Caused by: + network failure seems to have happened + if a proxy or similar is necessary `net.git-fetch-with-cli` may help here + https://[..] + +Caused by: + An IO error occurred when talking to the server + +Caused by: + failed to fill whole buffer +" + } else { + "\ [UPDATING] git repository `ssh://needs-proxy.invalid/git` warning: spurious network error[..] warning: spurious network error[..] @@ -317,8 +356,11 @@ Caused by: Caused by: failed to resolve address for needs-proxy.invalid[..] -", - ) +" + }; + p.cargo("build -v") + .with_status(101) + .with_stderr(expected) .run(); p.change_file( diff --git a/tests/testsuite/git_gc.rs b/tests/testsuite/git_gc.rs index 045bc78ad5e0..50e41d18e377 100644 --- a/tests/testsuite/git_gc.rs +++ b/tests/testsuite/git_gc.rs @@ -95,6 +95,10 @@ fn use_git_gc() { } #[cargo_test] +#[cfg_attr( + always_test_gitoxide, + ignore = "file protocol without git binary is currently not possible - needs built-in upload-pack" +)] fn avoid_using_git() { let path = env::var_os("PATH").unwrap_or_default(); let mut paths = env::split_paths(&path).collect::>(); diff --git a/tests/testsuite/registry.rs b/tests/testsuite/registry.rs index 8aad97ce48ae..6932f1a8d824 100644 --- a/tests/testsuite/registry.rs +++ b/tests/testsuite/registry.rs @@ -2,7 +2,6 @@ use cargo::core::SourceId; use cargo_test_support::cargo_process; -use cargo_test_support::git::Gitoxide; use cargo_test_support::paths::{self, CargoPathExt}; use cargo_test_support::registry::{ self, registry_path, Dependency, Package, RegistryBuilder, TestRegistry, @@ -1423,7 +1422,7 @@ fn fetch_downloads_http() { fetch_downloads(cargo_http); } -#[cargo_test(gitoxide)] +#[cargo_test] fn fetch_downloads_git() { fetch_downloads(cargo_stable); } @@ -1448,7 +1447,6 @@ fn fetch_downloads(cargo: fn(&Project, &str) -> Execs) { Package::new("a", "0.1.0").publish(); cargo(&p, "fetch") - .maybe_fetch_with_gitoxide() .with_stderr( "\ [UPDATING] `[..]` index