From 00aeb993772ab0134e19d5deb44a668a3d50eff3 Mon Sep 17 00:00:00 2001 From: Thayne McCombs Date: Fri, 10 Feb 2023 23:31:28 -0700 Subject: [PATCH] Require = for --tmpdir in mktemp Fixes #3454 This implementation more closely matches the behavior of GNU mktemp. Specifically, if using the short-form `-p`, then DIR is required, and if using the long form `--tmpdir` then DIR is optional, but if provided, must use the `--tmpdir=DIR` form (not `--tempdir DIR`), so that there is no ambiguity with also passing a TEMPLATE. The downside to this implementation is we are now introducing a new workaround for https://github.com/clap-rs/clap/issues/4702 where we use separate calls to `.arg()` for the short `-p` and the long `--tmpdir`, which results in a less than ideal output for `--help`. --- src/uu/mktemp/src/mktemp.rs | 92 ++++++++++++++----------------------- 1 file changed, 34 insertions(+), 58 deletions(-) diff --git a/src/uu/mktemp/src/mktemp.rs b/src/uu/mktemp/src/mktemp.rs index 4a77da4c402..677b8dcd13d 100644 --- a/src/uu/mktemp/src/mktemp.rs +++ b/src/uu/mktemp/src/mktemp.rs @@ -8,7 +8,7 @@ // spell-checker:ignore (paths) GPGHome findxs -use clap::{crate_version, Arg, ArgAction, ArgMatches, Command}; +use clap::{builder::ValueParser, crate_version, Arg, ArgAction, ArgMatches, Command}; use uucore::display::{println_verbatim, Quotable}; use uucore::error::{FromIo, UError, UResult, UUsageError}; use uucore::{format_usage, help_about, help_usage}; @@ -38,6 +38,7 @@ static OPT_DRY_RUN: &str = "dry-run"; static OPT_QUIET: &str = "quiet"; static OPT_SUFFIX: &str = "suffix"; static OPT_TMPDIR: &str = "tmpdir"; +static OPT_P: &str = "p"; static OPT_T: &str = "t"; static ARG_TEMPLATE: &str = "template"; @@ -125,7 +126,7 @@ struct Options { /// The directory in which to create the temporary file. /// /// If `None`, the file will be created in the current directory. - tmpdir: Option, + tmpdir: Option, /// The suffix to append to the temporary file, if any. suffix: Option, @@ -137,63 +138,27 @@ struct Options { template: String, } -/// Decide whether the argument to `--tmpdir` should actually be the template. -/// -/// This function is required to work around a limitation of `clap`, -/// the command-line argument parsing library. In case the command -/// line is -/// -/// ```sh -/// mktemp --tmpdir XXX -/// ``` -/// -/// the program should behave like -/// -/// ```sh -/// mktemp --tmpdir=${TMPDIR:-/tmp} XXX -/// ``` -/// -/// However, `clap` thinks that `XXX` is the value of the `--tmpdir` -/// option. This function returns `true` in this case and `false` -/// in all other cases. -fn is_tmpdir_argument_actually_the_template(matches: &ArgMatches) -> bool { - if !matches.contains_id(ARG_TEMPLATE) { - if let Some(tmpdir) = matches.get_one::(OPT_TMPDIR) { - if !Path::new(tmpdir).is_dir() && tmpdir.contains("XXX") { - return true; - } - } - } - false -} - impl Options { fn from(matches: &ArgMatches) -> Self { - // Special case to work around a limitation of `clap`; see - // `is_tmpdir_argument_actually_the_template()` for more - // information. - // - // Fixed in clap 3 - // See https://github.com/clap-rs/clap/pull/1587 - let (tmpdir, template) = if is_tmpdir_argument_actually_the_template(matches) { - let tmpdir = Some(env::temp_dir().display().to_string()); - let template = matches.get_one::(OPT_TMPDIR).unwrap().to_string(); - (tmpdir, template) - } else { + let tmpdir = matches + .get_one::(OPT_TMPDIR) + .or_else(|| matches.get_one::(OPT_P)) + .cloned(); + let (tmpdir, template) = match matches.get_one::(ARG_TEMPLATE) { // If no template argument is given, `--tmpdir` is implied. - match matches.get_one::(ARG_TEMPLATE) { - None => { - let tmpdir = match matches.get_one::(OPT_TMPDIR) { - None => Some(env::temp_dir().display().to_string()), - Some(tmpdir) => Some(tmpdir.to_string()), - }; - let template = DEFAULT_TEMPLATE; - (tmpdir, template.to_string()) - } - Some(template) => { - let tmpdir = matches.get_one::(OPT_TMPDIR).map(String::from); - (tmpdir, template.to_string()) - } + None => { + let tmpdir = Some(tmpdir.unwrap_or_else(env::temp_dir)); + let template = DEFAULT_TEMPLATE; + (tmpdir, template.to_string()) + } + Some(template) => { + let tmpdir = match tmpdir { + // If --tmpdir is given without an argument, use the environment + // temporary directory + None if matches.contains_id(OPT_TMPDIR) => Some(env::temp_dir()), + t => t, + }; + (tmpdir, template.to_string()) } }; Self { @@ -358,7 +323,7 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> { if env::var("POSIXLY_CORRECT").is_ok() { // If POSIXLY_CORRECT was set, template MUST be the last argument. - if is_tmpdir_argument_actually_the_template(&matches) || matches.contains_id(ARG_TEMPLATE) { + if matches.contains_id(ARG_TEMPLATE) { // Template argument was provided, check if was the last one. if args.last().unwrap() != &options.template { return Err(Box::new(MkTempError::TooManyTemplates)); @@ -430,8 +395,16 @@ pub fn uu_app() -> Command { .value_name("SUFFIX"), ) .arg( - Arg::new(OPT_TMPDIR) + Arg::new(OPT_P) .short('p') + .help("short form of --tmpdir") + .value_name("DIR") + .num_args(1) + .value_parser(ValueParser::path_buf()) + .value_hint(clap::ValueHint::DirPath), + ) + .arg( + Arg::new(OPT_TMPDIR) .long(OPT_TMPDIR) .help( "interpret TEMPLATE relative to DIR; if DIR is not specified, use \ @@ -443,6 +416,9 @@ pub fn uu_app() -> Command { // Allows use of default argument just by setting --tmpdir. Else, // use provided input to generate tmpdir .num_args(0..=1) + // Require an equals to avoid ambiguity if no tmpdir is supplied + .require_equals(true) + .value_parser(ValueParser::path_buf()) .value_hint(clap::ValueHint::DirPath), ) .arg(