mv: revert error message to GNU default when overwriting non-empty directory

Author: aaron-ang

src/uu/mv/locales/en-US.ftl

@@ -32,6 +32,7 @@ mv-error-backup-might-destroy-source = backing up {$target} might destroy source
 mv-error-will-not-overwrite-just-created = will not overwrite just-created {$target} with {$source}
 mv-error-not-replacing = not replacing {$target}
 mv-error-cannot-move = cannot move {$source} to {$target}
+mv-error-cannot-overwrite-non-empty-directory = cannot overwrite {$target}
 mv-error-directory-not-empty = Directory not empty
 mv-error-dangling-symlink = can't determine symlink type, since it is dangling
 mv-error-no-symlink-support = your operating system does not support symlinks

src/uu/mv/locales/fr-FR.ftl

@@ -32,6 +32,7 @@ mv-error-backup-might-destroy-source = sauvegarder {$target} pourrait détruire
 mv-error-will-not-overwrite-just-created = ne va pas écraser le fichier qui vient d'être créé {$target} avec {$source}
 mv-error-not-replacing = ne remplace pas {$target}
 mv-error-cannot-move = impossible de déplacer {$source} vers {$target}
+mv-error-cannot-overwrite-non-empty-directory = impossible d'écraser {$target}
 mv-error-directory-not-empty = Répertoire non vide
 mv-error-dangling-symlink = impossible de déterminer le type de lien symbolique, car il est suspendu
 mv-error-no-symlink-support = votre système d'exploitation ne prend pas en charge les liens symboliques

src/uu/mv/src/mv.rs

@@ -36,7 +36,7 @@ use crate::hardlink::{
 };
 use uucore::backup_control::{self, source_is_target_backup};
 use uucore::display::Quotable;
-use uucore::error::{FromIo, UResult, USimpleError, UUsageError, set_exit_code};
+use uucore::error::{FromIo, UError, UResult, USimpleError, UUsageError, set_exit_code};
 #[cfg(unix)]
 use uucore::fs::display_permissions_unix;
 #[cfg(unix)]
@@ -415,8 +415,20 @@ fn handle_two_paths(source: &Path, target: &Path, opts: &Options) -> UResult<()>
                     hardlink_params.0,
                     hardlink_params.1,
                 )
-                .map_err_context(|| {
-                    translate!("mv-error-cannot-move", "source" => source.quote(), "target" => target.quote())
+                .map_err(|e| -> Box<dyn UError> {
+                    let message = if is_directory_not_empty_error(&e) {
+                        translate!(
+                            "mv-error-cannot-overwrite-non-empty-directory",
+                            "target" => target.quote()
+                        )
+                    } else {
+                        translate!(
+                            "mv-error-cannot-move",
+                            "source" => source.quote(),
+                            "target" => target.quote()
+                        )
+                    };
+                    e.map_err_context(|| message)
                 })
             } else {
                 Err(MvError::DirectoryToNonDirectory(target.quote().to_string()).into())
@@ -680,20 +692,31 @@ fn move_files_into_dir(files: &[PathBuf], target_dir: &Path, options: &Options)
         ) {
             Err(e) if e.to_string().is_empty() => set_exit_code(1),
             Err(e) => {
-                let e = e.map_err_context(|| {
-                    translate!("mv-error-cannot-move", "source" => sourcepath.quote(), "target" => targetpath.quote())
-                });
+                let message = if is_directory_not_empty_error(&e) {
+                    translate!(
+                        "mv-error-cannot-overwrite-non-empty-directory",
+                        "target" => targetpath.quote()
+                    )
+                } else {
+                    translate!(
+                        "mv-error-cannot-move",
+                        "source" => sourcepath.quote(),
+                        "target" => targetpath.quote()
+                    )
+                };
+                let e = e.map_err_context(|| message);
                 match display_manager {
                     Some(ref pb) => pb.suspend(|| show!(e)),
                     None => show!(e),
                 }
             }
-            Ok(()) => (),
+            Ok(()) => {
+                moved_destinations.insert(targetpath.clone());
+            }
         }
         if let Some(ref pb) = count_progress {
             pb.inc(1);
         }
-        moved_destinations.insert(targetpath.clone());
     }
     Ok(())
 }
@@ -761,7 +784,10 @@ fn rename(
             if is_empty_dir(to) {
                 fs::remove_dir(to)?;
             } else {
-                return Err(io::Error::other(translate!("mv-error-directory-not-empty")));
+                return Err(io::Error::new(
+                    io::ErrorKind::DirectoryNotEmpty,
+                    translate!("mv-error-directory-not-empty"),
+                ));
             }
         }
     }
@@ -806,6 +832,10 @@ fn rename(
     Ok(())
 }
 
+fn is_directory_not_empty_error(err: &io::Error) -> bool {
+    err.kind() == io::ErrorKind::DirectoryNotEmpty
+}
+
 #[cfg(unix)]
 fn is_fifo(filetype: fs::FileType) -> bool {
     filetype.is_fifo()
@@ -937,10 +967,7 @@ fn rename_symlink_fallback(from: &Path, to: &Path) -> io::Result<()> {
 #[cfg(not(any(windows, unix)))]
 fn rename_symlink_fallback(from: &Path, to: &Path) -> io::Result<()> {
     let path_symlink_points_to = fs::read_link(from)?;
-    Err(io::Error::new(
-        io::ErrorKind::Other,
-        translate!("mv-error-no-symlink-support"),
-    ))
+    Err(io::Error::other(translate!("mv-error-no-symlink-support")))
 }
 
 fn rename_dir_fallback(

tests/by-util/test_mv.rs

@@ -119,7 +119,7 @@ fn test_mv_move_file_into_file_with_target_arg() {
         .arg(file1)
         .arg(file2)
         .fails()
-        .stderr_is(format!("mv: target directory '{file1}': Not a directory\n"));
+        .stderr_only(format!("mv: target directory '{file1}': Not a directory\n"));
 
     assert!(at.file_exists(file1));
 }
@@ -139,7 +139,7 @@ fn test_mv_move_multiple_files_into_file() {
         .arg(file2)
         .arg(file3)
         .fails()
-        .stderr_is(format!("mv: target '{file3}': Not a directory\n"));
+        .stderr_only(format!("mv: target '{file3}': Not a directory\n"));
 
     assert!(at.file_exists(file1));
     assert!(at.file_exists(file2));
@@ -332,7 +332,7 @@ fn test_mv_interactive_no_clobber_force_last_arg_wins() {
         .ucmd()
         .args(&[file_a, file_b, "-n", "-f", "-i"])
         .fails()
-        .stderr_is(format!("mv: overwrite '{file_b}'? "));
+        .stderr_only(format!("mv: overwrite '{file_b}'? "));
 
     at.write(file_a, "aa");
 
@@ -518,7 +518,7 @@ fn test_mv_same_file() {
     ucmd.arg(file_a)
         .arg(file_a)
         .fails()
-        .stderr_is(format!("mv: '{file_a}' and '{file_a}' are the same file\n"));
+        .stderr_only(format!("mv: '{file_a}' and '{file_a}' are the same file\n"));
 }
 
 #[test]
@@ -535,7 +535,7 @@ fn test_mv_same_hardlink() {
     ucmd.arg(file_a)
         .arg(file_b)
         .fails()
-        .stderr_is(format!("mv: '{file_a}' and '{file_b}' are the same file\n"));
+        .stderr_only(format!("mv: '{file_a}' and '{file_b}' are the same file\n"));
 }
 
 #[test]
@@ -566,7 +566,7 @@ fn test_mv_same_symlink() {
     ucmd.arg(file_b)
         .arg(file_a)
         .fails()
-        .stderr_is(format!("mv: '{file_b}' and '{file_a}' are the same file\n"));
+        .stderr_only(format!("mv: '{file_b}' and '{file_a}' are the same file\n"));
 
     let (at2, mut ucmd2) = at_and_ucmd!();
     at2.touch(file_a);
@@ -596,7 +596,7 @@ 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_only(format!("mv: '{file_c}' and '{file_a}' are the same file\n"));
 }
 
 #[test]
@@ -609,7 +609,7 @@ fn test_mv_same_broken_symlink() {
     ucmd.arg("broken")
         .arg("broken")
         .fails()
-        .stderr_is("mv: 'broken' and 'broken' are the same file\n");
+        .stderr_only("mv: 'broken' and 'broken' are the same file\n");
 }
 
 #[test]
@@ -707,7 +707,7 @@ fn test_mv_same_file_not_dot_dir() {
     let dir = "test_mv_errors_dir";
 
     at.mkdir(dir);
-    ucmd.arg(dir).arg(dir).fails().stderr_is(format!(
+    ucmd.arg(dir).arg(dir).fails().stderr_only(format!(
         "mv: cannot move '{dir}' to a subdirectory of itself, '{dir}/{dir}'\n",
     ));
 }
@@ -719,7 +719,7 @@ fn test_mv_same_file_dot_dir() {
     ucmd.arg(".")
         .arg(".")
         .fails()
-        .stderr_is("mv: '.' and '.' are the same file\n");
+        .stderr_only("mv: '.' and '.' are the same file\n");
 }
 
 #[test]
@@ -1452,15 +1452,34 @@ fn test_mv_overwrite_nonempty_dir() {
     at.mkdir(dir_a);
     at.mkdir(dir_b);
     at.touch(dummy);
-    // Not same error as GNU; the error message is a rust builtin
-    // TODO: test (and implement) correct error message (or at least decide whether to do so)
-    // Current: "mv: couldn't rename path (Directory not empty; from=a; to=b)"
-    // GNU:     "mv: cannot move 'a' to 'b': Directory not empty"
-
     // Verbose output for the move should not be shown on failure
-    let result = ucmd.arg("-vT").arg(dir_a).arg(dir_b).fails();
-    result.no_stdout();
-    assert!(!result.stderr_str().is_empty());
+    ucmd.arg("-vT")
+        .arg(dir_a)
+        .arg(dir_b)
+        .fails()
+        .stderr_contains("cannot overwrite");
+
+    assert!(at.dir_exists(dir_a));
+    assert!(at.dir_exists(dir_b));
+}
+
+#[test]
+fn test_mv_overwrite_nonempty_dir_into_dir() {
+    let (at, mut ucmd) = at_and_ucmd!();
+    let dir_a = "test_mv_overwrite_nonempty_dir_into_dir_a";
+    let dir_b = "test_mv_overwrite_nonempty_dir_into_dir_b";
+    let target_dir = format!("{dir_b}/{dir_a}");
+    let dummy = format!("{target_dir}/file");
+
+    at.mkdir(dir_a);
+    at.mkdir(dir_b);
+    at.mkdir(&target_dir);
+    at.touch(&dummy);
+
+    ucmd.arg(dir_a)
+        .arg(dir_b)
+        .fails()
+        .stderr_contains("cannot overwrite");
 
     assert!(at.dir_exists(dir_a));
     assert!(at.dir_exists(dir_b));
@@ -1499,7 +1518,6 @@ fn test_mv_errors() {
     at.touch(file_b);
 
     // $ mv -T -t a b
-    // mv: cannot combine --target-directory (-t) and --no-target-directory (-T)
     scene
         .ucmd()
         .arg("-T")
@@ -1512,29 +1530,26 @@ fn test_mv_errors() {
 
     // $ at.touch file && at.mkdir dir
     // $ mv -T file dir
-    // err == mv: cannot overwrite directory 'dir' with non-directory
     scene
         .ucmd()
         .arg("-T")
         .arg(file_a)
         .arg(dir)
         .fails()
-        .stderr_is(format!(
+        .stderr_only(format!(
             "mv: cannot overwrite directory '{dir}' with non-directory\n"
         ));
 
     // $ at.mkdir dir && at.touch file
     // $ mv dir file
-    // err == mv: cannot overwrite non-directory 'file' with directory 'dir'
-    assert!(
-        !scene
-            .ucmd()
-            .arg(dir)
-            .arg(file_a)
-            .fails()
-            .stderr_str()
-            .is_empty()
-    );
+    scene
+        .ucmd()
+        .arg(dir)
+        .arg(file_a)
+        .fails()
+        .stderr_only(format!(
+            "mv: cannot overwrite non-directory '{file_a}' with directory '{dir}'\n"
+        ));
 }
 
 #[test]
@@ -1621,8 +1636,7 @@ fn test_mv_arg_interactive_skipped() {
         .pipe_in("N\n")
         .ignore_stdin_write_error()
         .fails()
-        .stderr_is("mv: overwrite 'b'? ")
-        .no_stdout();
+        .stderr_only("mv: overwrite 'b'? ");
 }
 
 #[test]

util/build-gnu.sh

@@ -282,9 +282,6 @@ sed -i -Ez "s/\n([^\n#]*pad-3\.2[^\n]*)\n([^\n]*)\n([^\n]*)/\n# uutils\/numfmt s
 sed -i -e "s/\$prog: multiple field specifications/error: the argument '--field <FIELDS>' cannot be used multiple times\n\nUsage: numfmt [OPTION]... [NUMBER]...\n\nFor more information, try '--help'./g" tests/numfmt/numfmt.pl
 sed -i -e "s/Try 'mv --help' for more information/For more information, try '--help'/g" -e "s/mv: missing file operand/error: the following required arguments were not provided:\n  <files>...\n\nUsage: mv [OPTION]... [-T] SOURCE DEST\n       mv [OPTION]... SOURCE... DIRECTORY\n       mv [OPTION]... -t DIRECTORY SOURCE...\n/g" -e "s/mv: missing destination file operand after 'no-file'/error: The argument '<files>...' requires at least 2 values, but only 1 was provided\n\nUsage: mv [OPTION]... [-T] SOURCE DEST\n       mv [OPTION]... SOURCE... DIRECTORY\n       mv [OPTION]... -t DIRECTORY SOURCE...\n/g" tests/mv/diag.sh
 
-# our error message is better
-sed -i -e "s|mv: cannot overwrite 'a/t': Directory not empty|mv: cannot move 'b/t' to 'a/t': Directory not empty|" tests/mv/dir2dir.sh
-
 # GNU doesn't support width > INT_MAX
 # disable these test cases
 sed -i -E "s|^([^#]*2_31.*)$|#\1|g" tests/printf/printf-cov.pl