From c7f7a222b9a2e86a68b204b417fbe23e7df01e3f Mon Sep 17 00:00:00 2001 From: electricboogie <32370782+electricboogie@users.noreply.github.com> Date: Sun, 12 Dec 2021 10:49:38 -0600 Subject: [PATCH] Fix mv bug: Should be able to stat files, but not able to mv if source and target are the same (#2763) Closes #2760 --- src/uu/mv/src/mv.rs | 79 ++++++++++++++++++++++++++-------------- tests/by-util/test_mv.rs | 34 +++++++++++++++++ 2 files changed, 85 insertions(+), 28 deletions(-) diff --git a/src/uu/mv/src/mv.rs b/src/uu/mv/src/mv.rs index 9d23f86de..90c6eeaa8 100644 --- a/src/uu/mv/src/mv.rs +++ b/src/uu/mv/src/mv.rs @@ -13,6 +13,7 @@ extern crate uucore; use clap::{crate_version, App, Arg, ArgMatches}; use std::env; +use std::ffi::OsString; use std::fs; use std::io::{self, stdin}; #[cfg(unix)] @@ -30,9 +31,10 @@ pub struct Behavior { backup: BackupMode, suffix: String, update: bool, - target_dir: Option, + target_dir: Option, no_target_dir: bool, verbose: bool, + strip_slashes: bool, } #[derive(Clone, Eq, PartialEq)] @@ -77,10 +79,11 @@ pub fn uumain(args: impl uucore::Args) -> i32 { .usage(&usage[..]) .get_matches_from(args); - let files: Vec = matches - .values_of(ARG_FILES) - .map(|v| v.map(ToString::to_string).collect()) - .unwrap_or_default(); + let files: Vec = matches + .values_of_os(ARG_FILES) + .unwrap_or_default() + .map(|v| v.to_os_string()) + .collect(); let overwrite_mode = determine_overwrite_mode(&matches); let backup_mode = match backup_control::determine_backup_mode(&matches) { @@ -103,26 +106,15 @@ pub fn uumain(args: impl uucore::Args) -> i32 { backup: backup_mode, suffix: backup_suffix, update: matches.is_present(OPT_UPDATE), - target_dir: matches.value_of(OPT_TARGET_DIRECTORY).map(String::from), + target_dir: matches + .value_of_os(OPT_TARGET_DIRECTORY) + .map(OsString::from), no_target_dir: matches.is_present(OPT_NO_TARGET_DIRECTORY), verbose: matches.is_present(OPT_VERBOSE), + strip_slashes: matches.is_present(OPT_STRIP_TRAILING_SLASHES), }; - let paths: Vec = { - fn strip_slashes(p: &Path) -> &Path { - p.components().as_path() - } - let to_owned = |p: &Path| p.to_owned(); - let paths = files.iter().map(Path::new); - - if matches.is_present(OPT_STRIP_TRAILING_SLASHES) { - paths.map(strip_slashes).map(to_owned).collect() - } else { - paths.map(to_owned).collect() - } - }; - - exec(&paths[..], behavior) + exec(&files[..], behavior) } pub fn uu_app() -> App<'static, 'static> { @@ -210,15 +202,28 @@ fn determine_overwrite_mode(matches: &ArgMatches) -> OverwriteMode { } } -fn exec(files: &[PathBuf], b: Behavior) -> i32 { +fn exec(files: &[OsString], b: Behavior) -> i32 { + let paths: Vec = { + let paths = files.iter().map(Path::new); + + // Strip slashes from path, if strip opt present + if b.strip_slashes { + paths + .map(|p| p.components().as_path().to_owned()) + .collect::>() + } else { + paths.map(|p| p.to_owned()).collect::>() + } + }; + if let Some(ref name) = b.target_dir { - return move_files_into_dir(files, &PathBuf::from(name), &b); + return move_files_into_dir(&paths, &PathBuf::from(name), &b); } - match files.len() { + match paths.len() { /* case 0/1 are not possible thanks to clap */ 2 => { - let source = &files[0]; - let target = &files[1]; + let source = &paths[0]; + let target = &paths[1]; // Here we use the `symlink_metadata()` method instead of `exists()`, // since it handles dangling symlinks correctly. The method gives an // `Ok()` results unless the source does not exist, or the user @@ -228,6 +233,24 @@ fn exec(files: &[PathBuf], b: Behavior) -> i32 { return 1; } + // GNU semantics are: if the source and target are the same, no move occurs and we print an error + if source.eq(target) { + // Done to match GNU semantics for the dot file + if source.eq(Path::new(".")) || source.ends_with("/.") || source.is_file() { + show_error!( + "'{}' and '{}' are the same file", + source.display(), + target.display(), + ) + } else { + show_error!( + "cannot move '{s}' to a subdirectory of itself, '{s}/{s}'", + s = source.display(), + ) + } + return 1; + } + if target.is_dir() { if b.no_target_dir { if !source.is_dir() { @@ -277,8 +300,8 @@ fn exec(files: &[PathBuf], b: Behavior) -> i32 { ); return 1; } - let target_dir = files.last().unwrap(); - move_files_into_dir(&files[..files.len() - 1], target_dir, &b); + let target_dir = paths.last().unwrap(); + move_files_into_dir(&paths[..paths.len() - 1], target_dir, &b); } } 0 diff --git a/tests/by-util/test_mv.rs b/tests/by-util/test_mv.rs index f6650cdba..9fccc90a2 100644 --- a/tests/by-util/test_mv.rs +++ b/tests/by-util/test_mv.rs @@ -232,6 +232,40 @@ fn test_mv_force_replace_file() { assert!(at.file_exists(file_b)); } +#[test] +fn test_mv_same_file() { + let (at, mut ucmd) = at_and_ucmd!(); + let file_a = "test_mv_same_file_a"; + + at.touch(file_a); + ucmd.arg(file_a).arg(file_a).fails().stderr_is(format!( + "mv: '{f}' and '{f}' are the same file\n", + f = file_a, + )); +} + +#[test] +fn test_mv_same_file_not_dot_dir() { + let (at, mut ucmd) = at_and_ucmd!(); + let dir = "test_mv_errors_dir"; + + at.mkdir(dir); + ucmd.arg(dir).arg(dir).fails().stderr_is(format!( + "mv: cannot move '{d}' to a subdirectory of itself, '{d}/{d}'", + d = dir, + )); +} + +#[test] +fn test_mv_same_file_dot_dir() { + let (_at, mut ucmd) = at_and_ucmd!(); + + ucmd.arg(".") + .arg(".") + .fails() + .stderr_is("mv: '.' and '.' are the same file\n".to_string()); +} + #[test] fn test_mv_simple_backup() { let (at, mut ucmd) = at_and_ucmd!();