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: link-deref gnu test fix #6378

Merged
merged 2 commits into from
May 19, 2024
Merged

cp: link-deref gnu test fix #6378

merged 2 commits into from
May 19, 2024

Conversation

matrixhead
Copy link
Contributor

@matrixhead matrixhead commented May 7, 2024

this pr tries to fix gnu test case "link deref" for cp utility
these are the changes made:

  1. when trying to copy a folder without -R before throwing "-r not specified" error, cp would make sure that source is not a symlink and noderef option is not given. This way, when both of them are given, it would treat it as a file and copy the symlink itself.

  2. to match gnu cp's behavior, -R won't disable dereference if --link is given

Copy link

github-actions bot commented May 7, 2024

GNU testsuite comparison:

Congrats! The gnu test tests/cp/link-deref is no longer failing!
Skipping an intermittent issue tests/tail/inotify-dir-recreate (passes in this run but fails in the 'main' branch)

@matrixhead matrixhead marked this pull request as ready for review May 7, 2024 17:47
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, except that one typo. (I understand that the typo is also present in the already-existing code; feel free to keep it or fix it as you see fit.)

(Remaining CI failures are known issues #6275 and #6333, so out of scope for this PR.)

Before I merge this, I want to give the other maintainers an opportunity to look at it, because I don't feel 100% confident around symlinks. But this seems very reasonable.

tests/by-util/test_cp.rs Outdated Show resolved Hide resolved
Co-authored-by: Ben Wiederhake <[email protected]>
Copy link

github-actions bot commented May 8, 2024

GNU testsuite comparison:

Congrats! The gnu test tests/cp/link-deref is no longer failing!

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. I'll wait for a few days for the others too look at it, because I don't feel 100% confident around the symlink code.

@matrixhead
Copy link
Contributor Author

is it because I tried to squeeze in deref logic into parsing code

@sylvestre sylvestre merged commit 2e16cbb into uutils:main May 19, 2024
67 of 68 checks passed
@sylvestre
Copy link
Sponsor Contributor

well done!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants