shred: report directories as 'failed to open for writing: Is a directory' by emirmrkaljevic · Pull Request #15631 · uutils/coreutils

emirmrkaljevic · 2026-10-22T13:10:46Z

shred d on a directory now prints shred: d: failed to open for writing: Is a directory (exit 1), matching GNU and the existing trailing-slash case. Other non-regular files keep the Not a file message.

Tests: new test_shred_dir_without_trailing_slash (fails on main, passes with the fix) and the 007 case of test_numeric_looking_file_names_are_kept_verbatim updated to the GNU wording. All 31 shred tests pass, clippy and fmt clean.

Found with behave.

oech3 · 2026-10-22T13:22:47Z

    // "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() => {


This should be caught as EISDIR wneh we actually write instead of calling stat.

sylvestre · 2026-10-22T14:23:55Z

    // "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() => {


with this, the path.is_dir() check in the trailing slash block above is redundant, could you please remove it?

sylvestre · 2026-10-22T14:24:01Z

 }

+#[test]
+fn test_shred_dir_without_trailing_slash() {


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?

xtqqczze · 2026-10-22T15:56:34Z


+#[test]
+fn test_shred_dir_without_trailing_slash() {
+    // GNU: "shred: d: failed to open for writing: Is a directory", exit 1


the comment seems unnecessary

no need to reference GNU; BSD shred behaves the same way

emirmrkaljevic · 2026-10-23T08:11:05Z

Thanks for the reviews! Updated in f1567cc (rebased on main):

directories are no longer special-cased: they skip the "Not a file" branch and fail when opened for writing, so the message comes from the real EISDIR (@oech3)
removed the path.is_dir() check in the trailing-slash block and the now unused shred-failed-to-open-for-writing-is-a-directory string (@sylvestre)
merged the two directory tests into one rstest with d and d/, using stderr_only, and dropped the GNU comment (@xtqqczze)

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.

oech3 · 2026-10-23T08:14:17Z

-            ));
-        }
+    if path_str.as_encoded_bytes().ends_with(b"/")
+        && fs::metadata(path).is_err_and(|e| e.kind() == io::ErrorKind::NotADirectory)


If you extracted it as e, uucore::error::strip_errno should provide same error message.

Done in 42e21b1, thanks!

github-actions · 2026-10-23T10:17:48Z

Binary size comparison:

Individual binary size comparison VS main (threshold: >=5% AND >=4 KB).

Total size of compared binaries: 155.04 MB (-312 KB, -0.20%)

Significant per-binary changes:
  date     2.61 MB ->    2.30 MB  (-316 KB, -11.81%)

oech3 · 2026-10-26T17:28:18Z

@@ -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) {


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.

Agreed, that would be cleaner: rely on the errno from the failed open (or stat only after an I/O error) instead of probing metadata up front. I'll leave it for a follow-up so this one stays small. Also rebased on main and fixed the Windows test, which expected the Unix "Not a directory" text for a/ (Windows says "The directory name is invalid.").

…ory' like GNU

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.

xtqqczze · 2026-10-27T11:35:21Z

-        .stderr_contains("Not a directory");
+    let result = scene.ucmd().arg("a/").fails();
+    result.stderr_contains("a/: failed to open for writing: ");
+    // Windows reports this as "The directory name is invalid."


test stderr contains this string on Windows?

Yes, the Windows jobs run this test and pass. Both paths that can hit a/ there (the trailing slash check and the open for writing below it) print the a/: failed to open for writing: prefix; only the OS text after it differs ("The directory name is invalid." on Windows), which is why only the "Not a directory" check is cfg(unix).

I mean test that stderr contains it on Windows, something like:

#[cfg(windows)] result.stderr_contains("a/: The directory name is invalid: ");

Ah, got it. Done in bafc9d0: the test now checks the whole a/: failed to open for writing: ... line on both, "Not a directory" on unix and "The directory name is invalid." on Windows.

oech3 reviewed Oct 22, 2026

View reviewed changes

sylvestre reviewed Oct 22, 2026

View reviewed changes

xtqqczze reviewed Oct 22, 2026

View reviewed changes

emirmrkaljevic force-pushed the shred-dir-message branch from 55ec4a0 to f1567cc Compare October 23, 2026 08:10

oech3 reviewed Oct 23, 2026

View reviewed changes

oech3 reviewed Oct 26, 2026

View reviewed changes

emirmrkaljevic added 3 commits October 27, 2026 10:06

shred: report directories as 'failed to open for writing: Is a direct…

7a819bd

…ory' like GNU

shred: use strip_errno for the trailing slash open error

9ae52c0

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.

shred: don't expect the Unix errno text on Windows for a/

aa9beae

emirmrkaljevic force-pushed the shred-dir-message branch from 42e21b1 to aa9beae Compare October 27, 2026 09:06

xtqqczze reviewed Oct 27, 2026

View reviewed changes

tests/shred: check the full a/ message on Windows too

bafc9d0

Conversation

emirmrkaljevic commented Oct 22, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

emirmrkaljevic commented Oct 23, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Oct 23, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants