From bd665ea44a23ab5a335ed174c6dc1da94886a53f Mon Sep 17 00:00:00 2001 From: Jeffrey Finkelstein Date: Sun, 6 Nov 2022 10:03:12 -0500 Subject: [PATCH 1/2] cp: correct --verbose --parents output for files This commit corrects the behavior of `cp --parents --verbose` when the source path is a file so that it prints the copied ancestor directories. For example, $ mkdir -p a/b d $ touch a/b/c $ cp --verbose --parents a/b/c d a -> d/a a/b -> d/a/b 'a/b/c' -> 'd/a/b/c' Fixes #3332. --- src/uu/cp/src/cp.rs | 101 ++++++++++++++++++++++++++++++++++----- tests/by-util/test_cp.rs | 38 ++++++--------- 2 files changed, 104 insertions(+), 35 deletions(-) diff --git a/src/uu/cp/src/cp.rs b/src/uu/cp/src/cp.rs index bb1ca867917..68739178fed 100644 --- a/src/uu/cp/src/cp.rs +++ b/src/uu/cp/src/cp.rs @@ -1311,6 +1311,47 @@ fn file_or_link_exists(path: &Path) -> bool { path.symlink_metadata().is_ok() } +/// Zip the ancestors of a source path and destination path. +/// +/// # Examples +/// +/// ```rust,ignore +/// let actual = aligned_ancestors(&Path::new("a/b/c"), &Path::new("d/a/b/c")); +/// let expected = vec![ +/// (Path::new("a"), Path::new("d/a")), +/// (Path::new("a/b"), Path::new("d/a/b")), +/// ]; +/// assert_eq!(actual, expected); +/// ``` +fn aligned_ancestors<'a>(source: &'a Path, dest: &'a Path) -> Vec<(&'a Path, &'a Path)> { + // Collect the ancestors of each. For example, if `source` is + // "a/b/c", then the ancestors are "a/b/c", "a/b", "a/", and "". + let source_ancestors: Vec<&Path> = source.ancestors().collect(); + let dest_ancestors: Vec<&Path> = dest.ancestors().collect(); + + // For this particular application, we don't care about the null + // path "" and we don't care about the full path (e.g. "a/b/c"), + // so we exclude those. + let n = source_ancestors.len(); + let source_ancestors = &source_ancestors[1..n - 1]; + + // Get the matching number of elements from the ancestors of the + // destination path (for example, get "d/a" and "d/a/b"). + let k = source_ancestors.len(); + let dest_ancestors = &dest_ancestors[1..1 + k]; + + // Now we have two slices of the same length, so we zip them. + let mut result = vec![]; + for (x, y) in source_ancestors + .iter() + .rev() + .zip(dest_ancestors.iter().rev()) + { + result.push((*x, *y)); + } + result +} + /// Copy the a file from `source` to `dest`. `source` will be dereferenced if /// `options.dereference` is set to true. `dest` will be dereferenced only if /// the source was not a symlink. @@ -1370,9 +1411,33 @@ fn copy_file( if let Some(pb) = progress_bar { // Suspend (hide) the progress bar so the println won't overlap with the progress bar. pb.suspend(|| { + if options.parents { + // For example, if copying file `a/b/c` and its parents + // to directory `d/`, then print + // + // a -> d/a + // a/b -> d/a/b + // + for (x, y) in aligned_ancestors(source, dest) { + println!("{} -> {}", x.display(), y.display()); + } + } + println!("{}", context_for(source, dest)); }); } else { + if options.parents { + // For example, if copying file `a/b/c` and its parents + // to directory `d/`, then print + // + // a -> d/a + // a/b -> d/a/b + // + for (x, y) in aligned_ancestors(source, dest) { + println!("{} -> {}", x.display(), y.display()); + } + } + println!("{}", context_for(source, dest)); } } @@ -1671,15 +1736,29 @@ fn disk_usage_directory(p: &Path) -> io::Result { Ok(total) } -#[test] -fn test_cp_localize_to_target() { - assert!( - localize_to_target( - Path::new("a/source/"), - Path::new("a/source/c.txt"), - Path::new("target/") - ) - .unwrap() - == Path::new("target/c.txt") - ); +#[cfg(test)] +mod tests { + + use crate::{aligned_ancestors, localize_to_target}; + use std::path::Path; + + #[test] + fn test_cp_localize_to_target() { + let root = Path::new("a/source/"); + let source = Path::new("a/source/c.txt"); + let target = Path::new("target/"); + let actual = localize_to_target(root, source, target).unwrap(); + let expected = Path::new("target/c.txt"); + assert_eq!(actual, expected); + } + + #[test] + fn test_aligned_ancestors() { + let actual = aligned_ancestors(&Path::new("a/b/c"), &Path::new("d/a/b/c")); + let expected = vec![ + (Path::new("a"), Path::new("d/a")), + (Path::new("a/b"), Path::new("d/a/b")), + ]; + assert_eq!(actual, expected); + } } diff --git a/tests/by-util/test_cp.rs b/tests/by-util/test_cp.rs index 8c27f10ac5f..41d7ec671e7 100644 --- a/tests/by-util/test_cp.rs +++ b/tests/by-util/test_cp.rs @@ -26,7 +26,6 @@ use std::fs as std_fs; use std::thread::sleep; #[cfg(not(target_os = "freebsd"))] use std::time::Duration; -use uucore::display::Quotable; static TEST_EXISTING_FILE: &str = "existing_file.txt"; static TEST_HELLO_WORLD_SOURCE: &str = "hello_world.txt"; @@ -2054,46 +2053,37 @@ fn test_cp_parents_2_dirs() { } #[test] -#[ignore = "issue #3332"] fn test_cp_parents_2() { let (at, mut ucmd) = at_and_ucmd!(); at.mkdir_all("a/b"); at.touch("a/b/c"); at.mkdir("d"); - ucmd.args(&["--verbose", "-a", "--parents", "a/b/c", "d"]) + #[cfg(not(windows))] + let expected_stdout = "a -> d/a\na/b -> d/a/b\n'a/b/c' -> 'd/a/b/c'\n"; + #[cfg(windows)] + let expected_stdout = "a -> d\\a\na/b -> d\\a/b\n'a/b/c' -> 'd\\a/b/c'\n"; + ucmd.args(&["--verbose", "--parents", "a/b/c", "d"]) .succeeds() - .stdout_is(format!( - "{} -> {}\n{} -> {}\n{} -> {}\n", - "a", - path_concat!("d", "a"), - path_concat!("a", "b"), - path_concat!("d", "a", "b"), - path_concat!("a", "b", "c").quote(), - path_concat!("d", "a", "b", "c").quote() - )); + .stdout_only(expected_stdout); assert!(at.file_exists("d/a/b/c")); } #[test] -#[ignore = "issue #3332"] fn test_cp_parents_2_link() { let (at, mut ucmd) = at_and_ucmd!(); at.mkdir_all("a/b"); at.touch("a/b/c"); at.mkdir("d"); at.relative_symlink_file("b", "a/link"); - ucmd.args(&["--verbose", "-a", "--parents", "a/link/c", "d"]) + #[cfg(not(windows))] + let expected_stdout = "a -> d/a\na/link -> d/a/link\n'a/link/c' -> 'd/a/link/c'\n"; + #[cfg(windows)] + let expected_stdout = "a -> d\\a\na/link -> d\\a/link\n'a/link/c' -> 'd\\a/link/c'\n"; + ucmd.args(&["--verbose", "--parents", "a/link/c", "d"]) .succeeds() - .stdout_is(format!( - "{} -> {}\n{} -> {}\n{} -> {}\n", - "a", - path_concat!("d", "a"), - path_concat!("a", "link"), - path_concat!("d", "a", "link"), - path_concat!("a", "link", "c").quote(), - path_concat!("d", "a", "link", "c").quote() - )); - assert!(at.dir_exists("d/a/link") && !at.symlink_exists("d/a/link")); + .stdout_only(expected_stdout); + assert!(at.dir_exists("d/a/link")); + assert!(!at.symlink_exists("d/a/link")); assert!(at.file_exists("d/a/link/c")); } From 1a839fb2c430f028cb4ba0791bbf9fec3ddb07ef Mon Sep 17 00:00:00 2001 From: Jeffrey Finkelstein Date: Sat, 26 Nov 2022 12:52:48 -0800 Subject: [PATCH 2/2] cp: correct --verbose --parents output for dirs This commit corrects the behavior of `cp -r --parents --verbose` when the source path is a directory, so that it prints the copied ancestor directories. For example, $ mkdir -p a/b/c d $ cp -r --verbose --parents a/b/c d a -> d/a a/b -> d/a/b 'a/b/c' -> 'd/a/b/c' --- src/uu/cp/src/copydir.rs | 70 +++++++++++++++++++++++++++++++++++++--- tests/by-util/test_cp.rs | 45 ++++++++++++++++++++++++++ 2 files changed, 111 insertions(+), 4 deletions(-) diff --git a/src/uu/cp/src/copydir.rs b/src/uu/cp/src/copydir.rs index 86a4081ee1f..bc4cd1b9dc9 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::{ - copy_attributes, copy_file, copy_link, preserve_hardlinks, CopyResult, Error, Options, - TargetSlice, + aligned_ancestors, context_for, copy_attributes, copy_file, copy_link, preserve_hardlinks, + CopyResult, Error, Options, TargetSlice, }; /// Ensure a Windows path starts with a `\\?`. @@ -172,6 +172,27 @@ impl Entry { } } +/// Decide whether the given path ends with `/.`. +/// +/// # Examples +/// +/// ```rust,ignore +/// assert!(ends_with_slash_dot("/.")); +/// assert!(ends_with_slash_dot("./.")); +/// assert!(ends_with_slash_dot("a/.")); +/// +/// assert!(!ends_with_slash_dot(".")); +/// assert!(!ends_with_slash_dot("./")); +/// assert!(!ends_with_slash_dot("a/..")); +/// ``` +fn ends_with_slash_dot

(path: P) -> bool +where + P: AsRef, +{ + // `path.ends_with(".")` does not seem to work + path.as_ref().display().to_string().ends_with("/.") +} + /// Copy a single entry during a directory traversal. fn copy_direntry( progress_bar: &Option, @@ -196,7 +217,10 @@ fn copy_direntry( // If the source is a directory and the destination does not // exist, ... - if source_absolute.is_dir() && !local_to_target.exists() { + if source_absolute.is_dir() + && !ends_with_slash_dot(&source_absolute) + && !local_to_target.exists() + { if target_is_file { return Err("cannot overwrite non-directory with directory".into()); } else { @@ -205,7 +229,10 @@ fn copy_direntry( // `create_dir_all()` will have any benefit over // `create_dir()`, since all the ancestor directories // should have already been created. - fs::create_dir_all(local_to_target)?; + fs::create_dir_all(&local_to_target)?; + if options.verbose { + println!("{}", context_for(&source_relative, &local_to_target)); + } return Ok(()); } } @@ -324,6 +351,19 @@ pub(crate) fn copy_directory( if let Some(parent) = root.parent() { let new_target = target.join(parent); std::fs::create_dir_all(&new_target)?; + + if options.verbose { + // For example, if copying file `a/b/c` and its parents + // to directory `d/`, then print + // + // a -> d/a + // a/b -> d/a/b + // + for (x, y) in aligned_ancestors(root, &target.join(root)) { + println!("{} -> {}", x.display(), y.display()); + } + } + new_target } else { target.to_path_buf() @@ -393,3 +433,25 @@ pub fn path_has_prefix(p1: &Path, p2: &Path) -> io::Result { Ok(pathbuf1.starts_with(pathbuf2)) } + +#[cfg(test)] +mod tests { + use super::ends_with_slash_dot; + + #[test] + fn test_ends_with_slash_dot() { + assert!(ends_with_slash_dot("/.")); + assert!(ends_with_slash_dot("./.")); + assert!(ends_with_slash_dot("../.")); + assert!(ends_with_slash_dot("a/.")); + assert!(ends_with_slash_dot("/a/.")); + + assert!(!ends_with_slash_dot("")); + assert!(!ends_with_slash_dot(".")); + assert!(!ends_with_slash_dot("./")); + assert!(!ends_with_slash_dot("..")); + assert!(!ends_with_slash_dot("/..")); + assert!(!ends_with_slash_dot("a/..")); + assert!(!ends_with_slash_dot("/a/..")); + } +} diff --git a/tests/by-util/test_cp.rs b/tests/by-util/test_cp.rs index 41d7ec671e7..b0a3e6931bf 100644 --- a/tests/by-util/test_cp.rs +++ b/tests/by-util/test_cp.rs @@ -2087,6 +2087,36 @@ fn test_cp_parents_2_link() { assert!(at.file_exists("d/a/link/c")); } +#[test] +fn test_cp_parents_2_dir() { + let (at, mut ucmd) = at_and_ucmd!(); + at.mkdir_all("a/b/c"); + at.mkdir("d"); + #[cfg(not(windows))] + let expected_stdout = "a -> d/a\na/b -> d/a/b\n'a/b/c' -> 'd/a/b/c'\n"; + #[cfg(windows)] + let expected_stdout = "a -> d\\a\na/b -> d\\a/b\n'a/b/c' -> 'd\\a/b\\c'\n"; + ucmd.args(&["--verbose", "-r", "--parents", "a/b/c", "d"]) + .succeeds() + .stdout_only(expected_stdout); + assert!(at.dir_exists("d/a/b/c")); +} + +#[test] +fn test_cp_parents_2_deep_dir() { + let (at, mut ucmd) = at_and_ucmd!(); + at.mkdir_all("a/b/c"); + at.mkdir_all("d/e"); + #[cfg(not(windows))] + let expected_stdout = "a -> d/e/a\na/b -> d/e/a/b\n'a/b/c' -> 'd/e/a/b/c'\n"; + #[cfg(windows)] + let expected_stdout = "a -> d/e\\a\na/b -> d/e\\a/b\n'a/b/c' -> 'd/e\\a/b\\c'\n"; + ucmd.args(&["--verbose", "-r", "--parents", "a/b/c", "d/e"]) + .succeeds() + .stdout_only(expected_stdout); + assert!(at.dir_exists("d/e/a/b/c")); +} + #[test] fn test_cp_copy_symlink_contents_recursive() { let (at, mut ucmd) = at_and_ucmd!(); @@ -2409,3 +2439,18 @@ fn test_symbolic_link_file() { Path::new("src") ); } + +#[test] +fn test_src_base_dot() { + let ts = TestScenario::new(util_name!()); + let at = ts.fixtures.clone(); + at.mkdir("x"); + at.mkdir("y"); + let mut ucmd = UCommand::new(ts.bin_path, &Some(ts.util_name), at.plus("y"), true); + + ucmd.args(&["--verbose", "-r", "../x/.", "."]) + .succeeds() + .no_stderr() + .no_stdout(); + assert!(!at.dir_exists("y/x")); +}