From 6783b5252252416f3252ebf7dac377fb13dd1378 Mon Sep 17 00:00:00 2001 From: mattsu Date: Thu, 20 Nov 2025 17:11:56 +0900 Subject: [PATCH 01/17] feat: implement `--one-file-system` for `rm` to prevent recursive removal across filesystems. --- src/uu/rm/locales/en-US.ftl | 4 +- src/uu/rm/src/rm.rs | 37 ++++++++++++-- tests/by-util/test_rm.rs | 99 +++++++++++++++++++++++++++++++++++++ 3 files changed, 134 insertions(+), 6 deletions(-) diff --git a/src/uu/rm/locales/en-US.ftl b/src/uu/rm/locales/en-US.ftl index a84f746f231..5931766308b 100644 --- a/src/uu/rm/locales/en-US.ftl +++ b/src/uu/rm/locales/en-US.ftl @@ -21,8 +21,7 @@ rm-help-prompt-once = prompt once before removing more than three files, or when rm-help-interactive = prompt according to WHEN: never, once (-I), or always (-i). Without WHEN, prompts always rm-help-one-file-system = when removing a hierarchy recursively, skip any directory that is on a file - system different from that of the corresponding command line argument (NOT - IMPLEMENTED) + system different from that of the corresponding command line argument rm-help-no-preserve-root = do not treat '/' specially rm-help-preserve-root = do not remove '/' (default) rm-help-recursive = remove directories and their contents recursively @@ -42,6 +41,7 @@ rm-error-cannot-remove-is-directory = cannot remove {$file}: Is a directory 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 rm-error-cannot-remove = cannot remove {$file} # Verbose messages diff --git a/src/uu/rm/src/rm.rs b/src/uu/rm/src/rm.rs index a20a57d7f36..b8b629bfebf 100644 --- a/src/uu/rm/src/rm.rs +++ b/src/uu/rm/src/rm.rs @@ -16,6 +16,8 @@ use std::ops::BitOr; use std::os::unix::ffi::OsStrExt; #[cfg(unix)] use std::os::unix::fs::PermissionsExt; +#[cfg(unix)] +use std::os::unix::fs::MetadataExt; use std::path::MAIN_SEPARATOR; use std::path::{Path, PathBuf}; use thiserror::Error; @@ -45,6 +47,8 @@ enum RmError { UseNoPreserveRoot, #[error("{}", translate!("rm-error-refusing-to-remove-directory", "path" => _0.to_string_lossy()))] RefusingToRemoveDirectory(OsString), + #[error("{}", translate!("rm-error-skipping-directory-on-different-device", "path" => _0.to_string_lossy()))] + SkippingDirectoryOnDifferentDevice(OsString), } impl UError for RmError {} @@ -485,8 +489,18 @@ pub fn remove(files: &[&OsStr], options: &Options) -> bool { } any_files_processed = true; + + #[cfg(unix)] + let dev_id = if options.one_fs { + Some(metadata.dev()) + } else { + None + }; + #[cfg(not(unix))] + let dev_id = None; + if metadata.is_dir() { - handle_dir(file, options, progress_bar.as_ref()) + handle_dir(file, options, progress_bar.as_ref(), dev_id) } else if is_symlink_dir(&metadata) { remove_dir(file, options, progress_bar.as_ref()) } else { @@ -585,6 +599,7 @@ fn remove_dir_recursive( path: &Path, options: &Options, progress_bar: Option<&ProgressBar>, + parent_dev_id: Option, ) -> bool { // Base case 1: this is a file or a symbolic link. // @@ -605,6 +620,20 @@ fn remove_dir_recursive( return false; } + // Base case 3: this is a directory on a different device + #[cfg(unix)] + if let Some(parent_dev_id) = parent_dev_id { + if let Ok(metadata) = path.symlink_metadata() { + if metadata.dev() != parent_dev_id { + show_error!( + "{}", + RmError::SkippingDirectoryOnDifferentDevice(path.as_os_str().to_os_string()) + ); + return true; + } + } + } + // Use secure traversal on Linux for all recursive directory removals #[cfg(target_os = "linux")] { @@ -642,7 +671,7 @@ fn remove_dir_recursive( Err(_) => error = true, Ok(entry) => { let child_error = - remove_dir_recursive(&entry.path(), options, progress_bar); + remove_dir_recursive(&entry.path(), options, progress_bar, parent_dev_id); error = error || child_error; } } @@ -684,7 +713,7 @@ fn remove_dir_recursive( } } -fn handle_dir(path: &Path, options: &Options, progress_bar: Option<&ProgressBar>) -> bool { +fn handle_dir(path: &Path, options: &Options, progress_bar: Option<&ProgressBar>, dev_id: Option) -> bool { let mut had_err = false; let path = clean_trailing_slashes(path); @@ -698,7 +727,7 @@ fn handle_dir(path: &Path, options: &Options, progress_bar: Option<&ProgressBar> let is_root = path.has_root() && path.parent().is_none(); if options.recursive && (!is_root || !options.preserve_root) { - had_err = remove_dir_recursive(path, options, progress_bar); + had_err = remove_dir_recursive(path, options, progress_bar, dev_id); } else if options.dir && (!is_root || !options.preserve_root) { had_err = remove_dir(path, options, progress_bar).bitor(had_err); } else if options.recursive { diff --git a/tests/by-util/test_rm.rs b/tests/by-util/test_rm.rs index 38230f2ad36..0d0b47fd386 100644 --- a/tests/by-util/test_rm.rs +++ b/tests/by-util/test_rm.rs @@ -1217,3 +1217,102 @@ fn test_progress_no_output_on_error() { .stderr_contains("cannot remove") .stderr_contains("No such file or directory"); } + +#[test] +fn test_one_file_system() { + let scene = TestScenario::new(util_name!()); + let at = &scene.fixtures; + let t = "other_partition_tmpdir"; + let a_b = "a/b"; + let t_y = "other_partition_tmpdir/y"; + + at.mkdir_all(a_b); + at.mkdir_all(t_y); + + let root = at.as_string(); + let t_path = format!("{}/{}", root, t); + let a_b_path = format!("{}/{}", root, a_b); + + #[cfg(target_os = "linux")] + { + // This test requires root and mount --bind + let status = std::process::Command::new("mount") + .arg("--bind") + .arg(&t_path) + .arg(&a_b_path) + .status(); + + if status.is_err() || !status.unwrap().success() { + println!("Skipping test_one_file_system: mount failed (requires root?)"); + return; + } + } + + #[cfg(target_os = "macos")] + { + // Create a disk image + let dmg_path = format!("{}/auxiliary.dmg", root); + let status = std::process::Command::new("hdiutil") + .args(&["create", "-size", "10m", "-fs", "HFS+", "-volname", "auxiliary", &dmg_path]) + .status(); + + if status.is_err() || !status.unwrap().success() { + println!("Skipping test_one_file_system: hdiutil create failed"); + return; + } + + // Mount the dmg + let status = std::process::Command::new("hdiutil") + .args(&["attach", &dmg_path, "-mountpoint", &a_b_path]) + .status(); + + if status.is_err() || !status.unwrap().success() { + println!("Skipping test_one_file_system: hdiutil attach failed"); + return; + } + } + + #[cfg(not(any(target_os = "linux", target_os = "macos")))] + { + println!("Skipping test_one_file_system: unsupported OS"); + return; + } + + struct MountGuard { + path: String, + #[cfg(target_os = "macos")] + dmg_path: String, + } + impl Drop for MountGuard { + fn drop(&mut self) { + #[cfg(target_os = "linux")] + let _ = std::process::Command::new("umount") + .arg(&self.path) + .status(); + + #[cfg(target_os = "macos")] + { + let _ = std::process::Command::new("hdiutil") + .args(&["detach", &self.path]) + .status(); + let _ = std::fs::remove_file(&self.dmg_path); + } + } + } + + let _guard = MountGuard { + path: a_b_path.clone(), + #[cfg(target_os = "macos")] + dmg_path: format!("{}/auxiliary.dmg", root), + }; + + // rm --one-file-system -rf a + scene.ucmd() + .arg("--one-file-system") + .arg("-rf") + .arg("a") + .fails() + .stderr_contains("skipping 'a/b', since it's on a different device"); + + assert!(at.dir_exists(t_y)); +} From 7ac5f044de7e0307d3f1168d1d2b3934ad6d168d Mon Sep 17 00:00:00 2001 From: mattsu Date: Thu, 20 Nov 2025 17:23:04 +0900 Subject: [PATCH 02/17] refactor: update string formatting to use f-string like syntax --- tests/by-util/test_rm.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/by-util/test_rm.rs b/tests/by-util/test_rm.rs index 0d0b47fd386..dae8593b325 100644 --- a/tests/by-util/test_rm.rs +++ b/tests/by-util/test_rm.rs @@ -1230,8 +1230,8 @@ fn test_one_file_system() { at.mkdir_all(t_y); let root = at.as_string(); - let t_path = format!("{}/{}", root, t); - let a_b_path = format!("{}/{}", root, a_b); + let t_path = format!("{root}/{t}"); + let a_b_path = format!("{root}/{a_b}"); #[cfg(target_os = "linux")] { @@ -1251,7 +1251,7 @@ fn test_one_file_system() { #[cfg(target_os = "macos")] { // Create a disk image - let dmg_path = format!("{}/auxiliary.dmg", root); + let dmg_path = format!("{root}/auxiliary.dmg"); let status = std::process::Command::new("hdiutil") .args(&["create", "-size", "10m", "-fs", "HFS+", "-volname", "auxiliary", &dmg_path]) .status(); @@ -1303,7 +1303,7 @@ fn test_one_file_system() { let _guard = MountGuard { path: a_b_path.clone(), #[cfg(target_os = "macos")] - dmg_path: format!("{}/auxiliary.dmg", root), + dmg_path: format!("{root}/auxiliary.dmg"), }; // rm --one-file-system -rf a From efb91a309c5e2ae6524f094440d0b389e88e2a84 Mon Sep 17 00:00:00 2001 From: mattsu Date: Thu, 20 Nov 2025 17:24:24 +0900 Subject: [PATCH 03/17] style: Improve code readability by reformatting function calls, arguments, and `use` statements. --- src/uu/rm/src/rm.rs | 21 +++++++++++++++------ tests/by-util/test_rm.rs | 20 +++++++++++++++----- 2 files changed, 30 insertions(+), 11 deletions(-) diff --git a/src/uu/rm/src/rm.rs b/src/uu/rm/src/rm.rs index b8b629bfebf..22278d28eda 100644 --- a/src/uu/rm/src/rm.rs +++ b/src/uu/rm/src/rm.rs @@ -15,9 +15,9 @@ use std::ops::BitOr; #[cfg(unix)] use std::os::unix::ffi::OsStrExt; #[cfg(unix)] -use std::os::unix::fs::PermissionsExt; -#[cfg(unix)] use std::os::unix::fs::MetadataExt; +#[cfg(unix)] +use std::os::unix::fs::PermissionsExt; use std::path::MAIN_SEPARATOR; use std::path::{Path, PathBuf}; use thiserror::Error; @@ -489,7 +489,7 @@ pub fn remove(files: &[&OsStr], options: &Options) -> bool { } any_files_processed = true; - + #[cfg(unix)] let dev_id = if options.one_fs { Some(metadata.dev()) @@ -670,8 +670,12 @@ fn remove_dir_recursive( match entry { Err(_) => error = true, Ok(entry) => { - let child_error = - remove_dir_recursive(&entry.path(), options, progress_bar, parent_dev_id); + let child_error = remove_dir_recursive( + &entry.path(), + options, + progress_bar, + parent_dev_id, + ); error = error || child_error; } } @@ -713,7 +717,12 @@ fn remove_dir_recursive( } } -fn handle_dir(path: &Path, options: &Options, progress_bar: Option<&ProgressBar>, dev_id: Option) -> bool { +fn handle_dir( + path: &Path, + options: &Options, + progress_bar: Option<&ProgressBar>, + dev_id: Option, +) -> bool { let mut had_err = false; let path = clean_trailing_slashes(path); diff --git a/tests/by-util/test_rm.rs b/tests/by-util/test_rm.rs index dae8593b325..66024796a63 100644 --- a/tests/by-util/test_rm.rs +++ b/tests/by-util/test_rm.rs @@ -1253,9 +1253,18 @@ fn test_one_file_system() { // Create a disk image let dmg_path = format!("{root}/auxiliary.dmg"); let status = std::process::Command::new("hdiutil") - .args(&["create", "-size", "10m", "-fs", "HFS+", "-volname", "auxiliary", &dmg_path]) + .args(&[ + "create", + "-size", + "10m", + "-fs", + "HFS+", + "-volname", + "auxiliary", + &dmg_path, + ]) .status(); - + if status.is_err() || !status.unwrap().success() { println!("Skipping test_one_file_system: hdiutil create failed"); return; @@ -1299,15 +1308,16 @@ fn test_one_file_system() { } } } - - let _guard = MountGuard { + + let _guard = MountGuard { path: a_b_path.clone(), #[cfg(target_os = "macos")] dmg_path: format!("{root}/auxiliary.dmg"), }; // rm --one-file-system -rf a - scene.ucmd() + scene + .ucmd() .arg("--one-file-system") .arg("-rf") .arg("a") From e86426b106f4a7ebc0feaa4a9f3019286324cab3 Mon Sep 17 00:00:00 2001 From: mattsu Date: Thu, 20 Nov 2025 17:31:28 +0900 Subject: [PATCH 04/17] refactor: update `hdiutil` command arguments to use arrays directly and rename an unused variable. --- tests/by-util/test_rm.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/by-util/test_rm.rs b/tests/by-util/test_rm.rs index 66024796a63..f308bbdfd17 100644 --- a/tests/by-util/test_rm.rs +++ b/tests/by-util/test_rm.rs @@ -1230,7 +1230,7 @@ fn test_one_file_system() { at.mkdir_all(t_y); let root = at.as_string(); - let t_path = format!("{root}/{t}"); + let _t_path = format!("{root}/{t}"); let a_b_path = format!("{root}/{a_b}"); #[cfg(target_os = "linux")] @@ -1253,7 +1253,7 @@ fn test_one_file_system() { // Create a disk image let dmg_path = format!("{root}/auxiliary.dmg"); let status = std::process::Command::new("hdiutil") - .args(&[ + .args([ "create", "-size", "10m", @@ -1272,7 +1272,7 @@ fn test_one_file_system() { // Mount the dmg let status = std::process::Command::new("hdiutil") - .args(&["attach", &dmg_path, "-mountpoint", &a_b_path]) + .args(["attach", &dmg_path, "-mountpoint", &a_b_path]) .status(); if status.is_err() || !status.unwrap().success() { @@ -1302,7 +1302,7 @@ fn test_one_file_system() { #[cfg(target_os = "macos")] { let _ = std::process::Command::new("hdiutil") - .args(&["detach", &self.path]) + .args(["detach", &self.path]) .status(); let _ = std::fs::remove_file(&self.dmg_path); } From 185fe36a740ad11bfdc12567f2096d8137804b9c Mon Sep 17 00:00:00 2001 From: mattsu Date: Thu, 20 Nov 2025 17:35:00 +0900 Subject: [PATCH 05/17] chore: Add 'volname' to spell-checker ignore list. --- tests/by-util/test_rm.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/by-util/test_rm.rs b/tests/by-util/test_rm.rs index f308bbdfd17..f14d4eb374d 100644 --- a/tests/by-util/test_rm.rs +++ b/tests/by-util/test_rm.rs @@ -2,6 +2,7 @@ // // For the full copyright and license information, please view the LICENSE // file that was distributed with this source code. +// spell-checker:ignore volname #![allow(clippy::stable_sort_primitive)] use std::process::Stdio; From d9ea830850f2884bc50058a86ed438c6ccb024a0 Mon Sep 17 00:00:00 2001 From: mattsu Date: Thu, 20 Nov 2025 19:14:24 +0900 Subject: [PATCH 06/17] refactor: conditionally compile `SkippingDirectoryOnDifferentDevice` error for Unix and rename `parent_dev_id` parameter to `_parent_dev_id`. --- src/uu/rm/src/rm.rs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/uu/rm/src/rm.rs b/src/uu/rm/src/rm.rs index 22278d28eda..899226db790 100644 --- a/src/uu/rm/src/rm.rs +++ b/src/uu/rm/src/rm.rs @@ -48,6 +48,7 @@ enum RmError { #[error("{}", translate!("rm-error-refusing-to-remove-directory", "path" => _0.to_string_lossy()))] RefusingToRemoveDirectory(OsString), #[error("{}", translate!("rm-error-skipping-directory-on-different-device", "path" => _0.to_string_lossy()))] + #[cfg(unix)] SkippingDirectoryOnDifferentDevice(OsString), } @@ -599,7 +600,7 @@ fn remove_dir_recursive( path: &Path, options: &Options, progress_bar: Option<&ProgressBar>, - parent_dev_id: Option, + _parent_dev_id: Option, ) -> bool { // Base case 1: this is a file or a symbolic link. // @@ -622,7 +623,7 @@ fn remove_dir_recursive( // Base case 3: this is a directory on a different device #[cfg(unix)] - if let Some(parent_dev_id) = parent_dev_id { + if let Some(parent_dev_id) = _parent_dev_id { if let Ok(metadata) = path.symlink_metadata() { if metadata.dev() != parent_dev_id { show_error!( @@ -674,7 +675,7 @@ fn remove_dir_recursive( &entry.path(), options, progress_bar, - parent_dev_id, + _parent_dev_id, ); error = error || child_error; } From 1ad8a2c17caf78975384f160f4e8132b5ff1e53e Mon Sep 17 00:00:00 2001 From: mattsu Date: Thu, 20 Nov 2025 19:23:41 +0900 Subject: [PATCH 07/17] refactor: remove unused variable prefix for `t_path` in `rm` test --- tests/by-util/test_rm.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/by-util/test_rm.rs b/tests/by-util/test_rm.rs index f14d4eb374d..08027061e8a 100644 --- a/tests/by-util/test_rm.rs +++ b/tests/by-util/test_rm.rs @@ -1231,7 +1231,7 @@ fn test_one_file_system() { at.mkdir_all(t_y); let root = at.as_string(); - let _t_path = format!("{root}/{t}"); + let t_path = format!("{root}/{t}"); let a_b_path = format!("{root}/{a_b}"); #[cfg(target_os = "linux")] From 2058f028cc9aad6957dd5ac5b3faa4b668099144 Mon Sep 17 00:00:00 2001 From: mattsu Date: Thu, 20 Nov 2025 19:51:05 +0900 Subject: [PATCH 08/17] refactor(test): move 't' and 't_path' vars into Linux-specific block 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. --- tests/by-util/test_rm.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/by-util/test_rm.rs b/tests/by-util/test_rm.rs index 08027061e8a..298fb281829 100644 --- a/tests/by-util/test_rm.rs +++ b/tests/by-util/test_rm.rs @@ -1223,7 +1223,6 @@ fn test_progress_no_output_on_error() { fn test_one_file_system() { let scene = TestScenario::new(util_name!()); let at = &scene.fixtures; - let t = "other_partition_tmpdir"; let a_b = "a/b"; let t_y = "other_partition_tmpdir/y"; @@ -1231,12 +1230,13 @@ fn test_one_file_system() { at.mkdir_all(t_y); let root = at.as_string(); - let t_path = format!("{root}/{t}"); let a_b_path = format!("{root}/{a_b}"); #[cfg(target_os = "linux")] { + let t = "other_partition_tmpdir"; // This test requires root and mount --bind + let t_path = format!("{root}/{t}"); let status = std::process::Command::new("mount") .arg("--bind") .arg(&t_path) From 131728549f5f90cf1bad5b8cf73740f32183525d Mon Sep 17 00:00:00 2001 From: mattsu Date: Thu, 20 Nov 2025 20:16:17 +0900 Subject: [PATCH 09/17] refactor(test_rm)!: conditionally compile test_one_file_system for Linux 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. --- tests/by-util/test_rm.rs | 68 ++++++++++++++++++++++------------------ 1 file changed, 37 insertions(+), 31 deletions(-) diff --git a/tests/by-util/test_rm.rs b/tests/by-util/test_rm.rs index 298fb281829..6e72fc2ac1f 100644 --- a/tests/by-util/test_rm.rs +++ b/tests/by-util/test_rm.rs @@ -1229,7 +1229,10 @@ fn test_one_file_system() { at.mkdir_all(a_b); at.mkdir_all(t_y); + #[cfg(any(target_os = "linux", target_os = "macos"))] let root = at.as_string(); + + #[cfg(any(target_os = "linux", target_os = "macos"))] let a_b_path = format!("{root}/{a_b}"); #[cfg(target_os = "linux")] @@ -1288,42 +1291,45 @@ fn test_one_file_system() { return; } - struct MountGuard { - path: String, - #[cfg(target_os = "macos")] - dmg_path: String, - } - impl Drop for MountGuard { - fn drop(&mut self) { - #[cfg(target_os = "linux")] - let _ = std::process::Command::new("umount") - .arg(&self.path) - .status(); - + #[cfg(any(target_os = "linux", target_os = "macos"))] + { + struct MountGuard { + path: String, #[cfg(target_os = "macos")] - { - let _ = std::process::Command::new("hdiutil") - .args(["detach", &self.path]) + dmg_path: String, + } + impl Drop for MountGuard { + fn drop(&mut self) { + #[cfg(target_os = "linux")] + let _ = std::process::Command::new("umount") + .arg(&self.path) .status(); - let _ = std::fs::remove_file(&self.dmg_path); + + #[cfg(target_os = "macos")] + { + let _ = std::process::Command::new("hdiutil") + .args(["detach", &self.path]) + .status(); + let _ = std::fs::remove_file(&self.dmg_path); + } } } - } - let _guard = MountGuard { - path: a_b_path.clone(), - #[cfg(target_os = "macos")] - dmg_path: format!("{root}/auxiliary.dmg"), - }; + let _guard = MountGuard { + path: a_b_path.clone(), + #[cfg(target_os = "macos")] + dmg_path: format!("{root}/auxiliary.dmg"), + }; - // rm --one-file-system -rf a - scene - .ucmd() - .arg("--one-file-system") - .arg("-rf") - .arg("a") - .fails() - .stderr_contains("skipping 'a/b', since it's on a different device"); + // rm --one-file-system -rf a + scene + .ucmd() + .arg("--one-file-system") + .arg("-rf") + .arg("a") + .fails() + .stderr_contains("skipping 'a/b', since it's on a different device"); - assert!(at.dir_exists(t_y)); + assert!(at.dir_exists(t_y)); + } } From ca5707f721eadca7c30d746cbb0241954c91c668 Mon Sep 17 00:00:00 2001 From: mattsu Date: Thu, 20 Nov 2025 20:29:47 +0900 Subject: [PATCH 10/17] test(rm): Remove early return in test_one_file_system for unsupported OS 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. --- tests/by-util/test_rm.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/by-util/test_rm.rs b/tests/by-util/test_rm.rs index 6e72fc2ac1f..6b38c7be921 100644 --- a/tests/by-util/test_rm.rs +++ b/tests/by-util/test_rm.rs @@ -1288,7 +1288,6 @@ fn test_one_file_system() { #[cfg(not(any(target_os = "linux", target_os = "macos")))] { println!("Skipping test_one_file_system: unsupported OS"); - return; } #[cfg(any(target_os = "linux", target_os = "macos"))] From 7203d621a4b2f05400dce5a6defc45cd06e83697 Mon Sep 17 00:00:00 2001 From: mattsu Date: Thu, 20 Nov 2025 23:05:53 +0900 Subject: [PATCH 11/17] feat: implement --one-file-system and --preserve-root=all for rm 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. --- src/uu/rm/locales/en-US.ftl | 1 + src/uu/rm/locales/fr-FR.ftl | 4 +- src/uu/rm/src/platform/linux.rs | 60 ++++++++++++++++++++++-- src/uu/rm/src/rm.rs | 82 +++++++++++++++++++++++++-------- 4 files changed, 122 insertions(+), 25 deletions(-) diff --git a/src/uu/rm/locales/en-US.ftl b/src/uu/rm/locales/en-US.ftl index 5931766308b..26f0ee71f85 100644 --- a/src/uu/rm/locales/en-US.ftl +++ b/src/uu/rm/locales/en-US.ftl @@ -42,6 +42,7 @@ rm-error-dangerous-recursive-operation = it is dangerous to operate recursively 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 +rm-error-preserve-root-all = rm: and --preserve-root=all is in effect rm-error-cannot-remove = cannot remove {$file} # Verbose messages diff --git a/src/uu/rm/locales/fr-FR.ftl b/src/uu/rm/locales/fr-FR.ftl index a3da4ba0b2b..132405bbba6 100644 --- a/src/uu/rm/locales/fr-FR.ftl +++ b/src/uu/rm/locales/fr-FR.ftl @@ -21,8 +21,7 @@ rm-help-prompt-once = demander une fois avant de supprimer plus de trois fichier rm-help-interactive = demander selon QUAND : never, once (-I), ou always (-i). Sans QUAND, demande toujours rm-help-one-file-system = lors de la suppression récursive d'une hiérarchie, ignorer tout répertoire situé sur un - système de fichiers différent de celui de l'argument de ligne de commande correspondant (NON - IMPLÉMENTÉ) + système de fichiers différent de celui de l'argument de ligne de commande correspondant rm-help-no-preserve-root = ne pas traiter '/' spécialement rm-help-preserve-root = ne pas supprimer '/' (par défaut) rm-help-recursive = supprimer les répertoires et leur contenu récursivement @@ -42,6 +41,7 @@ rm-error-cannot-remove-is-directory = impossible de supprimer {$file} : C'est un rm-error-dangerous-recursive-operation = il est dangereux d'opérer récursivement sur '/' rm-error-use-no-preserve-root = utilisez --no-preserve-root pour outrepasser cette protection rm-error-refusing-to-remove-directory = refus de supprimer le répertoire '.' ou '..' : ignorer '{$path}' +rm-error-preserve-root-all = rm : et --preserve-root=all est actif rm-error-cannot-remove = impossible de supprimer {$file} # Messages verbeux diff --git a/src/uu/rm/src/platform/linux.rs b/src/uu/rm/src/platform/linux.rs index 6c7d3239572..683ed100108 100644 --- a/src/uu/rm/src/platform/linux.rs +++ b/src/uu/rm/src/platform/linux.rs @@ -18,7 +18,7 @@ use uucore::show_error; use uucore::translate; use super::super::{ - InteractiveMode, Options, is_dir_empty, is_readable_metadata, prompt_descend, prompt_dir, + InteractiveMode, Options, RmError, is_dir_empty, is_readable_metadata, prompt_descend, prompt_dir, prompt_file, remove_file, show_permission_denied_error, show_removal_error, verbose_removed_directory, verbose_removed_file, }; @@ -193,6 +193,7 @@ pub fn safe_remove_dir_recursive( path: &Path, options: &Options, progress_bar: Option<&ProgressBar>, + current_dev_id: Option, ) -> bool { // Base case 1: this is a file or a symbolic link. // Use lstat to avoid race condition between check and use @@ -206,6 +207,16 @@ pub fn safe_remove_dir_recursive( } } + let check_device = options.one_fs || options.preserve_root_all; + #[cfg(unix)] + let current_dev_id = if check_device { + current_dev_id.or_else(|| path.symlink_metadata().ok().map(|m| m.dev())) + } else { + None + }; + #[cfg(not(unix))] + let current_dev_id: Option = None; + // Try to open the directory using DirFd for secure traversal let dir_fd = match DirFd::open(path) { Ok(fd) => fd, @@ -226,7 +237,16 @@ pub fn safe_remove_dir_recursive( } }; - let error = safe_remove_dir_recursive_impl(path, &dir_fd, options); + let error = safe_remove_dir_recursive_impl( + path, + &dir_fd, + options, + if check_device { + current_dev_id + } else { + None + }, + ); // After processing all children, remove the directory itself if error { @@ -256,7 +276,12 @@ pub fn safe_remove_dir_recursive( } } -pub fn safe_remove_dir_recursive_impl(path: &Path, dir_fd: &DirFd, options: &Options) -> bool { +pub fn safe_remove_dir_recursive_impl( + path: &Path, + dir_fd: &DirFd, + options: &Options, + parent_dev_id: Option, +) -> bool { // Read directory entries using safe traversal let entries = match dir_fd.read_dir() { Ok(entries) => entries, @@ -272,6 +297,7 @@ pub fn safe_remove_dir_recursive_impl(path: &Path, dir_fd: &DirFd, options: &Opt }; let mut error = false; + let check_device = options.one_fs || options.preserve_root_all; // Process each entry for entry_name in entries { @@ -290,6 +316,23 @@ pub fn safe_remove_dir_recursive_impl(path: &Path, dir_fd: &DirFd, options: &Opt let is_dir = (entry_stat.st_mode & libc::S_IFMT) == libc::S_IFDIR; if is_dir { + if check_device { + if let Some(parent_dev_id) = parent_dev_id { + if entry_stat.st_dev != parent_dev_id { + show_error!( + "{}", + RmError::SkippingDirectoryOnDifferentDevice( + entry_path.as_os_str().to_os_string() + ) + ); + if options.preserve_root_all { + show_error!("{}", RmError::PreserveRootAllInEffect); + } + error = true; + continue; + } + } + } // Ask user if they want to descend into this directory if options.interactive == InteractiveMode::Always && !is_dir_empty(&entry_path) @@ -318,7 +361,16 @@ pub fn safe_remove_dir_recursive_impl(path: &Path, dir_fd: &DirFd, options: &Opt } }; - let child_error = safe_remove_dir_recursive_impl(&entry_path, &child_dir_fd, options); + let child_error = safe_remove_dir_recursive_impl( + &entry_path, + &child_dir_fd, + options, + if check_device { + Some(entry_stat.st_dev) + } else { + None + }, + ); error = error || child_error; // Ask user permission if needed for this subdirectory diff --git a/src/uu/rm/src/rm.rs b/src/uu/rm/src/rm.rs index 899226db790..e705fd24b2a 100644 --- a/src/uu/rm/src/rm.rs +++ b/src/uu/rm/src/rm.rs @@ -50,6 +50,8 @@ enum RmError { #[error("{}", translate!("rm-error-skipping-directory-on-different-device", "path" => _0.to_string_lossy()))] #[cfg(unix)] SkippingDirectoryOnDifferentDevice(OsString), + #[error("{}", translate!("rm-error-preserve-root-all"))] + PreserveRootAllInEffect, } impl UError for RmError {} @@ -158,6 +160,8 @@ pub struct Options { pub one_fs: bool, /// `--preserve-root`/`--no-preserve-root` pub preserve_root: bool, + /// `--preserve-root=all` + pub preserve_root_all: bool, /// `-r`, `--recursive` pub recursive: bool, /// `-d`, `--dir` @@ -179,6 +183,7 @@ impl Default for Options { interactive: InteractiveMode::PromptProtected, one_fs: false, preserve_root: true, + preserve_root_all: false, recursive: false, dir: false, verbose: false, @@ -248,6 +253,10 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> { }, one_fs: matches.get_flag(OPT_ONE_FILE_SYSTEM), preserve_root: !matches.get_flag(OPT_NO_PRESERVE_ROOT), + preserve_root_all: matches + .get_many::(OPT_PRESERVE_ROOT) + .map(|values| values.into_iter().any(|v| v == "all")) + .unwrap_or(false), recursive: matches.get_flag(OPT_RECURSIVE), dir: matches.get_flag(OPT_DIR), verbose: matches.get_flag(OPT_VERBOSE), @@ -346,7 +355,10 @@ pub fn uu_app() -> Command { Arg::new(OPT_PRESERVE_ROOT) .long(OPT_PRESERVE_ROOT) .help(translate!("rm-help-preserve-root")) - .action(ArgAction::SetTrue), + .num_args(0..=1) + .require_equals(true) + .value_parser(ShortcutValueParser::new([PossibleValue::new("all")])) + .action(ArgAction::Append), ) .arg( Arg::new(OPT_RECURSIVE) @@ -492,16 +504,18 @@ pub fn remove(files: &[&OsStr], options: &Options) -> bool { any_files_processed = true; #[cfg(unix)] - let dev_id = if options.one_fs { - Some(metadata.dev()) + let parent_dev_id = if options.one_fs || options.preserve_root_all { + file.parent() + .and_then(|p| p.symlink_metadata().ok()) + .map(|m| m.dev()) } else { None }; #[cfg(not(unix))] - let dev_id = None; + let parent_dev_id = None; if metadata.is_dir() { - handle_dir(file, options, progress_bar.as_ref(), dev_id) + handle_dir(file, options, progress_bar.as_ref(), parent_dev_id) } else if is_symlink_dir(&metadata) { remove_dir(file, options, progress_bar.as_ref()) } else { @@ -600,15 +614,20 @@ fn remove_dir_recursive( path: &Path, options: &Options, progress_bar: Option<&ProgressBar>, - _parent_dev_id: Option, + parent_dev_id: Option, ) -> bool { + let metadata = match path.symlink_metadata() { + Ok(metadata) => metadata, + Err(e) => return show_removal_error(e, path), + }; + // Base case 1: this is a file or a symbolic link. // // The symbolic link case is important because it could be a link to // a directory and we don't want to recurse. In particular, this // avoids an infinite recursion in the case of a link to the current // directory, like `ln -s . link`. - if !path.is_dir() || path.is_symlink() { + if !metadata.is_dir() || path.is_symlink() { return remove_file(path, options, progress_bar); } @@ -623,22 +642,39 @@ fn remove_dir_recursive( // Base case 3: this is a directory on a different device #[cfg(unix)] - if let Some(parent_dev_id) = _parent_dev_id { - if let Ok(metadata) = path.symlink_metadata() { - if metadata.dev() != parent_dev_id { - show_error!( - "{}", - RmError::SkippingDirectoryOnDifferentDevice(path.as_os_str().to_os_string()) - ); - return true; + if let Some(parent_dev_id) = parent_dev_id { + if metadata.dev() != parent_dev_id { + show_error!( + "{}", + RmError::SkippingDirectoryOnDifferentDevice(path.as_os_str().to_os_string()) + ); + if options.preserve_root_all { + show_error!("{}", RmError::PreserveRootAllInEffect); } + return true; } } // Use secure traversal on Linux for all recursive directory removals #[cfg(target_os = "linux")] { - safe_remove_dir_recursive(path, options, progress_bar) + safe_remove_dir_recursive( + path, + options, + progress_bar, + if options.one_fs || options.preserve_root_all { + #[cfg(unix)] + { + Some(metadata.dev()) + } + #[cfg(not(unix))] + { + None + } + } else { + None + }, + ) } // Fallback for non-Linux or use fs::remove_dir_all for very long paths @@ -661,6 +697,14 @@ fn remove_dir_recursive( // Recursive case: this is a directory. let mut error = false; + #[cfg(unix)] + let next_parent_dev_id = if options.one_fs || options.preserve_root_all { + Some(metadata.dev()) + } else { + None + }; + #[cfg(not(unix))] + let next_parent_dev_id = None; match fs::read_dir(path) { Err(e) if e.kind() == io::ErrorKind::PermissionDenied => { // This is not considered an error. @@ -675,7 +719,7 @@ fn remove_dir_recursive( &entry.path(), options, progress_bar, - _parent_dev_id, + next_parent_dev_id, ); error = error || child_error; } @@ -722,7 +766,7 @@ fn handle_dir( path: &Path, options: &Options, progress_bar: Option<&ProgressBar>, - dev_id: Option, + parent_dev_id: Option, ) -> bool { let mut had_err = false; @@ -737,7 +781,7 @@ fn handle_dir( let is_root = path.has_root() && path.parent().is_none(); if options.recursive && (!is_root || !options.preserve_root) { - had_err = remove_dir_recursive(path, options, progress_bar, dev_id); + had_err = remove_dir_recursive(path, options, progress_bar, parent_dev_id); } else if options.dir && (!is_root || !options.preserve_root) { had_err = remove_dir(path, options, progress_bar).bitor(had_err); } else if options.recursive { From e226f501418a312368027a33b25dbf3de43408fc Mon Sep 17 00:00:00 2001 From: mattsu Date: Thu, 20 Nov 2025 23:14:21 +0900 Subject: [PATCH 12/17] feat(rm/linux): add conditional import for Unix MetadataExt 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. --- src/uu/rm/src/platform/linux.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/uu/rm/src/platform/linux.rs b/src/uu/rm/src/platform/linux.rs index 683ed100108..2ef30bf3334 100644 --- a/src/uu/rm/src/platform/linux.rs +++ b/src/uu/rm/src/platform/linux.rs @@ -11,6 +11,8 @@ use indicatif::ProgressBar; use std::ffi::OsStr; use std::fs; use std::path::Path; +#[cfg(unix)] +use std::os::unix::fs::MetadataExt; use uucore::display::Quotable; use uucore::error::FromIo; use uucore::safe_traversal::DirFd; From cb71eb377bb4bd9a67c775e42cb91bf7ed961260 Mon Sep 17 00:00:00 2001 From: mattsu Date: Thu, 20 Nov 2025 23:28:47 +0900 Subject: [PATCH 13/17] feat: add PreserveRootAllInEffect error and conditional dev ID handling 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. --- src/uu/rm/src/rm.rs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/uu/rm/src/rm.rs b/src/uu/rm/src/rm.rs index e705fd24b2a..8012dda317d 100644 --- a/src/uu/rm/src/rm.rs +++ b/src/uu/rm/src/rm.rs @@ -50,6 +50,7 @@ enum RmError { #[error("{}", translate!("rm-error-skipping-directory-on-different-device", "path" => _0.to_string_lossy()))] #[cfg(unix)] SkippingDirectoryOnDifferentDevice(OsString), + #[cfg(unix)] #[error("{}", translate!("rm-error-preserve-root-all"))] PreserveRootAllInEffect, } @@ -614,8 +615,11 @@ fn remove_dir_recursive( path: &Path, options: &Options, progress_bar: Option<&ProgressBar>, - parent_dev_id: Option, + _parent_dev_id: Option, ) -> bool { + #[cfg(unix)] + let parent_dev_id = _parent_dev_id; + let metadata = match path.symlink_metadata() { Ok(metadata) => metadata, Err(e) => return show_removal_error(e, path), From ac13e6abf4d9ce50ae745c04bc409997b23c6d5a Mon Sep 17 00:00:00 2001 From: mattsu Date: Thu, 20 Nov 2025 23:37:58 +0900 Subject: [PATCH 14/17] 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<|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 --- src/uu/rm/src/platform/linux.rs | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/src/uu/rm/src/platform/linux.rs b/src/uu/rm/src/platform/linux.rs index 2ef30bf3334..44189b737c5 100644 --- a/src/uu/rm/src/platform/linux.rs +++ b/src/uu/rm/src/platform/linux.rs @@ -10,9 +10,9 @@ use indicatif::ProgressBar; use std::ffi::OsStr; use std::fs; -use std::path::Path; #[cfg(unix)] use std::os::unix::fs::MetadataExt; +use std::path::Path; use uucore::display::Quotable; use uucore::error::FromIo; use uucore::safe_traversal::DirFd; @@ -20,8 +20,8 @@ use uucore::show_error; use uucore::translate; use super::super::{ - InteractiveMode, Options, RmError, is_dir_empty, is_readable_metadata, prompt_descend, prompt_dir, - prompt_file, remove_file, show_permission_denied_error, show_removal_error, + InteractiveMode, Options, RmError, is_dir_empty, is_readable_metadata, prompt_descend, + prompt_dir, prompt_file, remove_file, show_permission_denied_error, show_removal_error, verbose_removed_directory, verbose_removed_file, }; @@ -243,11 +243,7 @@ pub fn safe_remove_dir_recursive( path, &dir_fd, options, - if check_device { - current_dev_id - } else { - None - }, + if check_device { current_dev_id } else { None }, ); // After processing all children, remove the directory itself From 37a462d72c3cda96b38765316c956fcb33133403 Mon Sep 17 00:00:00 2001 From: mattsu Date: Fri, 21 Nov 2025 08:16:09 +0900 Subject: [PATCH 15/17] fix(locales): remove redundant 'rm:' prefix from preserve-root error 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. --- src/uu/rm/locales/en-US.ftl | 2 +- src/uu/rm/locales/fr-FR.ftl | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/uu/rm/locales/en-US.ftl b/src/uu/rm/locales/en-US.ftl index 26f0ee71f85..185041cf5df 100644 --- a/src/uu/rm/locales/en-US.ftl +++ b/src/uu/rm/locales/en-US.ftl @@ -42,7 +42,7 @@ rm-error-dangerous-recursive-operation = it is dangerous to operate recursively 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 -rm-error-preserve-root-all = rm: and --preserve-root=all is in effect +rm-error-preserve-root-all = and --preserve-root=all is in effect rm-error-cannot-remove = cannot remove {$file} # Verbose messages diff --git a/src/uu/rm/locales/fr-FR.ftl b/src/uu/rm/locales/fr-FR.ftl index 132405bbba6..1169b1a438e 100644 --- a/src/uu/rm/locales/fr-FR.ftl +++ b/src/uu/rm/locales/fr-FR.ftl @@ -41,7 +41,7 @@ rm-error-cannot-remove-is-directory = impossible de supprimer {$file} : C'est un rm-error-dangerous-recursive-operation = il est dangereux d'opérer récursivement sur '/' rm-error-use-no-preserve-root = utilisez --no-preserve-root pour outrepasser cette protection rm-error-refusing-to-remove-directory = refus de supprimer le répertoire '.' ou '..' : ignorer '{$path}' -rm-error-preserve-root-all = rm : et --preserve-root=all est actif +rm-error-preserve-root-all = et --preserve-root=all est actif rm-error-cannot-remove = impossible de supprimer {$file} # Messages verbeux From 8c1ca946d96a5560d3d01792d5b983718977906d Mon Sep 17 00:00:00 2001 From: mattsu Date: Sat, 22 Nov 2025 08:38:51 +0900 Subject: [PATCH 16/17] feat: add French localization and fix --one-file-system tests for rm - 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. --- src/uu/rm/locales/fr-FR.ftl | 1 + tests/by-util/test_rm.rs | 188 +++++++++++++++++++----------------- 2 files changed, 102 insertions(+), 87 deletions(-) diff --git a/src/uu/rm/locales/fr-FR.ftl b/src/uu/rm/locales/fr-FR.ftl index 1169b1a438e..5bcb0e9e430 100644 --- a/src/uu/rm/locales/fr-FR.ftl +++ b/src/uu/rm/locales/fr-FR.ftl @@ -41,6 +41,7 @@ rm-error-cannot-remove-is-directory = impossible de supprimer {$file} : C'est un rm-error-dangerous-recursive-operation = il est dangereux d'opérer récursivement sur '/' rm-error-use-no-preserve-root = utilisez --no-preserve-root pour outrepasser cette protection rm-error-refusing-to-remove-directory = refus de supprimer le répertoire '.' ou '..' : ignorer '{$path}' +rm-error-skipping-directory-on-different-device = ignorer '{$path}', car il se trouve sur un périphérique différent rm-error-preserve-root-all = et --preserve-root=all est actif rm-error-cannot-remove = impossible de supprimer {$file} diff --git a/tests/by-util/test_rm.rs b/tests/by-util/test_rm.rs index 6b38c7be921..010e373b629 100644 --- a/tests/by-util/test_rm.rs +++ b/tests/by-util/test_rm.rs @@ -1219,8 +1219,9 @@ fn test_progress_no_output_on_error() { .stderr_contains("No such file or directory"); } +#[cfg(target_os = "linux")] #[test] -fn test_one_file_system() { +fn test_one_file_system_linux() { let scene = TestScenario::new(util_name!()); let at = &scene.fixtures; let a_b = "a/b"; @@ -1229,106 +1230,119 @@ fn test_one_file_system() { at.mkdir_all(a_b); at.mkdir_all(t_y); - #[cfg(any(target_os = "linux", target_os = "macos"))] let root = at.as_string(); - - #[cfg(any(target_os = "linux", target_os = "macos"))] let a_b_path = format!("{root}/{a_b}"); + let t_path = format!("{root}/other_partition_tmpdir"); + + // This test requires root privileges and mount --bind support. + let status = std::process::Command::new("mount") + .arg("--bind") + .arg(&t_path) + .arg(&a_b_path) + .status(); + + if status.is_err() || !status.unwrap().success() { + println!("Skipping test_one_file_system_linux: mount --bind failed (requires root?)"); + return; + } - #[cfg(target_os = "linux")] - { - let t = "other_partition_tmpdir"; - // This test requires root and mount --bind - let t_path = format!("{root}/{t}"); - let status = std::process::Command::new("mount") - .arg("--bind") - .arg(&t_path) - .arg(&a_b_path) - .status(); - - if status.is_err() || !status.unwrap().success() { - println!("Skipping test_one_file_system: mount failed (requires root?)"); - return; + struct MountGuard { + path: String, + } + impl Drop for MountGuard { + fn drop(&mut self) { + let _ = std::process::Command::new("umount") + .arg(&self.path) + .status(); } } + let _guard = MountGuard { + path: a_b_path.clone(), + }; - #[cfg(target_os = "macos")] - { - // Create a disk image - let dmg_path = format!("{root}/auxiliary.dmg"); - let status = std::process::Command::new("hdiutil") - .args([ - "create", - "-size", - "10m", - "-fs", - "HFS+", - "-volname", - "auxiliary", - &dmg_path, - ]) - .status(); - - if status.is_err() || !status.unwrap().success() { - println!("Skipping test_one_file_system: hdiutil create failed"); - return; - } + scene + .ucmd() + .arg("--one-file-system") + .arg("-rf") + .arg("a") + .fails() + .stderr_contains("skipping 'a/b', since it's on a different device"); - // Mount the dmg - let status = std::process::Command::new("hdiutil") - .args(["attach", &dmg_path, "-mountpoint", &a_b_path]) - .status(); + assert!(at.dir_exists(t_y)); +} - if status.is_err() || !status.unwrap().success() { - println!("Skipping test_one_file_system: hdiutil attach failed"); - return; - } +#[cfg(target_os = "macos")] +#[test] +fn test_one_file_system_macos() { + let scene = TestScenario::new(util_name!()); + let at = &scene.fixtures; + let a_b = "a/b"; + let t_y = "other_partition_tmpdir/y"; + + at.mkdir_all(a_b); + at.mkdir_all(t_y); + + let root = at.as_string(); + let a_b_path = format!("{root}/{a_b}"); + let dmg_path = format!("{root}/auxiliary.dmg"); + + // Create a disk image to simulate another filesystem. + let create_status = std::process::Command::new("hdiutil") + .args([ + "create", + "-size", + "10m", + "-fs", + "HFS+", + "-volname", + "auxiliary", + &dmg_path, + ]) + .status(); + if create_status.is_err() || !create_status.unwrap().success() { + println!("Skipping test_one_file_system_macos: hdiutil create failed"); + return; } - #[cfg(not(any(target_os = "linux", target_os = "macos")))] - { - println!("Skipping test_one_file_system: unsupported OS"); + let attach_status = std::process::Command::new("hdiutil") + .args(["attach", &dmg_path, "-mountpoint", &a_b_path]) + .status(); + if attach_status.is_err() || !attach_status.unwrap().success() { + println!("Skipping test_one_file_system_macos: hdiutil attach failed"); + let _ = std::fs::remove_file(&dmg_path); + return; } - #[cfg(any(target_os = "linux", target_os = "macos"))] - { - struct MountGuard { - path: String, - #[cfg(target_os = "macos")] - dmg_path: String, - } - impl Drop for MountGuard { - fn drop(&mut self) { - #[cfg(target_os = "linux")] - let _ = std::process::Command::new("umount") - .arg(&self.path) - .status(); - - #[cfg(target_os = "macos")] - { - let _ = std::process::Command::new("hdiutil") - .args(["detach", &self.path]) - .status(); - let _ = std::fs::remove_file(&self.dmg_path); - } - } + struct MountGuard { + mount_point: String, + dmg_path: String, + } + impl Drop for MountGuard { + fn drop(&mut self) { + let _ = std::process::Command::new("hdiutil") + .args(["detach", &self.mount_point]) + .status(); + let _ = std::fs::remove_file(&self.dmg_path); } + } + let _guard = MountGuard { + mount_point: a_b_path.clone(), + dmg_path: dmg_path.clone(), + }; - let _guard = MountGuard { - path: a_b_path.clone(), - #[cfg(target_os = "macos")] - dmg_path: format!("{root}/auxiliary.dmg"), - }; + scene + .ucmd() + .arg("--one-file-system") + .arg("-rf") + .arg("a") + .fails() + .stderr_contains("skipping 'a/b', since it's on a different device"); - // rm --one-file-system -rf a - scene - .ucmd() - .arg("--one-file-system") - .arg("-rf") - .arg("a") - .fails() - .stderr_contains("skipping 'a/b', since it's on a different device"); + assert!(at.dir_exists(t_y)); +} - assert!(at.dir_exists(t_y)); - } +#[cfg(not(any(target_os = "linux", target_os = "macos")))] +#[test] +fn test_one_file_system_unsupported() { + println!("Skipping test_one_file_system: unsupported OS"); } From 4b1cba877f1bd5c694e80bea35252154ed58ec21 Mon Sep 17 00:00:00 2001 From: mattsu Date: Sat, 22 Nov 2025 18:30:14 +0900 Subject: [PATCH 17/17] refactor(rm): extract next_parent_dev_id calculation to avoid duplication 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. --- src/uu/rm/src/rm.rs | 35 ++++++++++------------------------- 1 file changed, 10 insertions(+), 25 deletions(-) diff --git a/src/uu/rm/src/rm.rs b/src/uu/rm/src/rm.rs index 8012dda317d..9a3cdc5069c 100644 --- a/src/uu/rm/src/rm.rs +++ b/src/uu/rm/src/rm.rs @@ -659,26 +659,19 @@ fn remove_dir_recursive( } } + #[cfg(unix)] + let next_parent_dev_id = if options.one_fs || options.preserve_root_all { + Some(metadata.dev()) + } else { + None + }; + #[cfg(not(unix))] + let next_parent_dev_id = None; + // Use secure traversal on Linux for all recursive directory removals #[cfg(target_os = "linux")] { - safe_remove_dir_recursive( - path, - options, - progress_bar, - if options.one_fs || options.preserve_root_all { - #[cfg(unix)] - { - Some(metadata.dev()) - } - #[cfg(not(unix))] - { - None - } - } else { - None - }, - ) + safe_remove_dir_recursive(path, options, progress_bar, next_parent_dev_id) } // Fallback for non-Linux or use fs::remove_dir_all for very long paths @@ -701,14 +694,6 @@ fn remove_dir_recursive( // Recursive case: this is a directory. let mut error = false; - #[cfg(unix)] - let next_parent_dev_id = if options.one_fs || options.preserve_root_all { - Some(metadata.dev()) - } else { - None - }; - #[cfg(not(unix))] - let next_parent_dev_id = None; match fs::read_dir(path) { Err(e) if e.kind() == io::ErrorKind::PermissionDenied => { // This is not considered an error.