refactor(chmod): move from walker to walkdir, simplify the code and add tests (#1645)

Cargo.lock

@@ -471,6 +471,7 @@ version = "0.9.1"
 source = "registry+https://github.com/rust-lang/crates.io-index"
 dependencies = [
  "cfg-if 1.0.0 (registry+https://github.com/rust-lang/crates.io-index)",
+ "const_fn 0.4.3 (registry+https://github.com/rust-lang/crates.io-index)",
  "const_fn 0.4.4 (registry+https://github.com/rust-lang/crates.io-index)",
  "crossbeam-utils 0.8.1 (registry+https://github.com/rust-lang/crates.io-index)",
  "lazy_static 1.4.0 (registry+https://github.com/rust-lang/crates.io-index)",
@@ -1435,7 +1436,7 @@ dependencies = [
  "libc 0.2.66 (registry+https://github.com/rust-lang/crates.io-index)",
  "uucore 0.0.4",
  "uucore_procs 0.0.4",
- "walker 1.0.1 (registry+https://github.com/rust-lang/crates.io-index)",
+ "walkdir 2.3.1 (registry+https://github.com/rust-lang/crates.io-index)",
 ]
 
 [[package]]
@@ -2381,11 +2382,6 @@ dependencies = [
  "winapi-util 0.1.2 (registry+https://github.com/rust-lang/crates.io-index)",
 ]
 
-[[package]]
-name = "walker"
-version = "1.0.1"
-source = "registry+https://github.com/rust-lang/crates.io-index"
-
 [[package]]
 name = "wasi"
 version = "0.9.0+wasi-snapshot-preview1"
@@ -2664,7 +2660,6 @@ dependencies = [
 "checksum vec_map 0.8.2 (registry+https://github.com/rust-lang/crates.io-index)" = "f1bddf1187be692e79c5ffeab891132dfb0f236ed36a43c7ed39f1165ee20191"
 "checksum void 1.0.2 (registry+https://github.com/rust-lang/crates.io-index)" = "6a02e4885ed3bc0f2de90ea6dd45ebcbb66dacffe03547fadbb0eeae2770887d"
 "checksum walkdir 2.3.1 (registry+https://github.com/rust-lang/crates.io-index)" = "777182bc735b6424e1a57516d35ed72cb8019d85c8c9bf536dccb3445c1a2f7d"
-"checksum walker 1.0.1 (registry+https://github.com/rust-lang/crates.io-index)" = "44971d5e5ae4f7904dffb6260ebd3910e7bcae104a94730e04a24cb6af40646b"
 "checksum wasi 0.9.0+wasi-snapshot-preview1 (registry+https://github.com/rust-lang/crates.io-index)" = "cccddf32554fecc6acb585f82a32a72e28b48f8c4c1883ddfeeeaa96f7d8e519"
 "checksum wasm-bindgen 0.2.69 (registry+https://github.com/rust-lang/crates.io-index)" = "3cd364751395ca0f68cafb17666eee36b63077fb5ecd972bbcd74c90c4bf736e"
 "checksum wasm-bindgen-backend 0.2.69 (registry+https://github.com/rust-lang/crates.io-index)" = "1114f89ab1f4106e5b55e688b828c0ab0ea593a1ea7c094b141b14cbaaec2d62"

src/uu/chmod/Cargo.toml

@@ -18,7 +18,7 @@ path = "src/chmod.rs"
 libc = "0.2.42"
 uucore = { version=">=0.0.4", package="uucore", path="../../uucore", features=["fs", "mode"] }
 uucore_procs = { version=">=0.0.4", package="uucore_procs", path="../../uucore_procs" }
-walker = "1.0.0"
+walkdir = "2.2"
 
 [[bin]]
 name = "chmod"

src/uu/chmod/src/chmod.rs

@@ -9,7 +9,7 @@
 
 #[cfg(unix)]
 extern crate libc;
-extern crate walker;
+extern crate walkdir;
 
 #[macro_use]
 extern crate uucore;
@@ -20,7 +20,7 @@ use std::path::Path;
 use uucore::fs::display_permissions_unix;
 #[cfg(not(windows))]
 use uucore::mode;
-use walker::Walker;
+use walkdir::WalkDir;
 
 const NAME: &str = "chmod";
 static SUMMARY: &str = "Change the mode of each FILE to MODE.
@@ -150,62 +150,39 @@ impl Chmoder {
         for filename in &files {
             let filename = &filename[..];
             let file = Path::new(filename);
-            if file.exists() {
+            if !file.exists() {
-                if file.is_dir() {
-                    if !self.preserve_root || filename != "/" {
-                        if self.recursive {
-                            let walk_dir = match Walker::new(&file) {
-                                Ok(m) => m,
-                                Err(f) => {
-                                    crash!(1, "{}", f.to_string());
-                                }
-                            };
-                            // XXX: here (and elsewhere) we see that this impl will have issues
-                            // with non-UTF-8 filenames. Using OsString won't fix this because
-                            // on Windows OsStrings cannot be built out of non-UTF-8 chars. One
-                            // possible fix is to use CStrings rather than Strings in the args
-                            // to chmod() and chmod_file().
-                            r = self
-                                .chmod(
-                                    walk_dir
-                                        .filter_map(|x| match x {
-                                            Ok(o) => match o.path().into_os_string().to_str() {
-                                                Some(s) => Some(s.to_owned()),
-                                                None => None,
-                                            },
-                                            Err(_) => None,
-                                        })
-                                        .collect(),
-                                )
-                                .and(r);
-                            r = self.chmod_file(&file, filename).and(r);
-                        }
-                    } else {
-                        show_error!("could not change permissions of directory '{}'", filename);
-                        r = Err(1);
-                    }
-                } else {
-                    r = self.chmod_file(&file, filename).and(r);
-                }
-            } else {
                 show_error!("no such file or directory '{}'", filename);
-                r = Err(1);
+                return Err(1);
+            }
+            if self.recursive && self.preserve_root && filename == "/" {
+                show_error!(
+                    "it is dangerous to operate recursively on '{}'\nuse --no-preserve-root to override this failsafe",
+                    filename
+                );
+                return Err(1);
+            }
+            if !self.recursive {
+                r = self.chmod_file(&file).and(r);
+            } else {
+                for entry in WalkDir::new(&filename).into_iter().filter_map(|e| e.ok()) {
+                    let file = entry.path();
+                    r = self.chmod_file(&file).and(r);
+                }
             }
         }
-
         r
     }
 
     #[cfg(windows)]
-    fn chmod_file(&self, file: &Path, name: &str) -> Result<(), i32> {
+    fn chmod_file(&self, file: &Path) -> Result<(), i32> {
         // chmod is useless on Windows
         // it doesn't set any permissions at all
         // instead it just sets the readonly attribute on the file
         Err(0)
     }
     #[cfg(any(unix, target_os = "redox"))]
-    fn chmod_file(&self, file: &Path, name: &str) -> Result<(), i32> {
+    fn chmod_file(&self, file: &Path) -> Result<(), i32> {
-        let mut fperm = match fs::metadata(name) {
+        let mut fperm = match fs::metadata(file) {
             Ok(meta) => meta.mode() & 0o7777,
             Err(err) => {
                 if !self.quiet {
@@ -215,7 +192,7 @@ impl Chmoder {
             }
         };
         match self.fmode {
-            Some(mode) => self.change_file(fperm, mode, file, name)?,
+            Some(mode) => self.change_file(fperm, mode, file)?,
             None => {
                 let cmode_unwrapped = self.cmode.clone().unwrap();
                 for mode in cmode_unwrapped.split(',') {
@@ -228,7 +205,7 @@ impl Chmoder {
                     };
                     match result {
                         Ok(mode) => {
-                            self.change_file(fperm, mode, file, name)?;
+                            self.change_file(fperm, mode, file)?;
                             fperm = mode;
                         }
                         Err(f) => {
@@ -246,7 +223,7 @@ impl Chmoder {
     }
 
     #[cfg(unix)]
-    fn change_file(&self, fperm: u32, mode: u32, file: &Path, path: &str) -> Result<(), i32> {
+    fn change_file(&self, fperm: u32, mode: u32, file: &Path) -> Result<(), i32> {
         if fperm == mode {
             if self.verbose && !self.changes {
                 show_info!(
@@ -257,9 +234,7 @@ impl Chmoder {
                 );
             }
             Ok(())
-        } else if let Err(err) =
+        } else if let Err(err) = fs::set_permissions(file, fs::Permissions::from_mode(mode)) {
-            fs::set_permissions(Path::new(path), fs::Permissions::from_mode(mode))
-        {
             if !self.quiet {
                 show_error!("{}", err);
             }

tests/by-util/test_chmod.rs

@@ -279,3 +279,70 @@ fn test_chmod_reference_file() {
     mkfile(&at.plus_as_string(REFERENCE_FILE), REFERENCE_PERMS);
     run_single_test(&tests[0], at, ucmd);
 }
+
+#[test]
+fn test_chmod_recursive() {
+    let _guard = UMASK_MUTEX.lock();
+
+    let original_umask = unsafe { umask(0) };
+    let (at, mut ucmd) = at_and_ucmd!();
+    at.mkdir("a");
+    at.mkdir("a/b");
+    at.mkdir("a/b/c");
+    at.mkdir("z");
+    mkfile(&at.plus_as_string("a/a"), 0o100444);
+    mkfile(&at.plus_as_string("a/b/b"), 0o100444);
+    mkfile(&at.plus_as_string("a/b/c/c"), 0o100444);
+    mkfile(&at.plus_as_string("z/y"), 0o100444);
+
+    let result = ucmd
+        .arg("-R")
+        .arg("--verbose")
+        .arg("-r,a+w")
+        .arg("a")
+        .arg("z")
+        .succeeds();
+
+    assert_eq!(at.metadata("z/y").permissions().mode(), 0o100222);
+    assert_eq!(at.metadata("a/a").permissions().mode(), 0o100222);
+    assert_eq!(at.metadata("a/b/b").permissions().mode(), 0o100222);
+    assert_eq!(at.metadata("a/b/c/c").permissions().mode(), 0o100222);
+    println!("mode {:o}", at.metadata("a").permissions().mode());
+    assert_eq!(at.metadata("a").permissions().mode(), 0o40333);
+    assert_eq!(at.metadata("z").permissions().mode(), 0o40333);
+    assert!(result.stderr.contains("to 333 (-wx-wx-wx)"));
+    assert!(result.stderr.contains("to 222 (-w--w--w-)"));
+
+    unsafe {
+        umask(original_umask);
+    }
+}
+
+#[test]
+fn test_chmod_non_existing_file() {
+    let (_at, mut ucmd) = at_and_ucmd!();
+    let result = ucmd
+        .arg("-R")
+        .arg("--verbose")
+        .arg("-r,a+w")
+        .arg("dont-exist")
+        .fails();
+    assert_eq!(
+        result.stderr,
+        "chmod: error: no such file or directory 'dont-exist'\n"
+    );
+}
+
+#[test]
+fn test_chmod_preserve_root() {
+    let (_at, mut ucmd) = at_and_ucmd!();
+    let result = ucmd
+        .arg("-R")
+        .arg("--preserve-root")
+        .arg("755")
+        .arg("/")
+        .fails();
+    assert!(result
+        .stderr
+        .contains("chmod: error: it is dangerous to operate recursively on '/'"));
+}