From b77a08c1342421dd5e4141632e963288e4c178ee Mon Sep 17 00:00:00 2001 From: Thayne McCombs Date: Fri, 10 Feb 2023 23:31:28 -0700 Subject: [PATCH 1/4] 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 | 106 +++++++++++++----------------------- 1 file changed, 39 insertions(+), 67 deletions(-) diff --git a/src/uu/mktemp/src/mktemp.rs b/src/uu/mktemp/src/mktemp.rs index fc360d0f5..9466f7c63 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,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( From 6262a3e9d94db33c83fbeac03fd574cff8d6c8f9 Mon Sep 17 00:00:00 2001 From: Thayne McCombs Date: Mon, 13 Feb 2023 00:00:02 -0700 Subject: [PATCH 2/4] Add tests for mktemp tmpdir flags And set overrides_with for tmpdir flags. Tests were copied from #4275 Co-authored-by: David Matos --- src/uu/mktemp/src/mktemp.rs | 1 + tests/by-util/test_mktemp.rs | 60 ++++++++++++++++++++++++++++++++++++ 2 files changed, 61 insertions(+) diff --git a/src/uu/mktemp/src/mktemp.rs b/src/uu/mktemp/src/mktemp.rs index 9466f7c63..9c0860d92 100644 --- a/src/uu/mktemp/src/mktemp.rs +++ b/src/uu/mktemp/src/mktemp.rs @@ -428,6 +428,7 @@ pub fn uu_app() -> Command { .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), ) diff --git a/tests/by-util/test_mktemp.rs b/tests/by-util/test_mktemp.rs index 6bcb37f45..db60d2453 100644 --- a/tests/by-util/test_mktemp.rs +++ b/tests/by-util/test_mktemp.rs @@ -901,3 +901,63 @@ 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!()); + scene + .ucmd() + .arg("-p") + .arg(".") + .arg("--tmpdir") + .arg("foobarXXXX") + .succeeds() + .no_stderr() + .stdout_contains("/tmp/foobar"); + scene + .ucmd() + .arg("-p") + .arg(".") + .arg("--tmpdir=foobarXXXX") + .fails() + .no_stdout() + .stderr_contains("failed to create file via template"); + scene + .ucmd() + .arg("--tmpdir") + .arg("foobarXXXX") + .arg("-p") + .arg(".") + .succeeds() + .no_stderr() + .stdout_contains("./foobar"); +} + +#[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"); +} From 536db164bf119ac4624abcd2dcaa5542b4c830cf Mon Sep 17 00:00:00 2001 From: Thayne McCombs Date: Tue, 11 Apr 2023 00:43:15 -0600 Subject: [PATCH 3/4] Fix mktemp test for windows --- tests/by-util/test_mktemp.rs | 30 ++++++++++++++++++------------ 1 file changed, 18 insertions(+), 12 deletions(-) diff --git a/tests/by-util/test_mktemp.rs b/tests/by-util/test_mktemp.rs index db60d2453..0810fc43a 100644 --- a/tests/by-util/test_mktemp.rs +++ b/tests/by-util/test_mktemp.rs @@ -923,32 +923,38 @@ fn test_missing_xs_tmpdir_template() { #[test] fn test_both_tmpdir_flags_present() { let scene = TestScenario::new(util_name!()); - scene - .ucmd() + let template = format!(".{MAIN_SEPARATOR}foobarXXXX"); + let (at, mut ucmd) = at_and_ucmd!(); + let result = ucmd + .env(TMPDIR, ".") .arg("-p") - .arg(".") + .arg("nonsense") .arg("--tmpdir") .arg("foobarXXXX") - .succeeds() - .no_stderr() - .stdout_contains("/tmp/foobar"); + .succeeds(); + let filename = result.no_stderr().stdout_str().trim_end(); + assert_matches_template!(&template, filename); + assert!(at.file_exists(filename)); + scene .ucmd() .arg("-p") .arg(".") - .arg("--tmpdir=foobarXXXX") + .arg("--tmpdir=doesnotexist") .fails() .no_stdout() .stderr_contains("failed to create file via template"); - scene - .ucmd() + + let (at, mut ucmd) = at_and_ucmd!(); + let result = ucmd .arg("--tmpdir") .arg("foobarXXXX") .arg("-p") .arg(".") - .succeeds() - .no_stderr() - .stdout_contains("./foobar"); + .succeeds(); + let filename = result.no_stderr().stdout_str().trim_end(); + assert_matches_template!(&template, filename); + assert!(at.file_exists(filename)); } #[test] From 964b1d6e100e0b72846ac573a6ebedca44854d04 Mon Sep 17 00:00:00 2001 From: Terts Diepraam Date: Wed, 5 Jul 2023 12:51:56 +0200 Subject: [PATCH 4/4] mktemp: fix both_tmpdir_flags_present test on windows --- tests/by-util/test_mktemp.rs | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/tests/by-util/test_mktemp.rs b/tests/by-util/test_mktemp.rs index 0810fc43a..4544b6b30 100644 --- a/tests/by-util/test_mktemp.rs +++ b/tests/by-util/test_mktemp.rs @@ -923,7 +923,10 @@ fn test_missing_xs_tmpdir_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, ".") @@ -933,14 +936,19 @@ fn test_both_tmpdir_flags_present() { .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=doesnotexist") + .arg("--tmpdir=does_not_exist") .fails() .no_stdout() .stderr_contains("failed to create file via template"); @@ -953,7 +961,12 @@ fn test_both_tmpdir_flags_present() { .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)); }