From 991fcc548cce95318c73629ecbf710deecad4589 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ya=C4=9F=C4=B1z=20can=20De=C4=9Firmenci?= Date: Mon, 24 May 2021 21:07:45 +0300 Subject: [PATCH 1/2] fix: log error messages properly on permission errors --- src/uu/mv/src/mv.rs | 33 +++++++++++++++++++++++++-------- tests/by-util/test_mv.rs | 18 ++++++++++++++++++ 2 files changed, 43 insertions(+), 8 deletions(-) diff --git a/src/uu/mv/src/mv.rs b/src/uu/mv/src/mv.rs index f57178a09..95b2fd423 100644 --- a/src/uu/mv/src/mv.rs +++ b/src/uu/mv/src/mv.rs @@ -291,12 +291,22 @@ fn exec(files: &[PathBuf], b: Behavior) -> i32 { return match rename(source, target, &b) { Err(e) => { - show_error!( - "cannot move ‘{}’ to ‘{}’: {}", - source.display(), - target.display(), - e - ); + let error_as_str = e.to_string(); + let is_perm_denied = error_as_str.contains("Permission denied"); + match e.kind() { + _ => { + show_error!( + "cannot move ‘{}’ to ‘{}’: {}", + source.display(), + target.display(), + if is_perm_denied { + "Permission denied".to_string() + } else { + e.to_string() + } + ); + } + } 1 } _ => 0, @@ -357,15 +367,22 @@ fn move_files_into_dir(files: &[PathBuf], target_dir: &Path, b: &Behavior) -> i3 }; if let Err(e) = rename(sourcepath, &targetpath, b) { + let error_as_str = e.to_string(); + let is_perm_denied = error_as_str.contains("Permission denied"); show_error!( - "mv: cannot move ‘{}’ to ‘{}’: {}", + "cannot move ‘{}’ to ‘{}’: {}", sourcepath.display(), targetpath.display(), - e + if is_perm_denied { + "Permission denied".to_string() + } else { + e.to_string() + } ); all_successful = false; } } + if all_successful { 0 } else { diff --git a/tests/by-util/test_mv.rs b/tests/by-util/test_mv.rs index e8ba43282..47532e2e5 100644 --- a/tests/by-util/test_mv.rs +++ b/tests/by-util/test_mv.rs @@ -587,6 +587,24 @@ fn test_mv_verbose() { )); } +#[test] +fn test_mv_permission_error() { + let scene = TestScenario::new("mkdir"); + let folder1 = "bar"; + let folder2 = "foo"; + let folder_to_move = "bar/foo"; + scene.ucmd().arg("-m444").arg(folder1).succeeds(); + scene.ucmd().arg("-m777").arg(folder2).succeeds(); + + scene + .cmd_keepenv(util_name!()) + .arg(folder2) + .arg(folder_to_move) + .run() + .stderr_str() + .ends_with("Permission denied"); +} + // Todo: // $ at.touch a b From e5e7ca8dc5cc4f4a2cdb9cdc9d24bb24d24b3fb1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ya=C4=9F=C4=B1z=20can=20De=C4=9Firmenci?= Date: Mon, 24 May 2021 21:20:59 +0300 Subject: [PATCH 2/2] fix: simplify logic --- src/uu/mv/src/mv.rs | 38 ++++++++++++++------------------------ 1 file changed, 14 insertions(+), 24 deletions(-) diff --git a/src/uu/mv/src/mv.rs b/src/uu/mv/src/mv.rs index 95b2fd423..a0ff1bcc6 100644 --- a/src/uu/mv/src/mv.rs +++ b/src/uu/mv/src/mv.rs @@ -291,22 +291,12 @@ fn exec(files: &[PathBuf], b: Behavior) -> i32 { return match rename(source, target, &b) { Err(e) => { - let error_as_str = e.to_string(); - let is_perm_denied = error_as_str.contains("Permission denied"); - match e.kind() { - _ => { - show_error!( - "cannot move ‘{}’ to ‘{}’: {}", - source.display(), - target.display(), - if is_perm_denied { - "Permission denied".to_string() - } else { - e.to_string() - } - ); - } - } + show_error!( + "cannot move ‘{}’ to ‘{}’: {}", + source.display(), + target.display(), + e.to_string() + ); 1 } _ => 0, @@ -367,17 +357,11 @@ fn move_files_into_dir(files: &[PathBuf], target_dir: &Path, b: &Behavior) -> i3 }; if let Err(e) = rename(sourcepath, &targetpath, b) { - let error_as_str = e.to_string(); - let is_perm_denied = error_as_str.contains("Permission denied"); show_error!( "cannot move ‘{}’ to ‘{}’: {}", sourcepath.display(), targetpath.display(), - if is_perm_denied { - "Permission denied".to_string() - } else { - e.to_string() - } + e.to_string() ); all_successful = false; } @@ -469,7 +453,13 @@ fn rename_with_fallback(from: &Path, to: &Path) -> io::Result<()> { ..DirCopyOptions::new() }; if let Err(err) = move_dir(from, to, &options) { - return Err(io::Error::new(io::ErrorKind::Other, format!("{:?}", err))); + return match err.kind { + fs_extra::error::ErrorKind::PermissionDenied => Err(io::Error::new( + io::ErrorKind::PermissionDenied, + "Permission denied", + )), + _ => Err(io::Error::new(io::ErrorKind::Other, format!("{:?}", err))), + }; } } else { fs::copy(from, to).and_then(|_| fs::remove_file(from))?;