Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cp: correctly copy ancestor dirs in --parents mode #3894

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 46 additions & 2 deletions src/uu/cp/src/cp.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1086,6 +1086,38 @@ fn copy_directory(
.into());
}

// If in `--parents` mode, create all the necessary ancestor directories.
//
// For example, if the command is `cp --parents a/b/c d`, that
// means we need to copy the two ancestor directories first:
//
// a -> d/a
// a/b -> d/a/b
//
let tmp = if options.parents {
if let Some(parent) = root.parent() {
let new_target = target.join(parent);
std::fs::create_dir_all(&new_target)?;

if options.verbose {
let mut ancestors = vec![];
for p in parent.ancestors() {
ancestors.push(p);
}
Comment on lines +1103 to +1106
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would this work?

Suggested change
let mut ancestors = vec![];
for p in parent.ancestors() {
ancestors.push(p);
}
let ancestors: Vec<_> = parent.ancestors().collect();

for p in ancestors.iter().rev().skip(1) {
println!("{} -> {}", p.display(), target.join(p).display());
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ancestors are printed without quotes, so using context_for() is not appropriate here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably a good idea to add that info as a comment in the code!

}
}

new_target
} else {
target.to_path_buf()
}
} else {
target.to_path_buf()
};
let target = tmp.as_path();

let current_dir =
env::current_dir().unwrap_or_else(|e| crash!(1, "failed to get current directory {}", e));

Expand Down Expand Up @@ -1144,7 +1176,10 @@ fn copy_directory(
if target.is_file() {
return Err("cannot overwrite non-directory with directory".into());
}
fs::create_dir_all(local_to_target)?;
fs::create_dir_all(&local_to_target)?;
if options.verbose {
println!("{}", context_for(p.path(), &local_to_target));
}
} else if !path.is_dir() {
if preserve_hard_links {
let mut found_hard_link = false;
Expand Down Expand Up @@ -1316,7 +1351,16 @@ fn copy_attribute(source: &Path, dest: &Path, attribute: &Attribute) -> CopyResu
}
#[cfg(not(unix))]
{
return Err("XAttrs are only supported on unix.".to_string().into());
// The documentation for GNU cp states:
//
// > Try to preserve SELinux security context and
// > extended attributes (xattr), but ignore any failure
// > to do that and print no corresponding diagnostic.
//
// so we simply do nothing here.
//
// TODO Silently ignore failures in the `#[cfg(unix)]`
// block instead of terminating immediately on errors.
}
}
};
Expand Down
40 changes: 34 additions & 6 deletions tests/by-util/test_cp.rs
Original file line number Diff line number Diff line change
Expand Up @@ -217,15 +217,18 @@ fn test_cp_target_directory_is_file() {

#[test]
fn test_cp_arg_interactive() {
new_ucmd!()
.arg(TEST_HELLO_WORLD_SOURCE)
.arg(TEST_HOW_ARE_YOU_SOURCE)
.arg("-i")
let (at, mut ucmd) = at_and_ucmd!();
at.touch("a");
at.touch("b");
// TODO The prompt in GNU cp is different, and it doesn't have the
// response either.
//
// See <https://github.com/uutils/coreutils/issues/4023>.
ucmd.args(&["-i", "a", "b"])
.pipe_in("N\n")
.succeeds()
.no_stdout()
.stderr_contains(format!("overwrite '{}'?", TEST_HOW_ARE_YOU_SOURCE))
.stderr_contains("Not overwriting");
.stderr_is("cp: overwrite 'b'? [y/N]: cp: Not overwriting 'b' at user request\n");
}

#[test]
Expand Down Expand Up @@ -1994,6 +1997,31 @@ fn test_copy_same_symlink_no_dereference_dangling() {
ucmd.args(&["-d", "a", "b"]).succeeds();
}

#[cfg(not(windows))]
#[test]
fn test_cp_parents_2_dirs() {
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";
// TODO We should iron out exactly what the `--verbose` behavior
// should be on Windows. Currently, we have it printing, for
// example,
//
// a/b -> d\a/b
//
// Should the path separators all be forward slashes? All
// backslashes?
#[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", "-a", "--parents", "a/b/c", "d"])
.succeeds()
.no_stderr()
.stdout_is(expected_stdout);
assert!(at.dir_exists("d/a/b/c"));
}

#[test]
#[ignore = "issue #3332"]
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are two tests below here that are currently ignored. They are marked as "issue #3332", but they are actually covering use cases that are slightly different than the one originally reported in that issue. Because they are slightly different use cases (and completely different code paths), I decided not to try to resolve them in this pull request.

fn test_cp_parents_2() {
Expand Down