enable all applicable git tests and fix them

Author: Byron

.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

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)
     }
 

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> = 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<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)
+}

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,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;
                         }
                     }
@@ -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,