-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Conversation
There was a problem hiding this 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.
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! |
Don't worry about the failing Windows tests, I'm trying to fix those here: #3460 |
@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 ! |
@tertsdiepraam @sylvestre |
please rename the commit for something more explicit |
will do |
@sylvestre done |
sorry for the hassle @sylvestre my PC is driving me nuts. Just caught a nasty bug. fixed |
yeah, please push when it is ready ;) |
@sylvestre I'm done |
Can we enable feature is symlink or do you want me to revert this part of the commit @sylvestre |
For now, we haven't discussed about bumping MinRustV. The error for others: |
I've brought back the old method using metadata for now and added a comment, maybe you can relaunch the tests. |
rebased to pass tests from main @sylvestre |
@sylvestre failing test is unrelated, good to be merged |
why did you push over my changes ? |
@sylvestre thank the colleague who put force by default in my pull and push parameters. 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 also squash the add space commit to reduce noise |
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 <[email protected]>
Signed-off-by: anastygnome <[email protected]>
thanks :) |
Fixes an issue with cp not copying symlinks in spite of the -P (no dereference option)
fixes #3364