install: fix issue #3814 (#3950)

* install: fix installing one file when using -Dt options

* install: fix installing multiple files with -Dt

Code was missing the logic to create the target dir when multiple files
should be copied and target dir is given by -t option.
This simplifies the copy logic also when only one file should be copied
to the target dir.

* install: fix verbose output when using -D

Also adds a unit test to verify the same behaviour as the gnu tools.

* install: add more testcases for create leading dir

Tests various combinations of "-D" with and w/o "-t" when installing
either a single file or multiple files into a non existing directory.

  install -D file1 file2 (-t) not_existing_dir
  install -D file1 (-t) not_existing_dir/

Also fixes file formatting, spelling and adds some more test asserts.

* install: fix error for nonex. dir path ending on /

The install command failed with a different error message than the
original GNU install tool. Checking for a trailing slash fixes this.
Only works on unix though.

* install: add windows support when checking for '/'

* install.rs: fix spelling

* install.rs: add more tests regarding omitting dir

This increases the CI test coverage and also checks for more corner
cases to ensure uu_install is compliant with GNU's original.

    export C=coreutils/target/debug/

    rm -rf dir1 no-dir2 dir3 file
    mkdir -p dir1 dir3
    touch file

    ${C}install dir1/file1 dir1/.. no-dir2
    ${C}install dir1/file1 dir1/.. dir3
    ${C}install dir1/.. dir3

* install: improve test_install_missing_arguments

Also check that install returns the correct error messages, when only a
target directory is given via -t and that is is not created (-D option).

* install: rework the checks for missing file args

This ensures correct (GNU install like) behavior. Tests from the last
commit will pass now.

src/uu/install/src/install.rs

@@ -27,7 +27,9 @@ use std::fmt::{Debug, Display};
 use std::fs;
 use std::fs::File;
 use std::os::unix::fs::MetadataExt;
-use std::path::{Path, PathBuf};
+#[cfg(unix)]
+use std::os::unix::prelude::OsStrExt;
+use std::path::{Path, PathBuf, MAIN_SEPARATOR};
 use std::process;
 
 const DEFAULT_MODE: u32 = 0o755;
@@ -480,40 +482,73 @@ fn is_new_file_path(path: &Path) -> bool {
             || path.parent().unwrap().as_os_str().is_empty()) // In case of a simple file
 }
 
+/// Test if the path is an existing directory or ends with a trailing separator.
+///
+/// Returns true, if one of the conditions above is met; else false.
+///
+#[cfg(unix)]
+fn is_potential_directory_path(path: &Path) -> bool {
+    let separator = MAIN_SEPARATOR as u8;
+    path.as_os_str().as_bytes().last() == Some(&separator) || path.is_dir()
+}
+
+#[cfg(not(unix))]
+fn is_potential_directory_path(path: &Path) -> bool {
+    let path_str = path.to_string_lossy();
+    path_str.ends_with(MAIN_SEPARATOR) || path_str.ends_with('/') || path.is_dir()
+}
+
 /// Perform an install, given a list of paths and behavior.
 ///
 /// Returns a Result type with the Err variant containing the error message.
 ///
 fn standard(mut paths: Vec<String>, b: &Behavior) -> UResult<()> {
+    // first check that paths contains at least one element
+    if paths.is_empty() {
+        return Err(UUsageError::new(1, "missing file operand"));
+    }
+
+    // get the target from either "-t foo" param or from the last given paths argument
     let target: PathBuf = if let Some(path) = &b.target_dir {
         path.into()
     } else {
-        paths
+        let last_path: PathBuf = paths.pop().unwrap().into();
-            .pop()
+
-            .ok_or_else(|| UUsageError::new(1, "missing file operand"))?
+        // paths has to contain more elements
-            .into()
+        if paths.is_empty() {
+            return Err(UUsageError::new(
+                1,
+                format!(
+                    "missing destination file operand after '{}'",
+                    last_path.to_str().unwrap()
+                ),
+            ));
+        }
+
+        last_path
     };
 
     let sources = &paths.iter().map(PathBuf::from).collect::<Vec<_>>();
 
-    if sources.len() > 1 || (target.exists() && target.is_dir()) {
+    if b.create_leading {
-        copy_files_into_dir(sources, &target, b)
-    } else {
         // if -t is used in combination with -D, create whole target because it does not include filename
-        let to_create: Option<&Path> = if b.target_dir.is_some() && b.create_leading {
+        let to_create: Option<&Path> = if b.target_dir.is_some() {
             Some(target.as_path())
-        } else {
+        // if source and target are filenames used in combination with -D, create target's parent
+        } else if !(sources.len() > 1 || is_potential_directory_path(&target)) {
             target.parent()
+        } else {
+            None
         };
 
         if let Some(to_create) = to_create {
-            if !to_create.exists() && b.create_leading {
+            if !to_create.exists() {
                 if b.verbose {
                     let mut result = PathBuf::new();
                     // When creating directories with -Dv, show directory creations step by step
                     for part in to_create.components() {
                         result.push(part.as_os_str());
-                        if !Path::new(part.as_os_str()).is_dir() {
+                        if !result.is_dir() {
                             // Don't display when the directory already exists
                             println!("install: creating directory {}", result.quote());
                         }
@@ -531,25 +566,16 @@ fn standard(mut paths: Vec<String>, b: &Behavior) -> UResult<()> {
                 }
             }
         }
+    }
 
-        let source = sources.first().ok_or_else(|| {
+    if sources.len() > 1 || is_potential_directory_path(&target) {
-            UUsageError::new(
+        copy_files_into_dir(sources, &target, b)
-                1,
+    } else {
-                format!(
+        let source = sources.first().unwrap();
-                    "missing destination file operand after '{}'",
-                    target.to_str().unwrap()
-                ),
-            )
-        })?;
 
-        // If the -D flag was passed (target does not include filename),
+        if source.is_dir() {
-        // we need to add the source name to the target_dir
+            return Err(InstallError::OmittingDirectory(source.to_path_buf()).into());
-        // because `copy` expects `to` to be a file, not a directory
+        }
-        let target = if target.is_dir() && b.create_leading {
-            target.join(source)
-        } else {
-            target // already includes dest filename
-        };
 
         if target.is_file() || is_new_file_path(&target) {
             copy(source, &target, b)

tests/by-util/test_install.rs

@@ -1,4 +1,4 @@
-// spell-checker:ignore (words) helloworld objdump n'source
+// spell-checker:ignore (words) helloworld nodir objdump n'source
 
 use crate::common::util::*;
 use filetime::FileTime;
@@ -427,18 +427,42 @@ fn test_install_nested_paths_copy_file() {
 
 #[test]
 fn test_install_failing_omitting_directory() {
-    let (at, mut ucmd) = at_and_ucmd!();
+    let scene = TestScenario::new(util_name!());
-    let file1 = "source_file";
+    let at = &scene.fixtures;
-    let dir1 = "source_dir";
+    let file1 = "file1";
-    let dir2 = "target_dir";
+    let dir1 = "dir1";
+    let no_dir2 = "no-dir2";
+    let dir3 = "dir3";
 
     at.mkdir(dir1);
-    at.mkdir(dir2);
+    at.mkdir(dir3);
     at.touch(file1);
 
-    ucmd.arg(dir1)
+    // GNU install checks for existing target dir first before checking on source params
+    scene
+        .ucmd()
         .arg(file1)
-        .arg(dir2)
+        .arg(dir1)
+        .arg(no_dir2)
+        .fails()
+        .stderr_contains("is not a directory");
+
+    // file1 will be copied before install fails on dir1
+    scene
+        .ucmd()
+        .arg(file1)
+        .arg(dir1)
+        .arg(dir3)
+        .fails()
+        .code_is(1)
+        .stderr_contains("omitting directory");
+    assert!(at.file_exists(&format!("{}/{}", dir3, file1)));
+
+    // install also fails, when only one source param is given
+    scene
+        .ucmd()
+        .arg(dir1)
+        .arg(dir3)
         .fails()
         .code_is(1)
         .stderr_contains("omitting directory");
@@ -668,6 +692,106 @@ fn test_install_creating_leading_dirs() {
         .arg(at.plus(target))
         .succeeds()
         .no_stderr();
+
+    assert!(at.file_exists(target));
+}
+
+#[test]
+fn test_install_creating_leading_dirs_verbose() {
+    let scene = TestScenario::new(util_name!());
+    let at = &scene.fixtures;
+
+    let source = "create_leading_test_file";
+    let target = "dir1/no-dir2/no-dir3/test_file";
+
+    at.touch(source);
+    at.mkdir("dir1");
+
+    let creating_dir1 = regex::Regex::new("(?m)^install: creating directory.*dir1'$").unwrap();
+    let creating_nodir23 =
+        regex::Regex::new(r"(?m)^install: creating directory.*no-dir[23]'$").unwrap();
+
+    scene
+        .ucmd()
+        .arg("-Dv")
+        .arg(source)
+        .arg(at.plus(target))
+        .succeeds()
+        .stdout_matches(&creating_nodir23)
+        .stdout_does_not_match(&creating_dir1)
+        .no_stderr();
+
+    assert!(at.file_exists(target));
+}
+
+#[test]
+fn test_install_creating_leading_dirs_with_single_source_and_target_dir() {
+    let scene = TestScenario::new(util_name!());
+    let at = &scene.fixtures;
+
+    let source1 = "source_file_1";
+    let target_dir = "missing_target_dir/";
+
+    at.touch(source1);
+
+    // installing a single file into a missing directory will fail, when -D is used w/o -t parameter
+    scene
+        .ucmd()
+        .arg("-D")
+        .arg(source1)
+        .arg(at.plus(target_dir))
+        .fails()
+        .stderr_contains("missing_target_dir/' is not a directory");
+
+    assert!(!at.dir_exists(target_dir));
+
+    scene
+        .ucmd()
+        .arg("-D")
+        .arg(source1)
+        .arg("-t")
+        .arg(at.plus(target_dir))
+        .succeeds()
+        .no_stderr();
+
+    assert!(at.file_exists(&format!("{target_dir}/{source1}")));
+}
+
+#[test]
+fn test_install_creating_leading_dirs_with_multiple_sources_and_target_dir() {
+    let scene = TestScenario::new(util_name!());
+    let at = &scene.fixtures;
+
+    let source1 = "source_file_1";
+    let source2 = "source_file_2";
+    let target_dir = "missing_target_dir";
+
+    at.touch(source1);
+    at.touch(source2);
+
+    // installing multiple files into a missing directory will fail, when -D is used w/o -t parameter
+    scene
+        .ucmd()
+        .arg("-D")
+        .arg(source1)
+        .arg(source2)
+        .arg(at.plus(target_dir))
+        .fails()
+        .stderr_contains("missing_target_dir' is not a directory");
+
+    assert!(!at.dir_exists(target_dir));
+
+    scene
+        .ucmd()
+        .arg("-D")
+        .arg(source1)
+        .arg(source2)
+        .arg("-t")
+        .arg(at.plus(target_dir))
+        .succeeds()
+        .no_stderr();
+
+    assert!(at.dir_exists(target_dir));
 }
 
 #[test]
@@ -1125,11 +1249,25 @@ fn test_install_backup_off() {
 #[test]
 fn test_install_missing_arguments() {
     let scene = TestScenario::new(util_name!());
+    let at = &scene.fixtures;
+
+    let no_target_dir = "no-target_dir";
 
     scene
         .ucmd()
         .fails()
-        .stderr_contains("install: missing file operand");
+        .code_is(1)
+        .stderr_contains("install: missing file operand")
+        .stderr_contains("install --help' for more information.");
+
+    scene
+        .ucmd()
+        .arg("-D")
+        .arg(format!("-t {}", no_target_dir))
+        .fails()
+        .stderr_contains("install: missing file operand")
+        .stderr_contains("install --help' for more information.");
+    assert!(!at.dir_exists(no_target_dir));
 }
 
 #[test]
@@ -1138,12 +1276,33 @@ fn test_install_missing_destination() {
     let at = &scene.fixtures;
 
     let file_1 = "source_file1";
+    let dir_1 = "source_dir1";
 
     at.touch(file_1);
-    scene.ucmd().arg(file_1).fails().stderr_contains(format!(
+    at.mkdir(dir_1);
-        "install: missing destination file operand after '{}'",
+
-        file_1
+    // will fail and also print some info on correct usage
-    ));
+    scene
+        .ucmd()
+        .arg(file_1)
+        .fails()
+        .stderr_contains(format!(
+            "install: missing destination file operand after '{}'",
+            file_1
+        ))
+        .stderr_contains("install --help' for more information.");
+
+    // GNU's install will check for correct num of arguments and then fail
+    // and it does not recognize, that the source is not a file but a directory.
+    scene
+        .ucmd()
+        .arg(dir_1)
+        .fails()
+        .stderr_contains(format!(
+            "install: missing destination file operand after '{}'",
+            dir_1
+        ))
+        .stderr_contains("install --help' for more information.");
 }
 
 #[test]