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

jfinkels · 2022-09-03T02:32:59Z

Fix a bug where cp failed to copy ancestor directories when using
the --parents option. For example, before this commit:

$ mkdir -p a/b/c d
$ cp --parents a/b/c d
$ find d
d
d/c

After this commit

$ mkdir -p a/b/c d
$ cp --parents a/b/c d
$ find d
d
d/a
d/a/b
d/a/b/c

This commit also adds the correct messages for --verbose mode:

$ cp -r --parents --verbose a/b/c d
a -> d/a
a/b -> d/a/b
'a/b/c' -> 'd/a/b/c'

Fixes #3332.

jfinkels · 2022-09-03T02:36:31Z

tests/by-util/test_cp.rs

+        .stdout_is("a -> d/a\na/b -> d/a/b\n'a/b/c' -> 'd/a/b/c'\n");
+    assert!(at.dir_exists("d/a/b/c"));
+}
+
 #[test]
 #[ignore = "issue #3332"]


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.

jfinkels · 2022-09-03T03:45:56Z

Okay there are some WIndows issues I need to resolve, but there is another failure reported in the GNU test suite that I want to explain.

The test case tests/cp/src-base-dot.sh claims to verify that

mkdir x y
cd y
cp --verbose -ab ../x/. .

succeeds, prints nothing to stdout, and is a no-op. However, it doesn't actually verify that the operation is a no-op, it only verifies that --verbose prints nothing. It just assumes that the directory x does not get copied into the directory y.

On the main branch, uutils cp will cause the directory x to be copied into the directory y, and nothing will be printed by --verbose (this is incorrect behavior). On this new branch, uutils cp will cause the directory x to be copied into the directory y but now --verbose will print the message ../x/. -> ./x (this is still incorrect behavior).

So the tests/cp/src-base-dot.sh test case was incorrectly passing on the main branch, and now it is correctly marked as a failure.

sylvestre · 2022-09-03T10:51:21Z

nice:
Warning: Congrats! The gnu test tests/cp/cp-parents is no longer ERROR!

sylvestre · 2022-09-18T19:51:33Z

Did you see
Error: GNU test failed: tests/cp/src-base-dot. tests/cp/src-base-dot is passing on 'main'. Maybe you have to rebase?

jfinkels · 2022-09-21T20:51:25Z

Did you see Error: GNU test failed: tests/cp/src-base-dot. tests/cp/src-base-dot is passing on 'main'. Maybe you have to rebase?

Yes, it is an expected failure. The explanation is in my comment above #3894 (comment) and I created a new issue for that here #3897

sylvestre · 2022-09-21T20:54:54Z

sorry about that :(

sylvestre · 2022-10-10T06:13:33Z

Seems that it broke a few tests ?


Error: GNU test failed: tests/cp/cp-i. tests/cp/cp-i is passing on 'main'. Maybe you have to rebase?
Error: GNU test failed: tests/cp/src-base-dot. tests/cp/src-base-dot is passing on 'main'. Maybe you have to rebase?

jfinkels · 2022-10-10T14:24:38Z

tests/cp/src-base-dot is an expected new failure, see my comments above. However, tests/cp/cp-i does seem to be an unexpected new failure, so I'll fix that.

Before this commit, `cp -a` would terminate with a non-zero status code on Windows because there are no extended attributes (xattr) to copy. However, the GNU documentation for 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 it seems reasonable to do nothing instead of exiting with an error in this case.

jfinkels · 2022-10-10T17:23:28Z

I'm going to mark this as a draft pull request because there's still some weird behaviors of GNU cp that I haven't quite figure out yet. Specifically, I need to figure out how to get the quoting behavior for cp -v to work in both the cp-i.sh test case and the cp-parents.sh case.

Fix a bug where `cp` failed to copy ancestor directories when using the `--parents` option. For example, before this commit: $ mkdir -p a/b/c d $ cp --parents a/b/c d $ find d d d/c After this commit $ mkdir -p a/b/c d $ cp --parents a/b/c d $ find d d d/a d/a/b d/a/b/c This commit also adds the correct messages for `--verbose` mode: $ cp -r --parents --verbose a/b/c d a -> d/a a/b -> d/a/b 'a/b/c' -> 'd/a/b/c' Fixes uutils#3332.

jfinkels · 2022-10-11T02:12:45Z

Okay, this is ready for review again.

jfinkels · 2022-10-11T02:13:33Z

src/uu/cp/src/cp.rs

+                    ancestors.push(p);
+                }
+                for p in ancestors.iter().rev().skip(1) {
+                    println!("{} -> {}", p.display(), target.join(p).display());


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

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

tertsdiepraam

I'm a bit confused, because the messages don't show up for me with this PR:

❯ touch x/y/z/foo
❯ mkdir bar
❯ cargo run --quiet -- cp --parents --verbose x/y/z/foo bar
'x/y/z/foo' -> 'test2/x/y/z/foo'
❯ rm -r bar/*
❯ cp --parents --verbose x/y/z/foo bar
x -> bar/x
x/y -> bar/x/y
x/y/z -> bar/x/y/z
'x/y/z/foo' -> 'bar/x/y/z/foo'

Also I think this PR does not have the right messages if a part of the subdirectories already. For example, below there is no line x -> bar/x:

❯ mkdir bar/x
❯ cp --parents --verbose x/y/z/foo bar
x/y -> bar/x/y
x/y/z -> bar/x/y/z
'x/y/z/foo' -> 'bar/x/y/z/foo'

(I might be wrong because I couldn't trigger the messages)

tertsdiepraam · 2022-10-12T10:51:33Z

src/uu/cp/src/cp.rs

+                let mut ancestors = vec![];
+                for p in parent.ancestors() {
+                    ancestors.push(p);
+                }


Would this work?

Suggested change

let mut ancestors = vec![];

for p in parent.ancestors() {

ancestors.push(p);

}

let ancestors: Vec<_> = parent.ancestors().collect();

jfinkels · 2022-10-23T04:03:00Z

I'm going to close this and come back with a pull request that doesn't attempt to implement the --verbose behavior.

jfinkels force-pushed the cp-parents-dir branch from 6786fa0 to 89888d5 Compare September 3, 2022 02:34

jfinkels commented Sep 3, 2022

View reviewed changes

This was referenced Sep 3, 2022

cp: incorrectly copies directory when path ends in dot #3897

Closed

cp: move copy_directory() to its own module #3901

Merged

sylvestre force-pushed the cp-parents-dir branch from 4e70a21 to 6a18e00 Compare September 5, 2022 08:08

jfinkels force-pushed the cp-parents-dir branch from 6a18e00 to a035adc Compare September 5, 2022 21:30

sylvestre force-pushed the cp-parents-dir branch from b8328f6 to f61b123 Compare September 21, 2022 20:40

jfinkels mentioned this pull request Sep 22, 2022

Get remaining GNU test cases from ERROR to at least FAIL #3320

Closed

11 tasks

sylvestre force-pushed the cp-parents-dir branch from f392685 to 47c5bba Compare October 5, 2022 11:37

uutils deleted a comment from github-actions bot Oct 10, 2022

cp: make test for interactive mode more specific

38cbada

jfinkels force-pushed the cp-parents-dir branch 2 times, most recently from 824ed12 to 15e9566 Compare October 10, 2022 15:12

jfinkels force-pushed the cp-parents-dir branch from 15e9566 to 2d84ef3 Compare October 10, 2022 16:12

jfinkels marked this pull request as draft October 10, 2022 17:23

jfinkels force-pushed the cp-parents-dir branch from 2d84ef3 to ada7064 Compare October 11, 2022 00:34

jfinkels marked this pull request as ready for review October 11, 2022 02:12

jfinkels commented Oct 11, 2022

View reviewed changes

tertsdiepraam reviewed Oct 12, 2022

View reviewed changes

jfinkels closed this Oct 23, 2022

jfinkels mentioned this pull request Oct 23, 2022

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

Merged

jfinkels mentioned this pull request Dec 3, 2022

cp: correct --verbose --parents output for both files and directories #4184

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

jfinkels commented Sep 3, 2022

jfinkels Sep 3, 2022

jfinkels commented Sep 3, 2022

sylvestre commented Sep 3, 2022

sylvestre commented Sep 18, 2022

jfinkels commented Sep 21, 2022

sylvestre commented Sep 21, 2022

sylvestre commented Oct 10, 2022

jfinkels commented Oct 10, 2022

jfinkels commented Oct 10, 2022

jfinkels commented Oct 11, 2022 •

edited

Loading

jfinkels Oct 11, 2022

tertsdiepraam Oct 12, 2022

tertsdiepraam left a comment

tertsdiepraam Oct 12, 2022

jfinkels commented Oct 23, 2022

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

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

Conversation

jfinkels commented Sep 3, 2022

jfinkels Sep 3, 2022

Choose a reason for hiding this comment

jfinkels commented Sep 3, 2022

sylvestre commented Sep 3, 2022

sylvestre commented Sep 18, 2022

jfinkels commented Sep 21, 2022

sylvestre commented Sep 21, 2022

sylvestre commented Oct 10, 2022

jfinkels commented Oct 10, 2022

jfinkels commented Oct 10, 2022

jfinkels commented Oct 11, 2022 • edited Loading

jfinkels Oct 11, 2022

Choose a reason for hiding this comment

tertsdiepraam Oct 12, 2022

Choose a reason for hiding this comment

tertsdiepraam left a comment

Choose a reason for hiding this comment

tertsdiepraam Oct 12, 2022

Choose a reason for hiding this comment

jfinkels commented Oct 23, 2022

jfinkels commented Oct 11, 2022 •

edited

Loading