args: prevent panicking in 'rg -h | rg'

Previously, we relied on clap to handle printing either an error
message, or --help/--version output, in addition to setting the exit
status code. Unfortunately, for --help/--version output, clap was
panicking if the write failed, which can happen in fairly common
scenarios via a broken pipe error. e.g., `rg -h | head`.

We fix this by using clap's "safe" API and doing the printing ourselves.
We also set the exit code to `2` when an invalid command has been given.

Fixes #1125 and partially addresses #1159

1 files changed, 38 insertions, 8 deletions

diff --git a/src/args.rs b/src/args.rs
index e2a5a09f..cec3bca1 100644
--- a/src/args.rs
+++ b/src/args.rs
@@ -1,9 +1,10 @@
 use std::cmp;
 use std::env;
-use std::ffi::OsStr;
+use std::ffi::{OsStr, OsString};
 use std::fs;
-use std::io;
+use std::io::{self, Write};
 use std::path::{Path, PathBuf};
+use std::process;
 use std::sync::Arc;
 use std::time::SystemTime;
 
@@ -130,7 +131,7 @@ impl Args {
         // trying to parse config files. If a config file exists and has
         // arguments, then we re-parse argv, otherwise we just use the matches
         // we have here.
-        let early_matches = ArgMatches::new(app::app().get_matches());
+        let early_matches = ArgMatches::new(clap_matches(env::args_os())?);
         set_messages(!early_matches.is_present("no-messages"));
         set_ignore_messages(!early_matches.is_present("no-ignore-messages"));
 
@@ -145,7 +146,7 @@ impl Args {
             log::set_max_level(log::LevelFilter::Warn);
         }
 
-        let matches = early_matches.reconfigure();
+        let matches = early_matches.reconfigure()?;
         // The logging level may have changed if we brought in additional
         // arguments from a configuration file, so recheck it and set the log
         // level as appropriate.
@@ -490,19 +491,19 @@ impl ArgMatches {
     ///
     /// If there are no additional arguments from the environment (e.g., a
     /// config file), then the given matches are returned as is.
-    fn reconfigure(self) -> ArgMatches {
+    fn reconfigure(self) -> Result<ArgMatches> {
         // If the end user says no config, then respect it.
         if self.is_present("no-config") {
             log::debug!(
                 "not reading config files because --no-config is present"
             );
-            return self;
+            return Ok(self);
         }
         // If the user wants ripgrep to use a config file, then parse args
         // from that first.
         let mut args = config::args();
         if args.is_empty() {
-            return self;
+            return Ok(self);
         }
         let mut cliargs = env::args_os();
         if let Some(bin) = cliargs.next() {
@@ -510,7 +511,7 @@ impl ArgMatches {
         }
         args.extend(cliargs);
         log::debug!("final argv: {:?}", args);
-        ArgMatches::new(app::app().get_matches_from(args))
+        Ok(ArgMatches(clap_matches(args)?))
     }
 
     /// Convert the result of parsing CLI arguments into ripgrep's higher level
@@ -1618,3 +1619,32 @@ where G: Fn(&fs::Metadata) -> io::Result<SystemTime>
         t1.cmp(&t2)
     }
 }
+
+/// Returns a clap matches object if the given arguments parse successfully.
+///
+/// Otherwise, if an error occurred, then it is returned unless the error
+/// corresponds to a `--help` or `--version` request. In which case, the
+/// corresponding output is printed and the current process is exited
+/// successfully.
+fn clap_matches<I, T>(
+    args: I,
+) -> Result<clap::ArgMatches<'static>>
+where I: IntoIterator<Item=T>,
+      T: Into<OsString> + Clone
+{
+    let err = match app::app().get_matches_from_safe(args) {
+        Ok(matches) => return Ok(matches),
+        Err(err) => err,
+    };
+    if err.use_stderr() {
+        return Err(err.into());
+    }
+    // Explicitly ignore any error returned by writeln!. The most likely error
+    // at this point is a broken pipe error, in which case, we want to ignore
+    // it and exit quietly.
+    //
+    // (This is the point of this helper function. clap's functionality for
+    // doing this will panic on a broken pipe error.)
+    let _ = writeln!(io::stdout(), "{}", err);
+    process::exit(0);
+}