Merge pull request #1305 from rivy/fix.mv

fix "`mv` fails transfers between dirs"

src/mv/mv.rs

@@ -257,14 +257,23 @@ fn exec(files: &[PathBuf], b: Behaviour) -> i32 {
 
                     return match rename(source, target, &b) {
                         Err(e) => {
-                            show_error!("{}", e);
+                            show_error!(
+                                "cannot move ‘{}’ to ‘{}’: {}",
+                                source.display(), target.display(), e
+                            );
                             1
                         }
                         _ => 0,
                     };
                 }
 
                 return move_files_into_dir(&[source.clone()], target, &b);
+            } else if target.exists() && source.is_dir() {
+                show_error!(
+                    "cannot overwrite non-directory ‘{}’ with directory ‘{}’",
+                    target.display(), source.display()
+                );
+                return 1;
             }
 
             if let Err(e) = rename(source, target, &b) {
@@ -297,7 +306,7 @@ fn move_files_into_dir(files: &[PathBuf], target_dir: &PathBuf, b: &Behaviour) -
 
     let mut all_successful = true;
     for sourcepath in files.iter() {
-        let targetpath = match sourcepath.as_os_str().to_str() {
+        let targetpath = match sourcepath.file_name() {
             Some(name) => target_dir.join(name),
             None => {
                 show_error!(
@@ -349,18 +358,30 @@ fn rename(from: &PathBuf, to: &PathBuf, b: &Behaviour) -> Result<()> {
             BackupMode::ExistingBackup => Some(existing_backup_path(to, &b.suffix)),
         };
         if let Some(ref p) = backup_path {
-            try!(fs::rename(to, p));
+            fs::rename(to, p)?;
         }
 
         if b.update {
-            if try!(try!(fs::metadata(from)).modified()) <= try!(try!(fs::metadata(to)).modified())
+            if fs::metadata(from)?.modified()? <= fs::metadata(to)?.modified()?
             {
                 return Ok(());
             }
         }
     }
 
-    try!(fs::rename(from, to));
+    // "to" may no longer exist if it was backed up
+    if to.exists() && to.is_dir() {
+        // normalize behavior between *nix and windows
+        if from.is_dir() {
+            if is_empty_dir(to) {
+                fs::remove_dir(to)?
+            } else {
+                return Err(std::io::Error::new(std::io::ErrorKind::Other, "Directory not empty"));
+            }
+        }
+    }
+
+    fs::rename(from, to)?;
 
     if b.verbose {
         print!("‘{}’ -> ‘{}’", from.display(), to.display());
@@ -405,3 +426,12 @@ fn existing_backup_path(path: &PathBuf, suffix: &str) -> PathBuf {
         simple_backup_path(path, suffix)
     }
 }
+
+fn is_empty_dir(path: &PathBuf) -> bool {
+    match fs::read_dir(path) {
+        Ok(contents) => {
+            return contents.peekable().peek().is_none();
+        },
+        Err(_e) => { return false; }
+    }
+}

tests/test_mv.rs

@@ -44,6 +44,25 @@ fn test_mv_move_file_into_dir() {
     assert!(at.file_exists(&format!("{}/{}", dir, file)));
 }
 
+#[test]
+fn test_mv_move_file_between_dirs() {
+    let (at, mut ucmd) = at_and_ucmd!();
+    let dir1 = "test_mv_move_file_between_dirs_dir1";
+    let dir2 = "test_mv_move_file_between_dirs_dir2";
+    let file = "test_mv_move_file_between_dirs_file";
+
+    at.mkdir(dir1);
+    at.mkdir(dir2);
+    at.touch(&format!("{}/{}", dir1, file));
+
+    assert!(at.file_exists(&format!("{}/{}", dir1, file)));
+
+    ucmd.arg(&format!("{}/{}", dir1, file)).arg(dir2).succeeds().no_stderr();
+
+    assert!(!at.file_exists(&format!("{}/{}", dir1, file)));
+    assert!(at.file_exists(&format!("{}/{}", dir2, file)));
+}
+
 #[test]
 fn test_mv_strip_slashes() {
     let scene = TestScenario::new(util_name!());
@@ -368,8 +387,7 @@ fn test_mv_errors() {
     // $ mv -T -t a b
     // mv: cannot combine --target-directory (-t) and --no-target-directory (-T)
     scene.ucmd().arg("-T").arg("-t").arg(dir).arg(file_a).arg(file_b).fails()
-    .stderr_is("mv: error: cannot combine --target-directory (-t) and --no-target-directory \
-                (-T)\n");
+    .stderr_is("mv: error: cannot combine --target-directory (-t) and --no-target-directory (-T)\n");
 
     // $ at.touch file && at.mkdir dir
     // $ mv -T file dir

tests/tests.rs

@@ -23,7 +23,6 @@ unix_only! {
     "chown", test_chown;
     "chgrp", test_chgrp;
     "install", test_install;
-    "mv", test_mv;
     "pathchk", test_pathchk;
     "pinky", test_pinky;
     "stdbuf", test_stdbuf;
@@ -68,6 +67,7 @@ generic! {
     "ls", test_ls;
     "mkdir", test_mkdir;
     "mktemp", test_mktemp;
+    "mv", test_mv;
     "numfmt", test_numfmt;
     "nl", test_nl;
     "od", test_od;