rm: use recursive directory traversal with --recursive#7304
Merged
sylvestre merged 3 commits intouutils:mainfrom Feb 18, 2025
Merged
rm: use recursive directory traversal with --recursive#7304sylvestre merged 3 commits intouutils:mainfrom
sylvestre merged 3 commits intouutils:mainfrom
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
41122e0 to
ea9492f
Compare
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
07badcf to
cf49d13
Compare
This comment was marked as outdated.
This comment was marked as outdated.
cf49d13 to
fbd667b
Compare
Check the user write permission directly from the mode instead of using the `Permissions::readonly()` method. This seems to more closely match the behavior of GNU `rm`.
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.
fbd667b to
1606968
Compare
jfinkels
commented
Feb 17, 2025
Comment on lines
+386
to
+406
| // Special case: if we cannot access the metadata because the | ||
| // filename is too long, fall back to try | ||
| // `std::fs::remove_dir_all()`. | ||
| // | ||
| // TODO This is a temporary bandage; we shouldn't need to do this | ||
| // at all. Instead of using the full path like "x/y/z", which | ||
| // causes a `InvalidFilename` error when trying to access the file | ||
| // metadata, we should be able to use just the last part of the | ||
| // path, "z", and know that it is relative to the parent, "x/y". | ||
| if let Some(s) = path.to_str() { | ||
| if s.len() > 1000 { | ||
| match std::fs::remove_dir_all(path) { | ||
| Ok(_) => return false, | ||
| Err(e) => { | ||
| let e = e.map_err_context(|| format!("cannot remove {}", path.quote())); | ||
| show_error!("{e}"); | ||
| return true; | ||
| } | ||
| } | ||
| } | ||
| } |
Collaborator
Author
There was a problem hiding this comment.
This is here to pass the GNU test case tests/rm/deep-2.sh, where a very deep directory with a long name is created and then removed (like xxxx/xxxxx/xxxxx/...). The issue is that calling path.metadata() on a path that is too long returns an InvalidFilename error. Instead what we should do is use a relative path name and keep setting the working directory as we recurse. It must be possible because std::fs::remove_dir_all(path) seems to work on the very long path. I wasn't sure how to do that, so if someone knows how, I could update the code here. Otherwise, we can just leave this for now and fix it later.
|
GNU testsuite comparison: |
Contributor
|
kudos ! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This includes the changes from #7306 and #7308.
Change the implementation of
rm -rso that it is explicitly recursiveso that (1) there is one code path regardless of whether
--verboseisgiven and (2) it is easier to be compatible with GNU
rm.This change eliminates a dependency on the
walkdircrate.Fixes #7033, fixes #7305.