Auto merge of #8409 - alexcrichton:git-instead-of, r=Eh2406

Improve git error messages a bit

This commit is targeted at further improving the error messages
generated from git errors. For authentication errors the actual URL
fetched is now printed out as well if it's different from the original
URL. This should help handle `insteadOf` logic where SSH urls are used
instead of HTTPS urls and users can know to track that down.

Otherwise the logic about recommending `net.git-fetch-with-cli` was
tweaked a bit and moved to the same location as the rest of our error
reporting.

Note that a change piggy-backed here as well is that `Caused by:` errors
are now automatically all tabbed over a bit instead of only having the
first line tabbed over. This required a good number of tests to be
updated, but it's just an updated in renderings.

src/cargo/core/resolver/errors.rs

@@ -219,8 +219,8 @@ pub(super) fn activation_error(
             };
 
             let mut msg = format!(
-                "failed to select a version for the requirement `{} = \"{}\"`\n  \
-                 candidate versions found which didn't match: {}\n  \
+                "failed to select a version for the requirement `{} = \"{}\"`\n\
+                 candidate versions found which didn't match: {}\n\
                  location searched: {}\n",
                 dep.package_name(),
                 dep.version_req(),

src/cargo/lib.rs

@@ -163,7 +163,13 @@ fn _display_error(err: &Error, shell: &mut Shell, as_err: bool) -> bool {
             return true;
         }
         drop(writeln!(shell.err(), "\nCaused by:"));
-        drop(writeln!(shell.err(), "  {}", cause));
+        for line in cause.to_string().lines() {
+            if line.is_empty() {
+                drop(writeln!(shell.err(), ""));
+            } else {
+                drop(writeln!(shell.err(), "  {}", line));
+            }
+        }
     }
     false
 }

src/cargo/sources/git/utils.rs

@@ -3,7 +3,7 @@ use crate::util::errors::{CargoResult, CargoResultExt};
 use crate::util::paths;
 use crate::util::process_builder::process;
 use crate::util::{network, Config, IntoUrl, Progress};
-use anyhow::anyhow;
+use anyhow::{anyhow, Context};
 use curl::easy::List;
 use git2::{self, ErrorClass, ObjectType};
 use log::{debug, info};
@@ -85,46 +85,13 @@ impl GitRemote {
         locked_rev: Option<git2::Oid>,
         cargo_config: &Config,
     ) -> CargoResult<(GitDatabase, git2::Oid)> {
-        let format_error = |e: anyhow::Error, operation| {
-            let may_be_libgit_fault = e
-                .chain()
-                .filter_map(|e| e.downcast_ref::<git2::Error>())
-                .any(|e| match e.class() {
-                    ErrorClass::Net
-                    | ErrorClass::Ssl
-                    | ErrorClass::Submodule
-                    | ErrorClass::FetchHead
-                    | ErrorClass::Ssh
-                    | ErrorClass::Callback
-                    | ErrorClass::Http => true,
-                    _ => false,
-                });
-            let uses_cli = cargo_config
-                .net_config()
-                .ok()
-                .and_then(|config| config.git_fetch_with_cli)
-                .unwrap_or(false);
-            let msg = if !uses_cli && may_be_libgit_fault {
-                format!(
-                    r"failed to {} into: {}
-  If your environment requires git authentication or proxying, try enabling `git-fetch-with-cli`
-  https://doc.rust-lang.org/cargo/reference/config.html#netgit-fetch-with-cli",
-                    operation,
-                    into.display()
-                )
-            } else {
-                format!("failed to {} into: {}", operation, into.display())
-            };
-            e.context(msg)
-        };
-
         // If we have a previous instance of `GitDatabase` then fetch into that
         // if we can. If that can successfully load our revision then we've
         // populated the database with the latest version of `reference`, so
         // return that database and the rev we resolve to.
         if let Some(mut db) = db {
             fetch(&mut db.repo, self.url.as_str(), reference, cargo_config)
-                .map_err(|e| format_error(e, "fetch"))?;
+                .context(format!("failed to fetch into: {}", into.display()))?;
             match locked_rev {
                 Some(rev) => {
                     if db.contains(rev) {
@@ -148,7 +115,7 @@ impl GitRemote {
         paths::create_dir_all(into)?;
         let mut repo = init(into, true)?;
         fetch(&mut repo, self.url.as_str(), reference, cargo_config)
-            .map_err(|e| format_error(e, "clone"))?;
+            .context(format!("failed to clone into: {}", into.display()))?;
         let rev = match locked_rev {
             Some(rev) => rev,
             None => reference.resolve(&repo)?,
@@ -481,9 +448,14 @@ where
     let mut ssh_agent_attempts = Vec::new();
     let mut any_attempts = false;
     let mut tried_sshkey = false;
+    let mut url_attempt = None;
 
+    let orig_url = url;
     let mut res = f(&mut |url, username, allowed| {
         any_attempts = true;
+        if url != orig_url {
+            url_attempt = Some(url.to_string());
+        }
         // libgit2's "USERNAME" authentication actually means that it's just
         // asking us for a username to keep going. This is currently only really
         // used for SSH authentication and isn't really an authentication type.
@@ -615,47 +587,83 @@ where
             }
         }
     }
-
-    if res.is_ok() || !any_attempts {
-        return res.map_err(From::from);
-    }
+    let mut err = match res {
+        Ok(e) => return Ok(e),
+        Err(e) => e,
+    };
 
     // In the case of an authentication failure (where we tried something) then
     // we try to give a more helpful error message about precisely what we
     // tried.
-    let res = res.map_err(anyhow::Error::from).chain_err(|| {
+    if any_attempts {
         let mut msg = "failed to authenticate when downloading \
                        repository"
             .to_string();
+
+        if let Some(attempt) = &url_attempt {
+            if url != attempt {
+                msg.push_str(": ");
+                msg.push_str(attempt);
+            }
+        }
+        msg.push_str("\n");
         if !ssh_agent_attempts.is_empty() {
             let names = ssh_agent_attempts
                 .iter()
                 .map(|s| format!("`{}`", s))
                 .collect::<Vec<_>>()
                 .join(", ");
             msg.push_str(&format!(
-                "\nattempted ssh-agent authentication, but \
-                 none of the usernames {} succeeded",
+                "\n* attempted ssh-agent authentication, but \
+                 no usernames succeeded: {}",
                 names
             ));
         }
         if let Some(failed_cred_helper) = cred_helper_bad {
             if failed_cred_helper {
                 msg.push_str(
-                    "\nattempted to find username/password via \
+                    "\n* attempted to find username/password via \
                      git's `credential.helper` support, but failed",
                 );
             } else {
                 msg.push_str(
-                    "\nattempted to find username/password via \
+                    "\n* attempted to find username/password via \
                      `credential.helper`, but maybe the found \
                      credentials were incorrect",
                 );
             }
         }
-        msg
-    })?;
-    Ok(res)
+        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);
+
+    // Otherwise if we didn't even get to the authentication phase them we may
+    // have failed to set up a connection, in these cases hint on the
+    // `net.git-fetch-with-cli` configuration option.
+    } else if let Some(e) = err.downcast_ref::<git2::Error>() {
+        match e.class() {
+            ErrorClass::Net
+            | ErrorClass::Ssl
+            | ErrorClass::Submodule
+            | ErrorClass::FetchHead
+            | ErrorClass::Ssh
+            | ErrorClass::Callback
+            | ErrorClass::Http => {
+                let mut msg = "network failure seems to have happened\n".to_string();
+                msg.push_str(
+                    "if a proxy or similar is necessary `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)
 }
 
 fn reset(repo: &git2::Repository, obj: &git2::Object<'_>, config: &Config) -> CargoResult<()> {

src/cargo/util/network.rs

@@ -20,9 +20,9 @@ impl<'a> Retry<'a> {
         match f() {
             Err(ref e) if maybe_spurious(e) && self.remaining > 0 => {
                 let msg = format!(
-                    "spurious network error ({} tries \
-                     remaining): {}",
-                    self.remaining, e
+                    "spurious network error ({} tries remaining): {}",
+                    self.remaining,
+                    e.root_cause(),
                 );
                 self.config.shell().warn(msg)?;
                 self.remaining -= 1;

src/cargo/util/toml/mod.rs

@@ -81,7 +81,7 @@ fn do_read_manifest(
         add_unused(manifest.warnings_mut());
         if manifest.targets().iter().all(|t| t.is_custom_build()) {
             bail!(
-                "no targets specified in the manifest\n  \
+                "no targets specified in the manifest\n\
                  either src/lib.rs, src/main.rs, a [lib] section, or \
                  [[bin]] section must be present"
             )

tests/testsuite/bad_config.rs

@@ -1398,8 +1398,8 @@ Caused by:
 
 Caused by:
   invalid configuration for key `target.cfg(not(target_os = \"none\")).runner`
-expected a string or array of strings, but found a boolean for \
-`target.cfg(not(target_os = \"none\")).runner` in [..]/foo/.cargo/config
+  expected a string or array of strings, but found a boolean for \
+  `target.cfg(not(target_os = \"none\")).runner` in [..]/foo/.cargo/config
 ",
         )
         .run();

tests/testsuite/cargo_features.rs

@@ -30,7 +30,7 @@ Caused by:
 Caused by:
   feature `test-dummy-unstable` is required
 
-consider adding `cargo-features = [\"test-dummy-unstable\"]` to the manifest
+  consider adding `cargo-features = [\"test-dummy-unstable\"]` to the manifest
 ",
         )
         .run();
@@ -47,9 +47,9 @@ Caused by:
 Caused by:
   feature `test-dummy-unstable` is required
 
-this Cargo does not support nightly features, but if you
-switch to nightly channel you can add
-`cargo-features = [\"test-dummy-unstable\"]` to enable this feature
+  this Cargo does not support nightly features, but if you
+  switch to nightly channel you can add
+  `cargo-features = [\"test-dummy-unstable\"]` to enable this feature
 ",
         )
         .run();
@@ -148,7 +148,7 @@ error: failed to parse manifest at `[..]`
 Caused by:
   the cargo feature `test-dummy-unstable` requires a nightly version of Cargo, \
   but this is the `stable` channel
-See [..]
+  See [..]
 ",
         )
         .run();
@@ -213,7 +213,7 @@ Caused by:
 Caused by:
   the cargo feature `test-dummy-unstable` requires a nightly version of Cargo, \
   but this is the `stable` channel
-See [..]
+  See [..]
 ",
         )
         .run();
@@ -255,7 +255,7 @@ error: failed to parse manifest at `[..]`
 Caused by:
   the cargo feature `test-dummy-unstable` requires a nightly version of Cargo, \
   but this is the `stable` channel
-See [..]
+  See [..]
 ",
         )
         .run();

tests/testsuite/directory.rs

@@ -192,9 +192,9 @@ error: failed to compile `bar v0.1.0`, intermediate artifacts can be found at `[
 
 Caused by:
   no matching package named `baz` found
-location searched: registry `https://github.com/rust-lang/crates.io-index`
-perhaps you meant: bar or foo
-required by package `bar v0.1.0`
+  location searched: registry `https://github.com/rust-lang/crates.io-index`
+  perhaps you meant: bar or foo
+  required by package `bar v0.1.0`
 ",
         )
         .run();
@@ -663,11 +663,10 @@ Caused by:
 
 Caused by:
   the source my-git-repo requires a lock file to be present first before it can be
-used against vendored source code
-
-remove the source replacement configuration, generate a lock file, and then
-restore the source replacement configuration to continue the build
+  used against vendored source code
 
+  remove the source replacement configuration, generate a lock file, and then
+  restore the source replacement configuration to continue the build
 ",
         )
         .run();
@@ -765,8 +764,8 @@ Caused by:
   failed to select a version for the requirement `foo = \"^2\"`
   candidate versions found which didn't match: 0.0.1
   location searched: directory source `[..] (which is replacing registry `[..]`)
-required by package `bar v0.1.0`
-perhaps a crate was updated and forgotten to be re-vendored?
+  required by package `bar v0.1.0`
+  perhaps a crate was updated and forgotten to be re-vendored?
 ",
         )
         .with_status(101)

tests/testsuite/features.rs

@@ -98,7 +98,7 @@ fn invalid3() {
 
 Caused by:
   Feature `bar` depends on `baz` which is not an optional dependency.
-Consider adding `optional = true` to the dependency
+  Consider adding `optional = true` to the dependency
 ",
         )
         .run();
@@ -1456,7 +1456,7 @@ fn namespaced_non_optional_dependency() {
 
 Caused by:
   Feature `bar` includes `crate:baz` which is not an optional dependency.
-Consider adding `optional = true` to the dependency
+  Consider adding `optional = true` to the dependency
 ",
         )
         .run();
@@ -1519,7 +1519,7 @@ fn namespaced_shadowed_dep() {
 
 Caused by:
   Feature `baz` includes the optional dependency of the same name, but this is left implicit in the features included by this feature.
-Consider adding `crate:baz` to this feature's requirements.
+  Consider adding `crate:baz` to this feature's requirements.
 ",
         )
         .run();
@@ -1555,8 +1555,8 @@ fn namespaced_shadowed_non_optional() {
 
 Caused by:
   Feature `baz` includes the dependency of the same name, but this is left implicit in the features included by this feature.
-Additionally, the dependency must be marked as optional to be included in the feature definition.
-Consider adding `crate:baz` to this feature's requirements and marking the dependency as `optional = true`
+  Additionally, the dependency must be marked as optional to be included in the feature definition.
+  Consider adding `crate:baz` to this feature's requirements and marking the dependency as `optional = true`
 ",
         )
         .run();
@@ -1592,7 +1592,7 @@ fn namespaced_implicit_non_optional() {
 
 Caused by:
   Feature `bar` includes `baz` which is not defined as a feature.
-A non-optional dependency of the same name is defined; consider adding `optional = true` to its definition
+  A non-optional dependency of the same name is defined; consider adding `optional = true` to its definition
 ",
         ).run(
     );

tests/testsuite/features2.rs

@@ -1318,7 +1318,7 @@ error: failed to parse manifest at `[..]/foo/Cargo.toml`
 Caused by:
   feature `resolver` is required
 
-consider adding `cargo-features = [\"resolver\"]` to the manifest
+  consider adding `cargo-features = [\"resolver\"]` to the manifest
 ",
         )
         .run();
@@ -1347,7 +1347,7 @@ error: failed to parse manifest at `[..]/foo/Cargo.toml`
 Caused by:
   feature `resolver` is required
 
-consider adding `cargo-features = [\"resolver\"]` to the manifest
+  consider adding `cargo-features = [\"resolver\"]` to the manifest
 ",
         )
         .run();