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: Do not dereference symlink even when dangling - fix issue #3364 #3459

Merged
merged 2 commits into from
May 3, 2022
Merged

cp: Do not dereference symlink even when dangling - fix issue #3364 #3459

merged 2 commits into from
May 3, 2022

Conversation

anastygnome
Copy link
Contributor

@anastygnome anastygnome commented Apr 30, 2022

Fixes an issue with cp not copying symlinks in spite of the -P (no dereference option)
fixes #3364

> ln -s no-such-file dangle
> LANG=C cp dangle d2
cp: cannot stat 'dangle': No such file or directory
[1]> LANG=C cp -P dangle d2
> rm d2
>  ./uu_cp dangle d2
cp: cannot stat 'dangle': No such file or directory
[1]> ./uu_cp -P dangle d2
> rm d2

@anastygnome anastygnome changed the title WIP : Fix issue #3364 (tests are not working) [help needed for tests] issue #3364 (tests are not working) Apr 30, 2022
@anastygnome anastygnome changed the title [help needed for tests] issue #3364 (tests are not working) [help needed for tests] fix issue #3364 (tests are not working) Apr 30, 2022
src/uu/cp/src/cp.rs Outdated Show resolved Hide resolved
Copy link
Member

@tertsdiepraam tertsdiepraam left a comment

Choose a reason for hiding this comment

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

I think your test cases are backwards from your example in the PR. I.e. you do

cp (-P) link file # in the example
cp (-P) file link # in the test

It's probably best to test for both and check what the expected behaviour should be.

tests/by-util/test_cp.rs Outdated Show resolved Hide resolved
@anastygnome anastygnome changed the title [help needed for tests] fix issue #3364 (tests are not working) fix issue #3364 (tests are not working) May 1, 2022
@tertsdiepraam
Copy link
Member

Did you remove the tests from the PR? I thought they were really close to being correct and it would be great to have them!

@tertsdiepraam
Copy link
Member

Don't worry about the failing Windows tests, I'm trying to fix those here: #3460

@anastygnome anastygnome changed the title fix issue #3364 (tests are not working) fix issue #3364 May 1, 2022
@anastygnome
Copy link
Contributor Author

anastygnome commented May 1, 2022

@tertsdiepraam I had forgot to add the tests. There are now back in the PR.

I also managed to get rid when removing the symlink, which should make the whole thing faster !

@anastygnome
Copy link
Contributor Author

@tertsdiepraam @sylvestre
please resume the workflow

@sylvestre
Copy link
Sponsor Contributor

please rename the commit for something more explicit

@anastygnome
Copy link
Contributor Author

will do

@anastygnome
Copy link
Contributor Author

anastygnome commented May 1, 2022

@sylvestre done

@anastygnome
Copy link
Contributor Author

anastygnome commented May 1, 2022

sorry for the hassle @sylvestre my PC is driving me nuts.

Just caught a nasty bug. fixed

@sylvestre
Copy link
Sponsor Contributor

yeah, please push when it is ready ;)

@anastygnome
Copy link
Contributor Author

@sylvestre I'm done

@anastygnome
Copy link
Contributor Author

Can we enable feature is symlink or do you want me to revert this part of the commit @sylvestre

@sylvestre
Copy link
Sponsor Contributor

For now, we haven't discussed about bumping MinRustV.

The error for others:
error[E0658]: use of unstable library feature 'is_symlink'

@anastygnome
Copy link
Contributor Author

anastygnome commented May 1, 2022

I've brought back the old method using metadata for now and added a comment, maybe you can relaunch the tests.

@sylvestre sylvestre changed the title fix issue #3364 Do not dereference symlink even when dangling - fix issue #3364 May 1, 2022
@anastygnome
Copy link
Contributor Author

rebased to pass tests from main @sylvestre

@anastygnome anastygnome changed the title Do not dereference symlink even when dangling - fix issue #3364 cp: Do not dereference symlink even when dangling - fix issue #3364 May 2, 2022
@anastygnome
Copy link
Contributor Author

@sylvestre failing test is unrelated, good to be merged

tests/by-util/test_cp.rs Outdated Show resolved Hide resolved
tests/by-util/test_cp.rs Outdated Show resolved Hide resolved
@sylvestre
Copy link
Sponsor Contributor

why did you push over my changes ?

@anastygnome
Copy link
Contributor Author

anastygnome commented May 3, 2022

@sylvestre thank the colleague who put force by default in my pull and push parameters.
fixed and brought back yours.

The touch command you removed in the test actually encovered a bug (if you try to run the test twice in a row you get an error 17 as the first dangling link is not removed prior to recreation.
I replace an .exists with a .is_file in cp.rs to fix it .

I also squash the add space commit to reduce noise

@anastygnome
Copy link
Contributor Author

failing test has been corrected

Fixes an issue with cp not copying symlinks in spite of the -P  (no dereference option)
Fix issue #3364

Performance improvements

Avoid useless read from metadata and reuse previous dest information

Signed-off-by: anastygnome <noreplygit@protonmail.com>
Signed-off-by: anastygnome <noreplygitemail@protonmail.com>
@sylvestre sylvestre merged commit 7c090af into uutils:main May 3, 2022
@sylvestre
Copy link
Sponsor Contributor

thanks :)

@anastygnome anastygnome deleted the cp-fix branch May 4, 2022 11:01
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.

cp: --no-dereference option should copy dangling symbolic link but doesn't
3 participants