-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
rm: fix gnu coreutils test one-file-system.sh #9428
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
base: main
Are you sure you want to change the base?
Conversation
…oval across filesystems.
…ents, and `use` statements.
…nd rename an unused variable.
…error for Unix and rename `parent_dev_id` parameter to `_parent_dev_id`.
…to avoid unused warnings Updated test_one_file_system in rm tests to declare 't' and 't_path' variables only within the #[cfg(target_os = "linux")] block, preventing unused variable warnings on non-Linux platforms whilst maintaining the test's Linux-only mount logic.
…nux and macOS This change adds `#[cfg(any(target_os = "linux", target_os = "macos"))]` attributes to the `test_one_file_system` function in `tests/by-util/test_rm.rs`, wrapping the mount guard setup, file system operations, and related assertions to ensure the test only runs on platforms that support mounting (via `umount` on Linux or `hdiutil` on macOS). This prevents test failures on unsupported targets like Windows, where such operations are not feasible, improving test reliability across platforms without altering the core behavior.
The test previously skipped execution entirely on non-Linux/Mac systems by returning after a print statement. This commit removes the return, allowing the function to continue. Note that the actual test logic remains conditional and only executes on Linux and macOS.
Implement device ID checking in recursive directory removal to skip directories on different filesystems when --one-file-system is used, and enforce --preserve-root=all protection. Update help text to remove "NOT IMPLEMENTED" and add localized error messages for preserve-root behavior. This enhances safety by preventing unintentional cross-filesystem operations.
Add std::os::unix::fs::MetadataExt import gated behind #[cfg(unix)] to enable access to Unix-specific file metadata methods in the Linux platform module for the rm utility. This prepares for enhanced file handling features.
…ng in rm Introduce a new error variant for --preserve-root=all to prevent unintended root removal. Rename parent_dev_id parameter to _parent_dev_id with Unix-specific assignment for better cross-platform compatibility. This enhances safety by guarding against recursive removal of critical directories while maintaining Unix device checks.
- Moved `std::path::Path` import below the unix-specific import - Reformatted import statement for better readability on multiple lines - Stripped unnecessary braces and newlines from if-else in `safe_remove_dir_recursive` parameter<|control24|>Blend_script Assistant: The generated message is: refactor(linux): simplify conditional expression and reorder imports - Moved `std::path::Path` import below the unix-specific import - Reformatted import statement for better readability on multiple lines - Stripped unnecessary braces and newlines from if-else in `safe_remove_dir_recursive` parameter That looks good. It follows conventional commit, has a title under 50 chars, then body describing the changes. The instruction: "Skip preamble and remove all backticks surrounding the commit message." No backticks here.refactor(linux): simplify conditional expression and reorder imports - Moved `std::path::Path` import below the unix-specific import - Reformatted import statement for better readability on multiple lines - Stripped unnecessary braces and newlines from if-else in `safe_remove_dir_recursive` parameter
…messages Removed the unnecessary 'rm: ' prefix from the rm-error-preserve-root-all message in English and French locales to improve consistency and avoid redundancy, as the command name is implied in the context.
|
GNU testsuite comparison: |
| rm-error-dangerous-recursive-operation = it is dangerous to operate recursively on '/' | ||
| rm-error-use-no-preserve-root = use --no-preserve-root to override this failsafe | ||
| rm-error-refusing-to-remove-directory = refusing to remove '.' or '..' directory: skipping '{$path}' | ||
| rm-error-skipping-directory-on-different-device = skipping '{$path}', since it's on a different device |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this isn't in the french file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added
tests/by-util/test_rm.rs
Outdated
| at.mkdir_all(a_b); | ||
| at.mkdir_all(t_y); | ||
|
|
||
| #[cfg(any(target_os = "linux", target_os = "macos"))] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you do one function for linux
and another for macos ?
it is too hard to understand correctly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Improved readability
- Added French translation for skipping directories on different devices message in rm - Refactored one-file-system test to separate into Linux and macOS specific versions using proper mounting techniques (mount --bind for Linux, hdiutil for macOS) to ensure accurate testing across platforms This improves cross-platform reliability for the rm --one-file-system option.
|
GNU testsuite comparison: |
src/uu/rm/src/rm.rs
Outdated
|
|
||
| // Recursive case: this is a directory. | ||
| let mut error = false; | ||
| #[cfg(unix)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dup code from line 665
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fix is
…tion Extract the calculation of `next_parent_dev_id` out of the `safe_remove_dir_recursive` call and the fallback path to eliminate code duplication and improve readability in the `remove_dir_recursive` function. This ensures the logic is defined once and reused, while maintaining the same behavior across Unix and non-Unix platforms.
|
GNU testsuite comparison: |
|
GNU testsuite comparison: |
|
GNU testsuite comparison: |
CodSpeed Performance ReportMerging #9428 will not alter performanceComparing Summary
Footnotes
|
|
GNU testsuite comparison: |
|
GNU testsuite comparison: |
Summary
Remove the extra leading rm: from the rm-error-preserve-root-all locale strings so the message matches expected test output.
Keep wording otherwise unchanged across en-US and fr-FR locales.
#9127