From 5c100dd088eee3d5f990c651fb95753a8c318ee1 Mon Sep 17 00:00:00 2001
From: Mick van Gelderen <mickvangelderen@gmail.com>
Date: Sat, 28 Oct 2023 15:04:51 +0200
Subject: [PATCH] mv: Fix stderr output mv file into dir and dir into file
 where both are files (#5464)

* Add tests mv file into dir and dir into file where both are files

* Fix test_mv_dir_into_file_where_both_are_files

* Fix test_mv_file_into_dir_where_both_are_files

* Store String in error instead of PathBuf

* Implement path_ends_with_terminator for windows

* Fix compilation on windows
---
 src/uu/mv/src/error.rs   |  7 +++++++
 src/uu/mv/src/mv.rs      | 33 +++++++++++++++++++++++++++++++--
 tests/by-util/test_mv.rs | 29 +++++++++++++++++++++++++++++
 3 files changed, 67 insertions(+), 2 deletions(-)

diff --git a/src/uu/mv/src/error.rs b/src/uu/mv/src/error.rs
index e891fc2ec..f989d4e13 100644
--- a/src/uu/mv/src/error.rs
+++ b/src/uu/mv/src/error.rs
@@ -10,6 +10,7 @@ use uucore::error::UError;
 #[derive(Debug)]
 pub enum MvError {
     NoSuchFile(String),
+    CannotStatNotADirectory(String),
     SameFile(String, String),
     SelfSubdirectory(String),
     SelfTargetSubdirectory(String, String),
@@ -17,6 +18,7 @@ pub enum MvError {
     NonDirectoryToDirectory(String, String),
     NotADirectory(String),
     TargetNotADirectory(String),
+    FailedToAccessNotADirectory(String),
 }
 
 impl Error for MvError {}
@@ -25,6 +27,7 @@ impl Display for MvError {
     fn fmt(&self, f: &mut Formatter) -> Result {
         match self {
             Self::NoSuchFile(s) => write!(f, "cannot stat {s}: No such file or directory"),
+            Self::CannotStatNotADirectory(s) => write!(f, "cannot stat {s}: Not a directory"),
             Self::SameFile(s, t) => write!(f, "{s} and {t} are the same file"),
             Self::SelfSubdirectory(s) => write!(
                 f,
@@ -42,6 +45,10 @@ impl Display for MvError {
             }
             Self::NotADirectory(t) => write!(f, "target {t}: Not a directory"),
             Self::TargetNotADirectory(t) => write!(f, "target directory {t}: Not a directory"),
+
+            Self::FailedToAccessNotADirectory(t) => {
+                write!(f, "failed to access {t}: Not a directory")
+            }
         }
     }
 }
diff --git a/src/uu/mv/src/mv.rs b/src/uu/mv/src/mv.rs
index 5c52fef26..0ceda8e75 100644
--- a/src/uu/mv/src/mv.rs
+++ b/src/uu/mv/src/mv.rs
@@ -103,6 +103,25 @@ static OPT_VERBOSE: &str = "verbose";
 static OPT_PROGRESS: &str = "progress";
 static ARG_FILES: &str = "files";
 
+/// Returns true if the passed `path` ends with a path terminator.
+#[cfg(unix)]
+fn path_ends_with_terminator(path: &Path) -> bool {
+    use std::os::unix::prelude::OsStrExt;
+    path.as_os_str()
+        .as_bytes()
+        .last()
+        .map_or(false, |&byte| byte == b'/' || byte == b'\\')
+}
+
+#[cfg(windows)]
+fn path_ends_with_terminator(path: &Path) -> bool {
+    use std::os::windows::prelude::OsStrExt;
+    path.as_os_str()
+        .encode_wide()
+        .last()
+        .map_or(false, |wide| wide == b'/'.into() || wide == b'\\'.into())
+}
+
 #[uucore::main]
 pub fn uumain(args: impl uucore::Args) -> UResult<()> {
     let mut app = uu_app();
@@ -299,7 +318,11 @@ fn handle_two_paths(source: &Path, target: &Path, opts: &Options) -> UResult<()>
         .into());
     }
     if source.symlink_metadata().is_err() {
-        return Err(MvError::NoSuchFile(source.quote().to_string()).into());
+        return Err(if path_ends_with_terminator(source) {
+            MvError::CannotStatNotADirectory(source.quote().to_string()).into()
+        } else {
+            MvError::NoSuchFile(source.quote().to_string()).into()
+        });
     }
 
     if (source.eq(target)
@@ -316,7 +339,13 @@ fn handle_two_paths(source: &Path, target: &Path, opts: &Options) -> UResult<()>
         }
     }
 
-    if target.is_dir() {
+    let target_is_dir = target.is_dir();
+
+    if path_ends_with_terminator(target) && !target_is_dir {
+        return Err(MvError::FailedToAccessNotADirectory(target.quote().to_string()).into());
+    }
+
+    if target_is_dir {
         if opts.no_target_dir {
             if source.is_dir() {
                 rename(source, target, opts, None).map_err_context(|| {
diff --git a/tests/by-util/test_mv.rs b/tests/by-util/test_mv.rs
index e88667320..c54d24ea9 100644
--- a/tests/by-util/test_mv.rs
+++ b/tests/by-util/test_mv.rs
@@ -1414,6 +1414,35 @@ fn test_mv_directory_into_subdirectory_of_itself_fails() {
             "mv: cannot move 'mydir/' to a subdirectory of itself, 'mydir/mydir_2/mydir/'",
         );
 }
+
+#[test]
+fn test_mv_file_into_dir_where_both_are_files() {
+    let scene = TestScenario::new(util_name!());
+    let at = &scene.fixtures;
+    at.touch("a");
+    at.touch("b");
+    scene
+        .ucmd()
+        .arg("a")
+        .arg("b/")
+        .fails()
+        .stderr_contains("mv: failed to access 'b/': Not a directory");
+}
+
+#[test]
+fn test_mv_dir_into_file_where_both_are_files() {
+    let scene = TestScenario::new(util_name!());
+    let at = &scene.fixtures;
+    at.touch("a");
+    at.touch("b");
+    scene
+        .ucmd()
+        .arg("a/")
+        .arg("b")
+        .fails()
+        .stderr_contains("mv: cannot stat 'a/': Not a directory");
+}
+
 // Todo:
 
 // $ at.touch a b