uucore/perms: correct some error messages

- prevent duplicate errors from both us and `walkdir` by instructing `walkdir'
  to skip directories we failed to read metadata for.
- don't directly display `walkdir`'s errors, but format them ourselves to
  match gnu's format

src/uucore/src/lib/features/perms.rs

@@ -5,6 +5,7 @@
 
 //! Common functions to manage permissions
 
+use crate::error::strip_errno;
 use crate::error::UResult;
 pub use crate::features::entries;
 use crate::fs::resolve_relative_path;
@@ -172,15 +173,6 @@ pub struct ChownExecutor {
     pub dereference: bool,
 }
 
-macro_rules! unwrap {
-    ($m:expr, $e:ident, $err:block) => {
-        match $m {
-            Ok(meta) => meta,
-            Err($e) => $err,
-        }
-    };
-}
-
 pub const FTS_COMFOLLOW: u8 = 1;
 pub const FTS_PHYSICAL: u8 = 1 << 1;
 pub const FTS_LOGICAL: u8 = 1 << 2;
@@ -269,17 +261,42 @@ impl ChownExecutor {
         let mut ret = 0;
         let root = root.as_ref();
         let follow = self.dereference || self.bit_flag & FTS_LOGICAL != 0;
-        for entry in WalkDir::new(root).follow_links(follow).min_depth(1) {
+        let mut iterator = WalkDir::new(root)
-            let entry = unwrap!(entry, e, {
+            .follow_links(follow)
-                ret = 1;
+            .min_depth(1)
-                show_error!("{}", e);
+            .into_iter();
-                continue;
+        // We can't use a for loop because we need to manipulate the iterator inside the loop.
-            });
+        while let Some(entry) = iterator.next() {
+            let entry = match entry {
+                Err(e) => {
+                    ret = 1;
+                    if let Some(path) = e.path() {
+                        show_error!(
+                            "cannot access '{}': {}",
+                            path.display(),
+                            if let Some(error) = e.io_error() {
+                                strip_errno(error)
+                            } else {
+                                "Too many levels of symbolic links".into()
+                            }
+                        )
+                    } else {
+                        show_error!("{}", e)
+                    }
+                    continue;
+                }
+                Ok(entry) => entry,
+            };
             let path = entry.path();
             let meta = match self.obtain_meta(path, follow) {
                 Some(m) => m,
                 _ => {
                     ret = 1;
+                    if entry.file_type().is_dir() {
+                        // Instruct walkdir to skip this directory to avoid getting another error
+                        // when walkdir tries to query the children of this directory.
+                        iterator.skip_current_dir();
+                    }
                     continue;
                 }
             };
@@ -316,23 +333,20 @@ impl ChownExecutor {
     fn obtain_meta<P: AsRef<Path>>(&self, path: P, follow: bool) -> Option<Metadata> {
         let path = path.as_ref();
         let meta = if follow {
-            unwrap!(path.metadata(), e, {
+            path.metadata()
-                match self.verbosity.level {
-                    VerbosityLevel::Silent => (),
-                    _ => show_error!("cannot access '{}': {}", path.display(), e),
-                }
-                return None;
-            })
         } else {
-            unwrap!(path.symlink_metadata(), e, {
+            path.symlink_metadata()
+        };
+        match meta {
+            Err(e) => {
                 match self.verbosity.level {
                     VerbosityLevel::Silent => (),
-                    _ => show_error!("cannot dereference '{}': {}", path.display(), e),
+                    _ => show_error!("cannot access '{}': {}", path.display(), strip_errno(&e)),
                 }
-                return None;
+                None
-            })
+            }
-        };
+            Ok(meta) => Some(meta),
-        Some(meta)
+        }
     }
 
     #[inline]

tests/by-util/test_chgrp.rs

@@ -230,7 +230,7 @@ fn test_big_h() {
 }
 
 #[test]
-#[cfg(target_os = "linux")]
+#[cfg(not(target_vendor = "apple"))]
 fn basic_succeeds() {
     let (at, mut ucmd) = at_and_ucmd!();
     let one_group = nix::unistd::getgroups().unwrap();
@@ -251,3 +251,40 @@ fn test_no_change() {
     at.touch("file");
     ucmd.arg("").arg(at.plus("file")).succeeds();
 }
+
+#[test]
+#[cfg(not(target_vendor = "apple"))]
+fn test_permission_denied() {
+    use std::os::unix::prelude::PermissionsExt;
+
+    if let Some(group) = nix::unistd::getgroups().unwrap().first() {
+        let (at, mut ucmd) = at_and_ucmd!();
+        at.mkdir("dir");
+        at.touch("dir/file");
+        std::fs::set_permissions(at.plus("dir"), PermissionsExt::from_mode(0o0000)).unwrap();
+        ucmd.arg("-R")
+            .arg(group.as_raw().to_string())
+            .arg("dir")
+            .fails()
+            .stderr_only("chgrp: cannot access 'dir': Permission denied");
+    }
+}
+
+#[test]
+#[cfg(not(target_vendor = "apple"))]
+fn test_subdir_permission_denied() {
+    use std::os::unix::prelude::PermissionsExt;
+
+    if let Some(group) = nix::unistd::getgroups().unwrap().first() {
+        let (at, mut ucmd) = at_and_ucmd!();
+        at.mkdir("dir");
+        at.mkdir("dir/subdir");
+        at.touch("dir/subdir/file");
+        std::fs::set_permissions(at.plus("dir/subdir"), PermissionsExt::from_mode(0o0000)).unwrap();
+        ucmd.arg("-R")
+            .arg(group.as_raw().to_string())
+            .arg("dir")
+            .fails()
+            .stderr_only("chgrp: cannot access 'dir/subdir': Permission denied");
+    }
+}