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

CHANGELOG.md

@@ -37,6 +37,9 @@ Bug fixes:
   Fix handling of literal slashes in gitignore patterns.
 * [BUG #1121](https://github.com/BurntSushi/ripgrep/issues/1121):
   Fix bug that was triggering Windows antimalware when using the --files flag.
+* [BUG #1125](https://github.com/BurntSushi/ripgrep/issues/1125),
+  [BUG #1159](https://github.com/BurntSushi/ripgrep/issues/1159):
+  ripgrep shouldn't panic for `rg -h | rg` and should emit correct exit status.
 * [BUG #1173](https://github.com/BurntSushi/ripgrep/issues/1173):
   Fix handling of `**` patterns in gitignore files.
 * [BUG #1174](https://github.com/BurntSushi/ripgrep/issues/1174):

doc/rg.1.txt.tpl

@@ -89,7 +89,8 @@ cases, the flag specified last takes precedence.
 EXIT STATUS
 -----------
 If ripgrep finds a match, then the exit status of the program is 0. If no match
-could be found, then the exit status is non-zero.
+could be found, then the exit status is 1. If an error occurred, then the exit
+status is 2.
 
 
 CONFIGURATION FILES

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,27 +491,27 @@ 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() {
             args.insert(0, bin);
         }
         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);
+}

tests/regression.rs

@@ -592,6 +592,11 @@ rgtest!(r1130, |dir: Dir, mut cmd: TestCommand| {
     );
 });
 
+// See: https://github.com/BurntSushi/ripgrep/issues/1159
+rgtest!(r1159, |_: Dir, mut cmd: TestCommand| {
+    cmd.arg("--wat").assert_exit_code(2);
+});
+
 // See: https://github.com/BurntSushi/ripgrep/issues/1163
 rgtest!(r1163, |dir: Dir, mut cmd: TestCommand| {
     dir.create("bom.txt", "\u{FEFF}test123\ntest123");