Fix mv bug: Should be able to stat files, but not able to mv if source and target are the same (#2763)

Closes #2760

src/uu/mv/src/mv.rs

@@ -13,6 +13,7 @@ extern crate uucore;
 
 use clap::{crate_version, App, Arg, ArgMatches};
 use std::env;
+use std::ffi::OsString;
 use std::fs;
 use std::io::{self, stdin};
 #[cfg(unix)]
@@ -30,9 +31,10 @@ pub struct Behavior {
     backup: BackupMode,
     suffix: String,
     update: bool,
-    target_dir: Option<String>,
+    target_dir: Option<OsString>,
     no_target_dir: bool,
     verbose: bool,
+    strip_slashes: bool,
 }
 
 #[derive(Clone, Eq, PartialEq)]
@@ -77,10 +79,11 @@ pub fn uumain(args: impl uucore::Args) -> i32 {
         .usage(&usage[..])
         .get_matches_from(args);
 
-    let files: Vec<String> = matches
+    let files: Vec<OsString> = matches
-        .values_of(ARG_FILES)
+        .values_of_os(ARG_FILES)
-        .map(|v| v.map(ToString::to_string).collect())
+        .unwrap_or_default()
-        .unwrap_or_default();
+        .map(|v| v.to_os_string())
+        .collect();
 
     let overwrite_mode = determine_overwrite_mode(&matches);
     let backup_mode = match backup_control::determine_backup_mode(&matches) {
@@ -103,26 +106,15 @@ pub fn uumain(args: impl uucore::Args) -> i32 {
         backup: backup_mode,
         suffix: backup_suffix,
         update: matches.is_present(OPT_UPDATE),
-        target_dir: matches.value_of(OPT_TARGET_DIRECTORY).map(String::from),
+        target_dir: matches
+            .value_of_os(OPT_TARGET_DIRECTORY)
+            .map(OsString::from),
         no_target_dir: matches.is_present(OPT_NO_TARGET_DIRECTORY),
         verbose: matches.is_present(OPT_VERBOSE),
+        strip_slashes: matches.is_present(OPT_STRIP_TRAILING_SLASHES),
     };
 
-    let paths: Vec<PathBuf> = {
+    exec(&files[..], behavior)
-        fn strip_slashes(p: &Path) -> &Path {
-            p.components().as_path()
-        }
-        let to_owned = |p: &Path| p.to_owned();
-        let paths = files.iter().map(Path::new);
-
-        if matches.is_present(OPT_STRIP_TRAILING_SLASHES) {
-            paths.map(strip_slashes).map(to_owned).collect()
-        } else {
-            paths.map(to_owned).collect()
-        }
-    };
-
-    exec(&paths[..], behavior)
 }
 
 pub fn uu_app() -> App<'static, 'static> {
@@ -210,15 +202,28 @@ fn determine_overwrite_mode(matches: &ArgMatches) -> OverwriteMode {
     }
 }
 
-fn exec(files: &[PathBuf], b: Behavior) -> i32 {
+fn exec(files: &[OsString], b: Behavior) -> i32 {
-    if let Some(ref name) = b.target_dir {
+    let paths: Vec<PathBuf> = {
-        return move_files_into_dir(files, &PathBuf::from(name), &b);
+        let paths = files.iter().map(Path::new);
+
+        // Strip slashes from path, if strip opt present
+        if b.strip_slashes {
+            paths
+                .map(|p| p.components().as_path().to_owned())
+                .collect::<Vec<PathBuf>>()
+        } else {
+            paths.map(|p| p.to_owned()).collect::<Vec<PathBuf>>()
         }
-    match files.len() {
+    };
+
+    if let Some(ref name) = b.target_dir {
+        return move_files_into_dir(&paths, &PathBuf::from(name), &b);
+    }
+    match paths.len() {
         /* case 0/1 are not possible thanks to clap */
         2 => {
-            let source = &files[0];
+            let source = &paths[0];
-            let target = &files[1];
+            let target = &paths[1];
             // Here we use the `symlink_metadata()` method instead of `exists()`,
             // since it handles dangling symlinks correctly. The method gives an
             // `Ok()` results unless the source does not exist, or the user
@@ -228,6 +233,24 @@ fn exec(files: &[PathBuf], b: Behavior) -> i32 {
                 return 1;
             }
 
+            // GNU semantics are: if the source and target are the same, no move occurs and we print an error
+            if source.eq(target) {
+                // Done to match GNU semantics for the dot file
+                if source.eq(Path::new(".")) || source.ends_with("/.") || source.is_file() {
+                    show_error!(
+                        "'{}' and '{}' are the same file",
+                        source.display(),
+                        target.display(),
+                    )
+                } else {
+                    show_error!(
+                        "cannot move '{s}' to a subdirectory of itself, '{s}/{s}'",
+                        s = source.display(),
+                    )
+                }
+                return 1;
+            }
+
             if target.is_dir() {
                 if b.no_target_dir {
                     if !source.is_dir() {
@@ -277,8 +300,8 @@ fn exec(files: &[PathBuf], b: Behavior) -> i32 {
                 );
                 return 1;
             }
-            let target_dir = files.last().unwrap();
+            let target_dir = paths.last().unwrap();
-            move_files_into_dir(&files[..files.len() - 1], target_dir, &b);
+            move_files_into_dir(&paths[..paths.len() - 1], target_dir, &b);
         }
     }
     0

tests/by-util/test_mv.rs

@@ -232,6 +232,40 @@ fn test_mv_force_replace_file() {
     assert!(at.file_exists(file_b));
 }
 
+#[test]
+fn test_mv_same_file() {
+    let (at, mut ucmd) = at_and_ucmd!();
+    let file_a = "test_mv_same_file_a";
+
+    at.touch(file_a);
+    ucmd.arg(file_a).arg(file_a).fails().stderr_is(format!(
+        "mv: '{f}' and '{f}' are the same file\n",
+        f = file_a,
+    ));
+}
+
+#[test]
+fn test_mv_same_file_not_dot_dir() {
+    let (at, mut ucmd) = at_and_ucmd!();
+    let dir = "test_mv_errors_dir";
+
+    at.mkdir(dir);
+    ucmd.arg(dir).arg(dir).fails().stderr_is(format!(
+        "mv: cannot move '{d}' to a subdirectory of itself, '{d}/{d}'",
+        d = dir,
+    ));
+}
+
+#[test]
+fn test_mv_same_file_dot_dir() {
+    let (_at, mut ucmd) = at_and_ucmd!();
+
+    ucmd.arg(".")
+        .arg(".")
+        .fails()
+        .stderr_is("mv: '.' and '.' are the same file\n".to_string());
+}
+
 #[test]
 fn test_mv_simple_backup() {
     let (at, mut ucmd) = at_and_ucmd!();