-
-
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: Fix broken symlinks to parent-dir #6464
base: main
Are you sure you want to change the base?
cp: Fix broken symlinks to parent-dir #6464
Conversation
According to the GNU cp official documentation[1], a symlink is only created if all source paths are absolute, unless the destination files are in the current directory. This fix solves this GNU cp incompatibility by: - Adding verifications when handling symlinks, to ensure that a symlink with relative path are only created if the destination is within the current directory. - Adding unit tests to cover this modifications, and ensure the behavior is correctly align with GNU documentation. [1]: https://www.gnu.org/software/coreutils/manual/html_node/cp-invocation.html#index-_002ds-16
61f114c
to
0589e94
Compare
GNU testsuite comparison:
|
GNU testsuite comparison:
|
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.
Looks good! The new feature is in a sensible place, the tests cover both the positive and the negative case, awesome.
I can't merge this yet because GitHub says there are conflicts (even though they actually are only trivial conflicts), and also because one of the tests is broken (see below).
tests/by-util/test_cp.rs
Outdated
ucmd.args(&["--symbolic", "file", "dir"]) | ||
.fails() | ||
.no_stdout() | ||
.stderr_is("cp: dir/file: can make relative symbolic links only in current directory\n"); |
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.
This fails on Windows, because there it generates the error message with a backslash.
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.
Oh I see! Really appreciate the review. About the broken test in Windows, I think stderr_contains()
can be used instead of stderr_is()
to address this, I'm going to try it to solve this compatibility issue with Windows, along with the conflicts.
Due to the nature of Windows using backslash instead of regular slash to separate the directories, the test would fail because it was using a hardcoded output. To address that was changed the method to evaluate the output to a more flexible one, that way the backslash would not be a problem.
GNU testsuite comparison:
|
CI still says the new test is broken, and at first glance the error message seems reasonable. |
The error caused by the backslash in Windows was solved, but there are still 4 tests failing only in Windows, I'll try to figure out why of those failures. |
@luigieli do you have an update on this? thanks |
@sylvestre Sorry for disappearing, I was really busy due to college, so I couldn't exhaust my debugging options and the bugs in Windows are still persisting. Would it be okay if I bring you a more detailed update on this by Monday? |
As I promised, I tried to debug the Windows errors. I used a virtual machine with Windows 11 in it to try reproducing the errors, but it was unsuccessful, for some reason when I |
Fails on Windows on:
|
@luigieli it is still marked as draft, are you still working on it? thanks |
@sylvestre Yes, I'm still trying to debug and solve the fails on Windows. I'll bring news as soon as possible. Appreciate the waiting. Thanks! |
According to the GNU cp official documentation, a symlink is only created if all source paths are absolute, unless the destination files are in the current directory.
This fix solves this GNU cp incompatibility by:
Fixes #6385.