diff options
author | Andrew Gallant <jamslam@gmail.com> | 2019-01-26 14:36:34 -0500 |
---|---|---|
committer | Andrew Gallant <jamslam@gmail.com> | 2019-01-26 14:39:40 -0500 |
commit | 31d3e241306f305c1cb94e1882511da2b48dcd36 (patch) | |
tree | 9e7ca2b52a93780a20585356ce6cac98ac8db1c2 /src | |
parent | bf842dbc7fba45befe9b0d9e28f94336c809c9e3 (diff) |
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
Diffstat (limited to 'src')
-rw-r--r-- | src/args.rs | 46 |
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); +} |