From 0121dce2c58697e55c8e14819ad5969fec0c6dec Mon Sep 17 00:00:00 2001 From: Aaron Ang <67321817+aaron-ang@users.noreply.github.com> Date: Mon, 5 Jan 2026 12:33:18 -0800 Subject: [PATCH] mv: revert error message to GNU default when overwriting non-empty directory --- src/uu/mv/locales/en-US.ftl | 1 + src/uu/mv/locales/fr-FR.ftl | 1 + src/uu/mv/src/mv.rs | 53 ++++++++++++++++++------ tests/by-util/test_mv.rs | 80 ++++++++++++++++++++++--------------- util/build-gnu.sh | 3 -- 5 files changed, 89 insertions(+), 49 deletions(-) diff --git a/src/uu/mv/locales/en-US.ftl b/src/uu/mv/locales/en-US.ftl index 4bb5339fe32..484d8af4548 100644 --- a/src/uu/mv/locales/en-US.ftl +++ b/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 diff --git a/src/uu/mv/locales/fr-FR.ftl b/src/uu/mv/locales/fr-FR.ftl index 9ea2f2114b1..5470d5f10ce 100644 --- a/src/uu/mv/locales/fr-FR.ftl +++ b/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 diff --git a/src/uu/mv/src/mv.rs b/src/uu/mv/src/mv.rs index 44540abfa3e..8ce1dc27120 100644 --- a/src/uu/mv/src/mv.rs +++ b/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 { + 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( diff --git a/tests/by-util/test_mv.rs b/tests/by-util/test_mv.rs index 5592f9c1e5c..e9f716f5dcd 100644 --- a/tests/by-util/test_mv.rs +++ b/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] diff --git a/util/build-gnu.sh b/util/build-gnu.sh index 7a9f45c385b..785fada8a02 100755 --- a/util/build-gnu.sh +++ b/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 ' 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 ...\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 '...' 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