rm: Check filesystem checks in rm command

- Rename `validate_single_filesystem` to `check_one_fs` for improved clarity and alignment with its functionality.
- Refactore `check_one_fs` to directly handle options such as `--one-file-system` and `--preserve-root=all` and return detailed error messages when the conditions are not met.
- Integrate `check_one_fs` into `remove_dir_recursive` and `handle_dir` for consistent handling of files and directories on different file systems.

Author: shskwmt

src/uu/rm/src/rm.rs

@@ -431,6 +431,17 @@ fn remove_dir_recursive(path: &Path, options: &Options) -> bool {
         }
     }
 
+    if let Err(additional_reason) = check_one_fs(path, options) {
+        if !additional_reason.is_empty() {
+            show_error!("{}", additional_reason);
+        }
+        show_error!(
+            "skipping {}, since it's on a different device",
+            path.quote()
+        );
+        return true;
+    }
+
     // Base case 1: this is a file or a symbolic link.
     //
     // The symbolic link case is important because it could be a link to
@@ -525,10 +536,16 @@ fn mount_for_path<'a>(path: &Path, mounts: &'a [MountInfo]) -> Option<&'a MountI
     best.map(|(mi, _len)| mi)
 }
 
-/// Validates that a path is on the same file system as its parent.
+/// Check if a path is on the same file system when `--one-file-system` or `--preserve-root=all` options are enabled.
 /// Return `OK(())` if the path is on the same file system,
 /// or an additional error describing why it should be skipped.
-fn validate_single_filesystem(path: &Path) -> Result<(), String> {
+fn check_one_fs(path: &Path, options: &Options) -> Result<(), String> {
+    // If neither `--one-file-system` nor `--preserve-root=all` is active,
+    // always proceed
+    if !options.one_fs && options.preserve_root != PreserveRoot::YesAll {
+        return Ok(());
+    }
+
     // Read mount information
     let fs_list = read_fs_list().map_err(|err| format!("cannot read mount info: {err}"))?;
 
@@ -553,42 +570,20 @@ fn validate_single_filesystem(path: &Path) -> Result<(), String> {
     Ok(())
 }
 
-/// Check if a path is on the same file system when `--one-file-system` or `--preserve-root=all` options are enabled.
-/// Return `true` if the path can be processed, `false` if it should be skipped.
-fn check_one_fs(path: &Path, options: &Options) -> bool {
-    // If neither `--one-file-system` nor `--preserve-root=all` is active,
-    // always proceed
-    if !options.one_fs && options.preserve_root != PreserveRoot::YesAll {
-        return true;
-    }
-
-    let result = validate_single_filesystem(path);
-    if result.is_ok() {
-        return true;
-    }
+fn handle_dir(path: &Path, options: &Options) -> bool {
+    let mut had_err = false;
 
-    if let Err(additional_reason) = result {
+    if let Err(additional_reason) = check_one_fs(path, options) {
         if !additional_reason.is_empty() {
             show_error!("{}", additional_reason);
         }
-    }
-
-    show_error!(
-        "skipping {}, since it's on a different device",
-        path.quote()
-    );
-
-    if options.preserve_root == PreserveRoot::YesAll {
-        show_error!("and --preserve-root=all is in effect");
-    }
-
-    false
-}
-
-fn handle_dir(path: &Path, options: &Options) -> bool {
-    let mut had_err = false;
-
-    if !check_one_fs(path, options) {
+        show_error!(
+            "skipping {}, since it's on a different device",
+            path.quote()
+        );
+        if options.preserve_root == PreserveRoot::YesAll {
+            show_error!("and --preserve-root=all is in effect");
+        }
         return true;
     }
 

tests/by-util/test_rm.rs

@@ -971,8 +971,8 @@ fn test_rm_one_file_system() {
     // Define paths for temporary files and directories
     let img_path = "fs.img";
     let mount_point = "fs";
-    let src_dir = "a/b";
-    let bind_mount_point = "a";
+    let remove_dir = "a";
+    let bind_mount_point = "a/b";
 
     at.touch(img_path);
 
@@ -991,8 +991,8 @@ fn test_rm_one_file_system() {
     scene.cmd("/sbin/mkfs.ext4").arg(img_path).succeeds();
 
     // Prepare directory structure
-    at.mkdir_all(src_dir);
     at.mkdir_all(mount_point);
+    at.mkdir_all(bind_mount_point);
 
     // Mount as loop device
     scene
@@ -1012,7 +1012,7 @@ fn test_rm_one_file_system() {
     // Run the test
     scene
         .ucmd()
-        .args(&["--one-file-system", "-rf", bind_mount_point])
+        .args(&["--one-file-system", "-rf", remove_dir])
         .fails()
         .stderr_contains(format!("rm: skipping '{}'", bind_mount_point));
 
@@ -1040,8 +1040,7 @@ fn test_rm_preserve_root() {
     // Define paths for temporary files and directories
     let img_path = "fs.img";
     let mount_point = "fs";
-    let src_dir = "a/b";
-    let bind_mount_point = "a";
+    let bind_mount_point = "a/b";
 
     at.touch(img_path);
 
@@ -1060,8 +1059,8 @@ fn test_rm_preserve_root() {
     scene.cmd("/sbin/mkfs.ext4").arg(img_path).succeeds();
 
     // Prepare directory structure
-    at.mkdir_all(src_dir);
     at.mkdir_all(mount_point);
+    at.mkdir_all(bind_mount_point);
 
     // Mount as loop device
     scene

util/build-gnu.sh

@@ -207,7 +207,7 @@ sed -i -e "s|removed directory 'a/'|removed directory 'a'|g" tests/rm/v-slash.sh
 
 if test "$(grep -c 'rm: skipping '  tests/rm/one-file-system.sh)" -eq 1; then
      # Do it only once.
-     sed -i -e "s|rm: skipping 'a/b'|rm: skipping 'a'|g" -e "s/ >> exp/ > exp/g"  -e "s|rm: and --preserve-root=all is in effect|rm: skipping 'a/b', since it's on a different device\nrm: and --preserve-root=all is in effect|g" tests/rm/one-file-system.sh
+     sed -i -e "s/ >> exp/ > exp/g"  -e "s|rm: and --preserve-root=all is in effect|rm: skipping 'a/b', since it's on a different device\nrm: and --preserve-root=all is in effect|g" tests/rm/one-file-system.sh
 fi
 
 # 'rel' doesn't exist. Our implementation is giving a better message.