chgrp: replace getopts with clap (#2118)

Cargo.lock

@@ -1772,6 +1772,7 @@ dependencies = [
 name = "uu_chgrp"
 version = "0.0.6"
 dependencies = [
+ "clap",
  "uucore",
  "uucore_procs",
  "walkdir",

src/uu/chgrp/Cargo.toml

@@ -15,6 +15,7 @@ edition = "2018"
 path = "src/chgrp.rs"
 
 [dependencies]
+clap = "2.33"
 uucore = { version=">=0.0.8", package="uucore", path="../../uucore", features=["entries", "fs", "perms"] }
 uucore_procs = { version=">=0.0.5", package="uucore_procs", path="../../uucore_procs" }
 walkdir = "2.2"

src/uu/chgrp/src/chgrp.rs

@@ -14,6 +14,8 @@ use uucore::fs::resolve_relative_path;
 use uucore::libc::gid_t;
 use uucore::perms::{wrap_chgrp, Verbosity};
 
+use clap::{App, Arg};
+
 extern crate walkdir;
 use walkdir::WalkDir;
 
@@ -24,76 +26,194 @@ use std::os::unix::fs::MetadataExt;
 use std::path::Path;
 use uucore::InvalidEncodingHandling;
 
-static SYNTAX: &str =
-    "chgrp [OPTION]... GROUP FILE...\n or :  chgrp [OPTION]... --reference=RFILE FILE...";
-static SUMMARY: &str = "Change the group of each FILE to GROUP.";
+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";
+}
 
 const FTS_COMFOLLOW: u8 = 1;
 const FTS_PHYSICAL: u8 = 1 << 1;
 const FTS_LOGICAL: u8 = 1 << 2;
 
+fn get_usage() -> String {
+    format!(
+        "{0} [OPTION]... GROUP FILE...\n    {0} [OPTION]... --reference=RFILE FILE...",
+        executable!()
+    )
+}
+
 pub fn uumain(args: impl uucore::Args) -> i32 {
     let args = args
         .collect_str(InvalidEncodingHandling::ConvertLossy)
         .accept_any();
 
-    let mut opts = app!(SYNTAX, SUMMARY, "");
-    opts.optflag("c",
-                 "changes",
-                 "like verbose but report only when a change is made")
-        .optflag("f", "silent", "")
-        .optflag("", "quiet", "suppress most error messages")
-        .optflag("v",
-                 "verbose",
-                 "output a diagnostic for every file processed")
-        .optflag("", "dereference", "affect the referent of each symbolic link (this is the default), rather than the symbolic link itself")
-        .optflag("h", "no-dereference", "affect symbolic links instead of any referenced file (useful only on systems that can change the ownership of a symlink)")
-        .optflag("",
-                 "no-preserve-root",
-                 "do not treat '/' specially (the default)")
-        .optflag("", "preserve-root", "fail to operate recursively on '/'")
-        .optopt("",
-                "reference",
-                "use RFILE's owner and group rather than specifying OWNER:GROUP values",
-                "RFILE")
-        .optflag("R",
-                 "recursive",
-                 "operate on files and directories recursively")
-        .optflag("H",
-                 "",
-                 "if a command line argument is a symbolic link to a directory, traverse it")
-        .optflag("L",
-                 "",
-                 "traverse every symbolic link to a directory encountered")
-        .optflag("P", "", "do not traverse any symbolic links (default)");
+    let usage = get_usage();
 
-    let mut bit_flag = FTS_PHYSICAL;
-    let mut preserve_root = false;
-    let mut derefer = -1;
-    let flags: &[char] = &['H', 'L', 'P'];
-    for opt in &args {
-        match opt.as_str() {
-            // If more than one is specified, only the final one takes effect.
-            s if s.contains(flags) => {
-                if let Some(idx) = s.rfind(flags) {
-                    match s.chars().nth(idx).unwrap() {
-                        'H' => bit_flag = FTS_COMFOLLOW | FTS_PHYSICAL,
-                        'L' => bit_flag = FTS_LOGICAL,
-                        'P' => bit_flag = FTS_PHYSICAL,
-                        _ => (),
-                    }
-                }
-            }
-            "--no-preserve-root" => preserve_root = false,
-            "--preserve-root" => preserve_root = true,
-            "--dereference" => derefer = 1,
-            "--no-dereference" => derefer = 0,
-            _ => (),
+    let mut app = App::new(executable!())
+        .version(VERSION)
+        .about(ABOUT)
+        .usage(&usage[..])
+        .arg(
+            Arg::with_name(options::verbosity::CHANGES)
+                .short("c")
+                .long(options::verbosity::CHANGES)
+                .help("like verbose but report only when a change is made"),
+        )
+        .arg(
+            Arg::with_name(options::verbosity::SILENT)
+                .short("f")
+                .long(options::verbosity::SILENT),
+        )
+        .arg(
+            Arg::with_name(options::verbosity::QUIET)
+                .long(options::verbosity::QUIET)
+                .help("suppress most error messages"),
+        )
+        .arg(
+            Arg::with_name(options::verbosity::VERBOSE)
+                .short("v")
+                .long(options::verbosity::VERBOSE)
+                .help("output a diagnostic for every file processed"),
+        )
+        .arg(
+            Arg::with_name(options::dereference::DEREFERENCE)
+                .long(options::dereference::DEREFERENCE),
+        )
+        .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)",
+               ),
+        )
+        .arg(
+            Arg::with_name(options::preserve_root::PRESERVE)
+                .long(options::preserve_root::PRESERVE)
+                .help("fail to operate recursively on '/'"),
+        )
+        .arg(
+            Arg::with_name(options::preserve_root::NO_PRESERVE)
+                .long(options::preserve_root::NO_PRESERVE)
+                .help("do not treat '/' specially (the default)"),
+        )
+        .arg(
+            Arg::with_name(options::REFERENCE)
+                .long(options::REFERENCE)
+                .value_name("RFILE")
+                .help("use RFILE's group rather than specifying GROUP values")
+                .takes_value(true)
+                .multiple(false),
+        )
+        .arg(
+            Arg::with_name(options::RECURSIVE)
+                .short("R")
+                .long(options::RECURSIVE)
+                .help("operate on files and directories recursively"),
+        )
+        .arg(
+            Arg::with_name(options::traverse::TRAVERSE)
+                .short(options::traverse::TRAVERSE)
+                .help("if a command line argument is a symbolic link to a directory, traverse it"),
+        )
+        .arg(
+            Arg::with_name(options::traverse::NO_TRAVERSE)
+                .short(options::traverse::NO_TRAVERSE)
+                .help("do not traverse any symbolic links (default)")
+                .overrides_with_all(&[options::traverse::TRAVERSE, options::traverse::EVERY]),
+        )
+        .arg(
+            Arg::with_name(options::traverse::EVERY)
+                .short(options::traverse::EVERY)
+                .help("traverse every symbolic link to a directory encountered"),
+        );
+
+    // 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;
         }
     }
 
-    let matches = opts.parse(args);
-    let recursive = matches.opt_present("recursive");
+    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 {
@@ -106,27 +226,20 @@ pub fn uumain(args: impl uucore::Args) -> i32 {
         bit_flag = FTS_PHYSICAL;
     }
 
-    let verbosity = if matches.opt_present("changes") {
+    let verbosity = if matches.is_present(options::verbosity::CHANGES) {
         Verbosity::Changes
-    } else if matches.opt_present("silent") || matches.opt_present("quiet") {
+    } else if matches.is_present(options::verbosity::SILENT)
+        || matches.is_present(options::verbosity::QUIET)
+    {
         Verbosity::Silent
-    } else if matches.opt_present("verbose") {
+    } else if matches.is_present(options::verbosity::VERBOSE) {
         Verbosity::Verbose
     } else {
         Verbosity::Normal
     };
 
-    if matches.free.is_empty() {
-        show_usage_error!("missing operand");
-        return 1;
-    } else if matches.free.len() < 2 && !matches.opt_present("reference") {
-        show_usage_error!("missing operand after ‘{}’", matches.free[0]);
-        return 1;
-    }
-
-    let dest_gid: gid_t;
-    let mut files;
-    if let Some(file) = matches.opt_str("reference") {
+    let dest_gid: u32;
+    if let Some(file) = matches.value_of(options::REFERENCE) {
         match fs::metadata(&file) {
             Ok(meta) => {
                 dest_gid = meta.gid();
@@ -136,19 +249,17 @@ pub fn uumain(args: impl uucore::Args) -> i32 {
                 return 1;
             }
         }
-        files = matches.free;
     } else {
-        match entries::grp2gid(&matches.free[0]) {
+        let group = matches.value_of(options::ARG_GROUP).unwrap_or_default();
+        match entries::grp2gid(&group) {
             Ok(g) => {
                 dest_gid = g;
             }
             _ => {
-                show_error!("invalid group: {}", matches.free[0].as_str());
+                show_error!("invalid group: {}", group);
                 return 1;
             }
         }
-        files = matches.free;
-        files.remove(0);
     }
 
     let executor = Chgrper {

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

@@ -92,7 +92,7 @@ pub fn wrap_chgrp<P: AsRef<Path>>(
             out = format!(
                 "group of '{}' retained as {}",
                 path.display(),
-                entries::gid2grp(dest_gid).unwrap()
+                entries::gid2grp(dest_gid).unwrap_or_default()
             );
         }
     }

tests/by-util/test_chgrp.rs

@@ -10,6 +10,33 @@ fn test_invalid_option() {
 
 static DIR: &str = "/tmp";
 
+// we should always get both arguments, regardless of whether --refernce was used
+#[test]
+fn test_help() {
+    new_ucmd!()
+        .arg("--help")
+        .succeeds()
+        .stdout_contains("ARGS:\n    <GROUP>      \n    <FILE>...    ");
+}
+
+#[test]
+fn test_help_ref() {
+    new_ucmd!()
+        .arg("--help")
+        .arg("--reference=ref_file")
+        .succeeds()
+        .stdout_contains("ARGS:\n    <GROUP>      \n    <FILE>...    ");
+}
+
+#[test]
+fn test_ref_help() {
+    new_ucmd!()
+        .arg("--reference=ref_file")
+        .arg("--help")
+        .succeeds()
+        .stdout_contains("ARGS:\n    <GROUP>      \n    <FILE>...    ");
+}
+
 #[test]
 fn test_invalid_group() {
     new_ucmd!()
@@ -121,9 +148,52 @@ fn test_reference() {
 fn test_reference() {
     new_ucmd!()
         .arg("-v")
-        .arg("--reference=/etc/passwd")
+        .arg("--reference=ref_file")
         .arg("/etc")
-        .succeeds();
+        .fails()
+        // group name can differ, so just check the first part of the message
+        .stderr_contains("chgrp: changing group of '/etc': Operation not permitted (os error 1)\nfailed to change group of '/etc' from ");
+}
+
+#[test]
+#[cfg(any(target_os = "linux", target_vendor = "apple"))]
+fn test_reference_multi_no_equal() {
+    new_ucmd!()
+        .arg("-v")
+        .arg("--reference")
+        .arg("ref_file")
+        .arg("file1")
+        .arg("file2")
+        .succeeds()
+        .stderr_contains("chgrp: group of 'file1' retained as ")
+        .stderr_contains("\nchgrp: group of 'file2' retained as ");
+}
+
+#[test]
+#[cfg(any(target_os = "linux", target_vendor = "apple"))]
+fn test_reference_last() {
+    new_ucmd!()
+        .arg("-v")
+        .arg("file1")
+        .arg("file2")
+        .arg("file3")
+        .arg("--reference")
+        .arg("ref_file")
+        .succeeds()
+        .stderr_contains("chgrp: group of 'file1' retained as ")
+        .stderr_contains("\nchgrp: group of 'file2' retained as ")
+        .stderr_contains("\nchgrp: group of 'file3' retained as ");
+}
+
+#[test]
+fn test_missing_files() {
+    new_ucmd!()
+        .arg("-v")
+        .arg("groupname")
+        .fails()
+        .stderr_contains(
+            "error: The following required arguments were not provided:\n    <FILE>...\n",
+        );
 }
 
 #[test]
@@ -135,7 +205,7 @@ fn test_big_p() {
             .arg("bin")
             .arg("/proc/self/cwd")
             .fails()
-            .stderr_is(
+            .stderr_contains(
                 "chgrp: changing group of '/proc/self/cwd': Operation not permitted (os error 1)\n",
             );
     }

tests/fixtures/chgrp/file1

@@ -0,0 +1 @@
+target file 1

tests/fixtures/chgrp/file2

@@ -0,0 +1 @@
+target file 2

tests/fixtures/chgrp/file3

@@ -0,0 +1 @@
+target file 3

tests/fixtures/chgrp/ref_file

@@ -0,0 +1 @@
+Reference file