Skip to content

Commit 4c79c9e

Browse files
aaron-angaaron-ang
committed
mv: improve error message when overwriting non-empty directory
1 parent 2da2c90 commit 4c79c9e

4 files changed

Lines changed: 90 additions & 46 deletions

File tree

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,7 @@ mv-error-backup-might-destroy-source = backing up {$target} might destroy source
3232
mv-error-will-not-overwrite-just-created = will not overwrite just-created {$target} with {$source}
3333
mv-error-not-replacing = not replacing {$target}
3434
mv-error-cannot-move = cannot move {$source} to {$target}
35+
mv-error-cannot-overwrite-non-empty-directory = cannot overwrite destination {$target}
3536
mv-error-directory-not-empty = Directory not empty
3637
mv-error-dangling-symlink = can't determine symlink type, since it is dangling
3738
mv-error-no-symlink-support = your operating system does not support symlinks

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,7 @@ mv-error-backup-might-destroy-source = sauvegarder {$target} pourrait détruire
3232
mv-error-will-not-overwrite-just-created = ne va pas écraser le fichier qui vient d'être créé {$target} avec {$source}
3333
mv-error-not-replacing = ne remplace pas {$target}
3434
mv-error-cannot-move = impossible de déplacer {$source} vers {$target}
35+
mv-error-cannot-overwrite-non-empty-directory = impossible d'écraser la destination {$target}
3536
mv-error-directory-not-empty = Répertoire non vide
3637
mv-error-dangling-symlink = impossible de déterminer le type de lien symbolique, car il est suspendu
3738
mv-error-no-symlink-support = votre système d'exploitation ne prend pas en charge les liens symboliques

‎src/uu/mv/src/mv.rs‎

Lines changed: 40 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,7 @@ use crate::hardlink::{
3636
};
3737
use uucore::backup_control::{self, source_is_target_backup};
3838
use uucore::display::Quotable;
39-
use uucore::error::{FromIo, UResult, USimpleError, UUsageError, set_exit_code};
39+
use uucore::error::{FromIo, UError, UResult, USimpleError, UUsageError, set_exit_code};
4040
#[cfg(unix)]
4141
use uucore::fs::display_permissions_unix;
4242
#[cfg(unix)]
@@ -415,8 +415,20 @@ fn handle_two_paths(source: &Path, target: &Path, opts: &Options) -> UResult<()>
415415
hardlink_params.0,
416416
hardlink_params.1,
417417
)
418-
.map_err_context(|| {
419-
translate!("mv-error-cannot-move", "source" => source.quote(), "target" => target.quote())
418+
.map_err(|e| -> Box<dyn UError> {
419+
let message = if is_directory_not_empty_error(&e) {
420+
translate!(
421+
"mv-error-cannot-overwrite-non-empty-directory",
422+
"target" => target.quote()
423+
)
424+
} else {
425+
translate!(
426+
"mv-error-cannot-move",
427+
"source" => source.quote(),
428+
"target" => target.quote()
429+
)
430+
};
431+
e.map_err_context(|| message)
420432
})
421433
} else {
422434
Err(MvError::DirectoryToNonDirectory(target.quote().to_string()).into())
@@ -680,20 +692,31 @@ fn move_files_into_dir(files: &[PathBuf], target_dir: &Path, options: &Options)
680692
) {
681693
Err(e) if e.to_string().is_empty() => set_exit_code(1),
682694
Err(e) => {
683-
let e = e.map_err_context(|| {
684-
translate!("mv-error-cannot-move", "source" => sourcepath.quote(), "target" => targetpath.quote())
685-
});
695+
let message = if is_directory_not_empty_error(&e) {
696+
translate!(
697+
"mv-error-cannot-overwrite-non-empty-directory",
698+
"target" => targetpath.quote()
699+
)
700+
} else {
701+
translate!(
702+
"mv-error-cannot-move",
703+
"source" => sourcepath.quote(),
704+
"target" => targetpath.quote()
705+
)
706+
};
707+
let e = e.map_err_context(|| message);
686708
match display_manager {
687709
Some(ref pb) => pb.suspend(|| show!(e)),
688710
None => show!(e),
689711
}
690712
}
691-
Ok(()) => (),
713+
Ok(()) => {
714+
moved_destinations.insert(targetpath.clone());
715+
}
692716
}
693717
if let Some(ref pb) = count_progress {
694718
pb.inc(1);
695719
}
696-
moved_destinations.insert(targetpath.clone());
697720
}
698721
Ok(())
699722
}
@@ -761,7 +784,10 @@ fn rename(
761784
if is_empty_dir(to) {
762785
fs::remove_dir(to)?;
763786
} else {
764-
return Err(io::Error::other(translate!("mv-error-directory-not-empty")));
787+
return Err(io::Error::new(
788+
io::ErrorKind::DirectoryNotEmpty,
789+
translate!("mv-error-directory-not-empty"),
790+
));
765791
}
766792
}
767793
}
@@ -806,6 +832,10 @@ fn rename(
806832
Ok(())
807833
}
808834

835+
fn is_directory_not_empty_error(err: &io::Error) -> bool {
836+
err.kind() == io::ErrorKind::DirectoryNotEmpty
837+
}
838+
809839
#[cfg(unix)]
810840
fn is_fifo(filetype: fs::FileType) -> bool {
811841
filetype.is_fifo()
@@ -932,10 +962,7 @@ fn rename_symlink_fallback(from: &Path, to: &Path) -> io::Result<()> {
932962
#[cfg(not(any(windows, unix)))]
933963
fn rename_symlink_fallback(from: &Path, to: &Path) -> io::Result<()> {
934964
let path_symlink_points_to = fs::read_link(from)?;
935-
Err(io::Error::new(
936-
io::ErrorKind::Other,
937-
translate!("mv-error-no-symlink-support"),
938-
))
965+
Err(io::Error::other(translate!("mv-error-no-symlink-support")))
939966
}
940967

941968
fn rename_dir_fallback(

‎tests/by-util/test_mv.rs‎

Lines changed: 48 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -119,7 +119,7 @@ fn test_mv_move_file_into_file_with_target_arg() {
119119
.arg(file1)
120120
.arg(file2)
121121
.fails()
122-
.stderr_is(format!("mv: target directory '{file1}': Not a directory\n"));
122+
.stderr_only(format!("mv: target directory '{file1}': Not a directory\n"));
123123

124124
assert!(at.file_exists(file1));
125125
}
@@ -139,7 +139,7 @@ fn test_mv_move_multiple_files_into_file() {
139139
.arg(file2)
140140
.arg(file3)
141141
.fails()
142-
.stderr_is(format!("mv: target '{file3}': Not a directory\n"));
142+
.stderr_only(format!("mv: target '{file3}': Not a directory\n"));
143143

144144
assert!(at.file_exists(file1));
145145
assert!(at.file_exists(file2));
@@ -332,7 +332,7 @@ fn test_mv_interactive_no_clobber_force_last_arg_wins() {
332332
.ucmd()
333333
.args(&[file_a, file_b, "-n", "-f", "-i"])
334334
.fails()
335-
.stderr_is(format!("mv: overwrite '{file_b}'? "));
335+
.stderr_only(format!("mv: overwrite '{file_b}'? "));
336336

337337
at.write(file_a, "aa");
338338

@@ -518,7 +518,7 @@ fn test_mv_same_file() {
518518
ucmd.arg(file_a)
519519
.arg(file_a)
520520
.fails()
521-
.stderr_is(format!("mv: '{file_a}' and '{file_a}' are the same file\n"));
521+
.stderr_only(format!("mv: '{file_a}' and '{file_a}' are the same file\n"));
522522
}
523523

524524
#[test]
@@ -535,7 +535,7 @@ fn test_mv_same_hardlink() {
535535
ucmd.arg(file_a)
536536
.arg(file_b)
537537
.fails()
538-
.stderr_is(format!("mv: '{file_a}' and '{file_b}' are the same file\n"));
538+
.stderr_only(format!("mv: '{file_a}' and '{file_b}' are the same file\n"));
539539
}
540540

541541
#[test]
@@ -566,7 +566,7 @@ fn test_mv_same_symlink() {
566566
ucmd.arg(file_b)
567567
.arg(file_a)
568568
.fails()
569-
.stderr_is(format!("mv: '{file_b}' and '{file_a}' are the same file\n"));
569+
.stderr_only(format!("mv: '{file_b}' and '{file_a}' are the same file\n"));
570570

571571
let (at2, mut ucmd2) = at_and_ucmd!();
572572
at2.touch(file_a);
@@ -596,7 +596,7 @@ fn test_mv_same_symlink() {
596596
.arg(file_c)
597597
.arg(file_a)
598598
.fails()
599-
.stderr_is(format!("mv: '{file_c}' and '{file_a}' are the same file\n"));
599+
.stderr_only(format!("mv: '{file_c}' and '{file_a}' are the same file\n"));
600600
}
601601

602602
#[test]
@@ -609,7 +609,7 @@ fn test_mv_same_broken_symlink() {
609609
ucmd.arg("broken")
610610
.arg("broken")
611611
.fails()
612-
.stderr_is("mv: 'broken' and 'broken' are the same file\n");
612+
.stderr_only("mv: 'broken' and 'broken' are the same file\n");
613613
}
614614

615615
#[test]
@@ -742,7 +742,7 @@ fn test_mv_same_file_not_dot_dir() {
742742
let dir = "test_mv_errors_dir";
743743

744744
at.mkdir(dir);
745-
ucmd.arg(dir).arg(dir).fails().stderr_is(format!(
745+
ucmd.arg(dir).arg(dir).fails().stderr_only(format!(
746746
"mv: cannot move '{dir}' to a subdirectory of itself, '{dir}/{dir}'\n",
747747
));
748748
}
@@ -754,7 +754,7 @@ fn test_mv_same_file_dot_dir() {
754754
ucmd.arg(".")
755755
.arg(".")
756756
.fails()
757-
.stderr_is("mv: '.' and '.' are the same file\n");
757+
.stderr_only("mv: '.' and '.' are the same file\n");
758758
}
759759

760760
#[test]
@@ -1487,15 +1487,35 @@ fn test_mv_overwrite_nonempty_dir() {
14871487
at.mkdir(dir_a);
14881488
at.mkdir(dir_b);
14891489
at.touch(dummy);
1490-
// Not same error as GNU; the error message is a rust builtin
1491-
// TODO: test (and implement) correct error message (or at least decide whether to do so)
1492-
// Current: "mv: couldn't rename path (Directory not empty; from=a; to=b)"
1493-
// GNU: "mv: cannot move 'a' to 'b': Directory not empty"
1494-
14951490
// Verbose output for the move should not be shown on failure
1496-
let result = ucmd.arg("-vT").arg(dir_a).arg(dir_b).fails();
1497-
result.no_stdout();
1498-
assert!(!result.stderr_str().is_empty());
1491+
ucmd.arg("-vT")
1492+
.arg(dir_a)
1493+
.arg(dir_b)
1494+
.fails()
1495+
.stderr_only(format!(
1496+
"mv: cannot overwrite destination '{dir_b}': Directory not empty\n"
1497+
));
1498+
1499+
assert!(at.dir_exists(dir_a));
1500+
assert!(at.dir_exists(dir_b));
1501+
}
1502+
1503+
#[test]
1504+
fn test_mv_overwrite_nonempty_dir_into_dir() {
1505+
let (at, mut ucmd) = at_and_ucmd!();
1506+
let dir_a = "test_mv_overwrite_nonempty_dir_into_dir_a";
1507+
let dir_b = "test_mv_overwrite_nonempty_dir_into_dir_b";
1508+
let target_dir = format!("{dir_b}/{dir_a}");
1509+
let dummy = format!("{target_dir}/file");
1510+
1511+
at.mkdir(dir_a);
1512+
at.mkdir(dir_b);
1513+
at.mkdir(&target_dir);
1514+
at.touch(&dummy);
1515+
1516+
ucmd.arg(dir_a).arg(dir_b).fails().stderr_only(format!(
1517+
"mv: cannot overwrite destination '{dir_b}/{dir_a}': Directory not empty\n"
1518+
));
14991519

15001520
assert!(at.dir_exists(dir_a));
15011521
assert!(at.dir_exists(dir_b));
@@ -1534,7 +1554,6 @@ fn test_mv_errors() {
15341554
at.touch(file_b);
15351555

15361556
// $ mv -T -t a b
1537-
// mv: cannot combine --target-directory (-t) and --no-target-directory (-T)
15381557
scene
15391558
.ucmd()
15401559
.arg("-T")
@@ -1547,29 +1566,26 @@ fn test_mv_errors() {
15471566

15481567
// $ at.touch file && at.mkdir dir
15491568
// $ mv -T file dir
1550-
// err == mv: cannot overwrite directory 'dir' with non-directory
15511569
scene
15521570
.ucmd()
15531571
.arg("-T")
15541572
.arg(file_a)
15551573
.arg(dir)
15561574
.fails()
1557-
.stderr_is(format!(
1575+
.stderr_only(format!(
15581576
"mv: cannot overwrite directory '{dir}' with non-directory\n"
15591577
));
15601578

15611579
// $ at.mkdir dir && at.touch file
15621580
// $ mv dir file
1563-
// err == mv: cannot overwrite non-directory 'file' with directory 'dir'
1564-
assert!(
1565-
!scene
1566-
.ucmd()
1567-
.arg(dir)
1568-
.arg(file_a)
1569-
.fails()
1570-
.stderr_str()
1571-
.is_empty()
1572-
);
1581+
scene
1582+
.ucmd()
1583+
.arg(dir)
1584+
.arg(file_a)
1585+
.fails()
1586+
.stderr_only(format!(
1587+
"mv: cannot overwrite directory '{dir}' with non-directory\n"
1588+
));
15731589
}
15741590

15751591
#[test]
@@ -1656,8 +1672,7 @@ fn test_mv_arg_interactive_skipped() {
16561672
.pipe_in("N\n")
16571673
.ignore_stdin_write_error()
16581674
.fails()
1659-
.stderr_is("mv: overwrite 'b'? ")
1660-
.no_stdout();
1675+
.stderr_only("mv: overwrite 'b'? ");
16611676
}
16621677

16631678
#[test]

0 commit comments

Comments
 (0)