Merge pull request #4342 from tmccombs/mktemp-req-equals

Require = for --tmpdir in mktemp

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<String>,
+    tmpdir: Option<PathBuf>,
 
     /// The suffix to append to the temporary file, if any.
     suffix: Option<String>,
@@ -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::<String>(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
+        let tmpdir = matches
-        // `is_tmpdir_argument_actually_the_template()` for more
+            .get_one::<PathBuf>(OPT_TMPDIR)
-        // information.
+            .or_else(|| matches.get_one::<PathBuf>(OPT_P))
-        //
+            .cloned();
-        // Fixed in clap 3
+        let (tmpdir, template) = match matches.get_one::<String>(ARG_TEMPLATE) {
-        // 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::<String>(OPT_TMPDIR).unwrap().to_string();
-            (tmpdir, template)
-        } else {
             // If no template argument is given, `--tmpdir` is implied.
-            match matches.get_one::<String>(ARG_TEMPLATE) {
+            None => {
-                None => {
+                let tmpdir = Some(tmpdir.unwrap_or_else(env::temp_dir));
-                    let tmpdir = match matches.get_one::<String>(OPT_TMPDIR) {
+                let template = DEFAULT_TEMPLATE;
-                        None => Some(env::temp_dir().display().to_string()),
+                (tmpdir, template.to_string())
-                        Some(tmpdir) => Some(tmpdir.to_string()),
+            }
-                    };
+            Some(template) => {
-                    let template = DEFAULT_TEMPLATE;
+                let tmpdir = if env::var(TMPDIR_ENV_VAR).is_ok() && matches.get_flag(OPT_T) {
-                    (tmpdir, template.to_string())
+                    env::var_os(TMPDIR_ENV_VAR).map(|t| t.into())
-                }
+                } else if tmpdir.is_some() {
-                Some(template) => {
+                    tmpdir
-                    let tmpdir = if env::var(TMPDIR_ENV_VAR).is_ok() && matches.get_flag(OPT_T) {
+                } else if matches.get_flag(OPT_T) || matches.contains_id(OPT_TMPDIR) {
-                        env::var(TMPDIR_ENV_VAR).ok()
+                    // If --tmpdir is given without an argument, or -t is given
-                    } else if matches.contains_id(OPT_TMPDIR) {
+                    // export in TMPDIR
-                        matches.get_one::<String>(OPT_TMPDIR).map(String::from)
+                    Some(env::temp_dir())
-                    } else if matches.get_flag(OPT_T) {
+                } else {
-                        // mktemp -t foo.xxx should export in TMPDIR
+                    None
-                        Some(env::temp_dir().display().to_string())
+                };
-                    } else {
+                (tmpdir, template.to_string())
-                        matches.get_one::<String>(OPT_TMPDIR).map(String::from)
-                    };
-                    (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(

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 <DIR>' but none was supplied");
+}