Fixed #1769: rm: -d should match GNU's output #1780
Conversation
I noticed that the verbose option was not implemented at all for rm -rf -v, so I added a quick if statement to fix it. This commit would also fix uutils#1769, as it's one commit ahead of uutils#1780
I noticed that the verbose option was not implemented at all for rm -rf -v, so I added a quick if statement to fix it. This commit would also fix uutils#1769, as it's one commit ahead of uutils#1780
|
All the extra commits are rebases. |
|
Added the test that you wanted |
Using some code from @growse 's pull request, I made a quick match statement that checks if the error is the Directory not found error. If so, it uses the show_custom_error_description macro to output said error, just without the "(os error)" part.
I didn't realize that changing the Cargo.lock would make builds on some platforms fail, so I reverted it back
Just another test in the test_rm_errors fn that runs rm -d on the same dir as the previous test
my PC is very low on space, so Im kind of pushing commits blindly
The output should have been cannot remove, instead of removing. That was a mistake on my end.
There were some minor formatting issues with how the test_rm.rs file looked (I wrote the code on my phone, jaja) so I tidied it up.
|
Yea that's my bad, I wrote the code on my phone. The formatting should be good now. |
Apparently, I will different platforms have different error codes for a directory being full, so I instead check if the error message starts with "Directory not found"
Windows gives a different error message then Unix based OS, so I added some Windows specific code
MinRustV fails if the lock file isnt built for Rust 1.33.0, so I ran cargo +1.33.0 update like it asked
src/uu/rm/src/rm.rs
Outdated
| @@ -306,7 +307,15 @@ fn remove_dir(path: &Path, options: &Options) -> bool { | |||
| } | |||
| } | |||
| Err(e) => { | |||
| show_error!("removing '{}': {}", path.display(), e); | |||
| if e.to_string().starts_with("Directory not empty") || (cfg!(windows) && e.to_string().starts_with("The directory is not empty")) { | |||
There was a problem hiding this comment.
I don't think we can rely on the error message itself; it might change in rustc itself
there is no stable error code here?
There was a problem hiding this comment.
I've tried looking, and I honestly can't find one. Do you have any ideas? I thought that the raw error code would work, but it's different across Windows, Linux, etc. The two error messages given should work for all Unix based operating systems, and Windows is its own beast.
There was a problem hiding this comment.
It also does pass all of the tests, and any non Unix-based OS would be difficult to plan for, since there error codes could be anything at all.
There was a problem hiding this comment.
On Unix, it’s probably best to check errno for ENOTEMPTY/EEXIST (you can use either nix or libc directly). I’m not exactly sure how to check the windows error code, but there’s likely a function in winapi (or the new crate made by Microsoft itself?).
|
@syvlestre that's totally fine |
|
I am not a huge fan for this PR. I think it adds unnecessary complexity. Sorry :( |
Using some code from @growse 's pull request, I made a quick match statement that checks if the error is the Directory not found error. If so, it uses the show_custom_error_description macro to output said error, just without the "(os error)" part.