Repository navigation
shred: report directories as 'failed to open for writing: Is a directory' - #15631
emirmrkaljevic wants to merge 2 commits into
Conversation
| // "no such file" error while a permission error falls through to the | ||
| // open-for-writing below, which surfaces the real reason. | ||
| match fs::metadata(path) { | ||
| Ok(md) if md.is_dir() => { |
There was a problem hiding this comment.
This should be caught as EISDIR wneh we actually write instead of calling stat.
| // "no such file" error while a permission error falls through to the | ||
| // open-for-writing below, which surfaces the real reason. | ||
| match fs::metadata(path) { | ||
| Ok(md) if md.is_dir() => { |
There was a problem hiding this comment.
with this, the path.is_dir() check in the trailing slash block above is redundant, could you please remove it?
| } | ||
|
|
||
| #[test] | ||
| fn test_shred_dir_without_trailing_slash() { |
There was a problem hiding this comment.
almost the same as test_shred_trailing_slash_on_dir, could be merged into a rstest with d and d/ (and stderr_only there too), no?
|
|
||
| #[test] | ||
| fn test_shred_dir_without_trailing_slash() { | ||
| // GNU: "shred: d: failed to open for writing: Is a directory", exit 1 |
There was a problem hiding this comment.
the comment seems unnecessary
no need to reference GNU; BSD shred behaves the same way
55ec4a0 to
f1567cc
Compare
|
Thanks for the reviews! Updated in f1567cc (rebased on main):
I also noticed #15182 touches the same area (FIFOs etc.); happy to rebase on top of it or close this one if you prefer that PR. |
| )); | ||
| } | ||
| if path_str.as_encoded_bytes().ends_with(b"/") | ||
| && fs::metadata(path).is_err_and(|e| e.kind() == io::ErrorKind::NotADirectory) |
There was a problem hiding this comment.
If you extracted it as e, uucore::error::strip_errno should provide same error message.
|
Binary size comparison: |
Build the 'failed to open for writing' message from the io::Error with strip_errno instead of a dedicated 'Not a directory' string, and drop the now unused locale key.
| @@ -657,7 +656,8 @@ fn wipe_file( | |||
| // "no such file" error while a permission error falls through to the | |||
| // open-for-writing below, which surfaces the real reason. | |||
| match fs::metadata(path) { | |||
There was a problem hiding this comment.
This PR does not need to do that. But we should remove usage of metadata (or do after I/O error) and use proper inspection for errno.
Fixes #15630
shred don a directory now printsshred: d: failed to open for writing: Is a directory(exit 1), matching GNU and the existing trailing-slash case. Other non-regular files keep theNot a filemessage.Tests: new
test_shred_dir_without_trailing_slash(fails on main, passes with the fix) and the007case oftest_numeric_looking_file_names_are_kept_verbatimupdated to the GNU wording. All 31 shred tests pass, clippy and fmt clean.Found with behave.