Auto merge of #2778 - alexcrichton:dont-print-on-q-when-run, r=brson

Don't print extra info on -q + `cargo run`

If a process fails then it's probably printing out why and Cargo doesn't need to
do it all over again.

Closes #2735

src/bin/bench.rs

@@ -1,5 +1,5 @@
 use cargo::ops;
-use cargo::util::{CliResult, CliError, Human, Config};
+use cargo::util::{CliResult, CliError, Human, Config, human};
 use cargo::util::important_paths::{find_root_manifest_for_wd};
 
 #[derive(RustcDecodable)]
@@ -96,8 +96,8 @@ pub fn execute(options: Options, config: &Config) -> CliResult<Option<()>> {
         None => Ok(None),
         Some(err) => {
             Err(match err.exit.as_ref().and_then(|e| e.code()) {
-                Some(i) => CliError::new("bench failed", i),
-                None => CliError::from_error(Human(err), 101)
+                Some(i) => CliError::new(human("bench failed"), i),
+                None => CliError::new(Box::new(Human(err)), 101)
             })
         }
     }

src/bin/cargo.rs

@@ -221,9 +221,9 @@ fn execute_subcommand(config: &Config,
     };
 
     if let Some(code) = err.exit.as_ref().and_then(|c| c.code()) {
-        Err(CliError::new("", code))
+        Err(CliError::code(code))
     } else {
-        Err(CliError::from_error(err, 101))
+        Err(CliError::new(Box::new(err), 101))
     }
 }
 

src/bin/git_checkout.rs

@@ -35,15 +35,15 @@ pub fn execute(options: Options, config: &Config) -> CliResult<Option<()>> {
                        human(format!("The URL `{}` you passed was \
                                       not a valid URL: {}", url, e))
                    })
-                   .map_err(|e| CliError::from_boxed(e, 1)));
+                   .map_err(|e| CliError::new(e, 1)));
 
     let reference = GitReference::Branch(reference.clone());
     let source_id = SourceId::for_git(&url, reference);
 
     let mut source = GitSource::new(&source_id, config);
 
     try!(source.update().map_err(|e| {
-        CliError::new(&format!("Couldn't update {:?}: {:?}", source, e), 1)
+        CliError::new(human(format!("Couldn't update {:?}: {:?}", source, e)), 1)
     }));
 
     Ok(None)

src/bin/help.rs

@@ -1,4 +1,4 @@
-use cargo::util::{CliResult, CliError, Config};
+use cargo::util::{CliResult, CliError, Config, human};
 
 #[derive(RustcDecodable)]
 pub struct Options;
@@ -18,5 +18,5 @@ pub fn execute(_: Options, _: &Config) -> CliResult<Option<()>> {
     // This is a dummy command just so that `cargo help help` works.
     // The actual delegation of help flag to subcommands is handled by the
     // cargo command.
-    Err(CliError::new("Help command should not be executed directly.", 101))
+    Err(CliError::new(human("help command should not be executed directly"), 101))
 }

src/bin/locate_project.rs

@@ -30,7 +30,7 @@ pub fn execute(flags: LocateProjectFlags,
                       .chain_error(|| human("Your project path contains \
                                              characters not representable in \
                                              Unicode"))
-                      .map_err(|e| CliError::from_boxed(e, 1)));
+                      .map_err(|e| CliError::new(e, 1)));
 
     Ok(Some(ProjectLocation { root: string.to_string() }))
 }

src/bin/run.rs

@@ -88,9 +88,21 @@ pub fn execute(options: Options, config: &Config) -> CliResult<Option<()>> {
     match try!(ops::run(&root, &compile_opts, &options.arg_args)) {
         None => Ok(None),
         Some(err) => {
-            Err(match err.exit.as_ref().and_then(|e| e.code()) {
-                Some(code) => CliError::from_error(Human(err), code),
-                None => CliError::from_error(err, 101),
+            // If we never actually spawned the process then that sounds pretty
+            // bad and we always want to forward that up.
+            let exit = match err.exit.clone() {
+                Some(exit) => exit,
+                None => return Err(CliError::new(Box::new(Human(err)), 101)),
+            };
+
+            // If `-q` was passed then we suppress extra error information about
+            // a failed process, we assume the process itself printed out enough
+            // information about why it failed so we don't do so as well
+            let exit_code = exit.code().unwrap_or(101);
+            Err(if options.flag_quiet == Some(true) {
+                CliError::code(exit_code)
+            } else {
+                CliError::new(Box::new(Human(err)), exit_code)
             })
         }
     }

src/bin/rustc.rs

@@ -3,7 +3,7 @@ use std::env;
 use cargo::ops::{CompileOptions, CompileMode};
 use cargo::ops;
 use cargo::util::important_paths::{find_root_manifest_for_wd};
-use cargo::util::{CliResult, CliError, Config};
+use cargo::util::{CliResult, CliError, Config, human};
 
 #[derive(RustcDecodable)]
 pub struct Options {
@@ -79,8 +79,9 @@ pub fn execute(options: Options, config: &Config) -> CliResult<Option<()>> {
         Some("test") => CompileMode::Test,
         Some("bench") => CompileMode::Bench,
         Some(mode) => {
-            return Err(CliError::new(&format!("unknown profile: `{}`, use dev,
-                                               test, or bench", mode), 101))
+            let err = human(format!("unknown profile: `{}`, use dev,
+                                     test, or bench", mode));
+            return Err(CliError::new(err, 101))
         }
     };
 

src/bin/test.rs

@@ -1,5 +1,5 @@
 use cargo::ops;
-use cargo::util::{CliResult, CliError, Human, Config};
+use cargo::util::{CliResult, CliError, Human, human, Config};
 use cargo::util::important_paths::{find_root_manifest_for_wd};
 
 #[derive(RustcDecodable)]
@@ -124,8 +124,8 @@ pub fn execute(options: Options, config: &Config) -> CliResult<Option<()>> {
         None => Ok(None),
         Some(err) => {
             Err(match err.exit.as_ref().and_then(|e| e.code()) {
-                Some(i) => CliError::new("test failed", i),
-                None => CliError::from_error(Human(err), 101)
+                Some(i) => CliError::new(human("test failed"), i),
+                None => CliError::new(Box::new(Human(err)), 101)
             })
         }
     }

src/cargo/lib.rs

@@ -186,17 +186,19 @@ pub fn handle_error(err: CliError, shell: &mut MultiShell) {
 
     let hide = unknown && shell.get_verbose() != Verbose;
 
-    let _ignored_result = if hide {
-        shell.error("An unknown error occurred")
-    } else if fatal {
-        shell.error(&error)
-    } else {
-        shell.say(&error, BLACK)
-    };
-
-    if !handle_cause(&error, shell) || hide {
-        let _ = shell.err().say("\nTo learn more, run the command again \
-                                 with --verbose.".to_string(), BLACK);
+    if let Some(error) = error {
+        let _ignored_result = if hide {
+            shell.error("An unknown error occurred")
+        } else if fatal {
+            shell.error(&error)
+        } else {
+            shell.say(&error, BLACK)
+        };
+
+        if !handle_cause(&error, shell) || hide {
+            let _ = shell.err().say("\nTo learn more, run the command again \
+                                     with --verbose.".to_string(), BLACK);
+        }
     }
 
     std::process::exit(exit_code);
@@ -247,23 +249,23 @@ fn flags_from_args<T>(usage: &str, args: &[String],
                                    .help(true);
     docopt.decode().map_err(|e| {
         let code = if e.fatal() {1} else {0};
-        CliError::from_error(human(e.to_string()), code)
+        CliError::new(human(e.to_string()), code)
     })
 }
 
 fn json_from_stdin<T: Decodable>() -> CliResult<T> {
     let mut reader = io::stdin();
     let mut input = String::new();
     try!(reader.read_to_string(&mut input).map_err(|_| {
-        CliError::new("Standard in did not exist or was not UTF-8", 1)
+        CliError::new(human("Standard in did not exist or was not UTF-8"), 1)
     }));
 
     let json = try!(Json::from_str(&input).map_err(|_| {
-        CliError::new("Could not parse standard in as JSON", 1)
+        CliError::new(human("Could not parse standard in as JSON"), 1)
     }));
     let mut decoder = json::Decoder::new(json);
 
     Decodable::decode(&mut decoder).map_err(|_| {
-        CliError::new("Could not process standard in as input", 1)
+        CliError::new(human("Could not process standard in as input"), 1)
     })
 }

src/cargo/util/errors.rs

@@ -246,42 +246,46 @@ pub type CliResult<T> = Result<T, CliError>;
 
 #[derive(Debug)]
 pub struct CliError {
-    pub error: Box<CargoError>,
+    pub error: Option<Box<CargoError>>,
     pub unknown: bool,
     pub exit_code: i32
 }
 
 impl Error for CliError {
-    fn description(&self) -> &str { self.error.description() }
-    fn cause(&self) -> Option<&Error> { self.error.cause() }
+    fn description(&self) -> &str {
+        self.error.as_ref().map(|e| e.description())
+            .unwrap_or("unknown cli error")
+    }
+
+    fn cause(&self) -> Option<&Error> {
+        self.error.as_ref().and_then(|e| e.cause())
+    }
 }
 
 impl fmt::Display for CliError {
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
-        fmt::Display::fmt(&self.error, f)
+        if let Some(ref error) = self.error {
+            error.fmt(f)
+        } else {
+            self.description().fmt(f)
+        }
     }
 }
 
 impl CliError {
-    pub fn new(error: &str, code: i32) -> CliError {
-        let error = human(error.to_string());
-        CliError::from_boxed(error, code)
-    }
-
-    pub fn from_error<E: CargoError>(error: E, code: i32) -> CliError {
-        let error = Box::new(error);
-        CliError::from_boxed(error, code)
+    pub fn new(error: Box<CargoError>, code: i32) -> CliError {
+        let human = error.is_human();
+        CliError { error: Some(error), exit_code: code, unknown: !human }
     }
 
-    pub fn from_boxed(error: Box<CargoError>, code: i32) -> CliError {
-        let human = error.is_human();
-        CliError { error: error, exit_code: code, unknown: !human }
+    pub fn code(code: i32) -> CliError {
+        CliError { error: None, exit_code: code, unknown: false }
     }
 }
 
 impl From<Box<CargoError>> for CliError {
     fn from(err: Box<CargoError>) -> CliError {
-        CliError::from_boxed(err, 101)
+        CliError::new(err, 101)
     }
 }
 

tests/run.rs

@@ -610,3 +610,24 @@ fn run_with_library_paths() {
 
     assert_that(p.cargo_process("run"), execs().with_status(0));
 }
+
+#[test]
+fn fail_no_extra_verbose() {
+    let p = project("foo")
+        .file("Cargo.toml", r#"
+            [project]
+            name = "foo"
+            version = "0.0.1"
+            authors = []
+        "#)
+        .file("src/main.rs", r#"
+            fn main() {
+                std::process::exit(1);
+            }
+        "#);
+
+    assert_that(p.cargo_process("run").arg("-q"),
+                execs().with_status(1)
+                       .with_stdout("")
+                       .with_stderr(""));
+}