Skip to content

cp: modify archive flag to copy dir contents rather than dir #3954

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Feb 26, 2023
Merged

cp: modify archive flag to copy dir contents rather than dir #3954

merged 1 commit into from
Feb 26, 2023

Conversation

dmatos2012
Copy link
Contributor

Hello,
This PR fixes #3886 to handle copying source's directory contents rather than the directory itself.

Its my first ever Rust PR. Thanks for the helpful responses, and in case something else needs to be added, i can do so np.

Thanks:)

@sylvestre
Copy link
Contributor

I guess you saw that it failed on some other archs:


---- test_cp::test_cp_archive_on_directory_ending_dot stdout ----
current_directory_resolved: 
mkdir: C:\Users\RUNNER~1\AppData\Local\Temp\.tmphZwpwf\dir1
mkdir: C:\Users\RUNNER~1\AppData\Local\Temp\.tmphZwpwf\dir2
touch: C:\Users\RUNNER~1\AppData\Local\Temp\.tmphZwpwf\dir1/file
run: D:\a\coreutils\coreutils\target\debug\coreutils.exe cp -a dir1/. dir2
thread 'test_cp::test_cp_archive_on_directory_ending_dot' panicked at 'Command was expected to succeed.
stdout = 
 stderr = cp: XAttrs are only supported on unix.
', tests\common\util.rs:176:9


failures:
    test_cp::test_cp_archive_on_directory_ending_dot

@dmatos2012
Copy link
Contributor Author

dmatos2012 commented Sep 19, 2022 •
edited
Loading

Thanks for the quick feedback. I just noticed, however I also ran the same command cp -a dir1/. dir2(on windows) on the main branch and I get the same failure. Thus, I think this behavior was present from before, but it was exposed via the test. Unless by adding that test myself, I added a non-supported feature on Windows, but I am not clear whether that is related or not.

That being said, I dont have the knowledge so far to know how to make it work for windows(given the lack of Xattrs). So if maybe you can give me some pointers, I can perhaps read up on my own, and propose a fix on different PR(or this one as well)

Thanks again

@sylvestre
Copy link
Contributor

maybe it could be a unix test only ?

@dmatos2012 dmatos2012 force-pushed the modify-cp-archive-flag-behavior branch from 73c66fc to 24da098 Compare September 19, 2022 20:45
@dmatos2012
Copy link
Contributor Author

Yeah, nice solution. Done :)

@sylvestre sylvestre force-pushed the modify-cp-archive-flag-behavior branch 2 times, most recently from 294ea51 to a8839e7 Compare September 21, 2022 20:40
@jfinkels jfinkels mentioned this pull request Sep 23, 2022
Closed
@dmatos2012
Copy link
Contributor Author

dmatos2012 commented Sep 27, 2022 •
edited
Loading

Is there anything I shall do to get that final test passing? It looks like its unrelated to this PR. I am just making sure I have addressed the issues. Let me know :) and thanks again for your time and guidance!

@tertsdiepraam
Copy link
Member

tertsdiepraam commented Sep 27, 2022 •
edited
Loading

No need to worry about that test. The android build is failing on all PRs at the moment. There are some conflicts that need to be resolved though.

@jtracey
Copy link
Contributor

jtracey commented Oct 13, 2022 •
edited
Loading

That failed test is actually not because of the spurious failures from before, it's because we still need to decide how to fix #3477.

@sylvestre
Copy link
Contributor

@dmatos2012 sorry but it needs to be rebased

@dmatos2012 dmatos2012 force-pushed the modify-cp-archive-flag-behavior branch from cd74bb8 to fe4da2b Compare February 20, 2023 15:49
@dmatos2012
Copy link
Contributor Author

uff I had completely forgotten about this PR :P @sylvestre , but managed to rebase :)

@github-actions
Copy link

GNU testsuite comparison:

GNU test failed: tests/misc/tee. tests/misc/tee is passing on 'main'. Maybe you have to rebase?
GNU test failed: tests/tail-2/inotify-dir-recreate. tests/tail-2/inotify-dir-recreate is passing on 'main'. Maybe you have to rebase?

tertsdiepraam
tertsdiepraam reviewed Feb 25, 2023
View reviewed changes
src/uu/cp/src/copydir.rs
@@ -93,7 +93,7 @@ impl<'a> Context<'a> {
fn new(root: &'a Path, target: &'a Path) -> std::io::Result<Self> {
let current_dir = env::current_dir()?;
let root_path = current_dir.join(root);
let root_parent = if target.exists() {
let root_parent = if target.exists() && !root.to_str().unwrap().ends_with("/.") {
Copy link
Member

Choose a reason for hiding this comment

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

Is this the only edge case in which the files are copied?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought so from the original issue, but was so long ago I dont remember if there was something else present at the time 😛

Copy link
Member

Choose a reason for hiding this comment

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

Oh yeah it is super long ago 😄

Copy link
Member

Choose a reason for hiding this comment

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

Well, looks good. Sorry it took so long!

@tertsdiepraam tertsdiepraam merged commit c148215 into uutils:main Feb 26, 2023
@dmatos2012 dmatos2012 deleted the modify-cp-archive-flag-behavior branch October 24, 2023 20:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tertsdiepraam tertsdiepraam tertsdiepraam left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

cp -a has a different behavior that GNU's
4 participants
@dmatos2012 @sylvestre @tertsdiepraam @jtracey