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.

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

cp: Added overwrite detection for existing symlinks #6380

Merged
merged 4 commits into from
May 8, 2024
Merged

cp: Added overwrite detection for existing symlinks #6380

merged 4 commits into from
May 8, 2024

Conversation

AnirbanHalder654322
Copy link
Contributor

Fixes #6265

@AnirbanHalder654322 AnirbanHalder654322 changed the title Added overwrite detection for existing symlinks cp: Added overwrite detection for existing symlinks May 7, 2024
BenWiederhake
BenWiederhake requested changes May 7, 2024
View reviewed changes
Copy link
Collaborator

@BenWiederhake BenWiederhake left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for tackling this issue!

By slightly changing the test setup, you (accidentally?) created another interesting test case, please keep it! :D

EDIT: Regarding the clippy failure: I'm afraid the sheer amount of "state" that is passed between the various copy-functions means that this really should be changed to a struct, perhaps?

tests/by-util/test_cp.rs Outdated Show resolved Hide resolved
tests/by-util/test_cp.rs Outdated Show resolved Hide resolved
@github-actions GitHub Actions
Copy link

github-actions bot commented May 7, 2024

GNU testsuite comparison:

GNU test failed: tests/cp/link-deref. tests/cp/link-deref is passing on 'main'. Maybe you have to rebase?
Congrats! The gnu test tests/cksum/cksum-a is no longer failing!
Congrats! The gnu test tests/cksum/sm3sum is no longer failing!

@AnirbanHalder654322
Copy link
Contributor Author

The state can be refactored and encapsulated within a struct . Perhaps that can be addressed in a future PR .

@github-actions GitHub Actions
Copy link

github-actions bot commented May 7, 2024

GNU testsuite comparison:

Skip an intermittent issue tests/tail/inotify-dir-recreate (fails in this run but passes in the 'main' branch)

1 similar comment
@github-actions GitHub Actions
Copy link

github-actions bot commented May 8, 2024

GNU testsuite comparison:

Skip an intermittent issue tests/tail/inotify-dir-recreate (fails in this run but passes in the 'main' branch)

@BenWiederhake BenWiederhake mentioned this pull request May 8, 2024
Open
BenWiederhake
BenWiederhake requested changes May 8, 2024
View reviewed changes
Copy link
Collaborator

@BenWiederhake BenWiederhake left a comment

Choose a reason for hiding this comment

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

Mostly just questions, there's probably not much to do anymore.

Also, can I convince you to remove #[allow(clippy::too_many_arguments)] in your next PR?

src/uu/cp/src/copydir.rs Show resolved Hide resolved
tests/by-util/test_cp.rs Show resolved Hide resolved
tests/by-util/test_cp.rs Outdated Show resolved Hide resolved
src/uu/cp/src/cp.rs Show resolved Hide resolved
BenWiederhake
BenWiederhake approved these changes May 8, 2024
View reviewed changes
Copy link
Collaborator

@BenWiederhake BenWiederhake left a comment

Choose a reason for hiding this comment

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

LGTM! We're still bad at handling symlinks, but this is a step in the right direction. Yay! :)

@BenWiederhake BenWiederhake merged commit 97f8361 into uutils:main May 8, 2024
67 of 68 checks passed
@AnirbanHalder654322 AnirbanHalder654322 deleted the cp-symlink-overwrite-detection branch May 9, 2024 10:25
@BrewTestBot BrewTestBot mentioned this pull request Jun 23, 2024
Merged
@moonfruit moonfruit mentioned this pull request Jun 24, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BenWiederhake BenWiederhake BenWiederhake approved these changes

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

Successfully merging this pull request may close these issues.

cp: symlink overwrite-detection is broken
2 participants
@AnirbanHalder654322 @BenWiederhake