diff --git a/src/uu/mv/src/error.rs b/src/uu/mv/src/error.rs index f989d4e133..6daa8188ec 100644 --- a/src/uu/mv/src/error.rs +++ b/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") } diff --git a/src/uu/mv/src/mv.rs b/src/uu/mv/src/mv.rs index 7debf52c96..489db32f7e 100644 --- a/src/uu/mv/src/mv.rs +++ b/src/uu/mv/src/mv.rs @@ -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,50 @@ 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<()> { + let canonicalized_source = source.canonicalize()?; + let canonicalized_target = match target_is_dir { + true => target + .canonicalize()? + .join(canonicalized_source.file_name().unwrap_or_default()), + false => match target.parent() { + Some(parent) if parent.to_str() != Some("") => parent + .canonicalize()? + .join(target.file_name().unwrap_or_default()), + _ => target.to_path_buf(), + }, + }; + + 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; + + let target_display = match source.file_name() { + Some(file_name) if target_is_dir => target.join(file_name).quote().to_string(), + _ => target.quote().to_string(), + }; + + if same_file + && (canonicalized_source == 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.clone()) { + 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 +449,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 +499,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()) { diff --git a/tests/by-util/test_mv.rs b/tests/by-util/test_mv.rs index ac64fae7eb..734fe300e2 100644 --- a/tests/by-util/test_mv.rs +++ b/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] @@ -1389,24 +1390,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 +1439,31 @@ 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!["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 +1742,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'"); -}