From 43c6a52b63623d6c75d924ee85617c8556f86f5b Mon Sep 17 00:00:00 2001 From: Jan Scheer Date: Sun, 28 Mar 2021 13:11:39 +0200 Subject: [PATCH 1/2] chmod: move from getopts to clap --- src/uu/chmod/Cargo.toml | 1 + src/uu/chmod/src/chmod.rs | 235 ++++++++++++++++++++++++-------------- 2 files changed, 148 insertions(+), 88 deletions(-) diff --git a/src/uu/chmod/Cargo.toml b/src/uu/chmod/Cargo.toml index 41b73d8a6..d4917b525 100644 --- a/src/uu/chmod/Cargo.toml +++ b/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" } diff --git a/src/uu/chmod/src/chmod.rs b/src/uu/chmod/src/chmod.rs index 61c56dd77..119447b14 100644 --- a/src/uu/chmod/src/chmod.rs +++ b/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 = 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) -> Option { +// 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" +fn strip_minus_from_mode(args: &mut Vec) -> 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 { From a9a3794d5a366f185d65e8c4908b4db180cdec20 Mon Sep 17 00:00:00 2001 From: Jan Scheer Date: Sun, 28 Mar 2021 20:56:37 +0200 Subject: [PATCH 2/2] chmod: add tests --- src/uu/chmod/src/chmod.rs | 2 +- tests/by-util/test_chmod.rs | 31 +++++++++++++++++++++++++++++++ 2 files changed, 32 insertions(+), 1 deletion(-) diff --git a/src/uu/chmod/src/chmod.rs b/src/uu/chmod/src/chmod.rs index 119447b14..819c674a0 100644 --- a/src/uu/chmod/src/chmod.rs +++ b/src/uu/chmod/src/chmod.rs @@ -170,7 +170,7 @@ pub fn uumain(args: impl uucore::Args) -> i32 { // 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" -fn strip_minus_from_mode(args: &mut Vec) -> bool { +pub fn strip_minus_from_mode(args: &mut Vec) -> bool { for i in 0..args.len() { if args[i].starts_with("-") { if let Some(second) = args[i].chars().nth(1) { diff --git a/tests/by-util/test_chmod.rs b/tests/by-util/test_chmod.rs index e7bc72677..3a53202fc 100644 --- a/tests/by-util/test_chmod.rs +++ b/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 = 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(" ")); + } +}