rmdir: match GNU

- Implement all of GNU's fiddly little details
- Don't assume Linux for error codes
- Accept badly-encoded filenames
- Report errors after the fact instead of checking ahead of time
- General cleanup

rmdir now passes GNU's tests.

.vscode/cspell.dictionaries/workspace.wordlist.txt

@@ -107,14 +107,18 @@ whoami
 
 # * vars/errno
 errno
+EACCES
 EBADF
+EBUSY
 EEXIST
 EINVAL
 ENODATA
 ENOENT
 ENOSYS
+ENOTEMPTY
 EOPNOTSUPP
 EPERM
+EROFS
 
 # * vars/fcntl
 F_GETFL

Cargo.lock

@@ -2802,6 +2802,7 @@ name = "uu_rmdir"
 version = "0.0.7"
 dependencies = [
  "clap",
+ "libc",
  "uucore",
  "uucore_procs",
 ]

src/uu/rmdir/Cargo.toml

@@ -18,6 +18,7 @@ path = "src/rmdir.rs"
 clap = { version = "2.33", features = ["wrap_help"] }
 uucore = { version=">=0.0.9", package="uucore", path="../../uucore" }
 uucore_procs = { version=">=0.0.6", package="uucore_procs", path="../../uucore_procs" }
+libc = "0.2.42"
 
 [[bin]]
 name = "rmdir"

src/uu/rmdir/src/rmdir.rs

@@ -11,8 +11,11 @@
 extern crate uucore;
 
 use clap::{crate_version, App, Arg};
-use std::fs;
+use std::fs::{read_dir, remove_dir};
+use std::io;
 use std::path::Path;
+use uucore::error::{set_exit_code, strip_errno, UResult};
+use uucore::util_name;
 
 static ABOUT: &str = "Remove the DIRECTORY(ies), if they are empty.";
 static OPT_IGNORE_FAIL_NON_EMPTY: &str = "ignore-fail-on-non-empty";
@@ -21,35 +24,158 @@ static OPT_VERBOSE: &str = "verbose";
 
 static ARG_DIRS: &str = "dirs";
 
-#[cfg(unix)]
-static ENOTDIR: i32 = 20;
-#[cfg(windows)]
-static ENOTDIR: i32 = 267;
-
 fn usage() -> String {
     format!("{0} [OPTION]... DIRECTORY...", uucore::execution_phrase())
 }
 
-pub fn uumain(args: impl uucore::Args) -> i32 {
+#[uucore_procs::gen_uumain]
+pub fn uumain(args: impl uucore::Args) -> UResult<()> {
     let usage = usage();
 
     let matches = uu_app().usage(&usage[..]).get_matches_from(args);
 
-    let dirs: Vec<String> = matches
+    let opts = Opts {
-        .values_of(ARG_DIRS)
+        ignore: matches.is_present(OPT_IGNORE_FAIL_NON_EMPTY),
-        .map(|v| v.map(ToString::to_string).collect())
+        parents: matches.is_present(OPT_PARENTS),
-        .unwrap_or_default();
+        verbose: matches.is_present(OPT_VERBOSE),
+    };
 
-    let ignore = matches.is_present(OPT_IGNORE_FAIL_NON_EMPTY);
+    for path in matches
-    let parents = matches.is_present(OPT_PARENTS);
+        .values_of_os(ARG_DIRS)
-    let verbose = matches.is_present(OPT_VERBOSE);
+        .unwrap_or_default()
+        .map(Path::new)
+    {
+        if let Err(error) = remove(path, opts) {
+            let Error { error, path } = error;
 
-    match remove(dirs, ignore, parents, verbose) {
+            if opts.ignore && dir_not_empty(&error, path) {
-        Ok(()) => ( /* pass */ ),
+                continue;
-        Err(e) => return e,
+            }
+
+            set_exit_code(1);
+
+            // If `foo` is a symlink to a directory then `rmdir foo/` may give
+            // a "not a directory" error. This is confusing as `rm foo/` says
+            // "is a directory".
+            // This differs from system to system. Some don't give an error.
+            // Windows simply allows calling RemoveDirectory on symlinks so we
+            // don't need to worry about it here.
+            // GNU rmdir seems to print "Symbolic link not followed" if:
+            // - It has a trailing slash
+            // - It's a symlink
+            // - It either points to a directory or dangles
+            #[cfg(unix)]
+            {
+                use std::ffi::OsStr;
+                use std::os::unix::ffi::OsStrExt;
+
+                fn is_symlink(path: &Path) -> io::Result<bool> {
+                    Ok(path.symlink_metadata()?.file_type().is_symlink())
+                }
+
+                fn points_to_directory(path: &Path) -> io::Result<bool> {
+                    Ok(path.metadata()?.file_type().is_dir())
+                }
+
+                let path = path.as_os_str().as_bytes();
+                if error.raw_os_error() == Some(libc::ENOTDIR) && path.ends_with(b"/") {
+                    // Strip the trailing slash or .symlink_metadata() will follow the symlink
+                    let path: &Path = OsStr::from_bytes(&path[..path.len() - 1]).as_ref();
+                    if is_symlink(path).unwrap_or(false)
+                        && points_to_directory(path).unwrap_or(true)
+                    {
+                        show_error!(
+                            "failed to remove '{}/': Symbolic link not followed",
+                            path.display()
+                        );
+                        continue;
+                    }
+                }
+            }
+
+            show_error!(
+                "failed to remove '{}': {}",
+                path.display(),
+                strip_errno(&error)
+            );
+        }
     }
 
-    0
+    Ok(())
+}
+
+struct Error<'a> {
+    error: io::Error,
+    path: &'a Path,
+}
+
+fn remove(mut path: &Path, opts: Opts) -> Result<(), Error<'_>> {
+    remove_single(path, opts)?;
+    if opts.parents {
+        while let Some(new) = path.parent() {
+            path = new;
+            if path.as_os_str() == "" {
+                break;
+            }
+            remove_single(path, opts)?;
+        }
+    }
+    Ok(())
+}
+
+fn remove_single(path: &Path, opts: Opts) -> Result<(), Error<'_>> {
+    if opts.verbose {
+        println!("{}: removing directory, '{}'", util_name(), path.display());
+    }
+    remove_dir(path).map_err(|error| Error { error, path })
+}
+
+// POSIX: https://pubs.opengroup.org/onlinepubs/009696799/functions/rmdir.html
+#[cfg(not(windows))]
+const NOT_EMPTY_CODES: &[i32] = &[libc::ENOTEMPTY, libc::EEXIST];
+
+// 145 is ERROR_DIR_NOT_EMPTY, determined experimentally.
+#[cfg(windows)]
+const NOT_EMPTY_CODES: &[i32] = &[145];
+
+// Other error codes you might get for directories that could be found and are
+// not empty.
+// This is a subset of the error codes listed in rmdir(2) from the Linux man-pages
+// project. Maybe other systems have additional codes that apply?
+#[cfg(not(windows))]
+const PERHAPS_EMPTY_CODES: &[i32] = &[libc::EACCES, libc::EBUSY, libc::EPERM, libc::EROFS];
+
+// Probably incomplete, I can't find a list of possible errors for
+// RemoveDirectory anywhere.
+#[cfg(windows)]
+const PERHAPS_EMPTY_CODES: &[i32] = &[
+    5, // ERROR_ACCESS_DENIED, found experimentally.
+];
+
+fn dir_not_empty(error: &io::Error, path: &Path) -> bool {
+    if let Some(code) = error.raw_os_error() {
+        if NOT_EMPTY_CODES.contains(&code) {
+            return true;
+        }
+        // If --ignore-fail-on-non-empty is used then we want to ignore all errors
+        // for non-empty directories, even if the error was e.g. because there's
+        // no permission. So we do an additional check.
+        if PERHAPS_EMPTY_CODES.contains(&code) {
+            if let Ok(mut iterator) = read_dir(path) {
+                if iterator.next().is_some() {
+                    return true;
+                }
+            }
+        }
+    }
+    false
+}
+
+#[derive(Clone, Copy, Debug)]
+struct Opts {
+    ignore: bool,
+    parents: bool,
+    verbose: bool,
 }
 
 pub fn uu_app() -> App<'static, 'static> {
@@ -84,57 +210,3 @@ pub fn uu_app() -> App<'static, 'static> {
                 .required(true),
         )
 }
-
-fn remove(dirs: Vec<String>, ignore: bool, parents: bool, verbose: bool) -> Result<(), i32> {
-    let mut r = Ok(());
-
-    for dir in &dirs {
-        let path = Path::new(&dir[..]);
-        r = remove_dir(path, ignore, verbose).and(r);
-        if parents {
-            let mut p = path;
-            while let Some(new_p) = p.parent() {
-                p = new_p;
-                match p.as_os_str().to_str() {
-                    None => break,
-                    Some(s) => match s {
-                        "" | "." | "/" => break,
-                        _ => (),
-                    },
-                };
-                r = remove_dir(p, ignore, verbose).and(r);
-            }
-        }
-    }
-
-    r
-}
-
-fn remove_dir(path: &Path, ignore: bool, verbose: bool) -> Result<(), i32> {
-    let mut read_dir = fs::read_dir(path).map_err(|e| {
-        if e.raw_os_error() == Some(ENOTDIR) {
-            show_error!("failed to remove '{}': Not a directory", path.display());
-        } else {
-            show_error!("reading directory '{}': {}", path.display(), e);
-        }
-        1
-    })?;
-
-    let mut r = Ok(());
-
-    if read_dir.next().is_none() {
-        match fs::remove_dir(path) {
-            Err(e) => {
-                show_error!("removing directory '{}': {}", path.display(), e);
-                r = Err(1);
-            }
-            Ok(_) if verbose => println!("removing directory, '{}'", path.display()),
-            _ => (),
-        }
-    } else if !ignore {
-        show_error!("failed to remove '{}': Directory not empty", path.display());
-        r = Err(1);
-    }
-
-    r
-}

tests/by-util/test_rmdir.rs

@@ -1,126 +1,238 @@
 use crate::common::util::*;
 
+const DIR: &str = "dir";
+const DIR_FILE: &str = "dir/file";
+const NESTED_DIR: &str = "dir/ect/ory";
+const NESTED_DIR_FILE: &str = "dir/ect/ory/file";
+
+#[cfg(windows)]
+const NOT_FOUND: &str = "The system cannot find the file specified.";
+#[cfg(not(windows))]
+const NOT_FOUND: &str = "No such file or directory";
+
+#[cfg(windows)]
+const NOT_EMPTY: &str = "The directory is not empty.";
+#[cfg(not(windows))]
+const NOT_EMPTY: &str = "Directory not empty";
+
+#[cfg(windows)]
+const NOT_A_DIRECTORY: &str = "The directory name is invalid.";
+#[cfg(not(windows))]
+const NOT_A_DIRECTORY: &str = "Not a directory";
+
 #[test]
 fn test_rmdir_empty_directory_no_parents() {
     let (at, mut ucmd) = at_and_ucmd!();
-    let dir = "test_rmdir_empty_no_parents";
 
-    at.mkdir(dir);
+    at.mkdir(DIR);
-    assert!(at.dir_exists(dir));
 
-    ucmd.arg(dir).succeeds().no_stderr();
+    ucmd.arg(DIR).succeeds().no_stderr();
 
-    assert!(!at.dir_exists(dir));
+    assert!(!at.dir_exists(DIR));
 }
 
 #[test]
 fn test_rmdir_empty_directory_with_parents() {
     let (at, mut ucmd) = at_and_ucmd!();
-    let dir = "test_rmdir_empty/with/parents";
 
-    at.mkdir_all(dir);
+    at.mkdir_all(NESTED_DIR);
-    assert!(at.dir_exists(dir));
 
-    ucmd.arg("-p").arg(dir).succeeds().no_stderr();
+    ucmd.arg("-p").arg(NESTED_DIR).succeeds().no_stderr();
 
-    assert!(!at.dir_exists(dir));
+    assert!(!at.dir_exists(NESTED_DIR));
+    assert!(!at.dir_exists(DIR));
 }
 
 #[test]
 fn test_rmdir_nonempty_directory_no_parents() {
     let (at, mut ucmd) = at_and_ucmd!();
-    let dir = "test_rmdir_nonempty_no_parents";
-    let file = "test_rmdir_nonempty_no_parents/foo";
 
-    at.mkdir(dir);
+    at.mkdir(DIR);
-    assert!(at.dir_exists(dir));
+    at.touch(DIR_FILE);
 
-    at.touch(file);
+    ucmd.arg(DIR)
-    assert!(at.file_exists(file));
+        .fails()
+        .stderr_is(format!("rmdir: failed to remove 'dir': {}", NOT_EMPTY));
 
-    ucmd.arg(dir).fails().stderr_is(
+    assert!(at.dir_exists(DIR));
-        "rmdir: failed to remove 'test_rmdir_nonempty_no_parents': Directory not \
-         empty\n",
-    );
-
-    assert!(at.dir_exists(dir));
 }
 
 #[test]
 fn test_rmdir_nonempty_directory_with_parents() {
     let (at, mut ucmd) = at_and_ucmd!();
-    let dir = "test_rmdir_nonempty/with/parents";
-    let file = "test_rmdir_nonempty/with/parents/foo";
 
-    at.mkdir_all(dir);
+    at.mkdir_all(NESTED_DIR);
-    assert!(at.dir_exists(dir));
+    at.touch(NESTED_DIR_FILE);
 
-    at.touch(file);
+    ucmd.arg("-p").arg(NESTED_DIR).fails().stderr_is(format!(
-    assert!(at.file_exists(file));
+        "rmdir: failed to remove 'dir/ect/ory': {}",
+        NOT_EMPTY
+    ));
 
-    ucmd.arg("-p").arg(dir).fails().stderr_is(
+    assert!(at.dir_exists(NESTED_DIR));
-        "rmdir: failed to remove 'test_rmdir_nonempty/with/parents': Directory not \
-         empty\nrmdir: failed to remove 'test_rmdir_nonempty/with': Directory not \
-         empty\nrmdir: failed to remove 'test_rmdir_nonempty': Directory not \
-         empty\n",
-    );
-
-    assert!(at.dir_exists(dir));
 }
 
 #[test]
 fn test_rmdir_ignore_nonempty_directory_no_parents() {
     let (at, mut ucmd) = at_and_ucmd!();
-    let dir = "test_rmdir_ignore_nonempty_no_parents";
-    let file = "test_rmdir_ignore_nonempty_no_parents/foo";
 
-    at.mkdir(dir);
+    at.mkdir(DIR);
-    assert!(at.dir_exists(dir));
+    at.touch(DIR_FILE);
-
-    at.touch(file);
-    assert!(at.file_exists(file));
 
     ucmd.arg("--ignore-fail-on-non-empty")
-        .arg(dir)
+        .arg(DIR)
         .succeeds()
         .no_stderr();
 
-    assert!(at.dir_exists(dir));
+    assert!(at.dir_exists(DIR));
 }
 
 #[test]
 fn test_rmdir_ignore_nonempty_directory_with_parents() {
     let (at, mut ucmd) = at_and_ucmd!();
-    let dir = "test_rmdir_ignore_nonempty/with/parents";
-    let file = "test_rmdir_ignore_nonempty/with/parents/foo";
 
-    at.mkdir_all(dir);
+    at.mkdir_all(NESTED_DIR);
-    assert!(at.dir_exists(dir));
+    at.touch(NESTED_DIR_FILE);
-
-    at.touch(file);
-    assert!(at.file_exists(file));
 
     ucmd.arg("--ignore-fail-on-non-empty")
         .arg("-p")
-        .arg(dir)
+        .arg(NESTED_DIR)
         .succeeds()
         .no_stderr();
 
-    assert!(at.dir_exists(dir));
+    assert!(at.dir_exists(NESTED_DIR));
 }
 
 #[test]
-fn test_rmdir_remove_symlink_match_gnu_error() {
+fn test_rmdir_not_a_directory() {
     let (at, mut ucmd) = at_and_ucmd!();
 
-    let file = "file";
+    at.touch("file");
-    let fl = "fl";
-    at.touch(file);
-    assert!(at.file_exists(file));
-    at.symlink_file(file, fl);
-    assert!(at.file_exists(fl));
 
-    ucmd.arg("fl/")
+    ucmd.arg("--ignore-fail-on-non-empty")
+        .arg("file")
         .fails()
-        .stderr_is("rmdir: failed to remove 'fl/': Not a directory");
+        .no_stdout()
+        .stderr_is(format!(
+            "rmdir: failed to remove 'file': {}",
+            NOT_A_DIRECTORY
+        ));
+}
+
+#[test]
+fn test_verbose_single() {
+    let (at, mut ucmd) = at_and_ucmd!();
+
+    at.mkdir(DIR);
+
+    ucmd.arg("-v")
+        .arg(DIR)
+        .succeeds()
+        .no_stderr()
+        .stdout_is("rmdir: removing directory, 'dir'\n");
+}
+
+#[test]
+fn test_verbose_multi() {
+    let (at, mut ucmd) = at_and_ucmd!();
+
+    at.mkdir(DIR);
+
+    ucmd.arg("-v")
+        .arg("does_not_exist")
+        .arg(DIR)
+        .fails()
+        .stdout_is(
+            "rmdir: removing directory, 'does_not_exist'\n\
+             rmdir: removing directory, 'dir'\n",
+        )
+        .stderr_is(format!(
+            "rmdir: failed to remove 'does_not_exist': {}",
+            NOT_FOUND
+        ));
+}
+
+#[test]
+fn test_verbose_nested_failure() {
+    let (at, mut ucmd) = at_and_ucmd!();
+
+    at.mkdir_all(NESTED_DIR);
+    at.touch("dir/ect/file");
+
+    ucmd.arg("-pv")
+        .arg(NESTED_DIR)
+        .fails()
+        .stdout_is(
+            "rmdir: removing directory, 'dir/ect/ory'\n\
+             rmdir: removing directory, 'dir/ect'\n",
+        )
+        .stderr_is(format!("rmdir: failed to remove 'dir/ect': {}", NOT_EMPTY));
+}
+
+#[cfg(unix)]
+#[test]
+fn test_rmdir_ignore_nonempty_no_permissions() {
+    use std::fs;
+
+    let (at, mut ucmd) = at_and_ucmd!();
+
+    // We make the *parent* dir read-only to prevent deleting the dir in it.
+    at.mkdir_all("dir/ect/ory");
+    at.touch("dir/ect/ory/file");
+    let dir_ect = at.plus("dir/ect");
+    let mut perms = fs::metadata(&dir_ect).unwrap().permissions();
+    perms.set_readonly(true);
+    fs::set_permissions(&dir_ect, perms.clone()).unwrap();
+
+    // rmdir should now get a permissions error that it interprets as
+    // a non-empty error.
+    ucmd.arg("--ignore-fail-on-non-empty")
+        .arg("dir/ect/ory")
+        .succeeds()
+        .no_stderr();
+
+    assert!(at.dir_exists("dir/ect/ory"));
+
+    // Politely restore permissions for cleanup
+    perms.set_readonly(false);
+    fs::set_permissions(&dir_ect, perms).unwrap();
+}
+
+#[test]
+fn test_rmdir_remove_symlink_file() {
+    let (at, mut ucmd) = at_and_ucmd!();
+
+    at.touch("file");
+    at.symlink_file("file", "fl");
+
+    ucmd.arg("fl/").fails().stderr_is(format!(
+        "rmdir: failed to remove 'fl/': {}",
+        NOT_A_DIRECTORY
+    ));
+}
+
+// This behavior is known to happen on Linux but not all Unixes
+#[cfg(any(target_os = "linux", target_os = "android"))]
+#[test]
+fn test_rmdir_remove_symlink_dir() {
+    let (at, mut ucmd) = at_and_ucmd!();
+
+    at.mkdir("dir");
+    at.symlink_dir("dir", "dl");
+
+    ucmd.arg("dl/")
+        .fails()
+        .stderr_is("rmdir: failed to remove 'dl/': Symbolic link not followed");
+}
+
+#[cfg(any(target_os = "linux", target_os = "android"))]
+#[test]
+fn test_rmdir_remove_symlink_dangling() {
+    let (at, mut ucmd) = at_and_ucmd!();
+
+    at.symlink_dir("dir", "dl");
+
+    ucmd.arg("dl/")
+        .fails()
+        .stderr_is("rmdir: failed to remove 'dl/': Symbolic link not followed");
 }