Merge pull request #1839 from jhscheer/iss1769

rm: fix for -d to match GNU's output #1769

src/uu/rm/src/rm.rs

@@ -233,7 +233,7 @@ fn remove(files: Vec<String>, options: Options) -> bool {
                 // (e.g., permission), even rm -f should fail with
                 // outputting the error, but there's no easy eay.
                 if !options.force {
-                    show_error!("no such file or directory '{}'", filename);
+                    show_error!("cannot remove '{}': No such file or directory", filename);
                     true
                 } else {
                     false
@@ -289,7 +289,7 @@ fn handle_dir(path: &Path, options: &Options) -> bool {
         had_err = true;
     } else {
         show_error!(
-            "could not remove directory '{}' (did you mean to pass '-r' or '-R'?)",
+            "cannot remove '{}': Is a directory", // GNU's rm error message does not include help
             path.display()
         );
         had_err = true;
@@ -305,17 +305,35 @@ fn remove_dir(path: &Path, options: &Options) -> bool {
         true
     };
     if response {
+        if let Ok(mut read_dir) = fs::read_dir(path) {
+            if options.dir || options.recursive {
+                if read_dir.next().is_none() {
                     match fs::remove_dir(path) {
                         Ok(_) => {
                             if options.verbose {
-                    println!("removed '{}'", path.display());
+                                println!("removed directory '{}'", path.display());
                             }
                         }
                         Err(e) => {
-                show_error!("removing '{}': {}", path.display(), e);
+                            show_error!("cannot remove '{}': {}", path.display(), e);
                             return true;
                         }
                     }
+                } else {
+                    // directory can be read but is not empty
+                    show_error!("cannot remove '{}': Directory not empty", path.display());
+                    return true;
+                }
+            } else {
+                // called to remove a symlink_dir (windows) without "-r"/"-R" or "-d"
+                show_error!("cannot remove '{}': Is a directory", path.display());
+                return true;
+            }
+        } else {
+            // GNU's rm shows this message if directory is empty but not readable
+            show_error!("cannot remove '{}': Directory not empty", path.display());
+            return true;
+        }
     }
 
     false

tests/by-util/test_rm.rs

@@ -17,7 +17,12 @@ fn test_rm_failed() {
     let (_at, mut ucmd) = at_and_ucmd!();
     let file = "test_rm_one_file";
 
-    ucmd.arg(file).fails(); // Doesn't exist
+    let result = ucmd.arg(file).fails(); // Doesn't exist
+
+    assert!(result.stderr.contains(&format!(
+        "cannot remove '{}': No such file or directory",
+        file
+    )));
 }
 
 #[test]
@@ -115,6 +120,39 @@ fn test_rm_empty_directory() {
     assert!(!at.dir_exists(dir));
 }
 
+#[test]
+fn test_rm_empty_directory_verbose() {
+    let (at, mut ucmd) = at_and_ucmd!();
+    let dir = "test_rm_empty_directory_verbose";
+
+    at.mkdir(dir);
+
+    ucmd.arg("-d")
+        .arg("-v")
+        .arg(dir)
+        .succeeds()
+        .stdout_only(format!("removed directory '{}'\n", dir));
+
+    assert!(!at.dir_exists(dir));
+}
+
+#[test]
+fn test_rm_non_empty_directory() {
+    let (at, mut ucmd) = at_and_ucmd!();
+    let dir = "test_rm_non_empty_dir";
+    let file_a = &format!("{}/test_rm_non_empty_file_a", dir);
+
+    at.mkdir(dir);
+    at.touch(file_a);
+
+    let result = ucmd.arg("-d").arg(dir).fails();
+    assert!(result
+        .stderr
+        .contains(&format!("cannot remove '{}': Directory not empty", dir)));
+    assert!(at.file_exists(file_a));
+    assert!(at.dir_exists(dir));
+}
+
 #[test]
 fn test_rm_recursive() {
     let (at, mut ucmd) = at_and_ucmd!();
@@ -134,22 +172,17 @@ fn test_rm_recursive() {
 }
 
 #[test]
-fn test_rm_errors() {
+fn test_rm_directory_without_flag() {
     let (at, mut ucmd) = at_and_ucmd!();
-    let dir = "test_rm_errors_directory";
-    let file_a = "test_rm_errors_directory/test_rm_errors_file_a";
-    let file_b = "test_rm_errors_directory/test_rm_errors_file_b";
+    let dir = "test_rm_directory_without_flag_dir";
 
     at.mkdir(dir);
-    at.touch(file_a);
-    at.touch(file_b);
   
-    // $ rm test_rm_errors_directory
-    // rm: error: could not remove directory 'test_rm_errors_directory' (did you mean to pass '-r'?)
-    ucmd.arg(dir).fails().stderr_is(
-        "rm: error: could not remove directory 'test_rm_errors_directory' (did you mean \
-         to pass '-r' or '-R'?)\n",
-    );
+    let result = ucmd.arg(dir).fails();
+    println!("{}", result.stderr);
+    assert!(result
+        .stderr
+        .contains(&format!("cannot remove '{}': Is a directory", dir)));
 }
 
 #[test]
@@ -169,10 +202,13 @@ fn test_rm_verbose() {
 }
 
 #[test]
-fn test_rm_dir_symlink() {
+#[cfg(not(windows))]
+// on unix symlink_dir is a file
+fn test_rm_symlink_dir() {
     let (at, mut ucmd) = at_and_ucmd!();
-    let dir = "test_rm_dir_symlink_dir";
-    let link = "test_rm_dir_symlink_link";
+
+    let dir = "test_rm_symlink_dir_directory";
+    let link = "test_rm_symlink_dir_link";
 
     at.mkdir(dir);
     at.symlink_dir(dir, link);
@@ -180,6 +216,29 @@ fn test_rm_dir_symlink() {
     ucmd.arg(link).succeeds();
 }
 
+#[test]
+#[cfg(windows)]
+// on windows removing symlink_dir requires "-r" or "-d"
+fn test_rm_symlink_dir() {
+    let scene = TestScenario::new(util_name!());
+    let at = &scene.fixtures;
+
+    let dir = "test_rm_symlink_dir_directory";
+    let link = "test_rm_symlink_dir_link";
+
+    at.mkdir(dir);
+    at.symlink_dir(dir, link);
+
+    let result = scene.ucmd().arg(link).fails();
+    assert!(result
+        .stderr
+        .contains(&format!("cannot remove '{}': Is a directory", link)));
+
+    assert!(at.dir_exists(link));
+
+    scene.ucmd().arg("-r").arg(link).succeeds();
+}
+
 #[test]
 fn test_rm_invalid_symlink() {
     let (at, mut ucmd) = at_and_ucmd!();