From 5a945579b83e69d540fa5258be32d019bb9fd37e Mon Sep 17 00:00:00 2001 From: tommady Date: Mon, 10 Jul 2023 13:14:22 +0000 Subject: [PATCH 01/33] fix-issue-5031 --- src/uu/cp/src/cp.rs | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/src/uu/cp/src/cp.rs b/src/uu/cp/src/cp.rs index de9dd1c919f..52ffd00e683 100644 --- a/src/uu/cp/src/cp.rs +++ b/src/uu/cp/src/cp.rs @@ -13,7 +13,7 @@ use quick_error::quick_error; use std::borrow::Cow; -use std::collections::HashSet; +use std::collections::{HashMap, HashSet}; use std::env; #[cfg(not(windows))] use std::ffi::CString; @@ -1140,6 +1140,7 @@ fn copy(sources: &[Source], target: &TargetSlice, options: &Options) -> CopyResu let mut non_fatal_errors = false; let mut seen_sources = HashSet::with_capacity(sources.len()); let mut symlinked_files = HashSet::new(); + let mut hardlinked_files = HashMap::with_capacity(sources.len()); let progress_bar = if options.progress_bar { let pb = ProgressBar::new(disk_usage(sources, options.recursive)?) @@ -1157,12 +1158,19 @@ fn copy(sources: &[Source], target: &TargetSlice, options: &Options) -> CopyResu }; for source in sources.iter() { + let dest = construct_dest_path(source, target, &target_type, options)?; 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) show_warning!("source {} specified more than once", source.quote()); + } else if preserve_hard_links && source.is_symlink() { + if let Some(new_source) = hardlinked_files.get(&source.read_link()?) { + if file_or_link_exists(&dest) { + std::fs::remove_file(&dest)?; + } + std::fs::hard_link(new_source, &dest)?; + } } else { let found_hard_link = if preserve_hard_links && !source.is_dir() { - let dest = construct_dest_path(source, target, &target_type, options)?; preserve_hardlinks(&mut hard_links, source, &dest)? } else { false @@ -1181,6 +1189,7 @@ fn copy(sources: &[Source], target: &TargetSlice, options: &Options) -> CopyResu } } seen_sources.insert(source); + hardlinked_files.insert(source, dest); } } if non_fatal_errors { From 31c1ebffa48131888c5313d2fb02a0a515877b50 Mon Sep 17 00:00:00 2001 From: tommady Date: Mon, 10 Jul 2023 13:29:33 +0000 Subject: [PATCH 02/33] fix spelling --- src/uu/cp/src/cp.rs | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/src/uu/cp/src/cp.rs b/src/uu/cp/src/cp.rs index 52ffd00e683..c6cf5e462b5 100644 --- a/src/uu/cp/src/cp.rs +++ b/src/uu/cp/src/cp.rs @@ -1138,9 +1138,8 @@ fn copy(sources: &[Source], target: &TargetSlice, options: &Options) -> CopyResu let mut hard_links: Vec<(String, u64)> = vec![]; let mut non_fatal_errors = false; - let mut seen_sources = HashSet::with_capacity(sources.len()); + let mut seen_sources = HashMap::with_capacity(sources.len()); let mut symlinked_files = HashSet::new(); - let mut hardlinked_files = HashMap::with_capacity(sources.len()); let progress_bar = if options.progress_bar { let pb = ProgressBar::new(disk_usage(sources, options.recursive)?) @@ -1159,11 +1158,11 @@ fn copy(sources: &[Source], target: &TargetSlice, options: &Options) -> CopyResu for source in sources.iter() { let dest = construct_dest_path(source, target, &target_type, options)?; - if seen_sources.contains(source) { + if seen_sources.contains_key(source) { // FIXME: compare sources by the actual file they point to, not their path. (e.g. dir/file == dir/../dir/file in most cases) show_warning!("source {} specified more than once", source.quote()); } else if preserve_hard_links && source.is_symlink() { - if let Some(new_source) = hardlinked_files.get(&source.read_link()?) { + if let Some(new_source) = seen_sources.get(&source.read_link()?) { if file_or_link_exists(&dest) { std::fs::remove_file(&dest)?; } @@ -1188,8 +1187,7 @@ fn copy(sources: &[Source], target: &TargetSlice, options: &Options) -> CopyResu non_fatal_errors = true; } } - seen_sources.insert(source); - hardlinked_files.insert(source, dest); + seen_sources.insert(source, dest); } } if non_fatal_errors { From 1dfa23f97e7a3590aeda99b77536dc10078cc782 Mon Sep 17 00:00:00 2001 From: tommady Date: Tue, 11 Jul 2023 03:20:43 +0000 Subject: [PATCH 03/33] add explanation --- src/uu/cp/src/cp.rs | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/src/uu/cp/src/cp.rs b/src/uu/cp/src/cp.rs index c6cf5e462b5..7c8e9e45b43 100644 --- a/src/uu/cp/src/cp.rs +++ b/src/uu/cp/src/cp.rs @@ -1162,6 +1162,26 @@ fn copy(sources: &[Source], target: &TargetSlice, options: &Options) -> CopyResu // FIXME: compare sources by the actual file they point to, not their path. (e.g. dir/file == dir/../dir/file in most cases) show_warning!("source {} specified more than once", source.quote()); } else if preserve_hard_links && source.is_symlink() { + // issue 5031 case + // + // touch a && ln -s a b && mkdir c + // cp --preserve=links -R -H a b c + // inode of c/a and c/b should be the same + // + // inode number | filename + // 33473704 a + // 33473729 b -> a + // 35143635 c + // 35143846 ├── a + // 35143846 └── b + // + // runs | source | dest + // 1 a c/a + // 2 b->a c/b + // + // c/b must be the hard link of c/a + // so the solution we can have is to remember the files (source & dest) we visited + // to achive the goal. if let Some(new_source) = seen_sources.get(&source.read_link()?) { if file_or_link_exists(&dest) { std::fs::remove_file(&dest)?; From efbdf511713bf64e7883957d870e8db407a769e1 Mon Sep 17 00:00:00 2001 From: tommady Date: Thu, 13 Jul 2023 05:38:58 +0000 Subject: [PATCH 04/33] adding testcase for 5031 --- src/uu/cp/src/cp.rs | 41 ++++++++++++++++++---------------------- tests/by-util/test_cp.rs | 30 +++++++++++++++++++++++++++++ 2 files changed, 48 insertions(+), 23 deletions(-) diff --git a/src/uu/cp/src/cp.rs b/src/uu/cp/src/cp.rs index 7c8e9e45b43..29464bd98be 100644 --- a/src/uu/cp/src/cp.rs +++ b/src/uu/cp/src/cp.rs @@ -1134,6 +1134,7 @@ fn copy(sources: &[Source], target: &TargetSlice, options: &Options) -> CopyResu verify_target_type(target, &target_type)?; let preserve_hard_links = options.preserve_hard_links(); + let cli_dereference = options.cli_dereference; let mut hard_links: Vec<(String, u64)> = vec![]; @@ -1161,32 +1162,26 @@ fn copy(sources: &[Source], target: &TargetSlice, options: &Options) -> CopyResu if seen_sources.contains_key(source) { // FIXME: compare sources by the actual file they point to, not their path. (e.g. dir/file == dir/../dir/file in most cases) show_warning!("source {} specified more than once", source.quote()); - } else if preserve_hard_links && source.is_symlink() { + } else if preserve_hard_links && source.is_symlink() && cli_dereference { // issue 5031 case - // - // touch a && ln -s a b && mkdir c // cp --preserve=links -R -H a b c - // inode of c/a and c/b should be the same - // - // inode number | filename - // 33473704 a - // 33473729 b -> a - // 35143635 c - // 35143846 ├── a - // 35143846 └── b - // - // runs | source | dest - // 1 a c/a - // 2 b->a c/b + + let mut original_source = source.read_link()?; + while original_source.is_symlink() { + original_source = original_source.read_link()?; + } // - // c/b must be the hard link of c/a - // so the solution we can have is to remember the files (source & dest) we visited - // to achive the goal. - if let Some(new_source) = seen_sources.get(&source.read_link()?) { - if file_or_link_exists(&dest) { - std::fs::remove_file(&dest)?; + original_source = original_source.file_name().unwrap().into(); + + match seen_sources.get(&original_source) { + Some(new_source) => { + std::fs::hard_link(new_source, &dest)?; + } + None => { + // TODO: GNU cp can deal with this case + // for example: + // cp --preserve=links -R -H b a c } - std::fs::hard_link(new_source, &dest)?; } } else { let found_hard_link = if preserve_hard_links && !source.is_dir() { @@ -1207,8 +1202,8 @@ fn copy(sources: &[Source], target: &TargetSlice, options: &Options) -> CopyResu non_fatal_errors = true; } } - seen_sources.insert(source, dest); } + seen_sources.insert(source, dest); } if non_fatal_errors { Err(Error::NotAllFilesCopied) diff --git a/tests/by-util/test_cp.rs b/tests/by-util/test_cp.rs index 52f7e443017..2cf006e37ed 100644 --- a/tests/by-util/test_cp.rs +++ b/tests/by-util/test_cp.rs @@ -1349,6 +1349,36 @@ fn test_cp_preserve_xattr_fails_on_android() { .fails(); } +#[test] +fn test_cp_issue_5031_case_1() { + let (at, mut ucmd) = at_and_ucmd!(); + + at.touch("a"); + at.symlink_file("a", "b"); + at.mkdir("c"); + + assert!(at.file_exists("a")); + assert!(at.symlink_exists("b")); + assert!(at.dir_exists("c")); + + ucmd.arg("--preserve=links") + .arg("-R") + .arg("-H") + .arg("a") + .arg("b") + .arg("c") + .succeeds(); + + assert!(at.dir_exists("c")); + assert!(at.plus("c").join("a").exists()); + assert!(at.plus("c").join("b").exists()); + + let metadata_a = std_fs::metadata(at.subdir.join("c").join("a")).unwrap(); + let metadata_b = std_fs::metadata(at.subdir.join("c").join("b")).unwrap(); + + assert_eq!(metadata_a.ino(), metadata_b.ino()); +} + #[test] // For now, disable the test on Windows. Symlinks aren't well support on Windows. // It works on Unix for now and it works locally when run from a powershell From a3803afef91af64f4e68e6d4b2a0ab05ec6d0b02 Mon Sep 17 00:00:00 2001 From: tommady Date: Thu, 13 Jul 2023 07:27:45 +0000 Subject: [PATCH 05/33] adding new testcase for 5031 --- src/uu/cp/src/cp.rs | 23 ++++++++++++++++++-- tests/by-util/test_cp.rs | 46 +++++++++++++++++++++++++++++++++++----- 2 files changed, 62 insertions(+), 7 deletions(-) diff --git a/src/uu/cp/src/cp.rs b/src/uu/cp/src/cp.rs index 29464bd98be..5c286aeeb60 100644 --- a/src/uu/cp/src/cp.rs +++ b/src/uu/cp/src/cp.rs @@ -1140,6 +1140,7 @@ fn copy(sources: &[Source], target: &TargetSlice, options: &Options) -> CopyResu let mut non_fatal_errors = false; let mut seen_sources = HashMap::with_capacity(sources.len()); + let mut should_hard_linked_files = HashMap::with_capacity(sources.len()); let mut symlinked_files = HashSet::new(); let progress_bar = if options.progress_bar { @@ -1159,18 +1160,23 @@ fn copy(sources: &[Source], target: &TargetSlice, options: &Options) -> CopyResu for source in sources.iter() { let dest = construct_dest_path(source, target, &target_type, options)?; - if seen_sources.contains_key(source) { + if seen_sources.contains_key(source) && !should_hard_linked_files.contains_key(source) { // FIXME: compare sources by the actual file they point to, not their path. (e.g. dir/file == dir/../dir/file in most cases) show_warning!("source {} specified more than once", source.quote()); + } else if let Some(new_source) = should_hard_linked_files.get(source) { + std::fs::hard_link(new_source, &dest)?; + should_hard_linked_files.remove(source); } else if preserve_hard_links && source.is_symlink() && cli_dereference { // issue 5031 case + // touch a && ln -s a b && mkdir c // cp --preserve=links -R -H a b c let mut original_source = source.read_link()?; while original_source.is_symlink() { original_source = original_source.read_link()?; } - // + // unwrap it because the read_link method is successed + // so file_name won't be failed original_source = original_source.file_name().unwrap().into(); match seen_sources.get(&original_source) { @@ -1180,7 +1186,20 @@ fn copy(sources: &[Source], target: &TargetSlice, options: &Options) -> CopyResu None => { // TODO: GNU cp can deal with this case // for example: + // touch a && ln -s a b && mkdir c // cp --preserve=links -R -H b a c + if let Err(error) = copy_source( + &progress_bar, + source, + target, + &target_type, + options, + &mut symlinked_files, + ) { + show_error_if_needed(&error); + non_fatal_errors = true; + } + should_hard_linked_files.insert(original_source, dest.clone()); } } } else { diff --git a/tests/by-util/test_cp.rs b/tests/by-util/test_cp.rs index 2cf006e37ed..9f69522736c 100644 --- a/tests/by-util/test_cp.rs +++ b/tests/by-util/test_cp.rs @@ -1369,14 +1369,50 @@ fn test_cp_issue_5031_case_1() { .arg("c") .succeeds(); + #[cfg(not(any(target_os = "freebsd", target_os = "macos")))] + { + assert!(at.dir_exists("c")); + assert!(at.plus("c").join("a").exists()); + assert!(at.plus("c").join("b").exists()); + + let metadata_a = std_fs::metadata(at.subdir.join("c").join("a")).unwrap(); + let metadata_b = std_fs::metadata(at.subdir.join("c").join("b")).unwrap(); + + assert_eq!(metadata_a.ino(), metadata_b.ino()); + } +} + +#[test] +fn test_cp_issue_5031_case_2() { + let (at, mut ucmd) = at_and_ucmd!(); + + at.touch("a"); + at.symlink_file("a", "b"); + at.mkdir("c"); + + assert!(at.file_exists("a")); + assert!(at.symlink_exists("b")); assert!(at.dir_exists("c")); - assert!(at.plus("c").join("a").exists()); - assert!(at.plus("c").join("b").exists()); - let metadata_a = std_fs::metadata(at.subdir.join("c").join("a")).unwrap(); - let metadata_b = std_fs::metadata(at.subdir.join("c").join("b")).unwrap(); + ucmd.arg("--preserve=links") + .arg("-R") + .arg("-H") + .arg("b") + .arg("a") + .arg("c") + .succeeds(); + + #[cfg(not(any(target_os = "freebsd", target_os = "macos")))] + { + assert!(at.dir_exists("c")); + assert!(at.plus("c").join("a").exists()); + assert!(at.plus("c").join("b").exists()); + + let metadata_a = std_fs::metadata(at.subdir.join("c").join("a")).unwrap(); + let metadata_b = std_fs::metadata(at.subdir.join("c").join("b")).unwrap(); - assert_eq!(metadata_a.ino(), metadata_b.ino()); + assert_eq!(metadata_a.ino(), metadata_b.ino()); + } } #[test] From 37e3a7caadd76ab894ed89b4a1eea52e37308acd Mon Sep 17 00:00:00 2001 From: Sylvestre Ledru Date: Thu, 13 Jul 2023 10:21:29 +0200 Subject: [PATCH 06/33] Fix a typo --- src/uu/cp/src/cp.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/uu/cp/src/cp.rs b/src/uu/cp/src/cp.rs index 5c286aeeb60..0d241584330 100644 --- a/src/uu/cp/src/cp.rs +++ b/src/uu/cp/src/cp.rs @@ -1175,7 +1175,7 @@ fn copy(sources: &[Source], target: &TargetSlice, options: &Options) -> CopyResu while original_source.is_symlink() { original_source = original_source.read_link()?; } - // unwrap it because the read_link method is successed + // unwrap it because the read_link method is sucessfull // so file_name won't be failed original_source = original_source.file_name().unwrap().into(); From 0cd08e731c0de36dde027f18a64a6458ab5754b2 Mon Sep 17 00:00:00 2001 From: tommady Date: Thu, 13 Jul 2023 11:17:25 +0000 Subject: [PATCH 07/33] add testcase 3 --- src/uu/cp/src/cp.rs | 45 ++++++++++++++++++++++--------------- tests/by-util/test_cp.rs | 48 +++++++++++++++++++++++++++++++++++----- 2 files changed, 69 insertions(+), 24 deletions(-) diff --git a/src/uu/cp/src/cp.rs b/src/uu/cp/src/cp.rs index 0d241584330..9d9debff195 100644 --- a/src/uu/cp/src/cp.rs +++ b/src/uu/cp/src/cp.rs @@ -1165,7 +1165,7 @@ fn copy(sources: &[Source], target: &TargetSlice, options: &Options) -> CopyResu show_warning!("source {} specified more than once", source.quote()); } else if let Some(new_source) = should_hard_linked_files.get(source) { std::fs::hard_link(new_source, &dest)?; - should_hard_linked_files.remove(source); + // should_hard_linked_files.remove(source); } else if preserve_hard_links && source.is_symlink() && cli_dereference { // issue 5031 case // touch a && ln -s a b && mkdir c @@ -1175,8 +1175,8 @@ fn copy(sources: &[Source], target: &TargetSlice, options: &Options) -> CopyResu while original_source.is_symlink() { original_source = original_source.read_link()?; } - // unwrap it because the read_link method is sucessfull - // so file_name won't be failed + // unwrap it because the read_link method above is ok + // so this file_name won't be failed original_source = original_source.file_name().unwrap().into(); match seen_sources.get(&original_source) { @@ -1184,22 +1184,31 @@ fn copy(sources: &[Source], target: &TargetSlice, options: &Options) -> CopyResu std::fs::hard_link(new_source, &dest)?; } None => { - // TODO: GNU cp can deal with this case - // for example: - // touch a && ln -s a b && mkdir c - // cp --preserve=links -R -H b a c - if let Err(error) = copy_source( - &progress_bar, - source, - target, - &target_type, - options, - &mut symlinked_files, - ) { - show_error_if_needed(&error); - non_fatal_errors = true; + match should_hard_linked_files.get(&original_source) { + Some(new_source) => { + // GNU cp can deal with this case: + // touch a && ln -s a b && ln -s b c && mkdir d + // cp --preserve=links -R -H b c d + std::fs::hard_link(new_source, &dest)?; + } + None => { + // GNU cp can deal with this case: + // touch a && ln -s a b && mkdir c + // cp --preserve=links -R -H b a c + if let Err(error) = copy_source( + &progress_bar, + source, + target, + &target_type, + options, + &mut symlinked_files, + ) { + show_error_if_needed(&error); + non_fatal_errors = true; + } + should_hard_linked_files.insert(original_source, dest.clone()); + } } - should_hard_linked_files.insert(original_source, dest.clone()); } } } else { diff --git a/tests/by-util/test_cp.rs b/tests/by-util/test_cp.rs index 9f69522736c..9643d020bfc 100644 --- a/tests/by-util/test_cp.rs +++ b/tests/by-util/test_cp.rs @@ -1369,14 +1369,14 @@ fn test_cp_issue_5031_case_1() { .arg("c") .succeeds(); - #[cfg(not(any(target_os = "freebsd", target_os = "macos")))] + #[cfg(all(unix, not(target_os = "freebsd")))] { assert!(at.dir_exists("c")); assert!(at.plus("c").join("a").exists()); assert!(at.plus("c").join("b").exists()); - let metadata_a = std_fs::metadata(at.subdir.join("c").join("a")).unwrap(); - let metadata_b = std_fs::metadata(at.subdir.join("c").join("b")).unwrap(); + let metadata_a = std::fs::metadata(at.subdir.join("c").join("a")).unwrap(); + let metadata_b = std::fs::metadata(at.subdir.join("c").join("b")).unwrap(); assert_eq!(metadata_a.ino(), metadata_b.ino()); } @@ -1402,19 +1402,55 @@ fn test_cp_issue_5031_case_2() { .arg("c") .succeeds(); - #[cfg(not(any(target_os = "freebsd", target_os = "macos")))] + #[cfg(all(unix, not(target_os = "freebsd")))] { assert!(at.dir_exists("c")); assert!(at.plus("c").join("a").exists()); assert!(at.plus("c").join("b").exists()); - let metadata_a = std_fs::metadata(at.subdir.join("c").join("a")).unwrap(); - let metadata_b = std_fs::metadata(at.subdir.join("c").join("b")).unwrap(); + let metadata_a = std::fs::metadata(at.subdir.join("c").join("a")).unwrap(); + let metadata_b = std::fs::metadata(at.subdir.join("c").join("b")).unwrap(); assert_eq!(metadata_a.ino(), metadata_b.ino()); } } +#[test] +fn test_cp_issue_5031_case_3() { + let (at, mut ucmd) = at_and_ucmd!(); + + at.touch("a"); + at.symlink_file("a", "b"); + at.symlink_file("b", "c"); + at.mkdir("d"); + + assert!(at.file_exists("a")); + assert!(at.symlink_exists("b")); + assert!(at.symlink_exists("c")); + assert!(at.dir_exists("d")); + + ucmd.arg("--preserve=links") + .arg("-R") + .arg("-H") + .arg("b") + .arg("c") + .arg("d") + .succeeds(); + + #[cfg(all(unix, not(target_os = "freebsd")))] + { + assert!(at.dir_exists("d")); + assert!(!at.plus("d").join("a").exists()); + assert!(at.plus("d").join("b").exists()); + assert!(at.plus("d").join("c").exists()); + + let metadata_b = std::fs::metadata(at.subdir.join("d").join("b")).unwrap(); + let metadata_c = std::fs::metadata(at.subdir.join("d").join("c")).unwrap(); + + assert_eq!(metadata_b.ino(), metadata_c.ino()); + } +} + #[test] // For now, disable the test on Windows. Symlinks aren't well support on Windows. // It works on Unix for now and it works locally when run from a powershell From 930137ca6d9cea7802a75a10f8c274ecdd4e8362 Mon Sep 17 00:00:00 2001 From: tommady Date: Thu, 13 Jul 2023 13:34:00 +0000 Subject: [PATCH 08/33] remove unused code --- src/uu/cp/src/cp.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/src/uu/cp/src/cp.rs b/src/uu/cp/src/cp.rs index 9d9debff195..affcdc10395 100644 --- a/src/uu/cp/src/cp.rs +++ b/src/uu/cp/src/cp.rs @@ -1165,7 +1165,6 @@ fn copy(sources: &[Source], target: &TargetSlice, options: &Options) -> CopyResu show_warning!("source {} specified more than once", source.quote()); } else if let Some(new_source) = should_hard_linked_files.get(source) { std::fs::hard_link(new_source, &dest)?; - // should_hard_linked_files.remove(source); } else if preserve_hard_links && source.is_symlink() && cli_dereference { // issue 5031 case // touch a && ln -s a b && mkdir c From 8820a04d9608a8a7dfbad0efbb40b848dbee8cdb Mon Sep 17 00:00:00 2001 From: tommady Date: Wed, 19 Jul 2023 12:18:28 +0000 Subject: [PATCH 09/33] by pass case 3 unit test on android --- tests/by-util/test_cp.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/tests/by-util/test_cp.rs b/tests/by-util/test_cp.rs index 4d5d1bb1fc7..6f2b2fee5a0 100644 --- a/tests/by-util/test_cp.rs +++ b/tests/by-util/test_cp.rs @@ -1416,6 +1416,10 @@ fn test_cp_issue_5031_case_2() { } #[test] +// android will +// stderr = cp: Permission denied (os error 13) +// panicked at Command was expected to succeed. +#[cfg(not(target_os = "android"))] fn test_cp_issue_5031_case_3() { let (at, mut ucmd) = at_and_ucmd!(); From b94be4cb3a02352d0d942d5dd42f7e34ab8699a2 Mon Sep 17 00:00:00 2001 From: tommady Date: Wed, 19 Jul 2023 14:00:24 +0000 Subject: [PATCH 10/33] by pass case 1 and 2 unit tests on android --- tests/by-util/test_cp.rs | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/tests/by-util/test_cp.rs b/tests/by-util/test_cp.rs index 6f2b2fee5a0..631ca7f8589 100644 --- a/tests/by-util/test_cp.rs +++ b/tests/by-util/test_cp.rs @@ -1350,6 +1350,10 @@ fn test_cp_preserve_xattr_fails_on_android() { } #[test] +// android will +// stderr = cp: Permission denied (os error 13) +// panicked at Command was expected to succeed. +#[cfg(not(target_os = "android"))] fn test_cp_issue_5031_case_1() { let (at, mut ucmd) = at_and_ucmd!(); @@ -1383,6 +1387,10 @@ fn test_cp_issue_5031_case_1() { } #[test] +// android will +// stderr = cp: Permission denied (os error 13) +// panicked at Command was expected to succeed. +#[cfg(not(target_os = "android"))] fn test_cp_issue_5031_case_2() { let (at, mut ucmd) = at_and_ucmd!(); From 94fb21210b65b14fee003fa45f1977402ecc133e Mon Sep 17 00:00:00 2001 From: tommady Date: Tue, 25 Jul 2023 08:22:38 +0000 Subject: [PATCH 11/33] add 5031 ut case 4 and fix it --- src/uu/cp/src/copydir.rs | 15 ++++- src/uu/cp/src/cp.rs | 115 ++++++++++++++++++++++----------------- tests/by-util/test_cp.rs | 36 ++++++++++++ 3 files changed, 115 insertions(+), 51 deletions(-) diff --git a/src/uu/cp/src/copydir.rs b/src/uu/cp/src/copydir.rs index aaeb73f5acf..3a6e3f61129 100644 --- a/src/uu/cp/src/copydir.rs +++ b/src/uu/cp/src/copydir.rs @@ -8,7 +8,7 @@ //! See the [`copy_directory`] function for more information. #[cfg(windows)] use std::borrow::Cow; -use std::collections::HashSet; +use std::collections::{HashMap, HashSet}; use std::env; use std::fs; use std::io; @@ -201,6 +201,8 @@ fn copy_direntry( symlinked_files: &mut HashSet, preserve_hard_links: bool, hard_links: &mut Vec<(String, u64)>, + seen_sources: &mut HashMap, + should_hard_linked_files: &mut HashMap, ) -> CopyResult<()> { let Entry { source_absolute, @@ -249,6 +251,8 @@ fn copy_direntry( local_to_target.as_path(), options, symlinked_files, + seen_sources, + should_hard_linked_files, false, ) { Ok(_) => Ok(()), @@ -277,6 +281,8 @@ fn copy_direntry( local_to_target.as_path(), options, symlinked_files, + seen_sources, + should_hard_linked_files, false, ) { Ok(_) => {} @@ -292,6 +298,7 @@ fn copy_direntry( Err(e) => return Err(e), } } + seen_sources.insert(source_relative, local_to_target); } // In any other case, there is nothing to do, so we just return to @@ -310,6 +317,8 @@ pub(crate) fn copy_directory( target: &TargetSlice, options: &Options, symlinked_files: &mut HashSet, + seen_sources: &mut HashMap, + should_hard_linked_files: &mut HashMap, source_in_command_line: bool, ) -> CopyResult<()> { if !options.recursive { @@ -324,6 +333,8 @@ pub(crate) fn copy_directory( target, options, symlinked_files, + seen_sources, + should_hard_linked_files, source_in_command_line, ); } @@ -398,6 +409,8 @@ pub(crate) fn copy_directory( symlinked_files, preserve_hard_links, &mut hard_links, + seen_sources, + should_hard_linked_files, )?; } // Print an error message, but continue traversing the directory. diff --git a/src/uu/cp/src/cp.rs b/src/uu/cp/src/cp.rs index affcdc10395..6420f1552b2 100644 --- a/src/uu/cp/src/cp.rs +++ b/src/uu/cp/src/cp.rs @@ -1134,7 +1134,6 @@ fn copy(sources: &[Source], target: &TargetSlice, options: &Options) -> CopyResu verify_target_type(target, &target_type)?; let preserve_hard_links = options.preserve_hard_links(); - let cli_dereference = options.cli_dereference; let mut hard_links: Vec<(String, u64)> = vec![]; @@ -1160,56 +1159,10 @@ fn copy(sources: &[Source], target: &TargetSlice, options: &Options) -> CopyResu for source in sources.iter() { let dest = construct_dest_path(source, target, &target_type, options)?; + if seen_sources.contains_key(source) && !should_hard_linked_files.contains_key(source) { // FIXME: compare sources by the actual file they point to, not their path. (e.g. dir/file == dir/../dir/file in most cases) show_warning!("source {} specified more than once", source.quote()); - } else if let Some(new_source) = should_hard_linked_files.get(source) { - std::fs::hard_link(new_source, &dest)?; - } else if preserve_hard_links && source.is_symlink() && cli_dereference { - // issue 5031 case - // touch a && ln -s a b && mkdir c - // cp --preserve=links -R -H a b c - - let mut original_source = source.read_link()?; - while original_source.is_symlink() { - original_source = original_source.read_link()?; - } - // unwrap it because the read_link method above is ok - // so this file_name won't be failed - original_source = original_source.file_name().unwrap().into(); - - match seen_sources.get(&original_source) { - Some(new_source) => { - std::fs::hard_link(new_source, &dest)?; - } - None => { - match should_hard_linked_files.get(&original_source) { - Some(new_source) => { - // GNU cp can deal with this case: - // touch a && ln -s a b && ln -s b c && mkdir d - // cp --preserve=links -R -H b c d - std::fs::hard_link(new_source, &dest)?; - } - None => { - // GNU cp can deal with this case: - // touch a && ln -s a b && mkdir c - // cp --preserve=links -R -H b a c - if let Err(error) = copy_source( - &progress_bar, - source, - target, - &target_type, - options, - &mut symlinked_files, - ) { - show_error_if_needed(&error); - non_fatal_errors = true; - } - should_hard_linked_files.insert(original_source, dest.clone()); - } - } - } - } } else { let found_hard_link = if preserve_hard_links && !source.is_dir() { preserve_hardlinks(&mut hard_links, source, &dest)? @@ -1224,14 +1177,17 @@ fn copy(sources: &[Source], target: &TargetSlice, options: &Options) -> CopyResu &target_type, options, &mut symlinked_files, + &mut seen_sources, + &mut should_hard_linked_files, ) { show_error_if_needed(&error); non_fatal_errors = true; } } + seen_sources.insert(source.to_path_buf(), dest); } - seen_sources.insert(source, dest); } + if non_fatal_errors { Err(Error::NotAllFilesCopied) } else { @@ -1277,11 +1233,22 @@ fn copy_source( target_type: &TargetType, options: &Options, symlinked_files: &mut HashSet, + seen_sources: &mut HashMap, + should_hard_linked_files: &mut HashMap, ) -> CopyResult<()> { let source_path = Path::new(&source); if source_path.is_dir() { // Copy as directory - copy_directory(progress_bar, source, target, options, symlinked_files, true) + copy_directory( + progress_bar, + source, + target, + options, + symlinked_files, + seen_sources, + should_hard_linked_files, + true, + ) } else { // Copy as file let dest = construct_dest_path(source_path, target, target_type, options)?; @@ -1291,6 +1258,8 @@ fn copy_source( dest.as_path(), options, symlinked_files, + seen_sources, + should_hard_linked_files, true, ); if options.parents { @@ -1625,6 +1594,8 @@ fn copy_file( dest: &Path, options: &Options, symlinked_files: &mut HashSet, + seen_sources: &mut HashMap, + should_hard_linked_files: &mut HashMap, source_in_command_line: bool, ) -> CopyResult<()> { if (options.update == UpdateMode::ReplaceIfOlder || options.update == UpdateMode::ReplaceNone) @@ -1635,6 +1606,50 @@ fn copy_file( return Ok(()); } + // issue 5031 case + // touch a && ln -s a b && mkdir c + // cp --preserve=links -R -H a b c + if let Some(base_name) = source.file_name() { + let base_source: PathBuf = base_name.into(); + if let Some(new_source) = should_hard_linked_files.get(&base_source) { + std::fs::hard_link(new_source, &dest)?; + return Ok(()); + } + } + if source.is_symlink() + && options.dereference(source_in_command_line) + && options.preserve_hard_links() + { + let mut original_source = source.read_link()?; + while original_source.is_symlink() { + original_source = original_source.read_link()?; + } + // unwrap it because the read_link method above is ok + // so this file_name won't be failed + original_source = original_source.file_name().unwrap().into(); + + match seen_sources.get(&original_source) { + Some(new_source) => { + std::fs::hard_link(new_source, &dest)?; + return Ok(()); + } + None => { + match should_hard_linked_files.get(&original_source) { + Some(new_source) => { + // GNU cp can deal with this case: + // touch a && ln -s a b && ln -s b c && mkdir d + // cp --preserve=links -R -H b c d + std::fs::hard_link(new_source, &dest)?; + return Ok(()); + } + None => { + should_hard_linked_files.insert(original_source, dest.to_path_buf()); + } + } + } + } + } + // Fail if dest is a dangling symlink or a symlink this program created previously if dest.is_symlink() { if FileInformation::from_path(dest, false) diff --git a/tests/by-util/test_cp.rs b/tests/by-util/test_cp.rs index 631ca7f8589..b9a7ea8559b 100644 --- a/tests/by-util/test_cp.rs +++ b/tests/by-util/test_cp.rs @@ -1463,6 +1463,42 @@ fn test_cp_issue_5031_case_3() { } } +#[test] +// android will +// stderr = cp: Permission denied (os error 13) +// panicked at Command was expected to succeed. +#[cfg(not(target_os = "android"))] +fn test_cp_issue_5031_case_4() { + let (at, mut ucmd) = at_and_ucmd!(); + + at.mkdir("d"); + at.touch("d/a"); + at.symlink_file("d/a", "d/b"); + + assert!(at.dir_exists("d")); + assert!(at.file_exists("d/a")); + assert!(at.symlink_exists("d/b")); + + ucmd.arg("--preserve=links") + .arg("-R") + .arg("-L") + .arg("d") + .arg("c") + .succeeds(); + + #[cfg(all(unix, not(target_os = "freebsd")))] + { + assert!(at.dir_exists("c")); + assert!(at.plus("c").join("a").exists()); + assert!(at.plus("c").join("b").exists()); + + let metadata_a = std::fs::metadata(at.subdir.join("c").join("a")).unwrap(); + let metadata_b = std::fs::metadata(at.subdir.join("c").join("b")).unwrap(); + + assert_eq!(metadata_a.ino(), metadata_b.ino()); + } +} + #[test] // For now, disable the test on Windows. Symlinks aren't well support on Windows. // It works on Unix for now and it works locally when run from a powershell From f49b86b52c07bcaea54cc471460bd19fc59926e9 Mon Sep 17 00:00:00 2001 From: tommady Date: Tue, 25 Jul 2023 08:30:28 +0000 Subject: [PATCH 12/33] add 5031 ut case 5 --- tests/by-util/test_cp.rs | 38 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/tests/by-util/test_cp.rs b/tests/by-util/test_cp.rs index b9a7ea8559b..6d6c0a56fbc 100644 --- a/tests/by-util/test_cp.rs +++ b/tests/by-util/test_cp.rs @@ -1499,6 +1499,44 @@ fn test_cp_issue_5031_case_4() { } } +#[test] +// android will +// stderr = cp: Permission denied (os error 13) +// panicked at Command was expected to succeed. +#[cfg(not(target_os = "android"))] +fn test_cp_issue_5031_case_5() { + let (at, mut ucmd) = at_and_ucmd!(); + + at.mkdir("c"); + at.mkdir("c/d"); + at.touch("c/d/a"); + at.symlink_file("c/d/a", "c/d/b"); + + assert!(at.dir_exists("c")); + assert!(at.dir_exists("c/d")); + assert!(at.file_exists("c/d/a")); + assert!(at.symlink_exists("c/d/b")); + + ucmd.arg("--preserve=links") + .arg("-R") + .arg("-L") + .arg("c") + .arg("e") + .succeeds(); + + #[cfg(all(unix, not(target_os = "freebsd")))] + { + assert!(at.dir_exists("e")); + assert!(at.plus("e").join("d").join("a").exists()); + assert!(at.plus("e").join("d").join("b").exists()); + + let metadata_a = std::fs::metadata(at.subdir.join("e").join("d").join("a")).unwrap(); + let metadata_b = std::fs::metadata(at.subdir.join("e").join("d").join("b")).unwrap(); + + assert_eq!(metadata_a.ino(), metadata_b.ino()); + } +} + #[test] // For now, disable the test on Windows. Symlinks aren't well support on Windows. // It works on Unix for now and it works locally when run from a powershell From 28278e2e6080577d2cb0b21e6b7ae70208ab04d4 Mon Sep 17 00:00:00 2001 From: tommady Date: Tue, 25 Jul 2023 10:07:55 +0000 Subject: [PATCH 13/33] fix linter issues --- src/uu/cp/src/copydir.rs | 2 ++ src/uu/cp/src/cp.rs | 8 +++++--- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/src/uu/cp/src/copydir.rs b/src/uu/cp/src/copydir.rs index 3a6e3f61129..7d3fe11b9da 100644 --- a/src/uu/cp/src/copydir.rs +++ b/src/uu/cp/src/copydir.rs @@ -194,6 +194,7 @@ where } /// Copy a single entry during a directory traversal. +#[allow(clippy::too_many_arguments)] fn copy_direntry( progress_bar: &Option, entry: Entry, @@ -311,6 +312,7 @@ fn copy_direntry( /// /// Any errors encountered copying files in the tree will be logged but /// will not cause a short-circuit. +#[allow(clippy::too_many_arguments)] pub(crate) fn copy_directory( progress_bar: &Option, root: &Path, diff --git a/src/uu/cp/src/cp.rs b/src/uu/cp/src/cp.rs index 6420f1552b2..ee55ef29983 100644 --- a/src/uu/cp/src/cp.rs +++ b/src/uu/cp/src/cp.rs @@ -1226,6 +1226,7 @@ fn construct_dest_path( }) } +#[allow(clippy::too_many_arguments)] fn copy_source( progress_bar: &Option, source: &SourceSlice, @@ -1588,6 +1589,7 @@ fn aligned_ancestors<'a>(source: &'a Path, dest: &'a Path) -> Vec<(&'a Path, &'a /// The original permissions of `source` will be copied to `dest` /// after a successful copy. #[allow(clippy::cognitive_complexity)] +#[allow(clippy::too_many_arguments)] fn copy_file( progress_bar: &Option, source: &Path, @@ -1612,7 +1614,7 @@ fn copy_file( if let Some(base_name) = source.file_name() { let base_source: PathBuf = base_name.into(); if let Some(new_source) = should_hard_linked_files.get(&base_source) { - std::fs::hard_link(new_source, &dest)?; + std::fs::hard_link(new_source, dest)?; return Ok(()); } } @@ -1630,7 +1632,7 @@ fn copy_file( match seen_sources.get(&original_source) { Some(new_source) => { - std::fs::hard_link(new_source, &dest)?; + std::fs::hard_link(new_source, dest)?; return Ok(()); } None => { @@ -1639,7 +1641,7 @@ fn copy_file( // GNU cp can deal with this case: // touch a && ln -s a b && ln -s b c && mkdir d // cp --preserve=links -R -H b c d - std::fs::hard_link(new_source, &dest)?; + std::fs::hard_link(new_source, dest)?; return Ok(()); } None => { From 395cee43c2ba8847a70906b01a8777c549b69772 Mon Sep 17 00:00:00 2001 From: tommady Date: Tue, 25 Jul 2023 12:49:27 +0000 Subject: [PATCH 14/33] fix unit test failed --- src/uu/cp/src/copydir.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/uu/cp/src/copydir.rs b/src/uu/cp/src/copydir.rs index 7d3fe11b9da..9230e65e4d2 100644 --- a/src/uu/cp/src/copydir.rs +++ b/src/uu/cp/src/copydir.rs @@ -299,7 +299,7 @@ fn copy_direntry( Err(e) => return Err(e), } } - seen_sources.insert(source_relative, local_to_target); + seen_sources.insert(source_relative.file_name().unwrap().into(), local_to_target); } // In any other case, there is nothing to do, so we just return to From 87bae227987eb3e3ceb2f3bd6497d1b5478438e7 Mon Sep 17 00:00:00 2001 From: tommady Date: Wed, 2 Aug 2023 05:27:11 +0000 Subject: [PATCH 15/33] align preserve links unit tests with gnu cp tests --- tests/by-util/test_cp.rs | 130 +++++++++++++++------------------------ 1 file changed, 51 insertions(+), 79 deletions(-) diff --git a/tests/by-util/test_cp.rs b/tests/by-util/test_cp.rs index 6d6c0a56fbc..c936a3c08e7 100644 --- a/tests/by-util/test_cp.rs +++ b/tests/by-util/test_cp.rs @@ -1350,28 +1350,15 @@ fn test_cp_preserve_xattr_fails_on_android() { } #[test] -// android will -// stderr = cp: Permission denied (os error 13) -// panicked at Command was expected to succeed. #[cfg(not(target_os = "android"))] -fn test_cp_issue_5031_case_1() { +fn test_cp_preserve_links_case_1() { let (at, mut ucmd) = at_and_ucmd!(); at.touch("a"); - at.symlink_file("a", "b"); + at.hard_link("a", "b"); at.mkdir("c"); - assert!(at.file_exists("a")); - assert!(at.symlink_exists("b")); - assert!(at.dir_exists("c")); - - ucmd.arg("--preserve=links") - .arg("-R") - .arg("-H") - .arg("a") - .arg("b") - .arg("c") - .succeeds(); + ucmd.arg("-d").arg("a").arg("b").arg("c").succeeds(); #[cfg(all(unix, not(target_os = "freebsd")))] { @@ -1387,26 +1374,19 @@ fn test_cp_issue_5031_case_1() { } #[test] -// android will -// stderr = cp: Permission denied (os error 13) -// panicked at Command was expected to succeed. #[cfg(not(target_os = "android"))] -fn test_cp_issue_5031_case_2() { +fn test_cp_preserve_links_case_2() { let (at, mut ucmd) = at_and_ucmd!(); at.touch("a"); at.symlink_file("a", "b"); at.mkdir("c"); - assert!(at.file_exists("a")); - assert!(at.symlink_exists("b")); - assert!(at.dir_exists("c")); - ucmd.arg("--preserve=links") .arg("-R") .arg("-H") - .arg("b") .arg("a") + .arg("b") .arg("c") .succeeds(); @@ -1424,60 +1404,42 @@ fn test_cp_issue_5031_case_2() { } #[test] -// android will -// stderr = cp: Permission denied (os error 13) -// panicked at Command was expected to succeed. #[cfg(not(target_os = "android"))] -fn test_cp_issue_5031_case_3() { +fn test_cp_preserve_links_case_3() { let (at, mut ucmd) = at_and_ucmd!(); - at.touch("a"); - at.symlink_file("a", "b"); - at.symlink_file("b", "c"); at.mkdir("d"); - - assert!(at.file_exists("a")); - assert!(at.symlink_exists("b")); - assert!(at.symlink_exists("c")); - assert!(at.dir_exists("d")); + at.touch("d/a"); + at.symlink_file("d/a", "d/b"); ucmd.arg("--preserve=links") .arg("-R") - .arg("-H") - .arg("b") - .arg("c") + .arg("-L") .arg("d") + .arg("c") .succeeds(); #[cfg(all(unix, not(target_os = "freebsd")))] { - assert!(at.dir_exists("d")); - assert!(!at.plus("d").join("a").exists()); - assert!(at.plus("d").join("b").exists()); - assert!(at.plus("d").join("c").exists()); + assert!(at.dir_exists("c")); + assert!(at.plus("c").join("a").exists()); + assert!(at.plus("c").join("b").exists()); - let metadata_b = std::fs::metadata(at.subdir.join("d").join("b")).unwrap(); - let metadata_c = std::fs::metadata(at.subdir.join("d").join("c")).unwrap(); + let metadata_a = std::fs::metadata(at.subdir.join("c").join("a")).unwrap(); + let metadata_b = std::fs::metadata(at.subdir.join("c").join("b")).unwrap(); - assert_eq!(metadata_b.ino(), metadata_c.ino()); + assert_eq!(metadata_a.ino(), metadata_b.ino()); } } #[test] -// android will -// stderr = cp: Permission denied (os error 13) -// panicked at Command was expected to succeed. #[cfg(not(target_os = "android"))] -fn test_cp_issue_5031_case_4() { +fn test_cp_preserve_links_case_4() { let (at, mut ucmd) = at_and_ucmd!(); at.mkdir("d"); at.touch("d/a"); - at.symlink_file("d/a", "d/b"); - - assert!(at.dir_exists("d")); - assert!(at.file_exists("d/a")); - assert!(at.symlink_exists("d/b")); + at.hard_link("d/a", "d/b"); ucmd.arg("--preserve=links") .arg("-R") @@ -1500,38 +1462,48 @@ fn test_cp_issue_5031_case_4() { } #[test] -// android will -// stderr = cp: Permission denied (os error 13) -// panicked at Command was expected to succeed. #[cfg(not(target_os = "android"))] -fn test_cp_issue_5031_case_5() { +fn test_cp_preserve_links_case_5() { let (at, mut ucmd) = at_and_ucmd!(); - at.mkdir("c"); - at.mkdir("c/d"); - at.touch("c/d/a"); - at.symlink_file("c/d/a", "c/d/b"); + at.mkdir("d"); + at.touch("d/a"); + at.hard_link("d/a", "d/b"); - assert!(at.dir_exists("c")); - assert!(at.dir_exists("c/d")); - assert!(at.file_exists("c/d/a")); - assert!(at.symlink_exists("c/d/b")); + ucmd.arg("--preserve=links").arg("d").arg("c").succeeds(); - ucmd.arg("--preserve=links") - .arg("-R") - .arg("-L") - .arg("c") - .arg("e") - .succeeds(); + #[cfg(all(unix, not(target_os = "freebsd")))] + { + assert!(at.dir_exists("c")); + assert!(at.plus("c").join("a").exists()); + assert!(at.plus("c").join("b").exists()); + + let metadata_a = std::fs::metadata(at.subdir.join("c").join("a")).unwrap(); + let metadata_b = std::fs::metadata(at.subdir.join("c").join("b")).unwrap(); + + assert_eq!(metadata_a.ino(), metadata_b.ino()); + } +} + +#[test] +#[cfg(not(target_os = "android"))] +fn test_cp_preserve_links_case_6() { + let (at, mut ucmd) = at_and_ucmd!(); + + at.touch("a"); + at.hard_link("a", "b"); + at.mkdir("c"); + + ucmd.arg("-d").arg("a").arg("b").arg("c").succeeds(); #[cfg(all(unix, not(target_os = "freebsd")))] { - assert!(at.dir_exists("e")); - assert!(at.plus("e").join("d").join("a").exists()); - assert!(at.plus("e").join("d").join("b").exists()); + assert!(at.dir_exists("c")); + assert!(at.plus("c").join("a").exists()); + assert!(at.plus("c").join("b").exists()); - let metadata_a = std::fs::metadata(at.subdir.join("e").join("d").join("a")).unwrap(); - let metadata_b = std::fs::metadata(at.subdir.join("e").join("d").join("b")).unwrap(); + let metadata_a = std::fs::metadata(at.subdir.join("c").join("a")).unwrap(); + let metadata_b = std::fs::metadata(at.subdir.join("c").join("b")).unwrap(); assert_eq!(metadata_a.ino(), metadata_b.ino()); } From bf87a443ffa131b6dee5b82d9d6b6e4e3fef5d30 Mon Sep 17 00:00:00 2001 From: tommady Date: Wed, 2 Aug 2023 05:58:09 +0000 Subject: [PATCH 16/33] correct preserve links unit test case 5 --- tests/by-util/test_cp.rs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/tests/by-util/test_cp.rs b/tests/by-util/test_cp.rs index c936a3c08e7..28883a41950 100644 --- a/tests/by-util/test_cp.rs +++ b/tests/by-util/test_cp.rs @@ -1470,7 +1470,11 @@ fn test_cp_preserve_links_case_5() { at.touch("d/a"); at.hard_link("d/a", "d/b"); - ucmd.arg("--preserve=links").arg("d").arg("c").succeeds(); + ucmd.arg("-dR") + .arg("--no-preserve=links") + .arg("d") + .arg("c") + .succeeds(); #[cfg(all(unix, not(target_os = "freebsd")))] { From 340df6ba3944b32394c32e329de99d620b12c9f3 Mon Sep 17 00:00:00 2001 From: tommady Date: Thu, 3 Aug 2023 10:10:51 +0000 Subject: [PATCH 17/33] using single copied FileInformation map to do the job --- src/uu/cp/src/copydir.rs | 40 ++++++++++++--------- src/uu/cp/src/cp.rs | 77 +++++++++++----------------------------- 2 files changed, 43 insertions(+), 74 deletions(-) diff --git a/src/uu/cp/src/copydir.rs b/src/uu/cp/src/copydir.rs index 9230e65e4d2..dc16642c17e 100644 --- a/src/uu/cp/src/copydir.rs +++ b/src/uu/cp/src/copydir.rs @@ -194,7 +194,6 @@ where } /// Copy a single entry during a directory traversal. -#[allow(clippy::too_many_arguments)] fn copy_direntry( progress_bar: &Option, entry: Entry, @@ -202,8 +201,7 @@ fn copy_direntry( symlinked_files: &mut HashSet, preserve_hard_links: bool, hard_links: &mut Vec<(String, u64)>, - seen_sources: &mut HashMap, - should_hard_linked_files: &mut HashMap, + copied_files: &mut HashMap, ) -> CopyResult<()> { let Entry { source_absolute, @@ -242,8 +240,8 @@ fn copy_direntry( // If the source is not a directory, then we need to copy the file. if !source_absolute.is_dir() { + let dest = local_to_target.as_path().to_path_buf(); if preserve_hard_links { - let dest = local_to_target.as_path().to_path_buf(); let found_hard_link = preserve_hardlinks(hard_links, &source_absolute, &dest)?; if !found_hard_link { match copy_file( @@ -252,11 +250,19 @@ fn copy_direntry( local_to_target.as_path(), options, symlinked_files, - seen_sources, - should_hard_linked_files, + copied_files, false, ) { - Ok(_) => Ok(()), + Ok(_) => { + copied_files.insert( + FileInformation::from_path( + &source_absolute, + options.dereference(false), + )?, + dest, + ); + Ok(()) + } Err(err) => { if source_absolute.is_symlink() { // silent the error with a symlink @@ -282,11 +288,15 @@ fn copy_direntry( local_to_target.as_path(), options, symlinked_files, - seen_sources, - should_hard_linked_files, + copied_files, false, ) { - Ok(_) => {} + Ok(_) => { + copied_files.insert( + FileInformation::from_path(&source_absolute, options.dereference(false))?, + dest, + ); + } Err(Error::IoErrContext(e, _)) if e.kind() == std::io::ErrorKind::PermissionDenied => { @@ -299,7 +309,6 @@ fn copy_direntry( Err(e) => return Err(e), } } - seen_sources.insert(source_relative.file_name().unwrap().into(), local_to_target); } // In any other case, there is nothing to do, so we just return to @@ -319,8 +328,7 @@ pub(crate) fn copy_directory( target: &TargetSlice, options: &Options, symlinked_files: &mut HashSet, - seen_sources: &mut HashMap, - should_hard_linked_files: &mut HashMap, + copied_files: &mut HashMap, source_in_command_line: bool, ) -> CopyResult<()> { if !options.recursive { @@ -335,8 +343,7 @@ pub(crate) fn copy_directory( target, options, symlinked_files, - seen_sources, - should_hard_linked_files, + copied_files, source_in_command_line, ); } @@ -411,8 +418,7 @@ pub(crate) fn copy_directory( symlinked_files, preserve_hard_links, &mut hard_links, - seen_sources, - should_hard_linked_files, + copied_files, )?; } // Print an error message, but continue traversing the directory. diff --git a/src/uu/cp/src/cp.rs b/src/uu/cp/src/cp.rs index ee55ef29983..250c2b4824a 100644 --- a/src/uu/cp/src/cp.rs +++ b/src/uu/cp/src/cp.rs @@ -1138,9 +1138,9 @@ fn copy(sources: &[Source], target: &TargetSlice, options: &Options) -> CopyResu let mut hard_links: Vec<(String, u64)> = vec![]; let mut non_fatal_errors = false; - let mut seen_sources = HashMap::with_capacity(sources.len()); - let mut should_hard_linked_files = HashMap::with_capacity(sources.len()); + let mut seen_sources = HashSet::with_capacity(sources.len()); let mut symlinked_files = HashSet::new(); + let mut copied_files: HashMap = HashMap::with_capacity(sources.len()); let progress_bar = if options.progress_bar { let pb = ProgressBar::new(disk_usage(sources, options.recursive)?) @@ -1158,12 +1158,11 @@ fn copy(sources: &[Source], target: &TargetSlice, options: &Options) -> CopyResu }; for source in sources.iter() { - let dest = construct_dest_path(source, target, &target_type, options)?; - - if seen_sources.contains_key(source) && !should_hard_linked_files.contains_key(source) { + 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) show_warning!("source {} specified more than once", source.quote()); } else { + let dest = construct_dest_path(source, target, &target_type, options)?; let found_hard_link = if preserve_hard_links && !source.is_dir() { preserve_hardlinks(&mut hard_links, source, &dest)? } else { @@ -1177,14 +1176,17 @@ fn copy(sources: &[Source], target: &TargetSlice, options: &Options) -> CopyResu &target_type, options, &mut symlinked_files, - &mut seen_sources, - &mut should_hard_linked_files, + &mut copied_files, ) { show_error_if_needed(&error); non_fatal_errors = true; } + copied_files.insert( + FileInformation::from_path(source, options.dereference(true))?, + dest, + ); } - seen_sources.insert(source.to_path_buf(), dest); + seen_sources.insert(source); } } @@ -1226,7 +1228,6 @@ fn construct_dest_path( }) } -#[allow(clippy::too_many_arguments)] fn copy_source( progress_bar: &Option, source: &SourceSlice, @@ -1234,8 +1235,7 @@ fn copy_source( target_type: &TargetType, options: &Options, symlinked_files: &mut HashSet, - seen_sources: &mut HashMap, - should_hard_linked_files: &mut HashMap, + copied_files: &mut HashMap, ) -> CopyResult<()> { let source_path = Path::new(&source); if source_path.is_dir() { @@ -1246,8 +1246,7 @@ fn copy_source( target, options, symlinked_files, - seen_sources, - should_hard_linked_files, + copied_files, true, ) } else { @@ -1259,8 +1258,7 @@ fn copy_source( dest.as_path(), options, symlinked_files, - seen_sources, - should_hard_linked_files, + copied_files, true, ); if options.parents { @@ -1596,8 +1594,7 @@ fn copy_file( dest: &Path, options: &Options, symlinked_files: &mut HashSet, - seen_sources: &mut HashMap, - should_hard_linked_files: &mut HashMap, + copied_files: &mut HashMap, source_in_command_line: bool, ) -> CopyResult<()> { if (options.update == UpdateMode::ReplaceIfOlder || options.update == UpdateMode::ReplaceNone) @@ -1608,48 +1605,14 @@ fn copy_file( return Ok(()); } - // issue 5031 case - // touch a && ln -s a b && mkdir c - // cp --preserve=links -R -H a b c - if let Some(base_name) = source.file_name() { - let base_source: PathBuf = base_name.into(); - if let Some(new_source) = should_hard_linked_files.get(&base_source) { + if options.dereference(source_in_command_line) && options.preserve_hard_links() { + if let Some(new_source) = copied_files.get(&FileInformation::from_path( + source, + options.dereference(source_in_command_line), + )?) { std::fs::hard_link(new_source, dest)?; return Ok(()); - } - } - if source.is_symlink() - && options.dereference(source_in_command_line) - && options.preserve_hard_links() - { - let mut original_source = source.read_link()?; - while original_source.is_symlink() { - original_source = original_source.read_link()?; - } - // unwrap it because the read_link method above is ok - // so this file_name won't be failed - original_source = original_source.file_name().unwrap().into(); - - match seen_sources.get(&original_source) { - Some(new_source) => { - std::fs::hard_link(new_source, dest)?; - return Ok(()); - } - None => { - match should_hard_linked_files.get(&original_source) { - Some(new_source) => { - // GNU cp can deal with this case: - // touch a && ln -s a b && ln -s b c && mkdir d - // cp --preserve=links -R -H b c d - std::fs::hard_link(new_source, dest)?; - return Ok(()); - } - None => { - should_hard_linked_files.insert(original_source, dest.to_path_buf()); - } - } - } - } + }; } // Fail if dest is a dangling symlink or a symlink this program created previously From 5bf83d08bd7e1b21c6358fd4fd91f05b797e5399 Mon Sep 17 00:00:00 2001 From: tommady Date: Thu, 3 Aug 2023 10:53:07 +0000 Subject: [PATCH 18/33] remove unused clippy too many arg allowance --- src/uu/cp/src/copydir.rs | 1 - src/uu/cp/src/cp.rs | 4 ++-- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/src/uu/cp/src/copydir.rs b/src/uu/cp/src/copydir.rs index dc16642c17e..a2567053927 100644 --- a/src/uu/cp/src/copydir.rs +++ b/src/uu/cp/src/copydir.rs @@ -321,7 +321,6 @@ fn copy_direntry( /// /// Any errors encountered copying files in the tree will be logged but /// will not cause a short-circuit. -#[allow(clippy::too_many_arguments)] pub(crate) fn copy_directory( progress_bar: &Option, root: &Path, diff --git a/src/uu/cp/src/cp.rs b/src/uu/cp/src/cp.rs index 250c2b4824a..2f6dc0ad29c 100644 --- a/src/uu/cp/src/cp.rs +++ b/src/uu/cp/src/cp.rs @@ -1587,7 +1587,6 @@ fn aligned_ancestors<'a>(source: &'a Path, dest: &'a Path) -> Vec<(&'a Path, &'a /// The original permissions of `source` will be copied to `dest` /// after a successful copy. #[allow(clippy::cognitive_complexity)] -#[allow(clippy::too_many_arguments)] fn copy_file( progress_bar: &Option, source: &Path, @@ -1605,7 +1604,8 @@ fn copy_file( return Ok(()); } - if options.dereference(source_in_command_line) && options.preserve_hard_links() { + // issue 5031 + if options.preserve_hard_links() { if let Some(new_source) = copied_files.get(&FileInformation::from_path( source, options.dereference(source_in_command_line), From 26b5727acfb1cacf07efbfc5cd8b4065b8ad6a32 Mon Sep 17 00:00:00 2001 From: tommady Date: Thu, 3 Aug 2023 12:50:03 +0000 Subject: [PATCH 19/33] adding some comments to explain the copied_file hashmap and the hard_link --- src/uu/cp/src/cp.rs | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/src/uu/cp/src/cp.rs b/src/uu/cp/src/cp.rs index 2f6dc0ad29c..f6de3fa73ca 100644 --- a/src/uu/cp/src/cp.rs +++ b/src/uu/cp/src/cp.rs @@ -1140,6 +1140,14 @@ fn copy(sources: &[Source], target: &TargetSlice, options: &Options) -> CopyResu let mut non_fatal_errors = false; let mut seen_sources = HashSet::with_capacity(sources.len()); let mut symlinked_files = HashSet::new(); + + // to remember the copied files for further usage. + // the FileInformation implemented the Hash trait by using + // 1. inode number + // 2. device number + // the combination of a file's inode number and device number is unique throughout all the file systems. + // + // the copied_files hashmap's key is the source file's information and the value is the destination filepath. let mut copied_files: HashMap = HashMap::with_capacity(sources.len()); let progress_bar = if options.progress_bar { @@ -1606,6 +1614,9 @@ fn copy_file( // issue 5031 if options.preserve_hard_links() { + // if we encounter a matching device/inode pair in the source tree + // we can arrange to create a hard link between the corresponding names + // in the destination tree. if let Some(new_source) = copied_files.get(&FileInformation::from_path( source, options.dereference(source_in_command_line), From 36487a9a76e4a78616f4845b69721b00c31dd029 Mon Sep 17 00:00:00 2001 From: tommady Date: Thu, 3 Aug 2023 12:55:04 +0000 Subject: [PATCH 20/33] fix ERROR: Unknown word (hashmap's) --- src/uu/cp/src/cp.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/uu/cp/src/cp.rs b/src/uu/cp/src/cp.rs index f6de3fa73ca..00987ccf92e 100644 --- a/src/uu/cp/src/cp.rs +++ b/src/uu/cp/src/cp.rs @@ -1147,7 +1147,7 @@ fn copy(sources: &[Source], target: &TargetSlice, options: &Options) -> CopyResu // 2. device number // the combination of a file's inode number and device number is unique throughout all the file systems. // - // the copied_files hashmap's key is the source file's information and the value is the destination filepath. + // the copied_files's key is the source file's information and the value is the destination filepath. let mut copied_files: HashMap = HashMap::with_capacity(sources.len()); let progress_bar = if options.progress_bar { From d77e2a5f68d90c1f1082de6255501d2a338e32a3 Mon Sep 17 00:00:00 2001 From: tommady Date: Thu, 3 Aug 2023 13:02:19 +0000 Subject: [PATCH 21/33] fix ERROR: Unknown word (files's) --- src/uu/cp/src/cp.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/uu/cp/src/cp.rs b/src/uu/cp/src/cp.rs index 00987ccf92e..0c02a2bdd33 100644 --- a/src/uu/cp/src/cp.rs +++ b/src/uu/cp/src/cp.rs @@ -1147,7 +1147,7 @@ fn copy(sources: &[Source], target: &TargetSlice, options: &Options) -> CopyResu // 2. device number // the combination of a file's inode number and device number is unique throughout all the file systems. // - // the copied_files's key is the source file's information and the value is the destination filepath. + // key is the source file's information and the value is the destination filepath. let mut copied_files: HashMap = HashMap::with_capacity(sources.len()); let progress_bar = if options.progress_bar { From ba6984148ce031bcff46b33d555368738ec23ead Mon Sep 17 00:00:00 2001 From: tommady Date: Fri, 11 Aug 2023 11:38:20 +0000 Subject: [PATCH 22/33] let the unit tests run on android and freebsd --- tests/by-util/test_cp.rs | 96 +++++++++++++++------------------------- 1 file changed, 36 insertions(+), 60 deletions(-) diff --git a/tests/by-util/test_cp.rs b/tests/by-util/test_cp.rs index 28883a41950..949b59fa78e 100644 --- a/tests/by-util/test_cp.rs +++ b/tests/by-util/test_cp.rs @@ -1350,7 +1350,6 @@ fn test_cp_preserve_xattr_fails_on_android() { } #[test] -#[cfg(not(target_os = "android"))] fn test_cp_preserve_links_case_1() { let (at, mut ucmd) = at_and_ucmd!(); @@ -1360,21 +1359,17 @@ fn test_cp_preserve_links_case_1() { ucmd.arg("-d").arg("a").arg("b").arg("c").succeeds(); - #[cfg(all(unix, not(target_os = "freebsd")))] - { - assert!(at.dir_exists("c")); - assert!(at.plus("c").join("a").exists()); - assert!(at.plus("c").join("b").exists()); + assert!(at.dir_exists("c")); + assert!(at.plus("c").join("a").exists()); + assert!(at.plus("c").join("b").exists()); - let metadata_a = std::fs::metadata(at.subdir.join("c").join("a")).unwrap(); - let metadata_b = std::fs::metadata(at.subdir.join("c").join("b")).unwrap(); + let metadata_a = std::fs::metadata(at.subdir.join("c").join("a")).unwrap(); + let metadata_b = std::fs::metadata(at.subdir.join("c").join("b")).unwrap(); - assert_eq!(metadata_a.ino(), metadata_b.ino()); - } + assert_eq!(metadata_a.ino(), metadata_b.ino()); } #[test] -#[cfg(not(target_os = "android"))] fn test_cp_preserve_links_case_2() { let (at, mut ucmd) = at_and_ucmd!(); @@ -1390,21 +1385,17 @@ fn test_cp_preserve_links_case_2() { .arg("c") .succeeds(); - #[cfg(all(unix, not(target_os = "freebsd")))] - { - assert!(at.dir_exists("c")); - assert!(at.plus("c").join("a").exists()); - assert!(at.plus("c").join("b").exists()); + assert!(at.dir_exists("c")); + assert!(at.plus("c").join("a").exists()); + assert!(at.plus("c").join("b").exists()); - let metadata_a = std::fs::metadata(at.subdir.join("c").join("a")).unwrap(); - let metadata_b = std::fs::metadata(at.subdir.join("c").join("b")).unwrap(); + let metadata_a = std::fs::metadata(at.subdir.join("c").join("a")).unwrap(); + let metadata_b = std::fs::metadata(at.subdir.join("c").join("b")).unwrap(); - assert_eq!(metadata_a.ino(), metadata_b.ino()); - } + assert_eq!(metadata_a.ino(), metadata_b.ino()); } #[test] -#[cfg(not(target_os = "android"))] fn test_cp_preserve_links_case_3() { let (at, mut ucmd) = at_and_ucmd!(); @@ -1419,21 +1410,17 @@ fn test_cp_preserve_links_case_3() { .arg("c") .succeeds(); - #[cfg(all(unix, not(target_os = "freebsd")))] - { - assert!(at.dir_exists("c")); - assert!(at.plus("c").join("a").exists()); - assert!(at.plus("c").join("b").exists()); + assert!(at.dir_exists("c")); + assert!(at.plus("c").join("a").exists()); + assert!(at.plus("c").join("b").exists()); - let metadata_a = std::fs::metadata(at.subdir.join("c").join("a")).unwrap(); - let metadata_b = std::fs::metadata(at.subdir.join("c").join("b")).unwrap(); + let metadata_a = std::fs::metadata(at.subdir.join("c").join("a")).unwrap(); + let metadata_b = std::fs::metadata(at.subdir.join("c").join("b")).unwrap(); - assert_eq!(metadata_a.ino(), metadata_b.ino()); - } + assert_eq!(metadata_a.ino(), metadata_b.ino()); } #[test] -#[cfg(not(target_os = "android"))] fn test_cp_preserve_links_case_4() { let (at, mut ucmd) = at_and_ucmd!(); @@ -1448,21 +1435,17 @@ fn test_cp_preserve_links_case_4() { .arg("c") .succeeds(); - #[cfg(all(unix, not(target_os = "freebsd")))] - { - assert!(at.dir_exists("c")); - assert!(at.plus("c").join("a").exists()); - assert!(at.plus("c").join("b").exists()); + assert!(at.dir_exists("c")); + assert!(at.plus("c").join("a").exists()); + assert!(at.plus("c").join("b").exists()); - let metadata_a = std::fs::metadata(at.subdir.join("c").join("a")).unwrap(); - let metadata_b = std::fs::metadata(at.subdir.join("c").join("b")).unwrap(); + let metadata_a = std::fs::metadata(at.subdir.join("c").join("a")).unwrap(); + let metadata_b = std::fs::metadata(at.subdir.join("c").join("b")).unwrap(); - assert_eq!(metadata_a.ino(), metadata_b.ino()); - } + assert_eq!(metadata_a.ino(), metadata_b.ino()); } #[test] -#[cfg(not(target_os = "android"))] fn test_cp_preserve_links_case_5() { let (at, mut ucmd) = at_and_ucmd!(); @@ -1476,21 +1459,17 @@ fn test_cp_preserve_links_case_5() { .arg("c") .succeeds(); - #[cfg(all(unix, not(target_os = "freebsd")))] - { - assert!(at.dir_exists("c")); - assert!(at.plus("c").join("a").exists()); - assert!(at.plus("c").join("b").exists()); + assert!(at.dir_exists("c")); + assert!(at.plus("c").join("a").exists()); + assert!(at.plus("c").join("b").exists()); - let metadata_a = std::fs::metadata(at.subdir.join("c").join("a")).unwrap(); - let metadata_b = std::fs::metadata(at.subdir.join("c").join("b")).unwrap(); + let metadata_a = std::fs::metadata(at.subdir.join("c").join("a")).unwrap(); + let metadata_b = std::fs::metadata(at.subdir.join("c").join("b")).unwrap(); - assert_eq!(metadata_a.ino(), metadata_b.ino()); - } + assert_eq!(metadata_a.ino(), metadata_b.ino()); } #[test] -#[cfg(not(target_os = "android"))] fn test_cp_preserve_links_case_6() { let (at, mut ucmd) = at_and_ucmd!(); @@ -1500,17 +1479,14 @@ fn test_cp_preserve_links_case_6() { ucmd.arg("-d").arg("a").arg("b").arg("c").succeeds(); - #[cfg(all(unix, not(target_os = "freebsd")))] - { - assert!(at.dir_exists("c")); - assert!(at.plus("c").join("a").exists()); - assert!(at.plus("c").join("b").exists()); + assert!(at.dir_exists("c")); + assert!(at.plus("c").join("a").exists()); + assert!(at.plus("c").join("b").exists()); - let metadata_a = std::fs::metadata(at.subdir.join("c").join("a")).unwrap(); - let metadata_b = std::fs::metadata(at.subdir.join("c").join("b")).unwrap(); + let metadata_a = std::fs::metadata(at.subdir.join("c").join("a")).unwrap(); + let metadata_b = std::fs::metadata(at.subdir.join("c").join("b")).unwrap(); - assert_eq!(metadata_a.ino(), metadata_b.ino()); - } + assert_eq!(metadata_a.ino(), metadata_b.ino()); } #[test] From 2b7e3c4178fbb1b6670bbf51583489e701e047cb Mon Sep 17 00:00:00 2001 From: tommady Date: Fri, 11 Aug 2023 14:26:30 +0000 Subject: [PATCH 23/33] let the unit tests run on unix platform while the inode assertion --- tests/by-util/test_cp.rs | 54 ++++++++++++++++++++++++++-------------- 1 file changed, 36 insertions(+), 18 deletions(-) diff --git a/tests/by-util/test_cp.rs b/tests/by-util/test_cp.rs index 949b59fa78e..6f7b19fbc1d 100644 --- a/tests/by-util/test_cp.rs +++ b/tests/by-util/test_cp.rs @@ -1363,10 +1363,13 @@ fn test_cp_preserve_links_case_1() { assert!(at.plus("c").join("a").exists()); assert!(at.plus("c").join("b").exists()); - let metadata_a = std::fs::metadata(at.subdir.join("c").join("a")).unwrap(); - let metadata_b = std::fs::metadata(at.subdir.join("c").join("b")).unwrap(); + #[cfg(all(unix))] + { + let metadata_a = std::fs::metadata(at.subdir.join("c").join("a")).unwrap(); + let metadata_b = std::fs::metadata(at.subdir.join("c").join("b")).unwrap(); - assert_eq!(metadata_a.ino(), metadata_b.ino()); + assert_eq!(metadata_a.ino(), metadata_b.ino()); + } } #[test] @@ -1389,10 +1392,13 @@ fn test_cp_preserve_links_case_2() { assert!(at.plus("c").join("a").exists()); assert!(at.plus("c").join("b").exists()); - let metadata_a = std::fs::metadata(at.subdir.join("c").join("a")).unwrap(); - let metadata_b = std::fs::metadata(at.subdir.join("c").join("b")).unwrap(); + #[cfg(all(unix))] + { + let metadata_a = std::fs::metadata(at.subdir.join("c").join("a")).unwrap(); + let metadata_b = std::fs::metadata(at.subdir.join("c").join("b")).unwrap(); - assert_eq!(metadata_a.ino(), metadata_b.ino()); + assert_eq!(metadata_a.ino(), metadata_b.ino()); + } } #[test] @@ -1414,10 +1420,13 @@ fn test_cp_preserve_links_case_3() { assert!(at.plus("c").join("a").exists()); assert!(at.plus("c").join("b").exists()); - let metadata_a = std::fs::metadata(at.subdir.join("c").join("a")).unwrap(); - let metadata_b = std::fs::metadata(at.subdir.join("c").join("b")).unwrap(); + #[cfg(all(unix))] + { + let metadata_a = std::fs::metadata(at.subdir.join("c").join("a")).unwrap(); + let metadata_b = std::fs::metadata(at.subdir.join("c").join("b")).unwrap(); - assert_eq!(metadata_a.ino(), metadata_b.ino()); + assert_eq!(metadata_a.ino(), metadata_b.ino()); + } } #[test] @@ -1439,10 +1448,13 @@ fn test_cp_preserve_links_case_4() { assert!(at.plus("c").join("a").exists()); assert!(at.plus("c").join("b").exists()); - let metadata_a = std::fs::metadata(at.subdir.join("c").join("a")).unwrap(); - let metadata_b = std::fs::metadata(at.subdir.join("c").join("b")).unwrap(); + #[cfg(all(unix))] + { + let metadata_a = std::fs::metadata(at.subdir.join("c").join("a")).unwrap(); + let metadata_b = std::fs::metadata(at.subdir.join("c").join("b")).unwrap(); - assert_eq!(metadata_a.ino(), metadata_b.ino()); + assert_eq!(metadata_a.ino(), metadata_b.ino()); + } } #[test] @@ -1463,10 +1475,13 @@ fn test_cp_preserve_links_case_5() { assert!(at.plus("c").join("a").exists()); assert!(at.plus("c").join("b").exists()); - let metadata_a = std::fs::metadata(at.subdir.join("c").join("a")).unwrap(); - let metadata_b = std::fs::metadata(at.subdir.join("c").join("b")).unwrap(); + #[cfg(all(unix))] + { + let metadata_a = std::fs::metadata(at.subdir.join("c").join("a")).unwrap(); + let metadata_b = std::fs::metadata(at.subdir.join("c").join("b")).unwrap(); - assert_eq!(metadata_a.ino(), metadata_b.ino()); + assert_eq!(metadata_a.ino(), metadata_b.ino()); + } } #[test] @@ -1483,10 +1498,13 @@ fn test_cp_preserve_links_case_6() { assert!(at.plus("c").join("a").exists()); assert!(at.plus("c").join("b").exists()); - let metadata_a = std::fs::metadata(at.subdir.join("c").join("a")).unwrap(); - let metadata_b = std::fs::metadata(at.subdir.join("c").join("b")).unwrap(); + #[cfg(all(unix))] + { + let metadata_a = std::fs::metadata(at.subdir.join("c").join("a")).unwrap(); + let metadata_b = std::fs::metadata(at.subdir.join("c").join("b")).unwrap(); - assert_eq!(metadata_a.ino(), metadata_b.ino()); + assert_eq!(metadata_a.ino(), metadata_b.ino()); + } } #[test] From 3554e6277b655eb5ca84461fd53b2bbbeaedd5d8 Mon Sep 17 00:00:00 2001 From: tommady Date: Sat, 12 Aug 2023 06:36:50 +0000 Subject: [PATCH 24/33] let MetadataExt can be used while testing on freebsd platform --- tests/by-util/test_cp.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/by-util/test_cp.rs b/tests/by-util/test_cp.rs index 6f7b19fbc1d..321ae138fc6 100644 --- a/tests/by-util/test_cp.rs +++ b/tests/by-util/test_cp.rs @@ -7,7 +7,7 @@ use std::fs::set_permissions; #[cfg(not(windows))] use std::os::unix::fs; -#[cfg(all(unix, not(target_os = "freebsd")))] +#[cfg(all(unix))] use std::os::unix::fs::MetadataExt; #[cfg(all(unix, not(target_os = "freebsd")))] use std::os::unix::fs::PermissionsExt; From c592a0ab49b212ee03148c47b7bf9e9107aeac54 Mon Sep 17 00:00:00 2001 From: tommady Date: Sun, 13 Aug 2023 06:55:04 +0000 Subject: [PATCH 25/33] address comments --- src/uu/cp/src/cp.rs | 1 - tests/by-util/test_cp.rs | 26 +++++++++++++++++++------- 2 files changed, 19 insertions(+), 8 deletions(-) diff --git a/src/uu/cp/src/cp.rs b/src/uu/cp/src/cp.rs index 0c02a2bdd33..0183cec81b4 100644 --- a/src/uu/cp/src/cp.rs +++ b/src/uu/cp/src/cp.rs @@ -1612,7 +1612,6 @@ fn copy_file( return Ok(()); } - // issue 5031 if options.preserve_hard_links() { // if we encounter a matching device/inode pair in the source tree // we can arrange to create a hard link between the corresponding names diff --git a/tests/by-util/test_cp.rs b/tests/by-util/test_cp.rs index 321ae138fc6..19d29dbed84 100644 --- a/tests/by-util/test_cp.rs +++ b/tests/by-util/test_cp.rs @@ -7,7 +7,7 @@ use std::fs::set_permissions; #[cfg(not(windows))] use std::os::unix::fs; -#[cfg(all(unix))] +#[cfg(unix)] use std::os::unix::fs::MetadataExt; #[cfg(all(unix, not(target_os = "freebsd")))] use std::os::unix::fs::PermissionsExt; @@ -1350,6 +1350,8 @@ fn test_cp_preserve_xattr_fails_on_android() { } #[test] +// android platform will causing stderr = cp: Permission denied (os error 13) +#[cfg(not(target_os = "android"))] fn test_cp_preserve_links_case_1() { let (at, mut ucmd) = at_and_ucmd!(); @@ -1363,7 +1365,7 @@ fn test_cp_preserve_links_case_1() { assert!(at.plus("c").join("a").exists()); assert!(at.plus("c").join("b").exists()); - #[cfg(all(unix))] + #[cfg(unix)] { let metadata_a = std::fs::metadata(at.subdir.join("c").join("a")).unwrap(); let metadata_b = std::fs::metadata(at.subdir.join("c").join("b")).unwrap(); @@ -1373,6 +1375,8 @@ fn test_cp_preserve_links_case_1() { } #[test] +// android platform will causing stderr = cp: Permission denied (os error 13) +#[cfg(not(target_os = "android"))] fn test_cp_preserve_links_case_2() { let (at, mut ucmd) = at_and_ucmd!(); @@ -1392,7 +1396,7 @@ fn test_cp_preserve_links_case_2() { assert!(at.plus("c").join("a").exists()); assert!(at.plus("c").join("b").exists()); - #[cfg(all(unix))] + #[cfg(unix)] { let metadata_a = std::fs::metadata(at.subdir.join("c").join("a")).unwrap(); let metadata_b = std::fs::metadata(at.subdir.join("c").join("b")).unwrap(); @@ -1402,6 +1406,8 @@ fn test_cp_preserve_links_case_2() { } #[test] +// android platform will causing stderr = cp: Permission denied (os error 13) +#[cfg(not(target_os = "android"))] fn test_cp_preserve_links_case_3() { let (at, mut ucmd) = at_and_ucmd!(); @@ -1420,7 +1426,7 @@ fn test_cp_preserve_links_case_3() { assert!(at.plus("c").join("a").exists()); assert!(at.plus("c").join("b").exists()); - #[cfg(all(unix))] + #[cfg(unix)] { let metadata_a = std::fs::metadata(at.subdir.join("c").join("a")).unwrap(); let metadata_b = std::fs::metadata(at.subdir.join("c").join("b")).unwrap(); @@ -1430,6 +1436,8 @@ fn test_cp_preserve_links_case_3() { } #[test] +// android platform will causing stderr = cp: Permission denied (os error 13) +#[cfg(not(target_os = "android"))] fn test_cp_preserve_links_case_4() { let (at, mut ucmd) = at_and_ucmd!(); @@ -1448,7 +1456,7 @@ fn test_cp_preserve_links_case_4() { assert!(at.plus("c").join("a").exists()); assert!(at.plus("c").join("b").exists()); - #[cfg(all(unix))] + #[cfg(unix)] { let metadata_a = std::fs::metadata(at.subdir.join("c").join("a")).unwrap(); let metadata_b = std::fs::metadata(at.subdir.join("c").join("b")).unwrap(); @@ -1458,6 +1466,8 @@ fn test_cp_preserve_links_case_4() { } #[test] +// android platform will causing stderr = cp: Permission denied (os error 13) +#[cfg(not(target_os = "android"))] fn test_cp_preserve_links_case_5() { let (at, mut ucmd) = at_and_ucmd!(); @@ -1475,7 +1485,7 @@ fn test_cp_preserve_links_case_5() { assert!(at.plus("c").join("a").exists()); assert!(at.plus("c").join("b").exists()); - #[cfg(all(unix))] + #[cfg(unix)] { let metadata_a = std::fs::metadata(at.subdir.join("c").join("a")).unwrap(); let metadata_b = std::fs::metadata(at.subdir.join("c").join("b")).unwrap(); @@ -1485,6 +1495,8 @@ fn test_cp_preserve_links_case_5() { } #[test] +// android platform will causing stderr = cp: Permission denied (os error 13) +#[cfg(not(target_os = "android"))] fn test_cp_preserve_links_case_6() { let (at, mut ucmd) = at_and_ucmd!(); @@ -1498,7 +1510,7 @@ fn test_cp_preserve_links_case_6() { assert!(at.plus("c").join("a").exists()); assert!(at.plus("c").join("b").exists()); - #[cfg(all(unix))] + #[cfg(unix)] { let metadata_a = std::fs::metadata(at.subdir.join("c").join("a")).unwrap(); let metadata_b = std::fs::metadata(at.subdir.join("c").join("b")).unwrap(); From 5e59458aae68c71c66fdac27d1031deb7d56dbb1 Mon Sep 17 00:00:00 2001 From: tommady Date: Tue, 22 Aug 2023 06:11:41 +0000 Subject: [PATCH 26/33] address comment move the copied_files hashmap insertion from each copy_file function caller into the copy_file function itself --- src/uu/cp/src/copydir.rs | 18 ++---------------- src/uu/cp/src/cp.rs | 12 ++++++++---- 2 files changed, 10 insertions(+), 20 deletions(-) diff --git a/src/uu/cp/src/copydir.rs b/src/uu/cp/src/copydir.rs index 89b0c025eb0..2e4eb39b3e9 100644 --- a/src/uu/cp/src/copydir.rs +++ b/src/uu/cp/src/copydir.rs @@ -253,16 +253,7 @@ fn copy_direntry( copied_files, false, ) { - Ok(_) => { - copied_files.insert( - FileInformation::from_path( - &source_absolute, - options.dereference(false), - )?, - dest, - ); - Ok(()) - } + Ok(_) => Ok(()), Err(err) => { if source_absolute.is_symlink() { // silent the error with a symlink @@ -291,12 +282,7 @@ fn copy_direntry( copied_files, false, ) { - Ok(_) => { - copied_files.insert( - FileInformation::from_path(&source_absolute, options.dereference(false))?, - dest, - ); - } + Ok(_) => {} Err(Error::IoErrContext(e, _)) if e.kind() == std::io::ErrorKind::PermissionDenied => { diff --git a/src/uu/cp/src/cp.rs b/src/uu/cp/src/cp.rs index 0ff9f90a19a..aae687fc68e 100644 --- a/src/uu/cp/src/cp.rs +++ b/src/uu/cp/src/cp.rs @@ -1250,10 +1250,6 @@ pub fn copy(sources: &[PathBuf], target: &Path, options: &Options) -> CopyResult show_error_if_needed(&error); non_fatal_errors = true; } - copied_files.insert( - FileInformation::from_path(source, options.dereference(true))?, - dest, - ); } seen_sources.insert(source); } @@ -1906,6 +1902,14 @@ fn copy_file( copy_attributes(source, dest, &options.attributes)?; + copied_files.insert( + FileInformation::from_path( + source.to_path_buf(), + options.dereference(source_in_command_line), + )?, + dest.to_path_buf(), + ); + if let Some(progress_bar) = progress_bar { progress_bar.inc(fs::metadata(source)?.len()); } From abb2c0f747782737d27049a9e947d91e27792b75 Mon Sep 17 00:00:00 2001 From: tommady Date: Tue, 22 Aug 2023 06:22:36 +0000 Subject: [PATCH 27/33] restore the dest variable back to it's original place of definition --- src/uu/cp/src/copydir.rs | 2 +- src/uu/cp/src/cp.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/uu/cp/src/copydir.rs b/src/uu/cp/src/copydir.rs index 2e4eb39b3e9..7c46128c353 100644 --- a/src/uu/cp/src/copydir.rs +++ b/src/uu/cp/src/copydir.rs @@ -240,8 +240,8 @@ fn copy_direntry( // If the source is not a directory, then we need to copy the file. if !source_absolute.is_dir() { - let dest = local_to_target.as_path().to_path_buf(); if preserve_hard_links { + let dest = local_to_target.as_path().to_path_buf(); let found_hard_link = preserve_hardlinks(hard_links, &source_absolute, &dest)?; if !found_hard_link { match copy_file( diff --git a/src/uu/cp/src/cp.rs b/src/uu/cp/src/cp.rs index aae687fc68e..fb39f8526ff 100644 --- a/src/uu/cp/src/cp.rs +++ b/src/uu/cp/src/cp.rs @@ -1231,8 +1231,8 @@ pub fn copy(sources: &[PathBuf], target: &Path, options: &Options) -> CopyResult // FIXME: compare sources by the actual file they point to, not their path. (e.g. dir/file == dir/../dir/file in most cases) show_warning!("source {} specified more than once", source.quote()); } else { - let dest = construct_dest_path(source, target, &target_type, options)?; let found_hard_link = if preserve_hard_links && !source.is_dir() { + let dest = construct_dest_path(source, target, &target_type, options)?; preserve_hardlinks(&mut hard_links, source, &dest)? } else { false From 39ad651b78d51e0fd6700e88fa34a6ce337a76b1 Mon Sep 17 00:00:00 2001 From: tommady Date: Tue, 22 Aug 2023 06:49:53 +0000 Subject: [PATCH 28/33] fix clippy linter issue --- src/uu/cp/src/cp.rs | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/uu/cp/src/cp.rs b/src/uu/cp/src/cp.rs index fb39f8526ff..9bd23de8238 100644 --- a/src/uu/cp/src/cp.rs +++ b/src/uu/cp/src/cp.rs @@ -1903,10 +1903,7 @@ fn copy_file( copy_attributes(source, dest, &options.attributes)?; copied_files.insert( - FileInformation::from_path( - source.to_path_buf(), - options.dereference(source_in_command_line), - )?, + FileInformation::from_path(source, options.dereference(source_in_command_line))?, dest.to_path_buf(), ); From 04f4abc455a52af1fd27f61896ce5e10931d9e71 Mon Sep 17 00:00:00 2001 From: tommady Date: Sun, 27 Aug 2023 10:14:49 +0000 Subject: [PATCH 29/33] address comment, merge preserve_hardlinks function --- src/uu/cp/src/copydir.rs | 51 +++++++---------- src/uu/cp/src/cp.rs | 120 +++++++++++---------------------------- 2 files changed, 54 insertions(+), 117 deletions(-) diff --git a/src/uu/cp/src/copydir.rs b/src/uu/cp/src/copydir.rs index 7c46128c353..44481669f3b 100644 --- a/src/uu/cp/src/copydir.rs +++ b/src/uu/cp/src/copydir.rs @@ -24,8 +24,8 @@ use uucore::uio_error; use walkdir::{DirEntry, WalkDir}; use crate::{ - aligned_ancestors, context_for, copy_attributes, copy_file, copy_link, preserve_hardlinks, - CopyResult, Error, Options, + aligned_ancestors, context_for, copy_attributes, copy_file, copy_link, CopyResult, Error, + Options, }; /// Ensure a Windows path starts with a `\\?`. @@ -200,7 +200,6 @@ fn copy_direntry( options: &Options, symlinked_files: &mut HashSet, preserve_hard_links: bool, - hard_links: &mut Vec<(String, u64)>, copied_files: &mut HashMap, ) -> CopyResult<()> { let Entry { @@ -241,31 +240,27 @@ fn copy_direntry( // If the source is not a directory, then we need to copy the file. if !source_absolute.is_dir() { if preserve_hard_links { - let dest = local_to_target.as_path().to_path_buf(); - let found_hard_link = preserve_hardlinks(hard_links, &source_absolute, &dest)?; - if !found_hard_link { - match copy_file( - progress_bar, - &source_absolute, - local_to_target.as_path(), - options, - symlinked_files, - copied_files, - false, - ) { - Ok(_) => Ok(()), - Err(err) => { - if source_absolute.is_symlink() { - // silent the error with a symlink - // In case we do --archive, we might copy the symlink - // before the file itself - Ok(()) - } else { - Err(err) - } + match copy_file( + progress_bar, + &source_absolute, + local_to_target.as_path(), + options, + symlinked_files, + copied_files, + false, + ) { + Ok(_) => Ok(()), + Err(err) => { + if source_absolute.is_symlink() { + // silent the error with a symlink + // In case we do --archive, we might copy the symlink + // before the file itself + Ok(()) + } else { + Err(err) } - }?; - } + } + }?; } else { // At this point, `path` is just a plain old file. // Terminate this function immediately if there is any @@ -377,7 +372,6 @@ pub(crate) fn copy_directory( }; let target = tmp.as_path(); - let mut hard_links: Vec<(String, u64)> = vec![]; let preserve_hard_links = options.preserve_hard_links(); // Collect some paths here that are invariant during the traversal @@ -402,7 +396,6 @@ pub(crate) fn copy_directory( options, symlinked_files, preserve_hard_links, - &mut hard_links, copied_files, )?; } diff --git a/src/uu/cp/src/cp.rs b/src/uu/cp/src/cp.rs index 9bd23de8238..184d2ec02cb 100644 --- a/src/uu/cp/src/cp.rs +++ b/src/uu/cp/src/cp.rs @@ -1106,66 +1106,6 @@ fn parse_path_args( Ok((paths, target)) } -/// Get the inode information for a file. -fn get_inode(file_info: &FileInformation) -> u64 { - #[cfg(unix)] - let result = file_info.inode(); - #[cfg(windows)] - let result = file_info.file_index(); - result -} - -#[cfg(target_os = "redox")] -fn preserve_hardlinks( - hard_links: &mut Vec<(String, u64)>, - source: &std::path::Path, - dest: &std::path::Path, - found_hard_link: &mut bool, -) -> CopyResult<()> { - // Redox does not currently support hard links - Ok(()) -} - -/// Hard link a pair of files if needed _and_ record if this pair is a new hard link. -#[cfg(not(target_os = "redox"))] -fn preserve_hardlinks( - hard_links: &mut Vec<(String, u64)>, - source: &std::path::Path, - dest: &std::path::Path, -) -> CopyResult { - let info = FileInformation::from_path(source, false) - .context(format!("cannot stat {}", source.quote()))?; - let inode = get_inode(&info); - let nlinks = info.number_of_links(); - let mut found_hard_link = false; - for (link, link_inode) in hard_links.iter() { - if *link_inode == inode { - // Consider the following files: - // - // * `src/f` - a regular file - // * `src/link` - a hard link to `src/f` - // * `dest/src/f` - a different regular file - // - // In this scenario, if we do `cp -a src/ dest/`, it is - // possible that the order of traversal causes `src/link` - // to get copied first (to `dest/src/link`). In that case, - // in order to make sure `dest/src/link` is a hard link to - // `dest/src/f` and `dest/src/f` has the contents of - // `src/f`, we delete the existing file to allow the hard - // linking. - if file_or_link_exists(dest) && file_or_link_exists(Path::new(link)) { - std::fs::remove_file(dest)?; - } - std::fs::hard_link(link, dest).unwrap(); - found_hard_link = true; - } - } - if !found_hard_link && nlinks > 1 { - hard_links.push((dest.to_str().unwrap().to_string(), inode)); - } - Ok(found_hard_link) -} - /// When handling errors, we don't always want to show them to the user. This function handles that. fn show_error_if_needed(error: &Error) { match error { @@ -1194,10 +1134,6 @@ pub fn copy(sources: &[PathBuf], target: &Path, options: &Options) -> CopyResult let target_type = TargetType::determine(sources, target); verify_target_type(target, &target_type)?; - let preserve_hard_links = options.preserve_hard_links(); - - let mut hard_links: Vec<(String, u64)> = vec![]; - let mut non_fatal_errors = false; let mut seen_sources = HashSet::with_capacity(sources.len()); let mut symlinked_files = HashSet::new(); @@ -1231,28 +1167,20 @@ pub fn copy(sources: &[PathBuf], target: &Path, options: &Options) -> CopyResult // FIXME: compare sources by the actual file they point to, not their path. (e.g. dir/file == dir/../dir/file in most cases) show_warning!("source {} specified more than once", source.quote()); } else { - let found_hard_link = if preserve_hard_links && !source.is_dir() { - let dest = construct_dest_path(source, target, &target_type, options)?; - preserve_hardlinks(&mut hard_links, source, &dest)? - } else { - false - }; - if !found_hard_link { - if let Err(error) = copy_source( - &progress_bar, - source, - target, - &target_type, - options, - &mut symlinked_files, - &mut copied_files, - ) { - show_error_if_needed(&error); - non_fatal_errors = true; - } + if let Err(error) = copy_source( + &progress_bar, + source, + target, + &target_type, + options, + &mut symlinked_files, + &mut copied_files, + ) { + show_error_if_needed(&error); + non_fatal_errors = true; } - seen_sources.insert(source); } + seen_sources.insert(source); } if non_fatal_errors { @@ -1673,10 +1601,26 @@ fn copy_file( // if we encounter a matching device/inode pair in the source tree // we can arrange to create a hard link between the corresponding names // in the destination tree. - if let Some(new_source) = copied_files.get(&FileInformation::from_path( - source, - options.dereference(source_in_command_line), - )?) { + if let Some(new_source) = copied_files.get( + &FileInformation::from_path(source, options.dereference(source_in_command_line)) + .context(format!("cannot stat {}", source.quote()))?, + ) { + // Consider the following files: + // + // * `src/f` - a regular file + // * `src/link` - a hard link to `src/f` + // * `dest/src/f` - a different regular file + // + // In this scenario, if we do `cp -a src/ dest/`, it is + // possible that the order of traversal causes `src/link` + // to get copied first (to `dest/src/link`). In that case, + // in order to make sure `dest/src/link` is a hard link to + // `dest/src/f` and `dest/src/f` has the contents of + // `src/f`, we delete the existing file to allow the hard + // linking. + if file_or_link_exists(dest) && file_or_link_exists(Path::new(new_source)) { + std::fs::remove_file(dest)?; + } std::fs::hard_link(new_source, dest)?; return Ok(()); }; From 8dc3debe6663af57b90cf30a0a2f49f3f4a2042c Mon Sep 17 00:00:00 2001 From: tommady Date: Sun, 27 Aug 2023 10:45:20 +0000 Subject: [PATCH 30/33] fix clippy linter issue --- src/uu/cp/src/cp.rs | 24 +++++++++++------------- 1 file changed, 11 insertions(+), 13 deletions(-) diff --git a/src/uu/cp/src/cp.rs b/src/uu/cp/src/cp.rs index 42d8ca6be4c..9cf9f9a9986 100644 --- a/src/uu/cp/src/cp.rs +++ b/src/uu/cp/src/cp.rs @@ -1163,19 +1163,17 @@ pub fn copy(sources: &[PathBuf], target: &Path, options: &Options) -> CopyResult 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) show_warning!("source {} specified more than once", source.quote()); - } else { - if let Err(error) = copy_source( - &progress_bar, - source, - target, - &target_type, - options, - &mut symlinked_files, - &mut copied_files, - ) { - show_error_if_needed(&error); - non_fatal_errors = true; - } + } else if let Err(error) = copy_source( + &progress_bar, + source, + target, + &target_type, + options, + &mut symlinked_files, + &mut copied_files, + ) { + show_error_if_needed(&error); + non_fatal_errors = true; } seen_sources.insert(source); } From 0abb096860860436d1ef6845acd0ee235e3af89d Mon Sep 17 00:00:00 2001 From: tommady Date: Tue, 29 Aug 2023 13:45:33 +0000 Subject: [PATCH 31/33] address comments --- src/uu/cp/src/cp.rs | 60 +++++++++++++++++++++------------------- tests/by-util/test_cp.rs | 27 ++++++++++++++++++ 2 files changed, 58 insertions(+), 29 deletions(-) diff --git a/src/uu/cp/src/cp.rs b/src/uu/cp/src/cp.rs index 5f06c551554..998c33d76ad 100644 --- a/src/uu/cp/src/cp.rs +++ b/src/uu/cp/src/cp.rs @@ -1506,6 +1506,24 @@ fn handle_existing_dest( OverwriteMode::Clobber(ClobberMode::RemoveDestination) => { fs::remove_file(dest)?; } + OverwriteMode::Clobber(ClobberMode::Standard) => { + // Consider the following files: + // + // * `src/f` - a regular file + // * `src/link` - a hard link to `src/f` + // * `dest/src/f` - a different regular file + // + // In this scenario, if we do `cp -a src/ dest/`, it is + // possible that the order of traversal causes `src/link` + // to get copied first (to `dest/src/link`). In that case, + // in order to make sure `dest/src/link` is a hard link to + // `dest/src/f` and `dest/src/f` has the contents of + // `src/f`, we delete the existing file to allow the hard + // linking. + if !source_in_command_line { + fs::remove_file(dest)?; + } + } _ => (), }; @@ -1590,35 +1608,6 @@ fn copy_file( return Ok(()); } - if options.preserve_hard_links() { - // if we encounter a matching device/inode pair in the source tree - // we can arrange to create a hard link between the corresponding names - // in the destination tree. - if let Some(new_source) = copied_files.get( - &FileInformation::from_path(source, options.dereference(source_in_command_line)) - .context(format!("cannot stat {}", source.quote()))?, - ) { - // Consider the following files: - // - // * `src/f` - a regular file - // * `src/link` - a hard link to `src/f` - // * `dest/src/f` - a different regular file - // - // In this scenario, if we do `cp -a src/ dest/`, it is - // possible that the order of traversal causes `src/link` - // to get copied first (to `dest/src/link`). In that case, - // in order to make sure `dest/src/link` is a hard link to - // `dest/src/f` and `dest/src/f` has the contents of - // `src/f`, we delete the existing file to allow the hard - // linking. - if file_or_link_exists(dest) && file_or_link_exists(Path::new(new_source)) { - std::fs::remove_file(dest)?; - } - std::fs::hard_link(new_source, dest)?; - return Ok(()); - }; - } - // Fail if dest is a dangling symlink or a symlink this program created previously if dest.is_symlink() { if FileInformation::from_path(dest, false) @@ -1652,6 +1641,19 @@ fn copy_file( handle_existing_dest(source, dest, options, source_in_command_line)?; } + if options.preserve_hard_links() { + // if we encounter a matching device/inode pair in the source tree + // we can arrange to create a hard link between the corresponding names + // in the destination tree. + if let Some(new_source) = copied_files.get( + &FileInformation::from_path(source, options.dereference(source_in_command_line)) + .context(format!("cannot stat {}", source.quote()))?, + ) { + std::fs::hard_link(new_source, dest)?; + return Ok(()); + }; + } + if options.verbose { if let Some(pb) = progress_bar { // Suspend (hide) the progress bar so the println won't overlap with the progress bar. diff --git a/tests/by-util/test_cp.rs b/tests/by-util/test_cp.rs index d0b6c0ff2e0..56ea75b613c 100644 --- a/tests/by-util/test_cp.rs +++ b/tests/by-util/test_cp.rs @@ -1523,6 +1523,33 @@ fn test_cp_preserve_links_case_6() { } } +#[test] +// android platform will causing stderr = cp: Permission denied (os error 13) +#[cfg(not(target_os = "android"))] +fn test_cp_preserve_links_case_7() { + let (at, mut ucmd) = at_and_ucmd!(); + + at.mkdir("src"); + at.touch("src/f"); + at.hard_link("src/f", "src/g"); + + at.mkdir("dest"); + at.touch("dest/g"); + + ucmd.arg("-n") + .arg("--preserve=links") + .arg("src/f") + .arg("src/g") + .arg("dest") + .fails() + .stderr_contains("not replacing 'dest/g"); + (); + + assert!(at.dir_exists("dest")); + assert!(at.plus("dest").join("f").exists()); + assert!(at.plus("dest").join("g").exists()); +} + #[test] // For now, disable the test on Windows. Symlinks aren't well support on Windows. // It works on Unix for now and it works locally when run from a powershell From fd4b7cf3ad1c1c9dd2ff7dcc2871322a1cf79456 Mon Sep 17 00:00:00 2001 From: tommady Date: Wed, 30 Aug 2023 06:42:30 +0000 Subject: [PATCH 32/33] address comments --- tests/by-util/test_cp.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/by-util/test_cp.rs b/tests/by-util/test_cp.rs index 56ea75b613c..e06633b5d9a 100644 --- a/tests/by-util/test_cp.rs +++ b/tests/by-util/test_cp.rs @@ -1542,7 +1542,7 @@ fn test_cp_preserve_links_case_7() { .arg("src/g") .arg("dest") .fails() - .stderr_contains("not replacing 'dest/g"); + .stderr_contains("not replacing"); (); assert!(at.dir_exists("dest")); From f81145e5b0d0d4dfe63f26032357390bbb3fd9df Mon Sep 17 00:00:00 2001 From: tommady Date: Wed, 30 Aug 2023 12:02:41 +0000 Subject: [PATCH 33/33] address comments --- src/uu/cp/src/cp.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/uu/cp/src/cp.rs b/src/uu/cp/src/cp.rs index 998c33d76ad..42ef106067c 100644 --- a/src/uu/cp/src/cp.rs +++ b/src/uu/cp/src/cp.rs @@ -1520,7 +1520,7 @@ fn handle_existing_dest( // `dest/src/f` and `dest/src/f` has the contents of // `src/f`, we delete the existing file to allow the hard // linking. - if !source_in_command_line { + if options.preserve_hard_links() { fs::remove_file(dest)?; } }