Merge pull request #1955 from jhscheer/chmod2clap

chmod: move from getopts to clap

src/uu/chmod/Cargo.toml

@@ -15,6 +15,7 @@ edition = "2018"
 path = "src/chmod.rs"
 
 [dependencies]
+clap = "2.33.3"
 libc = "0.2.42"
 uucore = { version=">=0.0.7", package="uucore", path="../../uucore", features=["fs", "mode"] }
 uucore_procs = { version=">=0.0.5", package="uucore_procs", path="../../uucore_procs" }

src/uu/chmod/src/chmod.rs

@@ -10,6 +10,7 @@
 #[macro_use]
 extern crate uucore;
 
+use clap::{App, Arg};
 use std::fs;
 use std::os::unix::fs::{MetadataExt, PermissionsExt};
 use std::path::Path;
@@ -18,115 +19,173 @@ use uucore::fs::display_permissions_unix;
 use uucore::mode;
 use walkdir::WalkDir;
 
-const NAME: &str = "chmod";
-static SUMMARY: &str = "Change the mode of each FILE to MODE.
+static VERSION: &str = env!("CARGO_PKG_VERSION");
+static ABOUT: &str = "Change the mode of each FILE to MODE.
  With --reference, change the mode of each FILE to that of RFILE.";
-static LONG_HELP: &str = "
- Each MODE is of the form '[ugoa]*([-+=]([rwxXst]*|[ugo]))+|[-+=]?[0-7]+'.
-";
+
+mod options {
+    pub const CHANGES: &str = "changes";
+    pub const QUIET: &str = "quiet"; // visible_alias("silent")
+    pub const VERBOSE: &str = "verbose";
+    pub const NO_PRESERVE_ROOT: &str = "no-preserve-root";
+    pub const PRESERVE_ROOT: &str = "preserve-root";
+    pub const REFERENCE: &str = "RFILE";
+    pub const RECURSIVE: &str = "recursive";
+    pub const MODE: &str = "MODE";
+    pub const FILE: &str = "FILE";
+}
+
+fn get_usage() -> String {
+    format!(
+        "{0} [OPTION]... MODE[,MODE]... FILE...
+or: {0} [OPTION]... OCTAL-MODE FILE...
+or: {0} [OPTION]... --reference=RFILE FILE...",
+        executable!()
+    )
+}
+
+fn get_long_usage() -> String {
+    String::from("Each MODE is of the form '[ugoa]*([-+=]([rwxXst]*|[ugo]))+|[-+=]?[0-7]+'.")
+}
 
 pub fn uumain(args: impl uucore::Args) -> i32 {
     let mut args = args.collect_str();
 
-    let syntax = format!(
-        "[OPTION]... MODE[,MODE]... FILE...
- {0} [OPTION]... OCTAL-MODE FILE...
- {0} [OPTION]... --reference=RFILE FILE...",
-        NAME
-    );
-    let mut opts = app!(&syntax, SUMMARY, LONG_HELP);
-    opts.optflag(
-        "c",
-        "changes",
-        "like verbose but report only when a change is made",
-    )
-    // TODO: support --silent (can be done using clap)
-    .optflag("f", "quiet", "suppress most error messages")
-    .optflag(
-        "v",
-        "verbose",
-        "output a diagnostic for every file processed",
-    )
-    .optflag(
-        "",
-        "no-preserve-root",
-        "do not treat '/' specially (the default)",
-    )
-    .optflag("", "preserve-root", "fail to operate recursively on '/'")
-    .optopt(
-        "",
-        "reference",
-        "use RFILE's mode instead of MODE values",
-        "RFILE",
-    )
-    .optflag("R", "recursive", "change files and directories recursively");
+    // Before we can parse 'args' with clap (and previously getopts),
+    // a possible MODE prefix '-' needs to be removed (e.g. "chmod -x FILE").
+    let mode_had_minus_prefix = strip_minus_from_mode(&mut args);
 
-    // sanitize input for - at beginning (e.g. chmod -x test_file). Remove
-    // the option and save it for later, after parsing is finished.
-    let negative_option = sanitize_input(&mut args);
+    let usage = get_usage();
+    let after_help = get_long_usage();
 
-    let mut matches = opts.parse(args);
-    if matches.free.is_empty() {
-        show_error!("missing an argument");
-        show_error!("for help, try '{} --help'", NAME);
-        return 1;
-    } else {
-        let changes = matches.opt_present("changes");
-        let quiet = matches.opt_present("quiet");
-        let verbose = matches.opt_present("verbose");
-        let preserve_root = matches.opt_present("preserve-root");
-        let recursive = matches.opt_present("recursive");
-        let fmode = matches
-            .opt_str("reference")
+    let matches = App::new(executable!())
+        .version(VERSION)
+        .about(ABOUT)
+        .usage(&usage[..])
+        .after_help(&after_help[..])
+        .arg(
+            Arg::with_name(options::CHANGES)
+                .long(options::CHANGES)
+                .short("c")
+                .help("like verbose but report only when a change is made"),
+        )
+        .arg(
+            Arg::with_name(options::QUIET)
+                .long(options::QUIET)
+                .visible_alias("silent")
+                .short("f")
+                .help("suppress most error messages"),
+        )
+        .arg(
+            Arg::with_name(options::VERBOSE)
+                .long(options::VERBOSE)
+                .short("v")
+                .help("output a diagnostic for every file processed"),
+        )
+        .arg(
+            Arg::with_name(options::NO_PRESERVE_ROOT)
+                .long(options::NO_PRESERVE_ROOT)
+                .help("do not treat '/' specially (the default)"),
+        )
+        .arg(
+            Arg::with_name(options::PRESERVE_ROOT)
+                .long(options::PRESERVE_ROOT)
+                .help("fail to operate recursively on '/'"),
+        )
+        .arg(
+            Arg::with_name(options::RECURSIVE)
+                .long(options::RECURSIVE)
+                .short("R")
+                .help("change files and directories recursively"),
+        )
+        .arg(
+            Arg::with_name(options::REFERENCE)
+                .long("reference")
+                .takes_value(true)
+                .help("use RFILE's mode instead of MODE values"),
+        )
+        .arg(
+            Arg::with_name(options::MODE)
+                .required_unless(options::REFERENCE)
+                .takes_value(true),
+            // It would be nice if clap could parse with delimeter, e.g. "g-x,u+x",
+            // however .multiple(true) cannot be used here because FILE already needs that.
+            // Only one positional argument with .multiple(true) set is allowed per command
+        )
+        .arg(
+            Arg::with_name(options::FILE)
+                .required_unless(options::MODE)
+                .multiple(true),
+        )
+        .get_matches_from(args);
+
+    let changes = matches.is_present(options::CHANGES);
+    let quiet = matches.is_present(options::QUIET);
+    let verbose = matches.is_present(options::VERBOSE);
+    let preserve_root = matches.is_present(options::PRESERVE_ROOT);
+    let recursive = matches.is_present(options::RECURSIVE);
+    let fmode =
+        matches
+            .value_of(options::REFERENCE)
             .and_then(|ref fref| match fs::metadata(fref) {
                 Ok(meta) => Some(meta.mode()),
                 Err(err) => crash!(1, "cannot stat attributes of '{}': {}", fref, err),
             });
-        let cmode = if fmode.is_none() {
-            // If there was a negative option, now it's a good time to
-            // use it.
-            if negative_option.is_some() {
-                negative_option
-            } else {
-                Some(matches.free.remove(0))
-            }
-        } else {
-            None
-        };
-        let chmoder = Chmoder {
-            changes,
-            quiet,
-            verbose,
-            preserve_root,
-            recursive,
-            fmode,
-            cmode,
-        };
-        match chmoder.chmod(matches.free) {
-            Ok(()) => {}
-            Err(e) => return e,
-        }
+    let modes = matches.value_of(options::MODE).unwrap(); // should always be Some because required
+    let mut cmode = if mode_had_minus_prefix {
+        // clap parsing is finished, now put prefix back
+        Some(format!("-{}", modes))
+    } else {
+        Some(modes.to_string())
+    };
+    let mut files: Vec<String> = matches
+        .values_of(options::FILE)
+        .map(|v| v.map(ToString::to_string).collect())
+        .unwrap_or_default();
+    if fmode.is_some() {
+        // "--reference" and MODE are mutually exclusive
+        // if "--reference" was used MODE needs to be interpreted as another FILE
+        // it wasn't possible to implement this behavior directly with clap
+        files.push(cmode.unwrap());
+        cmode = None;
+    }
+
+    let chmoder = Chmoder {
+        changes,
+        quiet,
+        verbose,
+        preserve_root,
+        recursive,
+        fmode,
+        cmode,
+    };
+    match chmoder.chmod(files) {
+        Ok(()) => {}
+        Err(e) => return e,
     }
 
     0
 }
 
-fn sanitize_input(args: &mut Vec<String>) -> Option<String> {
+// Iterate 'args' and delete the first occurrence
+// of a prefix '-' if it's associated with MODE
+// e.g. "chmod -v -xw -R FILE" -> "chmod -v xw -R FILE"
+pub fn strip_minus_from_mode(args: &mut Vec<String>) -> bool {
     for i in 0..args.len() {
-        let first = args[i].chars().next().unwrap();
-        if first != '-' {
-            continue;
-        }
-        if let Some(second) = args[i].chars().nth(1) {
-            match second {
-                'r' | 'w' | 'x' | 'X' | 's' | 't' | 'u' | 'g' | 'o' | '0'..='7' => {
-                    return Some(args.remove(i));
+        if args[i].starts_with("-") {
+            if let Some(second) = args[i].chars().nth(1) {
+                match second {
+                    'r' | 'w' | 'x' | 'X' | 's' | 't' | 'u' | 'g' | 'o' | '0'..='7' => {
+                        // TODO: use strip_prefix() once minimum rust version reaches 1.45.0
+                        args[i] = args[i][1..args[i].len()].to_string();
+                        return true;
+                    }
+                    _ => {}
                 }
-                _ => {}
             }
         }
     }
-    None
+    false
 }
 
 struct Chmoder {

tests/by-util/test_chmod.rs

@@ -4,6 +4,8 @@ use std::os::unix::fs::{OpenOptionsExt, PermissionsExt};
 use std::sync::Mutex;
 
 extern crate libc;
+use self::chmod::strip_minus_from_mode;
+extern crate chmod;
 use self::libc::umask;
 
 static TEST_FILE: &'static str = "file";
@@ -374,3 +376,32 @@ fn test_chmod_symlink_non_existing_recursive() {
         .stderr
         .contains("neither symbolic link 'tmp/test-long.link' nor referent has been changed"));
 }
+
+#[test]
+fn test_chmod_strip_minus_from_mode() {
+    let tests = vec![
+        // ( before, after )
+        ("chmod -v -xw -R FILE", "chmod -v xw -R FILE"),
+        ("chmod g=rwx FILE -c", "chmod g=rwx FILE -c"),
+        (
+            "chmod -c -R -w,o+w FILE --preserve-root",
+            "chmod -c -R w,o+w FILE --preserve-root",
+        ),
+        ("chmod -c -R +w FILE ", "chmod -c -R +w FILE "),
+        ("chmod a=r,=xX FILE", "chmod a=r,=xX FILE"),
+        (
+            "chmod -v --reference RFILE -R FILE",
+            "chmod -v --reference RFILE -R FILE",
+        ),
+        ("chmod -Rvc -w-x FILE", "chmod -Rvc w-x FILE"),
+        ("chmod 755 -v FILE", "chmod 755 -v FILE"),
+        ("chmod -v +0004 FILE -R", "chmod -v +0004 FILE -R"),
+        ("chmod -v -0007 FILE -R", "chmod -v 0007 FILE -R"),
+    ];
+
+    for test in tests {
+        let mut args: Vec<String> = test.0.split(" ").map(|v| v.to_string()).collect();
+        let _mode_had_minus_prefix = strip_minus_from_mode(&mut args);
+        assert_eq!(test.1, args.join(" "));
+    }
+}