fix: touch -r: dangling symlink reference is accepted Fixes #9703 #9732
Conversation
tests/by-util/test_touch.rs
Outdated
| dangling_symlink.to_str().unwrap(), | ||
| "some_file", | ||
| ]) | ||
| .fails(); |
There was a problem hiding this comment.
Can you use the
.stderr_contains
To validate what the error message is?
| } | ||
| } | ||
|
|
||
| #[test] |
There was a problem hiding this comment.
So this test will run on many platforms and looking at std::os::unix::fs::symlink this will probably fail on non unix systems, ideally if you can find a way to also run this on windows or otherwise just gate it with a cfg[] for unix
There was a problem hiding this comment.
Ah right thanks, I'll look around for something cross-platform here
There was a problem hiding this comment.
Ok copied the approach in symlink_file in cp.rs, hopefully that's good enough
|
Just some minor comments about validating that the test gives the right error message, but it looks like the implementation is correct and I'm able to replicate the fix is working after building locally with |
|
Thanks for the review! Does uutils try to imitate error messages from GNU? This particular error message seems ambiguous to me. I could take a stab at updating the |
|
GNU testsuite comparison: |
by default, we do like GNU, but if we can provide a better message, we don't hesitate to do it :) |
Sounds reasonable! Ok, just want to double check how others feel about an error message update # Current
touch: failed to get attributes of 'ref': No such file or directory
# Proposed
touch: the target of the symlink 'ref' does not existHappy to go with another wording, just feel inclined to resolve the ambiguity in "No such file or directory" (the symlink is missing or the referent?). If yall are happy with this wording, I'm happy to have that change ride along in this PR, otherwise we can merge this (seemingly complete?) fix PR and do the message update in another. |
|
Sounds good |
|
@sylvestre Would you mind translating this to French so I can update the fr locale as well? :)
|
|
@dshemetov "la cible du lien symbolique {path} n'existe pas" |
3 participants
Took a stab at fixing #9703.
Trapping the dangling symlink error seems straightforward via ErrorKind::NotFound, but I'm not familiar enough with the other errors that could happen here to know whether falling back to fs::symlink_metadata(path) is appropriate. Seems safe to fix one error at a time I suppose.