mv: improve move-to-self error handling

- improve move-to-self detection, so this errors without data loss:

```diff
 mkdir mydir
 mv mydir mydir/subdir
-mv: No such file or directory (os error 2)
+mv: cannot move 'mydir' to a subdirectory of itself, 'mydir/subdir'
```

- align "cannot move source to a subdirectory of itself" and "same file"
  errors more closely with coreutils:

```diff
 mkdir mydir
 mv mydir/ mydir/..
-mv: cannot move 'mydir/' to a subdirectory of itself, 'mydir/../mydir/'
+mv: 'mydir/' and 'mydir/../mydir' are the same file
```

Author: sgvictorino

src/uu/mv/src/error.rs

@@ -12,7 +12,6 @@ pub enum MvError {
     NoSuchFile(String),
     CannotStatNotADirectory(String),
     SameFile(String, String),
-    SelfSubdirectory(String),
     SelfTargetSubdirectory(String, String),
     DirectoryToNonDirectory(String),
     NonDirectoryToDirectory(String, String),
@@ -29,14 +28,9 @@ impl Display for MvError {
             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,
-                "cannot move '{s}' to a subdirectory of itself, '{s}/{s}'"
-            ),
-            Self::SelfTargetSubdirectory(s, t) => write!(
-                f,
-                "cannot move '{s}' to a subdirectory of itself, '{t}/{s}'"
-            ),
+            Self::SelfTargetSubdirectory(s, t) => {
+                write!(f, "cannot move {s} to a subdirectory of itself, {t}")
+            }
             Self::DirectoryToNonDirectory(t) => {
                 write!(f, "cannot overwrite directory {t} with non-directory")
             }

src/uu/mv/src/mv.rs

@@ -19,13 +19,13 @@ use std::io;
 use std::os::unix;
 #[cfg(windows)]
 use std::os::windows;
-use std::path::{Path, PathBuf};
+use std::path::{absolute, Path, PathBuf};
 use uucore::backup_control::{self, source_is_target_backup};
 use uucore::display::Quotable;
 use uucore::error::{set_exit_code, FromIo, UResult, USimpleError, UUsageError};
 use uucore::fs::{
-    are_hardlinks_or_one_way_symlink_to_same_file, are_hardlinks_to_same_file,
-    path_ends_with_terminator,
+    are_hardlinks_or_one_way_symlink_to_same_file, are_hardlinks_to_same_file, canonicalize,
+    path_ends_with_terminator, MissingHandling, ResolveMode,
 };
 #[cfg(all(unix, not(any(target_os = "macos", target_os = "redox"))))]
 use uucore::fsxattr;
@@ -322,20 +322,6 @@ fn handle_two_paths(source: &Path, target: &Path, opts: &Options) -> UResult<()>
         });
     }
 
-    if (source.eq(target)
-        || are_hardlinks_to_same_file(source, target)
-        || are_hardlinks_or_one_way_symlink_to_same_file(source, target))
-        && opts.backup == BackupMode::NoBackup
-    {
-        if source.eq(Path::new(".")) || source.ends_with("/.") || source.is_file() {
-            return Err(
-                MvError::SameFile(source.quote().to_string(), target.quote().to_string()).into(),
-            );
-        } else {
-            return Err(MvError::SelfSubdirectory(source.display().to_string()).into());
-        }
-    }
-
     let target_is_dir = target.is_dir();
     let source_is_dir = source.is_dir();
 
@@ -347,6 +333,8 @@ fn handle_two_paths(source: &Path, target: &Path, opts: &Options) -> UResult<()>
         return Err(MvError::FailedToAccessNotADirectory(target.quote().to_string()).into());
     }
 
+    assert_not_same_file(source, target, target_is_dir, opts)?;
+
     if target_is_dir {
         if opts.no_target_dir {
             if source.is_dir() {
@@ -356,14 +344,6 @@ fn handle_two_paths(source: &Path, target: &Path, opts: &Options) -> UResult<()>
             } else {
                 Err(MvError::DirectoryToNonDirectory(target.quote().to_string()).into())
             }
-        // Check that source & target do not contain same subdir/dir when both exist
-        // mkdir dir1/dir2; mv dir1 dir1/dir2
-        } else if target.starts_with(source) {
-            Err(MvError::SelfTargetSubdirectory(
-                source.display().to_string(),
-                target.display().to_string(),
-            )
-            .into())
         } else {
             move_files_into_dir(&[source.to_path_buf()], target, opts)
         }
@@ -387,6 +367,88 @@ fn handle_two_paths(source: &Path, target: &Path, opts: &Options) -> UResult<()>
     }
 }
 
+fn assert_not_same_file(
+    source: &Path,
+    target: &Path,
+    target_is_dir: bool,
+    opts: &Options,
+) -> UResult<()> {
+    // we'll compare canonicalized_source and canonicalized_target for same file detection
+    let canonicalized_source = match canonicalize(
+        absolute(source)?,
+        MissingHandling::Normal,
+        ResolveMode::Logical,
+    ) {
+        Ok(source) if source.exists() => source,
+        _ => absolute(source)?, // file or symlink target doesn't exist but its absolute path is still used for comparison
+    };
+
+    // special case if the target exists, is a directory, and the `-T` flag wasn't used
+    let target_is_dir = target_is_dir && !opts.no_target_dir;
+    let canonicalized_target = if target_is_dir {
+        // `mv source_file target_dir` => target_dir/source_file
+        // canonicalize the path that exists (target directory) and join the source file name
+        canonicalize(
+            absolute(target)?,
+            MissingHandling::Normal,
+            ResolveMode::Logical,
+        )?
+        .join(source.file_name().unwrap_or_default())
+    } else {
+        // `mv source target_dir/target` => target_dir/target
+        // we canonicalize target_dir and join /target
+        match absolute(target)?.parent() {
+            Some(parent) if parent.to_str() != Some("") => {
+                canonicalize(parent, MissingHandling::Normal, ResolveMode::Logical)?
+                    .join(target.file_name().unwrap_or_default())
+            }
+            // path.parent() returns Some("") or None if there's no parent
+            _ => absolute(target)?, // absolute paths should always have a parent, but we'll fall back just in case
+        }
+    };
+
+    let same_file = (canonicalized_source.eq(&canonicalized_target)
+        || are_hardlinks_to_same_file(source, target)
+        || are_hardlinks_or_one_way_symlink_to_same_file(source, target))
+        && opts.backup == BackupMode::NoBackup;
+
+    // get the expected target path to show in errors
+    // this is based on the argument and not canonicalized
+    let target_display = match source.file_name() {
+        Some(file_name) if target_is_dir => {
+            // join target_dir/source_file in a platform-independent manner
+            let mut path = target
+                .display()
+                .to_string()
+                .trim_end_matches("/")
+                .to_owned();
+
+            path.push('/');
+            path.push_str(&file_name.to_string_lossy());
+
+            path.quote().to_string()
+        }
+        _ => target.quote().to_string(),
+    };
+
+    if same_file
+        && (canonicalized_source.eq(&canonicalized_target)
+            || source.eq(Path::new("."))
+            || source.ends_with("/.")
+            || source.is_file())
+    {
+        return Err(MvError::SameFile(source.quote().to_string(), target_display).into());
+    } else if (same_file || canonicalized_target.starts_with(canonicalized_source))
+        // don't error if we're moving a symlink of a directory into itself
+        && !source.is_symlink()
+    {
+        return Err(
+            MvError::SelfTargetSubdirectory(source.quote().to_string(), target_display).into(),
+        );
+    }
+    Ok(())
+}
+
 fn handle_multiple_paths(paths: &[PathBuf], opts: &Options) -> UResult<()> {
     if opts.no_target_dir {
         return Err(UUsageError::new(
@@ -425,10 +487,6 @@ fn move_files_into_dir(files: &[PathBuf], target_dir: &Path, options: &Options)
         return Err(MvError::NotADirectory(target_dir.quote().to_string()).into());
     }
 
-    let canonicalized_target_dir = target_dir
-        .canonicalize()
-        .unwrap_or_else(|_| target_dir.to_path_buf());
-
     let multi_progress = options.progress_bar.then(MultiProgress::new);
 
     let count_progress = if let Some(ref multi_progress) = multi_progress {
@@ -479,24 +537,9 @@ fn move_files_into_dir(files: &[PathBuf], target_dir: &Path, options: &Options)
 
         // Check if we have mv dir1 dir2 dir2
         // And generate an error if this is the case
-        if let Ok(canonicalized_source) = sourcepath.canonicalize() {
-            if canonicalized_source == canonicalized_target_dir {
-                // User tried to move directory to itself, warning is shown
-                // and process of moving files is continued.
-                show!(USimpleError::new(
-                    1,
-                    format!(
-                        "cannot move '{}' to a subdirectory of itself, '{}/{}'",
-                        sourcepath.display(),
-                        uucore::fs::normalize_path(target_dir).display(),
-                        canonicalized_target_dir.components().last().map_or_else(
-                            || target_dir.display().to_string(),
-                            |dir| { PathBuf::from(dir.as_os_str()).display().to_string() }
-                        )
-                    )
-                ));
-                continue;
-            }
+        if let Err(e) = assert_not_same_file(sourcepath, target_dir, true, options) {
+            show!(e);
+            continue;
         }
 
         match rename(sourcepath, &targetpath, options, multi_progress.as_ref()) {

tests/by-util/test_mv.rs

@@ -6,6 +6,7 @@
 // spell-checker:ignore mydir
 use crate::common::util::TestScenario;
 use filetime::FileTime;
+use rstest::rstest;
 use std::io::Write;
 
 #[test]
@@ -467,7 +468,31 @@ fn test_mv_same_symlink() {
         .arg(file_c)
         .arg(file_a)
         .fails()
-        .stderr_is(format!("mv: '{file_c}' and '{file_a}' are the same file\n",));
+        .stderr_is(format!("mv: '{file_c}' and '{file_a}' are the same file\n"));
+}
+
+#[test]
+#[cfg(all(unix, not(target_os = "android")))]
+fn test_mv_same_broken_symlink() {
+    let (at, mut ucmd) = at_and_ucmd!();
+
+    at.symlink_file("missing-target", "broken");
+
+    ucmd.arg("broken")
+        .arg("broken")
+        .fails()
+        .stderr_is("mv: 'broken' and 'broken' are the same file\n");
+}
+
+#[test]
+#[cfg(all(unix, not(target_os = "android")))]
+fn test_mv_symlink_into_target() {
+    let (at, mut ucmd) = at_and_ucmd!();
+
+    at.mkdir("dir");
+    at.symlink_file("dir", "dir-link");
+
+    ucmd.arg("dir-link").arg("dir").succeeds();
 }
 
 #[test]
@@ -1389,24 +1414,6 @@ fn test_mv_interactive_error() {
         .is_empty());
 }
 
-#[test]
-fn test_mv_into_self() {
-    let scene = TestScenario::new(util_name!());
-    let at = &scene.fixtures;
-    let dir1 = "dir1";
-    let dir2 = "dir2";
-    at.mkdir(dir1);
-    at.mkdir(dir2);
-
-    scene
-        .ucmd()
-        .arg(dir1)
-        .arg(dir2)
-        .arg(dir2)
-        .fails()
-        .stderr_contains("mv: cannot move 'dir2' to a subdirectory of itself, 'dir2/dir2'");
-}
-
 #[test]
 fn test_mv_arg_interactive_skipped() {
     let (at, mut ucmd) = at_and_ucmd!();
@@ -1456,27 +1463,32 @@ fn test_mv_into_self_data() {
     assert!(!at.file_exists(file1));
 }
 
-#[test]
-fn test_mv_directory_into_subdirectory_of_itself_fails() {
+#[rstest]
+#[case(vec!["mydir"], vec!["mydir", "mydir"], "mv: cannot move 'mydir' to a subdirectory of itself, 'mydir/mydir'")]
+#[case(vec!["mydir"], vec!["mydir/", "mydir/"], "mv: cannot move 'mydir/' to a subdirectory of itself, 'mydir/mydir'")]
+#[case(vec!["mydir"], vec!["./mydir", "mydir", "mydir/"], "mv: cannot move './mydir' to a subdirectory of itself, 'mydir/mydir'")]
+#[case(vec!["mydir"], vec!["mydir/", "mydir/mydir_2/"], "mv: cannot move 'mydir/' to a subdirectory of itself, 'mydir/mydir_2/'")]
+#[case(vec!["mydir/mydir_2"], vec!["mydir", "mydir/mydir_2"], "mv: cannot move 'mydir' to a subdirectory of itself, 'mydir/mydir_2/mydir'\n")]
+#[case(vec!["mydir/mydir_2"], vec!["mydir/", "mydir/mydir_2/"], "mv: cannot move 'mydir/' to a subdirectory of itself, 'mydir/mydir_2/mydir'\n")]
+#[case(vec!["mydir", "mydir_2"], vec!["mydir/", "mydir_2/", "mydir_2/"], "mv: cannot move 'mydir_2/' to a subdirectory of itself, 'mydir_2/mydir_2'")]
+#[case(vec!["mydir"], vec!["mydir/", "mydir"], "mv: cannot move 'mydir/' to a subdirectory of itself, 'mydir/mydir'")]
+#[case(vec!["mydir"], vec!["-T", "mydir", "mydir"], "mv: 'mydir' and 'mydir' are the same file")]
+#[case(vec!["mydir"], vec!["mydir/", "mydir/../"], "mv: 'mydir/' and 'mydir/../mydir' are the same file")]
+fn test_mv_directory_self(
+    #[case] dirs: Vec<&str>,
+    #[case] args: Vec<&str>,
+    #[case] expected_error: &str,
+) {
     let scene = TestScenario::new(util_name!());
     let at = &scene.fixtures;
-    let dir1 = "mydir";
-    let dir2 = "mydir/mydir_2";
-    at.mkdir(dir1);
-    at.mkdir(dir2);
-    scene.ucmd().arg(dir1).arg(dir2).fails().stderr_contains(
-        "mv: cannot move 'mydir' to a subdirectory of itself, 'mydir/mydir_2/mydir'",
-    );
-
-    // check that it also errors out with /
+    for dir in dirs {
+        at.mkdir_all(dir);
+    }
     scene
         .ucmd()
-        .arg(format!("{dir1}/"))
-        .arg(dir2)
+        .args(&args)
         .fails()
-        .stderr_contains(
-            "mv: cannot move 'mydir/' to a subdirectory of itself, 'mydir/mydir_2/mydir/'",
-        );
+        .stderr_contains(expected_error);
 }
 
 #[test]
@@ -1755,23 +1767,3 @@ fn test_mv_error_msg_with_multiple_sources_that_does_not_exist() {
         .stderr_contains("mv: cannot stat 'a': No such file or directory")
         .stderr_contains("mv: cannot stat 'b/': No such file or directory");
 }
-
-#[test]
-fn test_mv_error_cant_move_itself() {
-    let scene = TestScenario::new(util_name!());
-    let at = &scene.fixtures;
-    at.mkdir("b");
-    scene
-        .ucmd()
-        .arg("b")
-        .arg("b/")
-        .fails()
-        .stderr_contains("mv: cannot move 'b' to a subdirectory of itself, 'b/b'");
-    scene
-        .ucmd()
-        .arg("./b")
-        .arg("b")
-        .arg("b/")
-        .fails()
-        .stderr_contains("mv: cannot move 'b' to a subdirectory of itself, 'b/b'");
-}