diff --git a/src/uu/mktemp/src/mktemp.rs b/src/uu/mktemp/src/mktemp.rs index fc360d0f5..9c0860d92 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"; @@ -130,7 +131,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, @@ -142,72 +143,32 @@ 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 = if env::var(TMPDIR_ENV_VAR).is_ok() && matches.get_flag(OPT_T) { - env::var(TMPDIR_ENV_VAR).ok() - } else if matches.contains_id(OPT_TMPDIR) { - matches.get_one::(OPT_TMPDIR).map(String::from) - } else if matches.get_flag(OPT_T) { - // mktemp -t foo.xxx should export in TMPDIR - Some(env::temp_dir().display().to_string()) - } else { - 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 = if env::var(TMPDIR_ENV_VAR).is_ok() && matches.get_flag(OPT_T) { + env::var_os(TMPDIR_ENV_VAR).map(|t| t.into()) + } else if tmpdir.is_some() { + tmpdir + } else if matches.get_flag(OPT_T) || matches.contains_id(OPT_TMPDIR) { + // If --tmpdir is given without an argument, or -t is given + // export in TMPDIR + Some(env::temp_dir()) + } else { + None + }; + (tmpdir, template.to_string()) } }; Self { @@ -372,7 +333,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)); @@ -444,8 +405,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 \ @@ -457,6 +426,10 @@ 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) + .overrides_with(OPT_P) + .value_parser(ValueParser::path_buf()) .value_hint(clap::ValueHint::DirPath), ) .arg( diff --git a/tests/by-util/test_mktemp.rs b/tests/by-util/test_mktemp.rs index 6bcb37f45..4544b6b30 100644 --- a/tests/by-util/test_mktemp.rs +++ b/tests/by-util/test_mktemp.rs @@ -901,3 +901,82 @@ fn test_t_ensure_tmpdir_has_higher_priority_than_p() { println!("stdout = {stdout}"); assert!(stdout.contains(&pathname)); } + +#[test] +fn test_missing_xs_tmpdir_template() { + let scene = TestScenario::new(util_name!()); + scene + .ucmd() + .arg("--tmpdir") + .arg(TEST_TEMPLATE3) + .fails() + .no_stdout() + .stderr_contains("too few X's in template"); + scene + .ucmd() + .arg("--tmpdir=foobar") + .fails() + .no_stdout() + .stderr_contains("failed to create file via template"); +} + +#[test] +fn test_both_tmpdir_flags_present() { + let scene = TestScenario::new(util_name!()); + + #[cfg(not(windows))] + let template = format!(".{MAIN_SEPARATOR}foobarXXXX"); + + let (at, mut ucmd) = at_and_ucmd!(); + let result = ucmd + .env(TMPDIR, ".") + .arg("-p") + .arg("nonsense") + .arg("--tmpdir") + .arg("foobarXXXX") + .succeeds(); + let filename = result.no_stderr().stdout_str().trim_end(); + + #[cfg(not(windows))] + assert_matches_template!(&template, filename); + #[cfg(windows)] + assert_suffix_matches_template!("foobarXXXX", filename); + + assert!(at.file_exists(filename)); + + scene + .ucmd() + .arg("-p") + .arg(".") + .arg("--tmpdir=does_not_exist") + .fails() + .no_stdout() + .stderr_contains("failed to create file via template"); + + let (at, mut ucmd) = at_and_ucmd!(); + let result = ucmd + .arg("--tmpdir") + .arg("foobarXXXX") + .arg("-p") + .arg(".") + .succeeds(); + let filename = result.no_stderr().stdout_str().trim_end(); + + #[cfg(not(windows))] + assert_matches_template!(&template, filename); + #[cfg(windows)] + assert_suffix_matches_template!("foobarXXXX", filename); + + assert!(at.file_exists(filename)); +} + +#[test] +fn test_missing_short_tmpdir_flag() { + let scene = TestScenario::new(util_name!()); + scene + .ucmd() + .arg("-p") + .fails() + .no_stdout() + .stderr_contains("a value is required for '-p ' but none was supplied"); +}