Merge pull request #2629 from miDeb/chown-base

chown/chgrp: share more code & refactor

src/uu/chgrp/src/chgrp.rs

@@ -11,46 +11,16 @@
 extern crate uucore;
 pub use uucore::entries;
 use uucore::error::{FromIo, UResult, USimpleError};
-use uucore::perms::{
+use uucore::perms::{chown_base, options, IfFrom};
-    ChownExecutor, IfFrom, Verbosity, VerbosityLevel, FTS_COMFOLLOW, FTS_LOGICAL, FTS_PHYSICAL,
-};
 
-use clap::{App, Arg};
+use clap::{App, Arg, ArgMatches};
 
 use std::fs;
 use std::os::unix::fs::MetadataExt;
 
-use uucore::InvalidEncodingHandling;
-
 static ABOUT: &str = "Change the group of each FILE to GROUP.";
 static VERSION: &str = env!("CARGO_PKG_VERSION");
 
-pub mod options {
-    pub mod verbosity {
-        pub static CHANGES: &str = "changes";
-        pub static QUIET: &str = "quiet";
-        pub static SILENT: &str = "silent";
-        pub static VERBOSE: &str = "verbose";
-    }
-    pub mod preserve_root {
-        pub static PRESERVE: &str = "preserve-root";
-        pub static NO_PRESERVE: &str = "no-preserve-root";
-    }
-    pub mod dereference {
-        pub static DEREFERENCE: &str = "dereference";
-        pub static NO_DEREFERENCE: &str = "no-dereference";
-    }
-    pub static RECURSIVE: &str = "recursive";
-    pub mod traverse {
-        pub static TRAVERSE: &str = "H";
-        pub static NO_TRAVERSE: &str = "P";
-        pub static EVERY: &str = "L";
-    }
-    pub static REFERENCE: &str = "reference";
-    pub static ARG_GROUP: &str = "GROUP";
-    pub static ARG_FILES: &str = "FILE";
-}
-
 fn get_usage() -> String {
     format!(
         "{0} [OPTION]... GROUP FILE...\n    {0} [OPTION]... --reference=RFILE FILE...",
@@ -58,101 +28,7 @@ fn get_usage() -> String {
     )
 }
 
-#[uucore_procs::gen_uumain]
+fn parse_gid_and_uid(matches: &ArgMatches) -> UResult<(Option<u32>, Option<u32>, IfFrom)> {
-pub fn uumain(args: impl uucore::Args) -> UResult<()> {
-    let args = args
-        .collect_str(InvalidEncodingHandling::ConvertLossy)
-        .accept_any();
-
-    let usage = get_usage();
-
-    let mut app = uu_app().usage(&usage[..]);
-
-    // we change the positional args based on whether
-    // --reference was used.
-    let mut reference = false;
-    let mut help = false;
-    // stop processing options on --
-    for arg in args.iter().take_while(|s| *s != "--") {
-        if arg.starts_with("--reference=") || arg == "--reference" {
-            reference = true;
-        } else if arg == "--help" {
-            // we stop processing once we see --help,
-            // as it doesn't matter if we've seen reference or not
-            help = true;
-            break;
-        }
-    }
-
-    if help || !reference {
-        // add both positional arguments
-        app = app.arg(
-            Arg::with_name(options::ARG_GROUP)
-                .value_name(options::ARG_GROUP)
-                .required(true)
-                .takes_value(true)
-                .multiple(false),
-        )
-    }
-    app = app.arg(
-        Arg::with_name(options::ARG_FILES)
-            .value_name(options::ARG_FILES)
-            .multiple(true)
-            .takes_value(true)
-            .required(true)
-            .min_values(1),
-    );
-
-    let matches = app.get_matches_from(args);
-
-    /* Get the list of files */
-    let files: Vec<String> = matches
-        .values_of(options::ARG_FILES)
-        .map(|v| v.map(ToString::to_string).collect())
-        .unwrap_or_default();
-
-    let preserve_root = matches.is_present(options::preserve_root::PRESERVE);
-
-    let mut derefer = if matches.is_present(options::dereference::DEREFERENCE) {
-        1
-    } else if matches.is_present(options::dereference::NO_DEREFERENCE) {
-        0
-    } else {
-        -1
-    };
-
-    let mut bit_flag = if matches.is_present(options::traverse::TRAVERSE) {
-        FTS_COMFOLLOW | FTS_PHYSICAL
-    } else if matches.is_present(options::traverse::EVERY) {
-        FTS_LOGICAL
-    } else {
-        FTS_PHYSICAL
-    };
-
-    let recursive = matches.is_present(options::RECURSIVE);
-    if recursive {
-        if bit_flag == FTS_PHYSICAL {
-            if derefer == 1 {
-                return Err(USimpleError::new(1, "-R --dereference requires -H or -L"));
-            }
-            derefer = 0;
-        }
-    } else {
-        bit_flag = FTS_PHYSICAL;
-    }
-
-    let verbosity_level = if matches.is_present(options::verbosity::CHANGES) {
-        VerbosityLevel::Changes
-    } else if matches.is_present(options::verbosity::SILENT)
-        || matches.is_present(options::verbosity::QUIET)
-    {
-        VerbosityLevel::Silent
-    } else if matches.is_present(options::verbosity::VERBOSE) {
-        VerbosityLevel::Verbose
-    } else {
-        VerbosityLevel::Normal
-    };
-
     let dest_gid = if let Some(file) = matches.value_of(options::REFERENCE) {
         fs::metadata(&file)
             .map(|meta| Some(meta.gid()))
@@ -168,22 +44,20 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> {
             }
         }
     };
+    Ok((dest_gid, None, IfFrom::All))
+}
 
-    let executor = ChownExecutor {
+#[uucore_procs::gen_uumain]
-        bit_flag,
+pub fn uumain(args: impl uucore::Args) -> UResult<()> {
-        dest_gid,
+    let usage = get_usage();
-        verbosity: Verbosity {
+
-            groups_only: true,
+    chown_base(
-            level: verbosity_level,
+        uu_app().usage(&usage[..]),
-        },
+        args,
-        recursive,
+        options::ARG_GROUP,
-        dereference: derefer != 0,
+        parse_gid_and_uid,
-        preserve_root,
+        true,
-        files,
+    )
-        filter: IfFrom::All,
-        dest_uid: None,
-    };
-    executor.exec()
 }
 
 pub fn uu_app() -> App<'static, 'static> {

src/uu/chown/src/chown.rs

@@ -10,49 +10,17 @@
 #[macro_use]
 extern crate uucore;
 pub use uucore::entries::{self, Group, Locate, Passwd};
-use uucore::perms::{
+use uucore::perms::{chown_base, options, IfFrom};
-    ChownExecutor, IfFrom, Verbosity, VerbosityLevel, FTS_COMFOLLOW, FTS_LOGICAL, FTS_PHYSICAL,
-};
 
 use uucore::error::{FromIo, UResult, USimpleError};
 
-use clap::{crate_version, App, Arg};
+use clap::{crate_version, App, Arg, ArgMatches};
 
 use std::fs;
 use std::os::unix::fs::MetadataExt;
 
-use uucore::InvalidEncodingHandling;
-
 static ABOUT: &str = "change file owner and group";
 
-pub mod options {
-    pub mod verbosity {
-        pub static CHANGES: &str = "changes";
-        pub static QUIET: &str = "quiet";
-        pub static SILENT: &str = "silent";
-        pub static VERBOSE: &str = "verbose";
-    }
-    pub mod preserve_root {
-        pub static PRESERVE: &str = "preserve-root";
-        pub static NO_PRESERVE: &str = "no-preserve-root";
-    }
-    pub mod dereference {
-        pub static DEREFERENCE: &str = "dereference";
-        pub static NO_DEREFERENCE: &str = "no-dereference";
-    }
-    pub static FROM: &str = "from";
-    pub static RECURSIVE: &str = "recursive";
-    pub mod traverse {
-        pub static TRAVERSE: &str = "H";
-        pub static NO_TRAVERSE: &str = "P";
-        pub static EVERY: &str = "L";
-    }
-    pub static REFERENCE: &str = "reference";
-}
-
-static ARG_OWNER: &str = "owner";
-static ARG_FILES: &str = "files";
-
 fn get_usage() -> String {
     format!(
         "{0} [OPTION]... [OWNER][:[GROUP]] FILE...\n{0} [OPTION]... --reference=RFILE FILE...",
@@ -60,65 +28,7 @@ fn get_usage() -> String {
     )
 }
 
-#[uucore_procs::gen_uumain]
+fn parse_gid_uid_and_filter(matches: &ArgMatches) -> UResult<(Option<u32>, Option<u32>, IfFrom)> {
-pub fn uumain(args: impl uucore::Args) -> UResult<()> {
-    let args = args
-        .collect_str(InvalidEncodingHandling::Ignore)
-        .accept_any();
-
-    let usage = get_usage();
-
-    let matches = uu_app().usage(&usage[..]).get_matches_from(args);
-
-    /* First arg is the owner/group */
-    let owner = matches.value_of(ARG_OWNER).unwrap();
-
-    /* Then the list of files */
-    let files: Vec<String> = matches
-        .values_of(ARG_FILES)
-        .map(|v| v.map(ToString::to_string).collect())
-        .unwrap_or_default();
-
-    let preserve_root = matches.is_present(options::preserve_root::PRESERVE);
-
-    let mut derefer = if matches.is_present(options::dereference::NO_DEREFERENCE) {
-        1
-    } else {
-        0
-    };
-
-    let mut bit_flag = if matches.is_present(options::traverse::TRAVERSE) {
-        FTS_COMFOLLOW | FTS_PHYSICAL
-    } else if matches.is_present(options::traverse::EVERY) {
-        FTS_LOGICAL
-    } else {
-        FTS_PHYSICAL
-    };
-
-    let recursive = matches.is_present(options::RECURSIVE);
-    if recursive {
-        if bit_flag == FTS_PHYSICAL {
-            if derefer == 1 {
-                return Err(USimpleError::new(1, "-R --dereference requires -H or -L"));
-            }
-            derefer = 0;
-        }
-    } else {
-        bit_flag = FTS_PHYSICAL;
-    }
-
-    let verbosity = if matches.is_present(options::verbosity::CHANGES) {
-        VerbosityLevel::Changes
-    } else if matches.is_present(options::verbosity::SILENT)
-        || matches.is_present(options::verbosity::QUIET)
-    {
-        VerbosityLevel::Silent
-    } else if matches.is_present(options::verbosity::VERBOSE) {
-        VerbosityLevel::Verbose
-    } else {
-        VerbosityLevel::Normal
-    };
-
     let filter = if let Some(spec) = matches.value_of(options::FROM) {
         match parse_spec(spec)? {
             (Some(uid), None) => IfFrom::User(uid),
@@ -138,25 +48,24 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> {
         dest_gid = Some(meta.gid());
         dest_uid = Some(meta.uid());
     } else {
-        let (u, g) = parse_spec(owner)?;
+        let (u, g) = parse_spec(matches.value_of(options::ARG_OWNER).unwrap())?;
         dest_uid = u;
         dest_gid = g;
     }
-    let executor = ChownExecutor {
+    Ok((dest_gid, dest_uid, filter))
-        bit_flag,
+}
-        dest_uid,
+
-        dest_gid,
+#[uucore_procs::gen_uumain]
-        verbosity: Verbosity {
+pub fn uumain(args: impl uucore::Args) -> UResult<()> {
-            groups_only: false,
+    let usage = get_usage();
-            level: verbosity,
+
-        },
+    chown_base(
-        recursive,
+        uu_app().usage(&usage[..]),
-        dereference: derefer != 0,
+        args,
-        filter,
+        options::ARG_OWNER,
-        preserve_root,
+        parse_gid_uid_and_filter,
-        files,
+        false,
-    };
+    )
-    executor.exec()
 }
 
 pub fn uu_app() -> App<'static, 'static> {
@@ -169,22 +78,31 @@ pub fn uu_app() -> App<'static, 'static> {
                 .long(options::verbosity::CHANGES)
                 .help("like verbose but report only when a change is made"),
         )
-        .arg(Arg::with_name(options::dereference::DEREFERENCE).long(options::dereference::DEREFERENCE).help(
+        .arg(
-            "affect the referent of each symbolic link (this is the default), rather than the symbolic link itself",
+            Arg::with_name(options::dereference::DEREFERENCE)
-        ))
+                .long(options::dereference::DEREFERENCE)
+                .help(
+                    "affect the referent of each symbolic link (this is the default), \
+                    rather than the symbolic link itself",
+                ),
+        )
         .arg(
             Arg::with_name(options::dereference::NO_DEREFERENCE)
                 .short("h")
                 .long(options::dereference::NO_DEREFERENCE)
                 .help(
-                    "affect symbolic links instead of any referenced file (useful only on systems that can change the ownership of a symlink)",
+                    "affect symbolic links instead of any referenced file \
+                    (useful only on systems that can change the ownership of a symlink)",
                 ),
         )
         .arg(
             Arg::with_name(options::FROM)
                 .long(options::FROM)
                 .help(
-                    "change the owner and/or group of each file only if its current owner and/or group match those specified here. Either may be omitted, in which case a match is not required for the omitted attribute",
+                    "change the owner and/or group of each file only if its \
+                    current owner and/or group match those specified here. \
+                    Either may be omitted, in which case a match is not required \
+                    for the omitted attribute",
                 )
                 .value_name("CURRENT_OWNER:CURRENT_GROUP"),
         )
@@ -216,7 +134,11 @@ pub fn uu_app() -> App<'static, 'static> {
                 .value_name("RFILE")
                 .min_values(1),
         )
-        .arg(Arg::with_name(options::verbosity::SILENT).short("f").long(options::verbosity::SILENT))
+        .arg(
+            Arg::with_name(options::verbosity::SILENT)
+                .short("f")
+                .long(options::verbosity::SILENT),
+        )
         .arg(
             Arg::with_name(options::traverse::TRAVERSE)
                 .short(options::traverse::TRAVERSE)
@@ -238,21 +160,9 @@ pub fn uu_app() -> App<'static, 'static> {
         .arg(
             Arg::with_name(options::verbosity::VERBOSE)
                 .long(options::verbosity::VERBOSE)
+                .short("v")
                 .help("output a diagnostic for every file processed"),
         )
-        .arg(
-            Arg::with_name(ARG_OWNER)
-                .multiple(false)
-                .takes_value(true)
-                .required(true),
-        )
-        .arg(
-            Arg::with_name(ARG_FILES)
-                .multiple(true)
-                .takes_value(true)
-                .required(true)
-                .min_values(1),
-        )
 }
 
 fn parse_spec(spec: &str) -> UResult<(Option<u32>, Option<u32>)> {

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

@@ -7,10 +7,14 @@
 
 use crate::error::strip_errno;
 use crate::error::UResult;
+use crate::error::USimpleError;
 pub use crate::features::entries;
 use crate::fs::resolve_relative_path;
 use crate::show_error;
-use libc::{self, gid_t, lchown, uid_t};
+use clap::App;
+use clap::Arg;
+use clap::ArgMatches;
+use libc::{self, gid_t, uid_t};
 use walkdir::WalkDir;
 
 use std::io::Error as IOError;
@@ -45,7 +49,7 @@ fn chown<P: AsRef<Path>>(path: P, uid: uid_t, gid: gid_t, follow: bool) -> IORes
         if follow {
             libc::chown(s.as_ptr(), uid, gid)
         } else {
-            lchown(s.as_ptr(), uid, gid)
+            libc::lchown(s.as_ptr(), uid, gid)
         }
     };
     if ret == 0 {
@@ -161,22 +165,26 @@ pub enum IfFrom {
     UserGroup(u32, u32),
 }
 
+#[derive(PartialEq, Eq)]
+pub enum TraverseSymlinks {
+    None,
+    First,
+    All,
+}
+
 pub struct ChownExecutor {
     pub dest_uid: Option<u32>,
     pub dest_gid: Option<u32>,
-    pub bit_flag: u8,
+    pub traverse_symlinks: TraverseSymlinks,
     pub verbosity: Verbosity,
     pub filter: IfFrom,
     pub files: Vec<String>,
     pub recursive: bool,
     pub preserve_root: bool,
+    // Must be true if traverse_symlinks is not None
     pub dereference: bool,
 }
 
-pub const FTS_COMFOLLOW: u8 = 1;
-pub const FTS_PHYSICAL: u8 = 1 << 1;
-pub const FTS_LOGICAL: u8 = 1 << 2;
-
 impl ChownExecutor {
     pub fn exec(&self) -> UResult<()> {
         let mut ret = 0;
@@ -190,9 +198,8 @@ impl ChownExecutor {
     }
 
     fn traverse<P: AsRef<Path>>(&self, root: P) -> i32 {
-        let follow_arg = self.dereference || self.bit_flag != FTS_PHYSICAL;
         let path = root.as_ref();
-        let meta = match self.obtain_meta(path, follow_arg) {
+        let meta = match self.obtain_meta(path, self.dereference) {
             Some(m) => m,
             _ => return 1,
         };
@@ -204,7 +211,7 @@ impl ChownExecutor {
         //     (argument is symlink && should follow argument && resolved to be '/')
         // )
         if self.recursive && self.preserve_root {
-            let may_exist = if follow_arg {
+            let may_exist = if self.dereference {
                 path.canonicalize().ok()
             } else {
                 let real = resolve_relative_path(path);
@@ -230,7 +237,7 @@ impl ChownExecutor {
                 &meta,
                 self.dest_uid,
                 self.dest_gid,
-                follow_arg,
+                self.dereference,
                 self.verbosity.clone(),
             ) {
                 Ok(n) => {
@@ -258,11 +265,21 @@ impl ChownExecutor {
     }
 
     fn dive_into<P: AsRef<Path>>(&self, root: P) -> i32 {
-        let mut ret = 0;
         let root = root.as_ref();
-        let follow = self.dereference || self.bit_flag & FTS_LOGICAL != 0;
+
+        // walkdir always dereferences the root directory, so we have to check it ourselves
+        // TODO: replace with `root.is_symlink()` once it is stable
+        if self.traverse_symlinks == TraverseSymlinks::None
+            && std::fs::symlink_metadata(root)
+                .map(|m| m.file_type().is_symlink())
+                .unwrap_or(false)
+        {
+            return 0;
+        }
+
+        let mut ret = 0;
         let mut iterator = WalkDir::new(root)
-            .follow_links(follow)
+            .follow_links(self.traverse_symlinks == TraverseSymlinks::All)
             .min_depth(1)
             .into_iter();
         // We can't use a for loop because we need to manipulate the iterator inside the loop.
@@ -288,7 +305,7 @@ impl ChownExecutor {
                 Ok(entry) => entry,
             };
             let path = entry.path();
-            let meta = match self.obtain_meta(path, follow) {
+            let meta = match self.obtain_meta(path, self.dereference) {
                 Some(m) => m,
                 _ => {
                     ret = 1;
@@ -310,7 +327,7 @@ impl ChownExecutor {
                 &meta,
                 self.dest_uid,
                 self.dest_gid,
-                follow,
+                self.dereference,
                 self.verbosity.clone(),
             ) {
                 Ok(n) => {
@@ -341,7 +358,12 @@ impl ChownExecutor {
             Err(e) => {
                 match self.verbosity.level {
                     VerbosityLevel::Silent => (),
-                    _ => show_error!("cannot access '{}': {}", path.display(), strip_errno(&e)),
+                    _ => show_error!(
+                        "cannot {} '{}': {}",
+                        if follow { "dereference" } else { "access" },
+                        path.display(),
+                        strip_errno(&e)
+                    ),
                 }
                 None
             }
@@ -359,3 +381,148 @@ impl ChownExecutor {
         }
     }
 }
+
+pub mod options {
+    pub mod verbosity {
+        pub const CHANGES: &str = "changes";
+        pub const QUIET: &str = "quiet";
+        pub const SILENT: &str = "silent";
+        pub const VERBOSE: &str = "verbose";
+    }
+    pub mod preserve_root {
+        pub const PRESERVE: &str = "preserve-root";
+        pub const NO_PRESERVE: &str = "no-preserve-root";
+    }
+    pub mod dereference {
+        pub const DEREFERENCE: &str = "dereference";
+        pub const NO_DEREFERENCE: &str = "no-dereference";
+    }
+    pub const FROM: &str = "from";
+    pub const RECURSIVE: &str = "recursive";
+    pub mod traverse {
+        pub const TRAVERSE: &str = "H";
+        pub const NO_TRAVERSE: &str = "P";
+        pub const EVERY: &str = "L";
+    }
+    pub const REFERENCE: &str = "reference";
+    pub const ARG_OWNER: &str = "OWNER";
+    pub const ARG_GROUP: &str = "GROUP";
+    pub const ARG_FILES: &str = "FILE";
+}
+
+type GidUidFilterParser<'a> = fn(&ArgMatches<'a>) -> UResult<(Option<u32>, Option<u32>, IfFrom)>;
+
+/// Base implementation for `chgrp` and `chown`.
+///
+/// An argument called `add_arg_if_not_reference` will be added to `app` if
+/// `args` does not contain the `--reference` option.
+/// `parse_gid_uid_and_filter` will be called to obtain the target gid and uid, and the filter,
+/// from `ArgMatches`.
+/// `groups_only` determines whether verbose output will only mention the group.
+pub fn chown_base<'a>(
+    mut app: App<'a, 'a>,
+    args: impl crate::Args,
+    add_arg_if_not_reference: &'a str,
+    parse_gid_uid_and_filter: GidUidFilterParser<'a>,
+    groups_only: bool,
+) -> UResult<()> {
+    let args: Vec<_> = args.collect();
+    let mut reference = false;
+    let mut help = false;
+    // stop processing options on --
+    for arg in args.iter().take_while(|s| *s != "--") {
+        if arg.to_string_lossy().starts_with("--reference=") || arg == "--reference" {
+            reference = true;
+        } else if arg == "--help" {
+            // we stop processing once we see --help,
+            // as it doesn't matter if we've seen reference or not
+            help = true;
+            break;
+        }
+    }
+
+    if help || !reference {
+        // add both positional arguments
+        // arg_group is only required if
+        app = app.arg(
+            Arg::with_name(add_arg_if_not_reference)
+                .value_name(add_arg_if_not_reference)
+                .required(true)
+                .takes_value(true)
+                .multiple(false),
+        )
+    }
+    app = app.arg(
+        Arg::with_name(options::ARG_FILES)
+            .value_name(options::ARG_FILES)
+            .multiple(true)
+            .takes_value(true)
+            .required(true)
+            .min_values(1),
+    );
+    let matches = app.get_matches_from(args);
+
+    let files: Vec<String> = matches
+        .values_of(options::ARG_FILES)
+        .map(|v| v.map(ToString::to_string).collect())
+        .unwrap_or_default();
+
+    let preserve_root = matches.is_present(options::preserve_root::PRESERVE);
+
+    let mut dereference = if matches.is_present(options::dereference::DEREFERENCE) {
+        Some(true)
+    } else if matches.is_present(options::dereference::NO_DEREFERENCE) {
+        Some(false)
+    } else {
+        None
+    };
+
+    let mut traverse_symlinks = if matches.is_present(options::traverse::TRAVERSE) {
+        TraverseSymlinks::First
+    } else if matches.is_present(options::traverse::EVERY) {
+        TraverseSymlinks::All
+    } else {
+        TraverseSymlinks::None
+    };
+
+    let recursive = matches.is_present(options::RECURSIVE);
+    if recursive {
+        if traverse_symlinks == TraverseSymlinks::None {
+            if dereference == Some(true) {
+                return Err(USimpleError::new(1, "-R --dereference requires -H or -L"));
+            }
+            dereference = Some(false);
+        }
+    } else {
+        traverse_symlinks = TraverseSymlinks::None;
+    }
+
+    let verbosity_level = if matches.is_present(options::verbosity::CHANGES) {
+        VerbosityLevel::Changes
+    } else if matches.is_present(options::verbosity::SILENT)
+        || matches.is_present(options::verbosity::QUIET)
+    {
+        VerbosityLevel::Silent
+    } else if matches.is_present(options::verbosity::VERBOSE) {
+        VerbosityLevel::Verbose
+    } else {
+        VerbosityLevel::Normal
+    };
+    let (dest_gid, dest_uid, filter) = parse_gid_uid_and_filter(&matches)?;
+
+    let executor = ChownExecutor {
+        traverse_symlinks,
+        dest_gid,
+        dest_uid,
+        verbosity: Verbosity {
+            groups_only,
+            level: verbosity_level,
+        },
+        recursive,
+        dereference: dereference.unwrap_or(true),
+        preserve_root,
+        files,
+        filter,
+    };
+    executor.exec()
+}

tests/by-util/test_chgrp.rs

@@ -288,3 +288,68 @@ fn test_subdir_permission_denied() {
             .stderr_only("chgrp: cannot access 'dir/subdir': Permission denied");
     }
 }
+
+#[test]
+#[cfg(not(target_vendor = "apple"))]
+fn test_traverse_symlinks() {
+    use std::os::unix::prelude::MetadataExt;
+    let groups = nix::unistd::getgroups().unwrap();
+    if groups.len() < 2 {
+        return;
+    }
+    let (first_group, second_group) = (groups[0], groups[1]);
+
+    for &(args, traverse_first, traverse_second) in &[
+        (&[][..] as &[&str], false, false),
+        (&["-H"][..], true, false),
+        (&["-P"][..], false, false),
+        (&["-L"][..], true, true),
+    ] {
+        let scenario = TestScenario::new("chgrp");
+
+        let (at, mut ucmd) = (scenario.fixtures.clone(), scenario.ucmd());
+
+        at.mkdir("dir");
+        at.mkdir("dir2");
+        at.touch("dir2/file");
+        at.mkdir("dir3");
+        at.touch("dir3/file");
+        at.symlink_dir("dir2", "dir/dir2_ln");
+        at.symlink_dir("dir3", "dir3_ln");
+
+        scenario
+            .ccmd("chgrp")
+            .arg(first_group.to_string())
+            .arg("dir2/file")
+            .arg("dir3/file")
+            .succeeds();
+
+        assert!(at.plus("dir2/file").metadata().unwrap().gid() == first_group.as_raw());
+        assert!(at.plus("dir3/file").metadata().unwrap().gid() == first_group.as_raw());
+
+        ucmd.arg("-R")
+            .args(args)
+            .arg(second_group.to_string())
+            .arg("dir")
+            .arg("dir3_ln")
+            .succeeds()
+            .no_stderr();
+
+        assert_eq!(
+            at.plus("dir2/file").metadata().unwrap().gid(),
+            if traverse_second {
+                second_group.as_raw()
+            } else {
+                first_group.as_raw()
+            }
+        );
+        assert_eq!(
+            at.plus("dir3/file").metadata().unwrap().gid(),
+            if traverse_first {
+                second_group.as_raw()
+            } else {
+                first_group.as_raw()
+            }
+        );
+    }
+}