mv: Fix stderr output mv file into dir and dir into file where both are files #5464
Conversation
mickvangelderen
force-pushed
the
5463-fix-misleading-error-msg-mv
branch
2 times, most recently
from
7a3ad9e to
cadfb80
Compare
src/uu/mv/src/error.rs
Outdated
| CannotStatNotADirectory(String), | ||
| SameFile(String, String), | ||
| SelfSubdirectory(String), | ||
| SelfTargetSubdirectory(String, String), | ||
| DirectoryToNonDirectory(String), | ||
| NonDirectoryToDirectory(String, String), | ||
| NotADirectory(String), | ||
| TargetNotADirectory(String), | ||
| FailedToAccessNotADirectory(PathBuf), |
There was a problem hiding this comment.
Why do you use two different errors?
There was a problem hiding this comment.
Don't they have different outputs?
There was a problem hiding this comment.
Hm, at least on my machine with Arch Linux GNU mv shows the same error in both cases (see the ticket). I could imagine it depends on the underlying system, so we can leave it.
There was a problem hiding this comment.
I'm on Linux pop-os 6.5.6-76060506-generic #202310061235~1697396945~22.04~9283e32 SMP PREEMPT_DYNAMIC Sun O x86_64 x86_64 x86_64 GNU/Linux.
There was a problem hiding this comment.
@cakebaker does this PR really achieve the goal of the issue then? Which mv are we trying to emulate?
There was a problem hiding this comment.
Yes, the goal is to get better error messages and your PR achieves it. We try to be compatible with the latest version of mv. In this case it is not possible, though, as the behavior depends on the underlying system. This is why there is no test for it in https://github.com/coreutils/coreutils/blob/master/tests/mv/trailing-slash.sh and the authors write in a comment:
We would like the erroneous-looking "mv any non-dir/" to fail,
but with the current implementation, it depends on how the
underlying rename syscall handles the trailing slash.
| /// Returns true if the passed `path` ends with a path terminator. | ||
| fn path_ends_with_terminator(path: &Path) -> bool { | ||
| path.as_os_str() | ||
| .as_bytes() |
There was a problem hiding this comment.
I guess you have seen the error from the CI that there is no as_bytes function under Windows?
There was a problem hiding this comment.
The pipeline wouldn't run until someone from this project had approved it. Thanks for pointing out that they are running now.
There was a problem hiding this comment.
@cakebaker can I run the workflows locally? They won't run until they're approved:
There was a problem hiding this comment.
@mickvangelderen the approval is no longer needed and the workflows should run automatically in the future.
|
Thanks for your PR :) |
Thanks for the guidance and swift reviews :) |
sylvestre
changed the title
Fixes #5463
The implementation of mv seems a bit messy, at least to a first time reader. My changes also feel a bit "band-aided" in. Glad we have a bunch of tests but the many repeated calls to
is_dirandsymlink_metadataand hard to understand flow may need to be addressed.