Fixes review comments

Fix some formatting items.
Changes Internal error kind to preserve the original error.
Changes network retry logic to inspect full error chain for spurious
errors.

Author: jluner

src/bin/cargo.rs

@@ -332,7 +332,7 @@ fn execute_external_subcommand(config: &Config, cmd: &str, args: &[String]) -> C
         Err(e) => e,
     };
 
-    if let CargoError(CargoErrorKind::ProcessErrorKind(ref perr), ..) = err {
+    if let &CargoErrorKind::ProcessErrorKind(ref perr) = err.kind() {
         if let Some(code) = perr.exit.as_ref().and_then(|c| c.code()) {
             return Err(CliError::code(code));
         }

src/cargo/lib.rs

@@ -33,6 +33,7 @@ extern crate url;
 use std::io;
 use std::fmt;
 use std::error::Error;
+
 use error_chain::ChainedError;
 use rustc_serialize::Decodable;
 use serde::ser;

src/cargo/ops/cargo_rustc/mod.rs

@@ -372,7 +372,7 @@ fn rustc(cx: &mut Context, unit: &Unit, exec: Arc<Executor>) -> CargoResult<Work
                 format!("Could not compile `{}`.", name)
             })?;
         } else {
-            exec.exec(rustc, &package_id).map_err(|e| e.to_internal()).chain_err(|| {
+            exec.exec(rustc, &package_id).map_err(|e| e.into_internal()).chain_err(|| {
                 format!("Could not compile `{}`.", name)
             })?;
         }

src/cargo/sources/directory.rs

@@ -120,8 +120,7 @@ impl<'cfg> Source for DirectorySource<'cfg> {
                         pkg.package_id().version())
 
             })?;
-            let cksum: Checksum = serde_json::from_str(&cksum)
-                .chain_err(|| {
+            let cksum: Checksum = serde_json::from_str(&cksum).chain_err(|| {
                 format!("failed to decode `.cargo-checksum.json` of \
                          {} v{}",
                         pkg.package_id().name(),

src/cargo/sources/git/source.rs

@@ -148,7 +148,7 @@ impl<'cfg> Source for GitSource<'cfg> {
             trace!("updating git source `{:?}`", self.remote);
 
             let repo = self.remote.checkout(&db_path, self.config)?;
-            let rev = repo.rev_for(&self.reference).map_err(CargoError::to_internal)?;
+            let rev = repo.rev_for(&self.reference).map_err(CargoError::into_internal)?;
             (repo, rev)
         } else {
             (self.remote.db_at(&db_path)?, actual_rev.unwrap())

src/cargo/sources/git/utils.rs

@@ -297,7 +297,7 @@ impl<'a> GitCheckout<'a> {
 
             for mut child in repo.submodules()?.into_iter() {
                 update_submodule(repo, &mut child, cargo_config)
-                    .map_err(CargoError::to_internal)
+                    .map_err(CargoError::into_internal)
                     .chain_err(|| {
                         format!("failed to update submodule `{}`",
                                 child.name().unwrap_or(""))
@@ -532,7 +532,7 @@ fn with_authentication<T, F>(url: &str, cfg: &git2::Config, mut f: F)
     // 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.
-    res.map_err(CargoError::from).map_err(|e| e.to_internal()).chain_err(|| {
+    res.map_err(CargoError::from).map_err(|e| e.into_internal()).chain_err(|| {
         let mut msg = "failed to authenticate when downloading \
                        repository".to_string();
         if !ssh_agent_attempts.is_empty() {

src/cargo/util/errors.rs

@@ -41,9 +41,9 @@ error_chain! {
     }
 
     errors {
-        Internal(description: String){
-            description(&description)
-            display("{}", &description)
+        Internal(err: Box<CargoErrorKind>) {
+            description(err.description())
+            display("{}", *err)
         }
         ProcessErrorKind(proc_err: ProcessError) {
             description(&proc_err.desc)
@@ -61,9 +61,8 @@ error_chain! {
 }
 
 impl CargoError {
-    pub fn to_internal(self) -> Self {
-        //This is actually bad, because it loses the cause information for foreign_link
-        CargoError(CargoErrorKind::Internal(self.description().to_string()), self.1)
+    pub fn into_internal(self) -> Self {
+        CargoError(CargoErrorKind::Internal(Box::new(self.0)), self.1)
     }
 
     fn is_human(&self) -> bool {
@@ -282,5 +281,5 @@ pub fn internal<S: fmt::Display>(error: S) -> CargoError {
 }
 
 fn _internal(error: &fmt::Display) -> CargoError {
-    CargoErrorKind::Internal(error.to_string()).into()
+    CargoError::from_kind(error.to_string().into()).into_internal()
 }

src/cargo/util/network.rs

@@ -1,29 +1,53 @@
+use std;
+use std::error::Error;
+
+use error_chain::ChainedError;
+
 use util::Config;
 use util::errors::{CargoError, CargoErrorKind, CargoResult};
 
 use git2;
+fn maybe_spurious<E, EKind>(err: &E) -> bool
+    where E: ChainedError<ErrorKind=EKind> + 'static {
+    //Error inspection in non-verbose mode requires inspecting the
+    //error kind to avoid printing Internal errors. The downcasting
+    //machinery requires &(Error + 'static), but the iterator (and
+    //underlying `cause`) return &Error. Because the borrows are
+    //constrained to this handling method, and because the original
+    //error object is constrained to be 'static, we're casting away
+    //the borrow's actual lifetime for purposes of downcasting and
+    //inspecting the error chain
+    unsafe fn extend_lifetime(r: &Error) -> &(Error + 'static) {
+        std::mem::transmute::<&Error, &Error>(r)    
+    }
 
-fn maybe_spurious(err: &CargoError) -> bool {
-    match err.kind() {
-            &CargoErrorKind::Git(ref git_err) => {
-                match git_err.class() {
-                    git2::ErrorClass::Net |
-                    git2::ErrorClass::Os => true,
-                    _ => false
+    for e in err.iter() {
+        let e = unsafe { extend_lifetime(e) };
+        if let Some(cargo_err) = e.downcast_ref::<CargoError>() {
+            match cargo_err.kind() {
+                &CargoErrorKind::Git(ref git_err) => {
+                    match git_err.class() {
+                        git2::ErrorClass::Net |
+                        git2::ErrorClass::Os => return true,
+                        _ => ()
+                    }
                 }
+                &CargoErrorKind::Curl(ref curl_err) 
+                    if curl_err.is_couldnt_connect() ||
+                        curl_err.is_couldnt_resolve_proxy() ||
+                        curl_err.is_couldnt_resolve_host() ||
+                        curl_err.is_operation_timedout() ||
+                        curl_err.is_recv_error() => {
+                    return true
+                }
+                &CargoErrorKind::HttpNot200(code, ref _url) if 500 <= code && code < 600 => {
+                    return true
+                }
+                _ => ()
             }
-            &CargoErrorKind::Curl(ref curl_err) => {
-                curl_err.is_couldnt_connect() ||
-                    curl_err.is_couldnt_resolve_proxy() ||
-                    curl_err.is_couldnt_resolve_host() ||
-                    curl_err.is_operation_timedout() ||
-                    curl_err.is_recv_error()
-            }
-            &CargoErrorKind::HttpNot200(code, ref _url)  => {
-                500 <= code && code < 600
-            }
-            _ => false
+        }
     }
+    false
 }
 
 /// Wrapper method for network call retry logic.
@@ -67,3 +91,17 @@ fn with_retry_repeats_the_call_then_works() {
     let result = with_retry(&config, || results.pop().unwrap());
     assert_eq!(result.unwrap(), ())
 }
+
+#[test]
+fn with_retry_finds_nested_spurious_errors() {
+    //Error HTTP codes (5xx) are considered maybe_spurious and will prompt retry
+    //String error messages are not considered spurious
+    let error1 : CargoError = CargoErrorKind::HttpNot200(501, "Uri".to_string()).into();
+    let error1 = CargoError::with_chain(error1, "A non-spurious wrapping err");
+    let error2 = CargoError::from_kind(CargoErrorKind::HttpNot200(502, "Uri".to_string()));
+    let error2 = CargoError::with_chain(error2, "A second chained error");
+    let mut results: Vec<CargoResult<()>> = vec![Ok(()), Err(error1), Err(error2)];
+    let config = Config::default().unwrap();
+    let result = with_retry(&config, || results.pop().unwrap());
+    assert_eq!(result.unwrap(), ())
+}