From 31d3e241306f305c1cb94e1882511da2b48dcd36 Mon Sep 17 00:00:00 2001 From: Andrew Gallant Date: Sat, 26 Jan 2019 14:36:34 -0500 Subject: [PATCH] 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 | 3 +++ doc/rg.1.txt.tpl | 3 ++- src/args.rs | 46 +++++++++++++++++++++++++++++++++++++-------- tests/regression.rs | 5 +++++ 4 files changed, 48 insertions(+), 9 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 616b7466..1c527fa9 100644 --- a/CHANGELOG.md +++ b/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): diff --git a/doc/rg.1.txt.tpl b/doc/rg.1.txt.tpl index 9988edc9..c9a08f97 100644 --- a/doc/rg.1.txt.tpl +++ b/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 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 { // 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 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( + args: I, +) -> Result> +where I: IntoIterator, + T: Into + 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); +} diff --git a/tests/regression.rs b/tests/regression.rs index 15dbcad7..d547c7e5 100644 --- a/tests/regression.rs +++ b/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");