Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

rm: -r option fails to remove unreadable directory #7033

Closed
jfinkels opened this issue Dec 30, 2024 · 3 comments · Fixed by #7304
Closed

rm: -r option fails to remove unreadable directory #7033

jfinkels opened this issue Dec 30, 2024 · 3 comments · Fixed by #7304
Labels

Comments

@jfinkels
Copy link
Collaborator

Environment: Ubuntu 20.04, main branch (git commit 00d1866), GNU coreutils v8.30

Steps to reproduce:

mkdir -m a-r -p a/unreadable
rm -rf a

What happens now: uutils rm terminates with an error message, removing neither a nor a/unreadable:

rm: cannot remove 'a': Permission denied

What I expected to happen: GNU rm terminates successfully and removes both directories.

Notes: this is causing a failure in GNU test file tests/rm/empty-inacc.sh.

@r3yc0n1c
Copy link

r3yc0n1c commented Jan 3, 2025

hi, i'd like to work on this

@cakebaker
Copy link
Contributor

@r3yc0n1c sure, go ahead :)

@jfinkels
Copy link
Collaborator Author

I made a first draft in pull request #7299 but it's not as straightforward as it seems. My implementation passed all of our Rust tests, but it caused lots of failures in the GNU test cases. So I think there are lots of holes in our tests.

Also our implementation has a different code path for the verbose and not verbose cases. You can get a sense of the differences here:

$ ./target/debug/rm -vrf a
./target/debug/rm: recursing in 'a': IO error for operation on a/unreadable: Permission denied (os error 13)
./target/debug/rm: cannot remove 'a/unreadable': Directory not empty
./target/debug/rm: cannot remove 'a': Directory not empty

$ ./target/debug/rm -rf a
./target/debug/rm: cannot remove 'a': Permission denied

For comparison, here are the --verbose messages from GNU rm:

$ ../gnu/src/rm -vrf a
removed directory 'a/unreadable'
removed directory 'a'

So I think some things to do here would be

  1. Use the failures in rm: correctly remove inaccessible but empty directory #7299 to create new test cases in Rust to improve our test coverage.
  2. Unify the two code paths for --verbose and not --verbose, if possible.
  3. Then allow deleting the unreadable directory in the test case originally reported.

@jfinkels jfinkels changed the title rm: -r option fails to remove unreadable dir rm: -r option fails to remove unreadable directory Feb 15, 2025
@jfinkels jfinkels removed their assignment Feb 15, 2025
jfinkels added a commit to jfinkels/coreutils that referenced this issue Feb 16, 2025
Change the implementation of `rm -r` so that it is explicitly recursive
so that (1) there is one code path regardless of whether `--verbose` is
given and (2) it is easier to be compatible with GNU `rm`.

This change eliminates a dependency on the `walkdir` crate.

Fixes uutils#7033, fixes uutils#7305, fixes uutils#7307.
jfinkels added a commit to jfinkels/coreutils that referenced this issue Feb 16, 2025
Change the implementation of `rm -r` so that it is explicitly recursive
so that (1) there is one code path regardless of whether `--verbose` is
given and (2) it is easier to be compatible with GNU `rm`.

This change eliminates a dependency on the `walkdir` crate.

Fixes uutils#7033, fixes uutils#7305, fixes uutils#7307.
jfinkels added a commit to jfinkels/coreutils that referenced this issue Feb 17, 2025
Change the implementation of `rm -r` so that it is explicitly recursive
so that (1) there is one code path regardless of whether `--verbose` is
given and (2) it is easier to be compatible with GNU `rm`.

This change eliminates a dependency on the `walkdir` crate.

Fixes uutils#7033, fixes uutils#7305, fixes uutils#7307.
sylvestre pushed a commit to sylvestre/coreutils that referenced this issue Feb 24, 2025
Change the implementation of `rm -r` so that it is explicitly recursive
so that (1) there is one code path regardless of whether `--verbose` is
given and (2) it is easier to be compatible with GNU `rm`.

This change eliminates a dependency on the `walkdir` crate.

Fixes uutils#7033, fixes uutils#7305, fixes uutils#7307.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
3 participants