From cff75f242afa013f58f98b1bac32f9f2a59f831e Mon Sep 17 00:00:00 2001 From: Walter Scheper Date: Wed, 19 May 2021 22:38:51 -0400 Subject: [PATCH] chgrp: replace getopts with clap (#2118) --- Cargo.lock | 1 + src/uu/chgrp/Cargo.toml | 1 + src/uu/chgrp/src/chgrp.rs | 261 +++++++++++++++++++-------- src/uucore/src/lib/features/perms.rs | 2 +- tests/by-util/test_chgrp.rs | 76 +++++++- tests/fixtures/chgrp/file1 | 1 + tests/fixtures/chgrp/file2 | 1 + tests/fixtures/chgrp/file3 | 1 + tests/fixtures/chgrp/ref_file | 1 + 9 files changed, 266 insertions(+), 79 deletions(-) create mode 100644 tests/fixtures/chgrp/file1 create mode 100644 tests/fixtures/chgrp/file2 create mode 100644 tests/fixtures/chgrp/file3 create mode 100644 tests/fixtures/chgrp/ref_file diff --git a/Cargo.lock b/Cargo.lock index 17fa9e2b7..9f4ed26b7 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1772,6 +1772,7 @@ dependencies = [ name = "uu_chgrp" version = "0.0.6" dependencies = [ + "clap", "uucore", "uucore_procs", "walkdir", diff --git a/src/uu/chgrp/Cargo.toml b/src/uu/chgrp/Cargo.toml index 9424ad35e..0e43f7c02 100644 --- a/src/uu/chgrp/Cargo.toml +++ b/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" diff --git a/src/uu/chgrp/src/chgrp.rs b/src/uu/chgrp/src/chgrp.rs index f6afc2805..c0dc2daf3 100644 --- a/src/uu/chgrp/src/chgrp.rs +++ b/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 = 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 { diff --git a/src/uucore/src/lib/features/perms.rs b/src/uucore/src/lib/features/perms.rs index eb6cca102..89c30b53b 100644 --- a/src/uucore/src/lib/features/perms.rs +++ b/src/uucore/src/lib/features/perms.rs @@ -92,7 +92,7 @@ pub fn wrap_chgrp>( out = format!( "group of '{}' retained as {}", path.display(), - entries::gid2grp(dest_gid).unwrap() + entries::gid2grp(dest_gid).unwrap_or_default() ); } } diff --git a/tests/by-util/test_chgrp.rs b/tests/by-util/test_chgrp.rs index 45380b80b..d886d674b 100644 --- a/tests/by-util/test_chgrp.rs +++ b/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 \n ... "); +} + +#[test] +fn test_help_ref() { + new_ucmd!() + .arg("--help") + .arg("--reference=ref_file") + .succeeds() + .stdout_contains("ARGS:\n \n ... "); +} + +#[test] +fn test_ref_help() { + new_ucmd!() + .arg("--reference=ref_file") + .arg("--help") + .succeeds() + .stdout_contains("ARGS:\n \n ... "); +} + #[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 ...\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", ); } diff --git a/tests/fixtures/chgrp/file1 b/tests/fixtures/chgrp/file1 new file mode 100644 index 000000000..73b6f48ab --- /dev/null +++ b/tests/fixtures/chgrp/file1 @@ -0,0 +1 @@ +target file 1 diff --git a/tests/fixtures/chgrp/file2 b/tests/fixtures/chgrp/file2 new file mode 100644 index 000000000..7ecd32965 --- /dev/null +++ b/tests/fixtures/chgrp/file2 @@ -0,0 +1 @@ +target file 2 diff --git a/tests/fixtures/chgrp/file3 b/tests/fixtures/chgrp/file3 new file mode 100644 index 000000000..73d293aba --- /dev/null +++ b/tests/fixtures/chgrp/file3 @@ -0,0 +1 @@ +target file 3 diff --git a/tests/fixtures/chgrp/ref_file b/tests/fixtures/chgrp/ref_file new file mode 100644 index 000000000..aba32d56e --- /dev/null +++ b/tests/fixtures/chgrp/ref_file @@ -0,0 +1 @@ +Reference file