diff --git a/src/mv/mv.rs b/src/mv/mv.rs index 19746c95679..b6e6b6870a3 100644 --- a/src/mv/mv.rs +++ b/src/mv/mv.rs @@ -257,7 +257,10 @@ 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, @@ -265,6 +268,12 @@ fn exec(files: &[PathBuf], b: Behaviour) -> i32 { } 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; } + } +} diff --git a/tests/test_mv.rs b/tests/test_mv.rs index 870524b27db..cccaa7e5647 100644 --- a/tests/test_mv.rs +++ b/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 diff --git a/tests/tests.rs b/tests/tests.rs index 5885d8f9ba4..942a00ece98 100644 --- a/tests/tests.rs +++ b/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;