From 6f75331bf49fe57c2312d6a3c16883f03a9b615c Mon Sep 17 00:00:00 2001 From: Roy Ivy III Date: Sat, 22 Sep 2018 23:29:18 -0500 Subject: [PATCH 1/6] mv: expand testing to windows platforms --- tests/tests.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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; From e00d586af1b02d76c8060a8a51fc63f85d26280e Mon Sep 17 00:00:00 2001 From: Roy Ivy III Date: Sat, 22 Sep 2018 23:30:24 -0500 Subject: [PATCH 2/6] mv: fix failing tests --- src/mv/mv.rs | 27 +++++++++++++++++++++++++++ tests/test_mv.rs | 3 +-- 2 files changed, 28 insertions(+), 2 deletions(-) diff --git a/src/mv/mv.rs b/src/mv/mv.rs index 1b1ee68db60..6cda48914ef 100644 --- a/src/mv/mv.rs +++ b/src/mv/mv.rs @@ -265,6 +265,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) { @@ -360,6 +366,18 @@ fn rename(from: &PathBuf, to: &PathBuf, b: &Behaviour) -> Result<()> { } } + // "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) { + try!(fs::remove_dir(to)) + } else { + return Err(std::io::Error::new(std::io::ErrorKind::Other, "Directory not empty")); + } + } + } + try!(fs::rename(from, to)); if b.verbose { @@ -405,3 +423,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..1cf56ce0b64 100644 --- a/tests/test_mv.rs +++ b/tests/test_mv.rs @@ -368,8 +368,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 From 556a96406c20540209d674d6c240cca88e43c78e Mon Sep 17 00:00:00 2001 From: Roy Ivy III Date: Sat, 22 Sep 2018 23:31:54 -0500 Subject: [PATCH 3/6] mv: improve error message --- src/mv/mv.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/mv/mv.rs b/src/mv/mv.rs index 6cda48914ef..bf06e89c4c4 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, From 31ca885c9cf7ac896216da1b7a2e9ad4fb82a39d Mon Sep 17 00:00:00 2001 From: Roy Ivy III Date: Sat, 22 Sep 2018 23:34:28 -0500 Subject: [PATCH 4/6] mv: add test for "`mv` fails transfers between dirs" --- tests/test_mv.rs | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/tests/test_mv.rs b/tests/test_mv.rs index 1cf56ce0b64..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!()); From fb1d844e147861deca0066ec4c4d3203fe2f984e Mon Sep 17 00:00:00 2001 From: Roy Ivy III Date: Sat, 22 Sep 2018 23:35:22 -0500 Subject: [PATCH 5/6] mv: fix "`mv` fails transfers between dirs" --- src/mv/mv.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/mv/mv.rs b/src/mv/mv.rs index bf06e89c4c4..d21d59bb786 100644 --- a/src/mv/mv.rs +++ b/src/mv/mv.rs @@ -306,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!( From 52c7f0aa34a7d4c8902bf007081046b67330b020 Mon Sep 17 00:00:00 2001 From: Roy Ivy III Date: Sat, 9 Feb 2019 13:23:43 -0600 Subject: [PATCH 6/6] mv: refactor try! to ? --- src/mv/mv.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/mv/mv.rs b/src/mv/mv.rs index d21d59bb786..5625a48fa2a 100644 --- a/src/mv/mv.rs +++ b/src/mv/mv.rs @@ -358,11 +358,11 @@ 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(()); } @@ -374,14 +374,14 @@ fn rename(from: &PathBuf, to: &PathBuf, b: &Behaviour) -> Result<()> { // normalize behavior between *nix and windows if from.is_dir() { if is_empty_dir(to) { - try!(fs::remove_dir(to)) + fs::remove_dir(to)? } else { return Err(std::io::Error::new(std::io::ErrorKind::Other, "Directory not empty")); } } } - try!(fs::rename(from, to)); + fs::rename(from, to)?; if b.verbose { print!("‘{}’ -> ‘{}’", from.display(), to.display());