Skip to content

Commit

Permalink
enable all applicable git tests and fix them
Browse files Browse the repository at this point in the history
  • Loading branch information
@Byron
Byron committed Dec 7, 2022
1 parent 48b782a commit 2552fec
Show file tree
Hide file tree
Showing 9 changed files with 291 additions and 74 deletions.
4 changes: 2 additions & 2 deletions .github/workflows/main.yml
View file
Expand Up @@ -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
Expand Down
15 changes: 15 additions & 0 deletions src/cargo/core/features.rs
View file
Expand Up @@ -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(
Expand Down Expand Up @@ -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)
}

Expand Down
121 changes: 103 additions & 18 deletions src/cargo/sources/git/oxide.rs
View file
@@ -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};
Expand All @@ -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> = git::progress::tree::root::Options {
initial_capacity: 10,
message_buffer_capacity: 10,
Expand Down Expand Up @@ -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<git::bstr::BString>,
) -> CargoResult<()> {
match res {
Ok(()) => Ok(()),
Err(mut err) => {
if let Some(e) = err
.downcast_ref::<git::remote::fetch::prepare::Error>()
.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::Repository> {
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)
}
112 changes: 80 additions & 32 deletions src/cargo/sources/git/utils.rs
View file
Expand Up @@ -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<()> {
Expand All @@ -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,
Expand Down Expand Up @@ -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
Expand All @@ -879,44 +878,93 @@ 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,
};
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::<git::open::Error>().map_or(false, |err| {
use git::open::Error;
matches!(
err,
Error::NotARepository { .. }| Error::Config(_)
)
})
|| err
.downcast_ref::<git::remote::fetch::prepare::Error>()
.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::<git::remote::fetch::Error>()
.map_or(false, |err| {
use git::remote::fetch::Error;
matches!(
err,
Error::Configuration { .. } | Error::RemovePackKeepFile { .. }
)
})
{
repo_reinitialized = true;
debug!(
"looks like this is a corrupt repository, reinitializing \
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;
}
}
Expand All @@ -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);
}
Expand 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,
Expand Down

0 comments on commit 2552fec

Please sign in to comment.