From 74cd797c8a1205721c34d484b71fa32c47ae7939 Mon Sep 17 00:00:00 2001 From: mhead Date: Sat, 26 Oct 2024 12:09:38 +0530 Subject: [PATCH 1/4] cp: normalize path when checking for duplicate source --- src/uu/cp/src/cp.rs | 8 ++++---- tests/by-util/test_cp.rs | 13 +++++++++++++ 2 files changed, 17 insertions(+), 4 deletions(-) diff --git a/src/uu/cp/src/cp.rs b/src/uu/cp/src/cp.rs index 62509b756..3cd026335 100644 --- a/src/uu/cp/src/cp.rs +++ b/src/uu/cp/src/cp.rs @@ -29,7 +29,7 @@ use platform::copy_on_write; use uucore::display::Quotable; use uucore::error::{set_exit_code, UClapError, UError, UResult, UUsageError}; use uucore::fs::{ - are_hardlinks_to_same_file, canonicalize, get_filename, is_symlink_loop, + are_hardlinks_to_same_file, canonicalize, get_filename, is_symlink_loop, normalize_path, path_ends_with_terminator, paths_refer_to_same_file, FileInformation, MissingHandling, ResolveMode, }; @@ -1264,8 +1264,8 @@ pub fn copy(sources: &[PathBuf], target: &Path, options: &Options) -> CopyResult }; for source in sources { - if seen_sources.contains(source) { - // FIXME: compare sources by the actual file they point to, not their path. (e.g. dir/file == dir/../dir/file in most cases) + let normalized_source = normalize_path(source); + if seen_sources.contains(&normalized_source) { show_warning!("source file {} specified more than once", source.quote()); } else { let dest = construct_dest_path(source, target, target_type, options) @@ -1309,7 +1309,7 @@ pub fn copy(sources: &[PathBuf], target: &Path, options: &Options) -> CopyResult copied_destinations.insert(dest.clone()); } } - seen_sources.insert(source); + seen_sources.insert(normalized_source); } if let Some(pb) = progress_bar { diff --git a/tests/by-util/test_cp.rs b/tests/by-util/test_cp.rs index 0fba59672..0ecb06bd8 100644 --- a/tests/by-util/test_cp.rs +++ b/tests/by-util/test_cp.rs @@ -121,6 +121,19 @@ fn test_cp_duplicate_files() { assert_eq!(at.read(TEST_COPY_TO_FOLDER_FILE), "Hello, World!\n"); } +#[test] +fn test_cp_duplicate_files_normalized_path() { + let (at, mut ucmd) = at_and_ucmd!(); + ucmd.arg(TEST_HELLO_WORLD_SOURCE) + .arg(format!("./{TEST_HELLO_WORLD_SOURCE}")) + .arg(TEST_COPY_TO_FOLDER) + .succeeds() + .stderr_contains(format!( + "source file './{TEST_HELLO_WORLD_SOURCE}' specified more than once" + )); + assert_eq!(at.read(TEST_COPY_TO_FOLDER_FILE), "Hello, World!\n"); +} + #[test] fn test_cp_same_file() { let (at, mut ucmd) = at_and_ucmd!(); From 4d5bf4205fc7f5d4519526a499edda850725801b Mon Sep 17 00:00:00 2001 From: mhead Date: Sun, 27 Oct 2024 14:18:20 +0530 Subject: [PATCH 2/4] cp: skip duplicate source check when --backup is given --- src/uu/cp/src/cp.rs | 2 +- tests/by-util/test_cp.rs | 27 +++++++++++++++++++++++++++ 2 files changed, 28 insertions(+), 1 deletion(-) diff --git a/src/uu/cp/src/cp.rs b/src/uu/cp/src/cp.rs index 3cd026335..3ccd0575f 100644 --- a/src/uu/cp/src/cp.rs +++ b/src/uu/cp/src/cp.rs @@ -1265,7 +1265,7 @@ pub fn copy(sources: &[PathBuf], target: &Path, options: &Options) -> CopyResult for source in sources { let normalized_source = normalize_path(source); - if seen_sources.contains(&normalized_source) { + if options.backup == BackupMode::NoBackup && seen_sources.contains(&normalized_source) { show_warning!("source file {} specified more than once", source.quote()); } else { let dest = construct_dest_path(source, target, target_type, options) diff --git a/tests/by-util/test_cp.rs b/tests/by-util/test_cp.rs index 0ecb06bd8..351ff6a90 100644 --- a/tests/by-util/test_cp.rs +++ b/tests/by-util/test_cp.rs @@ -134,6 +134,33 @@ fn test_cp_duplicate_files_normalized_path() { assert_eq!(at.read(TEST_COPY_TO_FOLDER_FILE), "Hello, World!\n"); } +#[test] +fn test_cp_duplicate_files_with_plain_backup() { + let (_, mut ucmd) = at_and_ucmd!(); + ucmd.arg(TEST_HELLO_WORLD_SOURCE) + .arg(TEST_HELLO_WORLD_SOURCE) + .arg(TEST_COPY_TO_FOLDER) + .arg("--backup") + .fails() + // cp would skip duplicate src check and fail when it tries to overwrite the "just-created" file. + .stderr_contains( + "will not overwrite just-created 'hello_dir/hello_world.txt' with 'hello_world.txt", + ); +} + +#[test] +fn test_cp_duplicate_files_with_numbered_backup() { + let (at, mut ucmd) = at_and_ucmd!(); + // cp would skip duplicate src check and succeeds + ucmd.arg(TEST_HELLO_WORLD_SOURCE) + .arg(TEST_HELLO_WORLD_SOURCE) + .arg(TEST_COPY_TO_FOLDER) + .arg("--backup=numbered") + .succeeds(); + at.file_exists(TEST_COPY_TO_FOLDER_FILE); + at.file_exists(format!("{TEST_COPY_TO_FOLDER}.~1~")); +} + #[test] fn test_cp_same_file() { let (at, mut ucmd) = at_and_ucmd!(); From ff586b47d40b1695515d0e56f2b1653ea5c6ba0a Mon Sep 17 00:00:00 2001 From: mhead Date: Thu, 31 Oct 2024 11:49:29 +0530 Subject: [PATCH 3/4] cp: Duplicate source error message specify the type of file. --- src/uu/cp/src/cp.rs | 10 +++++++++- tests/by-util/test_cp.rs | 14 +++++++++++++- 2 files changed, 22 insertions(+), 2 deletions(-) diff --git a/src/uu/cp/src/cp.rs b/src/uu/cp/src/cp.rs index 3ccd0575f..32168b090 100644 --- a/src/uu/cp/src/cp.rs +++ b/src/uu/cp/src/cp.rs @@ -1266,7 +1266,15 @@ pub fn copy(sources: &[PathBuf], target: &Path, options: &Options) -> CopyResult for source in sources { let normalized_source = normalize_path(source); if options.backup == BackupMode::NoBackup && seen_sources.contains(&normalized_source) { - show_warning!("source file {} specified more than once", source.quote()); + let file_type = if source.symlink_metadata()?.file_type().is_dir() { + "directory" + } else { + "file" + }; + show_warning!( + "source {file_type} {} specified more than once", + source.quote() + ); } else { let dest = construct_dest_path(source, target, target_type, options) .unwrap_or_else(|_| target.to_path_buf()); diff --git a/tests/by-util/test_cp.rs b/tests/by-util/test_cp.rs index 351ff6a90..df94bb28c 100644 --- a/tests/by-util/test_cp.rs +++ b/tests/by-util/test_cp.rs @@ -120,7 +120,19 @@ fn test_cp_duplicate_files() { )); assert_eq!(at.read(TEST_COPY_TO_FOLDER_FILE), "Hello, World!\n"); } - +#[test] +fn test_cp_duplicate_folder() { + let (at, mut ucmd) = at_and_ucmd!(); + ucmd.arg("-r") + .arg(TEST_COPY_FROM_FOLDER) + .arg(TEST_COPY_FROM_FOLDER) + .arg(TEST_COPY_TO_FOLDER) + .succeeds() + .stderr_contains(format!( + "source directory '{TEST_COPY_FROM_FOLDER}' specified more than once" + )); + assert!(at.dir_exists(format!("{TEST_COPY_TO_FOLDER}/{TEST_COPY_FROM_FOLDER}").as_str())); +} #[test] fn test_cp_duplicate_files_normalized_path() { let (at, mut ucmd) = at_and_ucmd!(); From e947c713c8c07504d0c60acabbc18eb6cb798235 Mon Sep 17 00:00:00 2001 From: Daniel Hofstetter Date: Thu, 31 Oct 2024 10:55:22 +0100 Subject: [PATCH 4/4] cp: separate tests with empty line --- tests/by-util/test_cp.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/by-util/test_cp.rs b/tests/by-util/test_cp.rs index df94bb28c..156daec1f 100644 --- a/tests/by-util/test_cp.rs +++ b/tests/by-util/test_cp.rs @@ -120,6 +120,7 @@ fn test_cp_duplicate_files() { )); assert_eq!(at.read(TEST_COPY_TO_FOLDER_FILE), "Hello, World!\n"); } + #[test] fn test_cp_duplicate_folder() { let (at, mut ucmd) = at_and_ucmd!(); @@ -133,6 +134,7 @@ fn test_cp_duplicate_folder() { )); assert!(at.dir_exists(format!("{TEST_COPY_TO_FOLDER}/{TEST_COPY_FROM_FOLDER}").as_str())); } + #[test] fn test_cp_duplicate_files_normalized_path() { let (at, mut ucmd) = at_and_ucmd!();