diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 1cb9b333a..c9a50cd1e 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -17,7 +17,7 @@ search the issues to make sure no one else is working on it. ## Best practices -1. Follow what GNU is doing in term of options and behavior. +1. Follow what GNU is doing in terms of options and behavior. It is recommended to look at the GNU Coreutils manual ([on the web](https://www.gnu.org/software/coreutils/manual/html_node/index.html), or locally using `info `). It is more in depth than the man pages and provides a good description of available features and their implementation details. 1. If possible, look at the GNU test suite execution in the CI and make the test work if failing. 1. Use clap for argument management. 1. Make sure that the code coverage is covering all of the cases, including errors. diff --git a/Cargo.lock b/Cargo.lock index dd1e697ba..1aa3c3416 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1,5 +1,7 @@ # This file is automatically @generated by Cargo. # It is not intended for manual editing. +version = 3 + [[package]] name = "Inflector" version = "0.11.4" @@ -2811,6 +2813,7 @@ dependencies = [ name = "uucore" version = "0.0.9" dependencies = [ + "clap", "data-encoding", "dns-lookup", "dunce", diff --git a/src/uu/chown/src/chown.rs b/src/uu/chown/src/chown.rs index e1d3ff22b..081d3a148 100644 --- a/src/uu/chown/src/chown.rs +++ b/src/uu/chown/src/chown.rs @@ -14,6 +14,8 @@ use uucore::fs::resolve_relative_path; use uucore::libc::{gid_t, uid_t}; use uucore::perms::{wrap_chown, Verbosity}; +use uucore::error::{FromIo, UError, UResult, USimpleError}; + use clap::{crate_version, App, Arg}; use walkdir::WalkDir; @@ -66,7 +68,8 @@ fn get_usage() -> String { ) } -pub fn uumain(args: impl uucore::Args) -> i32 { +#[uucore_procs::gen_uumain] +pub fn uumain(args: impl uucore::Args) -> UResult<()> { let args = args .collect_str(InvalidEncodingHandling::Ignore) .accept_any(); @@ -104,8 +107,10 @@ pub fn uumain(args: impl uucore::Args) -> i32 { if recursive { if bit_flag == FTS_PHYSICAL { if derefer == 1 { - show_error!("-R --dereference requires -H or -L"); - return 1; + return Err(USimpleError::new( + 1, + "-R --dereference requires -H or -L".to_string(), + )); } derefer = 0; } @@ -126,15 +131,11 @@ pub fn uumain(args: impl uucore::Args) -> i32 { }; let filter = if let Some(spec) = matches.value_of(options::FROM) { - match parse_spec(spec) { - Ok((Some(uid), None)) => IfFrom::User(uid), - Ok((None, Some(gid))) => IfFrom::Group(gid), - Ok((Some(uid), Some(gid))) => IfFrom::UserGroup(uid, gid), - Ok((None, None)) => IfFrom::All, - Err(e) => { - show_error!("{}", e); - return 1; - } + match parse_spec(spec)? { + (Some(uid), None) => IfFrom::User(uid), + (None, Some(gid)) => IfFrom::Group(gid), + (Some(uid), Some(gid)) => IfFrom::UserGroup(uid, gid), + (None, None) => IfFrom::All, } } else { IfFrom::All @@ -143,27 +144,14 @@ pub fn uumain(args: impl uucore::Args) -> i32 { let dest_uid: Option; let dest_gid: Option; if let Some(file) = matches.value_of(options::REFERENCE) { - match fs::metadata(&file) { - Ok(meta) => { - dest_gid = Some(meta.gid()); - dest_uid = Some(meta.uid()); - } - Err(e) => { - show_error!("failed to get attributes of '{}': {}", file, e); - return 1; - } - } + let meta = fs::metadata(&file) + .map_err_context(|| format!("failed to get attributes of '{}'", file))?; + dest_gid = Some(meta.gid()); + dest_uid = Some(meta.uid()); } else { - match parse_spec(owner) { - Ok((u, g)) => { - dest_uid = u; - dest_gid = g; - } - Err(e) => { - show_error!("{}", e); - return 1; - } - } + let (u, g) = parse_spec(owner)?; + dest_uid = u; + dest_gid = g; } let executor = Chowner { bit_flag, @@ -275,7 +263,7 @@ pub fn uu_app() -> App<'static, 'static> { ) } -fn parse_spec(spec: &str) -> Result<(Option, Option), String> { +fn parse_spec(spec: &str) -> UResult<(Option, Option)> { let args = spec.split_terminator(':').collect::>(); let usr_only = args.len() == 1 && !args[0].is_empty(); let grp_only = args.len() == 2 && args[0].is_empty(); @@ -283,7 +271,7 @@ fn parse_spec(spec: &str) -> Result<(Option, Option), String> { let uid = if usr_only || usr_grp { Some( Passwd::locate(args[0]) - .map_err(|_| format!("invalid user: '{}'", spec))? + .map_err(|_| USimpleError::new(1, format!("invalid user: '{}'", spec)))? .uid(), ) } else { @@ -292,7 +280,7 @@ fn parse_spec(spec: &str) -> Result<(Option, Option), String> { let gid = if grp_only || usr_grp { Some( Group::locate(args[1]) - .map_err(|_| format!("invalid group: '{}'", spec))? + .map_err(|_| USimpleError::new(1, format!("invalid group: '{}'", spec)))? .gid(), ) } else { @@ -330,12 +318,15 @@ macro_rules! unwrap { } impl Chowner { - fn exec(&self) -> i32 { + fn exec(&self) -> UResult<()> { let mut ret = 0; for f in &self.files { ret |= self.traverse(f); } - ret + if ret != 0 { + return Err(UError::from(ret)); + } + Ok(()) } fn traverse>(&self, root: P) -> i32 { diff --git a/src/uu/cp/src/cp.rs b/src/uu/cp/src/cp.rs index 4deaefa98..91ea7ef37 100644 --- a/src/uu/cp/src/cp.rs +++ b/src/uu/cp/src/cp.rs @@ -98,6 +98,9 @@ quick_error! { /// path, but those that are not implemented yet should return /// a NotImplemented error. NotImplemented(opt: String) { display("Option '{}' not yet implemented.", opt) } + + /// Invalid arguments to backup + Backup(description: String) { display("{}\nTry 'cp --help' for more information.", description) } } } @@ -359,7 +362,6 @@ pub fn uu_app() -> App<'static, 'static> { .takes_value(true) .require_equals(true) .min_values(0) - .possible_values(backup_control::BACKUP_CONTROL_VALUES) .value_name("CONTROL") ) .arg(Arg::with_name(options::BACKUP_NO_ARG) @@ -604,9 +606,17 @@ impl Options { || matches.is_present(options::ARCHIVE); let backup_mode = backup_control::determine_backup_mode( - matches.is_present(options::BACKUP_NO_ARG) || matches.is_present(options::BACKUP), + matches.is_present(options::BACKUP_NO_ARG), + matches.is_present(options::BACKUP), matches.value_of(options::BACKUP), ); + let backup_mode = match backup_mode { + Err(err) => { + return Err(Error::Backup(err)); + } + Ok(mode) => mode, + }; + let backup_suffix = backup_control::determine_backup_suffix(matches.value_of(options::SUFFIX)); diff --git a/src/uu/echo/src/echo.rs b/src/uu/echo/src/echo.rs index 8c976c2b4..acdd22948 100644 --- a/src/uu/echo/src/echo.rs +++ b/src/uu/echo/src/echo.rs @@ -6,6 +6,9 @@ // For the full copyright and license information, please view the LICENSE // file that was distributed with this source code. +// clippy bug https://github.com/rust-lang/rust-clippy/issues/7422 +#![allow(clippy::nonstandard_macro_braces)] + #[macro_use] extern crate uucore; @@ -13,6 +16,7 @@ use clap::{crate_version, App, Arg}; use std::io::{self, Write}; use std::iter::Peekable; use std::str::Chars; +use uucore::error::{FromIo, UResult}; use uucore::InvalidEncodingHandling; const NAME: &str = "echo"; @@ -113,7 +117,8 @@ fn print_escaped(input: &str, mut output: impl Write) -> io::Result { Ok(should_stop) } -pub fn uumain(args: impl uucore::Args) -> i32 { +#[uucore_procs::gen_uumain] +pub fn uumain(args: impl uucore::Args) -> UResult<()> { let args = args .collect_str(InvalidEncodingHandling::ConvertLossy) .accept_any(); @@ -126,13 +131,7 @@ pub fn uumain(args: impl uucore::Args) -> i32 { None => vec!["".to_string()], }; - match execute(no_newline, escaped, values) { - Ok(_) => 0, - Err(f) => { - show_error!("{}", f); - 1 - } - } + execute(no_newline, escaped, values).map_err_context(|| "could not write to stdout".to_string()) } pub fn uu_app() -> App<'static, 'static> { diff --git a/src/uu/false/src/false.rs b/src/uu/false/src/false.rs index 17c681129..232431142 100644 --- a/src/uu/false/src/false.rs +++ b/src/uu/false/src/false.rs @@ -5,12 +5,20 @@ // * For the full copyright and license information, please view the LICENSE // * file that was distributed with this source code. +// Clippy bug: https://github.com/rust-lang/rust-clippy/issues/7422 +#![allow(clippy::nonstandard_macro_braces)] + +#[macro_use] +extern crate uucore; + use clap::App; +use uucore::error::{UError, UResult}; use uucore::executable; -pub fn uumain(args: impl uucore::Args) -> i32 { +#[uucore_procs::gen_uumain] +pub fn uumain(args: impl uucore::Args) -> UResult<()> { uu_app().get_matches_from(args); - 1 + Err(UError::from(1)) } pub fn uu_app() -> App<'static, 'static> { diff --git a/src/uu/install/src/install.rs b/src/uu/install/src/install.rs index bd227da56..81d44bb6c 100644 --- a/src/uu/install/src/install.rs +++ b/src/uu/install/src/install.rs @@ -308,15 +308,25 @@ fn behavior(matches: &ArgMatches) -> Result { None }; + let backup_mode = backup_control::determine_backup_mode( + matches.is_present(OPT_BACKUP_NO_ARG), + matches.is_present(OPT_BACKUP), + matches.value_of(OPT_BACKUP), + ); + let backup_mode = match backup_mode { + Err(err) => { + show_usage_error!("{}", err); + return Err(1); + } + Ok(mode) => mode, + }; + let target_dir = matches.value_of(OPT_TARGET_DIRECTORY).map(|d| d.to_owned()); Ok(Behavior { main_function, specified_mode, - backup_mode: backup_control::determine_backup_mode( - matches.is_present(OPT_BACKUP_NO_ARG) || matches.is_present(OPT_BACKUP), - matches.value_of(OPT_BACKUP), - ), + backup_mode, suffix: backup_control::determine_backup_suffix(matches.value_of(OPT_SUFFIX)), owner: matches.value_of(OPT_OWNER).unwrap_or("").to_string(), group: matches.value_of(OPT_GROUP).unwrap_or("").to_string(), diff --git a/src/uu/kill/src/kill.rs b/src/uu/kill/src/kill.rs index 92868efdb..88eb08fa7 100644 --- a/src/uu/kill/src/kill.rs +++ b/src/uu/kill/src/kill.rs @@ -7,20 +7,21 @@ // spell-checker:ignore (ToDO) signalname pids +// clippy bug https://github.com/rust-lang/rust-clippy/issues/7422 +#![allow(clippy::nonstandard_macro_braces)] + #[macro_use] extern crate uucore; use clap::{crate_version, App, Arg}; use libc::{c_int, pid_t}; use std::io::Error; +use uucore::error::{UResult, USimpleError}; use uucore::signals::ALL_SIGNALS; use uucore::InvalidEncodingHandling; static ABOUT: &str = "Send signal to processes or list information about signals."; -static EXIT_OK: i32 = 0; -static EXIT_ERR: i32 = 1; - pub mod options { pub static PIDS_OR_SIGNALS: &str = "pids_of_signals"; pub static LIST: &str = "list"; @@ -36,7 +37,8 @@ pub enum Mode { List, } -pub fn uumain(args: impl uucore::Args) -> i32 { +#[uucore_procs::gen_uumain] +pub fn uumain(args: impl uucore::Args) -> UResult<()> { let args = args .collect_str(InvalidEncodingHandling::Ignore) .accept_any(); @@ -66,13 +68,14 @@ pub fn uumain(args: impl uucore::Args) -> i32 { (None, Some(s)) => s.to_owned(), (None, None) => "TERM".to_owned(), }; - return kill(&sig, &pids_or_signals); + kill(&sig, &pids_or_signals) + } + Mode::Table => { + table(); + Ok(()) } - Mode::Table => table(), Mode::List => list(pids_or_signals.get(0).cloned()), } - - EXIT_OK } pub fn uu_app() -> App<'static, 'static> { @@ -139,20 +142,23 @@ fn table() { println!(); } } - println!() + println!(); } -fn print_signal(signal_name_or_value: &str) { +fn print_signal(signal_name_or_value: &str) -> UResult<()> { for (value, &signal) in ALL_SIGNALS.iter().enumerate() { if signal == signal_name_or_value || (format!("SIG{}", signal)) == signal_name_or_value { println!("{}", value); - exit!(EXIT_OK as i32) + return Ok(()); } else if signal_name_or_value == value.to_string() { println!("{}", signal); - exit!(EXIT_OK as i32) + return Ok(()); } } - crash!(EXIT_ERR, "unknown signal name {}", signal_name_or_value) + Err(USimpleError::new( + 1, + format!("unknown signal name {}", signal_name_or_value), + )) } fn print_signals() { @@ -170,30 +176,41 @@ fn print_signals() { } } -fn list(arg: Option) { +fn list(arg: Option) -> UResult<()> { match arg { Some(ref x) => print_signal(x), - None => print_signals(), - }; + None => { + print_signals(); + Ok(()) + } + } } -fn kill(signalname: &str, pids: &[String]) -> i32 { - let mut status = 0; +fn kill(signalname: &str, pids: &[String]) -> UResult<()> { let optional_signal_value = uucore::signals::signal_by_name_or_value(signalname); let signal_value = match optional_signal_value { Some(x) => x, - None => crash!(EXIT_ERR, "unknown signal name {}", signalname), + None => { + return Err(USimpleError::new( + 1, + format!("unknown signal name {}", signalname), + )); + } }; for pid in pids { match pid.parse::() { Ok(x) => { if unsafe { libc::kill(x as pid_t, signal_value as c_int) } != 0 { - show_error!("{}", Error::last_os_error()); - status = 1; + show!(USimpleError::new(1, format!("{}", Error::last_os_error()))); } } - Err(e) => crash!(EXIT_ERR, "failed to parse argument {}: {}", pid, e), + Err(e) => { + return Err(USimpleError::new( + 1, + format!("failed to parse argument {}: {}", pid, e), + )); + } }; } - status + Ok(()) } diff --git a/src/uu/ln/src/ln.rs b/src/uu/ln/src/ln.rs index b08eba97a..d354acce9 100644 --- a/src/uu/ln/src/ln.rs +++ b/src/uu/ln/src/ln.rs @@ -22,6 +22,7 @@ use std::os::unix::fs::symlink; #[cfg(windows)] use std::os::windows::fs::{symlink_dir, symlink_file}; use std::path::{Path, PathBuf}; +use uucore::backup_control::{self, BackupMode}; use uucore::fs::{canonicalize, CanonicalizeMode}; pub struct Settings { @@ -43,14 +44,6 @@ pub enum OverwriteMode { Force, } -#[derive(Clone, Debug, Eq, PartialEq)] -pub enum BackupMode { - NoBackup, - SimpleBackup, - NumberedBackup, - ExistingBackup, -} - fn get_usage() -> String { format!( "{0} [OPTION]... [-T] TARGET LINK_NAME (1st form) @@ -78,7 +71,7 @@ fn get_long_usage() -> String { static ABOUT: &str = "change file owner and group"; mod options { - pub const B: &str = "b"; + pub const BACKUP_NO_ARG: &str = "b"; pub const BACKUP: &str = "backup"; pub const FORCE: &str = "force"; pub const INTERACTIVE: &str = "interactive"; @@ -99,7 +92,11 @@ pub fn uumain(args: impl uucore::Args) -> i32 { let matches = uu_app() .usage(&usage[..]) - .after_help(&long_usage[..]) + .after_help(&*format!( + "{}\n{}", + long_usage, + backup_control::BACKUP_CONTROL_LONG_HELP + )) .get_matches_from(args); /* the list of files */ @@ -118,33 +115,25 @@ pub fn uumain(args: impl uucore::Args) -> i32 { OverwriteMode::NoClobber }; - let backup_mode = if matches.is_present(options::B) { - BackupMode::ExistingBackup - } else if matches.is_present(options::BACKUP) { - match matches.value_of(options::BACKUP) { - None => BackupMode::ExistingBackup, - Some(mode) => match mode { - "simple" | "never" => BackupMode::SimpleBackup, - "numbered" | "t" => BackupMode::NumberedBackup, - "existing" | "nil" => BackupMode::ExistingBackup, - "none" | "off" => BackupMode::NoBackup, - _ => panic!(), // cannot happen as it is managed by clap - }, + let backup_mode = backup_control::determine_backup_mode( + matches.is_present(options::BACKUP_NO_ARG), + matches.is_present(options::BACKUP), + matches.value_of(options::BACKUP), + ); + let backup_mode = match backup_mode { + Err(err) => { + show_usage_error!("{}", err); + return 1; } - } else { - BackupMode::NoBackup + Ok(mode) => mode, }; - let backup_suffix = if matches.is_present(options::SUFFIX) { - matches.value_of(options::SUFFIX).unwrap() - } else { - "~" - }; + let backup_suffix = backup_control::determine_backup_suffix(matches.value_of(options::SUFFIX)); let settings = Settings { overwrite: overwrite_mode, backup: backup_mode, - suffix: backup_suffix.to_string(), + suffix: backup_suffix, symbolic: matches.is_present(options::SYMBOLIC), relative: matches.is_present(options::RELATIVE), target_dir: matches @@ -162,22 +151,19 @@ pub fn uu_app() -> App<'static, 'static> { App::new(executable!()) .version(crate_version!()) .about(ABOUT) - .arg(Arg::with_name(options::B).short(options::B).help( - "make a backup of each file that would otherwise be overwritten or \ - removed", - )) .arg( Arg::with_name(options::BACKUP) .long(options::BACKUP) - .help( - "make a backup of each file that would otherwise be overwritten \ - or removed", - ) + .help("make a backup of each existing destination file") .takes_value(true) - .possible_values(&[ - "simple", "never", "numbered", "t", "existing", "nil", "none", "off", - ]) - .value_name("METHOD"), + .require_equals(true) + .min_values(0) + .value_name("CONTROL"), + ) + .arg( + Arg::with_name(options::BACKUP_NO_ARG) + .short(options::BACKUP_NO_ARG) + .help("like --backup but does not accept an argument"), ) // TODO: opts.arg( // Arg::with_name(("d", "directory", "allow users with appropriate privileges to attempt \ diff --git a/src/uu/mv/src/mv.rs b/src/uu/mv/src/mv.rs index 4a761861f..166e8cb1a 100644 --- a/src/uu/mv/src/mv.rs +++ b/src/uu/mv/src/mv.rs @@ -86,9 +86,17 @@ pub fn uumain(args: impl uucore::Args) -> i32 { let overwrite_mode = determine_overwrite_mode(&matches); let backup_mode = backup_control::determine_backup_mode( - matches.is_present(OPT_BACKUP_NO_ARG) || matches.is_present(OPT_BACKUP), + matches.is_present(OPT_BACKUP_NO_ARG), + matches.is_present(OPT_BACKUP), matches.value_of(OPT_BACKUP), ); + let backup_mode = match backup_mode { + Err(err) => { + show_usage_error!("{}", err); + return 1; + } + Ok(mode) => mode, + }; if overwrite_mode == OverwriteMode::NoClobber && backup_mode != BackupMode::NoBackup { show_usage_error!("options --backup and --no-clobber are mutually exclusive"); @@ -135,7 +143,6 @@ pub fn uu_app() -> App<'static, 'static> { .takes_value(true) .require_equals(true) .min_values(0) - .possible_values(backup_control::BACKUP_CONTROL_VALUES) .value_name("CONTROL") ) .arg( diff --git a/src/uu/pwd/src/pwd.rs b/src/uu/pwd/src/pwd.rs index 764a63a88..c72cc64e2 100644 --- a/src/uu/pwd/src/pwd.rs +++ b/src/uu/pwd/src/pwd.rs @@ -5,6 +5,9 @@ // * For the full copyright and license information, please view the LICENSE // * file that was distributed with this source code. +// clippy bug https://github.com/rust-lang/rust-clippy/issues/7422 +#![allow(clippy::nonstandard_macro_braces)] + #[macro_use] extern crate uucore; @@ -13,6 +16,8 @@ use std::env; use std::io; use std::path::{Path, PathBuf}; +use uucore::error::{FromIo, UResult, USimpleError}; + static ABOUT: &str = "Display the full filename of the current working directory."; static OPT_LOGICAL: &str = "logical"; static OPT_PHYSICAL: &str = "physical"; @@ -36,7 +41,8 @@ fn get_usage() -> String { format!("{0} [OPTION]... FILE...", executable!()) } -pub fn uumain(args: impl uucore::Args) -> i32 { +#[uucore_procs::gen_uumain] +pub fn uumain(args: impl uucore::Args) -> UResult<()> { let usage = get_usage(); let matches = uu_app().usage(&usage[..]).get_matches_from(args); @@ -46,16 +52,20 @@ pub fn uumain(args: impl uucore::Args) -> i32 { if matches.is_present(OPT_LOGICAL) { println!("{}", logical_path.display()); } else { - match absolute_path(&logical_path) { - Ok(physical_path) => println!("{}", physical_path.display()), - Err(e) => crash!(1, "failed to get absolute path {}", e), - }; + let physical_path = absolute_path(&logical_path) + .map_err_context(|| "failed to get absolute path".to_string())?; + println!("{}", physical_path.display()); } } - Err(e) => crash!(1, "failed to get current directory {}", e), + Err(e) => { + return Err(USimpleError::new( + 1, + format!("failed to get current directory {}", e), + )) + } }; - 0 + Ok(()) } pub fn uu_app() -> App<'static, 'static> { diff --git a/src/uu/sleep/src/sleep.rs b/src/uu/sleep/src/sleep.rs index ada3336df..1cb858a34 100644 --- a/src/uu/sleep/src/sleep.rs +++ b/src/uu/sleep/src/sleep.rs @@ -5,12 +5,17 @@ // * For the full copyright and license information, please view the LICENSE // * file that was distributed with this source code. +// clippy bug https://github.com/rust-lang/rust-clippy/issues/7422 +#![allow(clippy::nonstandard_macro_braces)] + #[macro_use] extern crate uucore; use std::thread; use std::time::Duration; +use uucore::error::{UResult, USimpleError}; + use clap::{crate_version, App, Arg}; static ABOUT: &str = "Pause for NUMBER seconds."; @@ -32,17 +37,18 @@ fn get_usage() -> String { ) } -pub fn uumain(args: impl uucore::Args) -> i32 { +#[uucore_procs::gen_uumain] +pub fn uumain(args: impl uucore::Args) -> UResult<()> { let usage = get_usage(); let matches = uu_app().usage(&usage[..]).get_matches_from(args); if let Some(values) = matches.values_of(options::NUMBER) { let numbers = values.collect(); - sleep(numbers); + return sleep(numbers); } - 0 + Ok(()) } pub fn uu_app() -> App<'static, 'static> { @@ -61,15 +67,15 @@ pub fn uu_app() -> App<'static, 'static> { ) } -fn sleep(args: Vec<&str>) { +fn sleep(args: Vec<&str>) -> UResult<()> { let sleep_dur = - args.iter().fold( + args.iter().try_fold( Duration::new(0, 0), |result, arg| match uucore::parse_time::from_str(&arg[..]) { - Ok(m) => m + result, - Err(f) => crash!(1, "{}", f), + Ok(m) => Ok(m + result), + Err(f) => Err(USimpleError::new(1, f)), }, - ); - + )?; thread::sleep(sleep_dur); + Ok(()) } diff --git a/src/uu/true/src/true.rs b/src/uu/true/src/true.rs index ea53b0075..e6b7b9025 100644 --- a/src/uu/true/src/true.rs +++ b/src/uu/true/src/true.rs @@ -5,12 +5,20 @@ // * For the full copyright and license information, please view the LICENSE // * file that was distributed with this source code. +// Clippy bug: https://github.com/rust-lang/rust-clippy/issues/7422 +#![allow(clippy::nonstandard_macro_braces)] + +#[macro_use] +extern crate uucore; + use clap::App; +use uucore::error::UResult; use uucore::executable; -pub fn uumain(args: impl uucore::Args) -> i32 { +#[uucore_procs::gen_uumain] +pub fn uumain(args: impl uucore::Args) -> UResult<()> { uu_app().get_matches_from(args); - 0 + Ok(()) } pub fn uu_app() -> App<'static, 'static> { diff --git a/src/uucore/Cargo.toml b/src/uucore/Cargo.toml index 9ded42bbd..d62d89393 100644 --- a/src/uucore/Cargo.toml +++ b/src/uucore/Cargo.toml @@ -30,6 +30,10 @@ time = { version="<= 0.1.43", optional=true } data-encoding = { version="~2.1", optional=true } ## data-encoding: require v2.1; but v2.2.0 breaks the build for MinSRV v1.31.0 libc = { version="0.2.15, <= 0.2.85", optional=true } ## libc: initial utmp support added in v0.2.15; but v0.2.68 breaks the build for MinSRV v1.31.0 +[dev-dependencies] +clap = "2.33.3" +lazy_static = "1.3" + [target.'cfg(target_os = "windows")'.dependencies] winapi = { version = "0.3", features = ["errhandlingapi", "fileapi", "handleapi", "winerror"] } diff --git a/src/uucore/src/lib/mods/backup_control.rs b/src/uucore/src/lib/mods/backup_control.rs index b8f389c83..6fa48d308 100644 --- a/src/uucore/src/lib/mods/backup_control.rs +++ b/src/uucore/src/lib/mods/backup_control.rs @@ -7,19 +7,15 @@ pub static BACKUP_CONTROL_VALUES: &[&str] = &[ "simple", "never", "numbered", "t", "existing", "nil", "none", "off", ]; -pub static BACKUP_CONTROL_LONG_HELP: &str = "The backup suffix is '~', unless set with --suffix or SIMPLE_BACKUP_SUFFIX. Here are the version control values: +pub static BACKUP_CONTROL_LONG_HELP: &str = + "The backup suffix is '~', unless set with --suffix or SIMPLE_BACKUP_SUFFIX. +The version control method may be selected via the --backup option or through +the VERSION_CONTROL environment variable. Here are the values: -none, off - never make backups (even if --backup is given) - -numbered, t - make numbered backups - -existing, nil - numbered if numbered backups exist, simple otherwise - -simple, never - always make simple backups"; + none, off never make backups (even if --backup is given) + numbered, t make numbered backups + existing, nil numbered if numbered backups exist, simple otherwise + simple, never always make simple backups"; #[derive(Debug, Clone, Copy, Eq, PartialEq)] pub enum BackupMode { @@ -37,35 +33,177 @@ pub fn determine_backup_suffix(supplied_suffix: Option<&str>) -> String { } } -/// # TODO +/// Determine the "mode" for the backup operation to perform, if any. /// -/// This function currently deviates slightly from how the [manual][1] describes -/// that it should work. In particular, the current implementation: +/// Parses the backup options according to the [GNU manual][1], and converts +/// them to an instance of `BackupMode` for further processing. /// -/// 1. Doesn't strictly respect the order in which to determine the backup type, -/// which is (in order of precedence) -/// 1. Take a valid value to the '--backup' option -/// 2. Take the value of the `VERSION_CONTROL` env var -/// 3. default to 'existing' -/// 2. Doesn't accept abbreviations to the 'backup_option' parameter +/// For an explanation of what the arguments mean, refer to the examples below. /// /// [1]: https://www.gnu.org/software/coreutils/manual/html_node/Backup-options.html -pub fn determine_backup_mode(backup_opt_exists: bool, backup_opt: Option<&str>) -> BackupMode { - if backup_opt_exists { - match backup_opt.map(String::from) { - // default is existing, see: - // https://www.gnu.org/software/coreutils/manual/html_node/Backup-options.html - None => BackupMode::ExistingBackup, - Some(mode) => match &mode[..] { - "simple" | "never" => BackupMode::SimpleBackup, - "numbered" | "t" => BackupMode::NumberedBackup, - "existing" | "nil" => BackupMode::ExistingBackup, - "none" | "off" => BackupMode::NoBackup, - _ => panic!(), // cannot happen as it is managed by clap - }, +/// +/// +/// # Errors +/// +/// If an argument supplied directly to the long `backup` option, or read in +/// through the `VERSION CONTROL` env var is ambiguous (i.e. may resolve to +/// multiple backup modes) or invalid, an error is returned. The error contains +/// the formatted error string which may then be passed to the +/// [`show_usage_error`] macro. +/// +/// +/// # Examples +/// +/// Here's how one would integrate the backup mode determination into an +/// application. +/// +/// ``` +/// #[macro_use] +/// extern crate uucore; +/// use uucore::backup_control::{self, BackupMode}; +/// use clap::{App, Arg}; +/// +/// fn main() { +/// let OPT_BACKUP: &str = "backup"; +/// let OPT_BACKUP_NO_ARG: &str = "b"; +/// let matches = App::new("app") +/// .arg(Arg::with_name(OPT_BACKUP_NO_ARG) +/// .short(OPT_BACKUP_NO_ARG)) +/// .arg(Arg::with_name(OPT_BACKUP) +/// .long(OPT_BACKUP) +/// .takes_value(true) +/// .require_equals(true) +/// .min_values(0)) +/// .get_matches_from(vec![ +/// "app", "-b", "--backup=t" +/// ]); +/// +/// let backup_mode = backup_control::determine_backup_mode( +/// matches.is_present(OPT_BACKUP_NO_ARG), matches.is_present(OPT_BACKUP), +/// matches.value_of(OPT_BACKUP) +/// ); +/// let backup_mode = match backup_mode { +/// Err(err) => { +/// show_usage_error!("{}", err); +/// return; +/// }, +/// Ok(mode) => mode, +/// }; +/// } +/// ``` +/// +/// This example shows an ambiguous input, as 'n' may resolve to 4 different +/// backup modes. +/// +/// +/// ``` +/// #[macro_use] +/// extern crate uucore; +/// use uucore::backup_control::{self, BackupMode}; +/// use clap::{crate_version, App, Arg, ArgMatches}; +/// +/// fn main() { +/// let OPT_BACKUP: &str = "backup"; +/// let OPT_BACKUP_NO_ARG: &str = "b"; +/// let matches = App::new("app") +/// .arg(Arg::with_name(OPT_BACKUP_NO_ARG) +/// .short(OPT_BACKUP_NO_ARG)) +/// .arg(Arg::with_name(OPT_BACKUP) +/// .long(OPT_BACKUP) +/// .takes_value(true) +/// .require_equals(true) +/// .min_values(0)) +/// .get_matches_from(vec![ +/// "app", "-b", "--backup=n" +/// ]); +/// +/// let backup_mode = backup_control::determine_backup_mode( +/// matches.is_present(OPT_BACKUP_NO_ARG), matches.is_present(OPT_BACKUP), +/// matches.value_of(OPT_BACKUP) +/// ); +/// let backup_mode = match backup_mode { +/// Err(err) => { +/// show_usage_error!("{}", err); +/// return; +/// }, +/// Ok(mode) => mode, +/// }; +/// } +/// ``` +pub fn determine_backup_mode( + short_opt_present: bool, + long_opt_present: bool, + long_opt_value: Option<&str>, +) -> Result { + if long_opt_present { + // Use method to determine the type of backups to make. When this option + // is used but method is not specified, then the value of the + // VERSION_CONTROL environment variable is used. And if VERSION_CONTROL + // is not set, the default backup type is ‘existing’. + if let Some(method) = long_opt_value { + // Second argument is for the error string that is returned. + match_method(method, "backup type") + } else if let Ok(method) = env::var("VERSION_CONTROL") { + // Second argument is for the error string that is returned. + match_method(&method, "$VERSION_CONTROL") + } else { + Ok(BackupMode::ExistingBackup) + } + } else if short_opt_present { + // the short form of this option, -b does not accept any argument. + // Using -b is equivalent to using --backup=existing. + Ok(BackupMode::ExistingBackup) + } else { + // No option was present at all + Ok(BackupMode::NoBackup) + } +} + +/// Match a backup option string to a `BackupMode`. +/// +/// The GNU manual specifies that abbreviations to options are valid as long as +/// they aren't ambiguous. This function matches the given `method` argument +/// against all valid backup options (via `starts_with`), and returns a valid +/// [`BackupMode`] if exactly one backup option matches the `method` given. +/// +/// `origin` is required in order to format the generated error message +/// properly, when an error occurs. +/// +/// +/// # Errors +/// +/// If `method` is ambiguous (i.e. may resolve to multiple backup modes) or +/// invalid, an error is returned. The error contains the formatted error string +/// which may then be passed to the [`show_usage_error`] macro. +fn match_method(method: &str, origin: &str) -> Result { + let matches: Vec<&&str> = BACKUP_CONTROL_VALUES + .iter() + .filter(|val| val.starts_with(method)) + .collect(); + if matches.len() == 1 { + match *matches[0] { + "simple" | "never" => Ok(BackupMode::SimpleBackup), + "numbered" | "t" => Ok(BackupMode::NumberedBackup), + "existing" | "nil" => Ok(BackupMode::ExistingBackup), + "none" | "off" => Ok(BackupMode::NoBackup), + _ => unreachable!(), // cannot happen as we must have exactly one match + // from the list above. } } else { - BackupMode::NoBackup + let error_type = if matches.is_empty() { + "invalid" + } else { + "ambiguous" + }; + Err(format!( + "{0} argument ‘{1}’ for ‘{2}’ +Valid arguments are: + - ‘none’, ‘off’ + - ‘simple’, ‘never’ + - ‘existing’, ‘nil’ + - ‘numbered’, ‘t’", + error_type, method, origin + )) } } @@ -82,13 +220,13 @@ pub fn get_backup_path( } } -pub fn simple_backup_path(path: &Path, suffix: &str) -> PathBuf { +fn simple_backup_path(path: &Path, suffix: &str) -> PathBuf { let mut p = path.to_string_lossy().into_owned(); p.push_str(suffix); PathBuf::from(p) } -pub fn numbered_backup_path(path: &Path) -> PathBuf { +fn numbered_backup_path(path: &Path) -> PathBuf { for i in 1_u64.. { let path_str = &format!("{}.~{}~", path.to_string_lossy(), i); let path = Path::new(path_str); @@ -99,7 +237,7 @@ pub fn numbered_backup_path(path: &Path) -> PathBuf { panic!("cannot create backup") } -pub fn existing_backup_path(path: &Path, suffix: &str) -> PathBuf { +fn existing_backup_path(path: &Path, suffix: &str) -> PathBuf { let test_path_str = &format!("{}.~1~", path.to_string_lossy()); let test_path = Path::new(test_path_str); if test_path.exists() { @@ -108,3 +246,210 @@ pub fn existing_backup_path(path: &Path, suffix: &str) -> PathBuf { simple_backup_path(path, suffix) } } + +// +// Tests for this module +// +#[cfg(test)] +mod tests { + use super::*; + use std::env; + // Required to instantiate mutex in shared context + use lazy_static::lazy_static; + use std::sync::Mutex; + + // The mutex is required here as by default all tests are run as separate + // threads under the same parent process. As environment variables are + // specific to processes (and thus shared among threads), data races *will* + // occur if no precautions are taken. Thus we have all tests that rely on + // environment variables lock this empty mutex to ensure they don't access + // it concurrently. + lazy_static! { + static ref TEST_MUTEX: Mutex<()> = Mutex::new(()); + } + + // Environment variable for "VERSION_CONTROL" + static ENV_VERSION_CONTROL: &str = "VERSION_CONTROL"; + + // Defaults to --backup=existing + #[test] + fn test_backup_mode_short_only() { + let short_opt_present = true; + let long_opt_present = false; + let long_opt_value = None; + let _dummy = TEST_MUTEX.lock().unwrap(); + + let result = + determine_backup_mode(short_opt_present, long_opt_present, long_opt_value).unwrap(); + + assert_eq!(result, BackupMode::ExistingBackup); + } + + // --backup takes precedence over -b + #[test] + fn test_backup_mode_long_preferred_over_short() { + let short_opt_present = true; + let long_opt_present = true; + let long_opt_value = Some("none"); + let _dummy = TEST_MUTEX.lock().unwrap(); + + let result = + determine_backup_mode(short_opt_present, long_opt_present, long_opt_value).unwrap(); + + assert_eq!(result, BackupMode::NoBackup); + } + + // --backup can be passed without an argument + #[test] + fn test_backup_mode_long_without_args_no_env() { + let short_opt_present = false; + let long_opt_present = true; + let long_opt_value = None; + let _dummy = TEST_MUTEX.lock().unwrap(); + + let result = + determine_backup_mode(short_opt_present, long_opt_present, long_opt_value).unwrap(); + + assert_eq!(result, BackupMode::ExistingBackup); + } + + // --backup can be passed with an argument only + #[test] + fn test_backup_mode_long_with_args() { + let short_opt_present = false; + let long_opt_present = true; + let long_opt_value = Some("simple"); + let _dummy = TEST_MUTEX.lock().unwrap(); + + let result = + determine_backup_mode(short_opt_present, long_opt_present, long_opt_value).unwrap(); + + assert_eq!(result, BackupMode::SimpleBackup); + } + + // --backup errors on invalid argument + #[test] + fn test_backup_mode_long_with_args_invalid() { + let short_opt_present = false; + let long_opt_present = true; + let long_opt_value = Some("foobar"); + let _dummy = TEST_MUTEX.lock().unwrap(); + + let result = determine_backup_mode(short_opt_present, long_opt_present, long_opt_value); + + assert!(result.is_err()); + let text = result.unwrap_err(); + assert!(text.contains("invalid argument ‘foobar’ for ‘backup type’")); + } + + // --backup errors on ambiguous argument + #[test] + fn test_backup_mode_long_with_args_ambiguous() { + let short_opt_present = false; + let long_opt_present = true; + let long_opt_value = Some("n"); + let _dummy = TEST_MUTEX.lock().unwrap(); + + let result = determine_backup_mode(short_opt_present, long_opt_present, long_opt_value); + + assert!(result.is_err()); + let text = result.unwrap_err(); + assert!(text.contains("ambiguous argument ‘n’ for ‘backup type’")); + } + + // --backup accepts shortened arguments (si for simple) + #[test] + fn test_backup_mode_long_with_arg_shortened() { + let short_opt_present = false; + let long_opt_present = true; + let long_opt_value = Some("si"); + let _dummy = TEST_MUTEX.lock().unwrap(); + + let result = + determine_backup_mode(short_opt_present, long_opt_present, long_opt_value).unwrap(); + + assert_eq!(result, BackupMode::SimpleBackup); + } + + // -b ignores the "VERSION_CONTROL" environment variable + #[test] + fn test_backup_mode_short_only_ignore_env() { + let short_opt_present = true; + let long_opt_present = false; + let long_opt_value = None; + let _dummy = TEST_MUTEX.lock().unwrap(); + env::set_var(ENV_VERSION_CONTROL, "none"); + + let result = + determine_backup_mode(short_opt_present, long_opt_present, long_opt_value).unwrap(); + + assert_eq!(result, BackupMode::ExistingBackup); + env::remove_var(ENV_VERSION_CONTROL); + } + + // --backup can be passed without an argument, but reads env var if existent + #[test] + fn test_backup_mode_long_without_args_with_env() { + let short_opt_present = false; + let long_opt_present = true; + let long_opt_value = None; + let _dummy = TEST_MUTEX.lock().unwrap(); + env::set_var(ENV_VERSION_CONTROL, "none"); + + let result = + determine_backup_mode(short_opt_present, long_opt_present, long_opt_value).unwrap(); + + assert_eq!(result, BackupMode::NoBackup); + env::remove_var(ENV_VERSION_CONTROL); + } + + // --backup errors on invalid VERSION_CONTROL env var + #[test] + fn test_backup_mode_long_with_env_var_invalid() { + let short_opt_present = false; + let long_opt_present = true; + let long_opt_value = None; + let _dummy = TEST_MUTEX.lock().unwrap(); + env::set_var(ENV_VERSION_CONTROL, "foobar"); + + let result = determine_backup_mode(short_opt_present, long_opt_present, long_opt_value); + + assert!(result.is_err()); + let text = result.unwrap_err(); + assert!(text.contains("invalid argument ‘foobar’ for ‘$VERSION_CONTROL’")); + env::remove_var(ENV_VERSION_CONTROL); + } + + // --backup errors on ambiguous VERSION_CONTROL env var + #[test] + fn test_backup_mode_long_with_env_var_ambiguous() { + let short_opt_present = false; + let long_opt_present = true; + let long_opt_value = None; + let _dummy = TEST_MUTEX.lock().unwrap(); + env::set_var(ENV_VERSION_CONTROL, "n"); + + let result = determine_backup_mode(short_opt_present, long_opt_present, long_opt_value); + + assert!(result.is_err()); + let text = result.unwrap_err(); + assert!(text.contains("ambiguous argument ‘n’ for ‘$VERSION_CONTROL’")); + env::remove_var(ENV_VERSION_CONTROL); + } + + // --backup accepts shortened env vars (si for simple) + #[test] + fn test_backup_mode_long_with_env_var_shortened() { + let short_opt_present = false; + let long_opt_present = true; + let long_opt_value = None; + let _dummy = TEST_MUTEX.lock().unwrap(); + env::set_var(ENV_VERSION_CONTROL, "si"); + + let result = + determine_backup_mode(short_opt_present, long_opt_present, long_opt_value).unwrap(); + + assert_eq!(result, BackupMode::SimpleBackup); + env::remove_var(ENV_VERSION_CONTROL); + } +} diff --git a/src/uucore/src/lib/mods/error.rs b/src/uucore/src/lib/mods/error.rs index ae509ff00..c64e7df66 100644 --- a/src/uucore/src/lib/mods/error.rs +++ b/src/uucore/src/lib/mods/error.rs @@ -138,6 +138,12 @@ pub enum UError { } impl UError { + /// The error code of [`UResult`] + /// + /// This function defines the error code associated with an instance of + /// [`UResult`]. To associate error codes for self-defined instances of + /// `UResult::Custom` (i.e. [`UCustomError`]), implement the + /// [`code`-function there](UCustomError::code). pub fn code(&self) -> i32 { match self { UError::Common(e) => e.code(), @@ -145,6 +151,13 @@ impl UError { } } + /// Whether to print usage help for a [`UResult`] + /// + /// Defines if a variant of [`UResult`] should print a short usage message + /// below the error. The usage message is printed when this function returns + /// `true`. To do this for self-defined instances of `UResult::Custom` (i.e. + /// [`UCustomError`]), implement the [`usage`-function + /// there](UCustomError::usage). pub fn usage(&self) -> bool { match self { UError::Common(e) => e.usage(), @@ -183,12 +196,13 @@ impl Display for UError { /// Custom errors defined by the utils. /// /// All errors should implement [`std::error::Error`], [`std::fmt::Display`] and -/// [`std::fmt::Debug`] and have an additional `code` method that specifies the exit code of the -/// program if the error is returned from `uumain`. +/// [`std::fmt::Debug`] and have an additional `code` method that specifies the +/// exit code of the program if the error is returned from `uumain`. /// /// An example of a custom error from `ls`: +/// /// ``` -/// use uucore::error::{UCustomError}; +/// use uucore::error::{UCustomError, UResult}; /// use std::{ /// error::Error, /// fmt::{Display, Debug}, @@ -221,13 +235,121 @@ impl Display for UError { /// } /// } /// ``` -/// A crate like [`quick_error`](https://crates.io/crates/quick-error) might also be used, but will -/// still require an `impl` for the `code` method. +/// +/// The main routine would look like this: +/// +/// ```ignore +/// #[uucore_procs::gen_uumain] +/// pub fn uumain(args: impl uucore::Args) -> UResult<()> { +/// // Perform computations here ... +/// return Err(LsError::InvalidLineWidth(String::from("test")).into()) +/// } +/// ``` +/// +/// The call to `into()` is required to convert the [`UCustomError`] to an +/// instance of [`UError`]. +/// +/// A crate like [`quick_error`](https://crates.io/crates/quick-error) might +/// also be used, but will still require an `impl` for the `code` method. pub trait UCustomError: Error { + /// Error code of a custom error. + /// + /// Set a return value for each variant of an enum-type to associate an + /// error code (which is returned to the system shell) with an error + /// variant. + /// + /// # Example + /// + /// ``` + /// use uucore::error::{UCustomError}; + /// use std::{ + /// error::Error, + /// fmt::{Display, Debug}, + /// path::PathBuf + /// }; + /// + /// #[derive(Debug)] + /// enum MyError { + /// Foo(String), + /// Bar(PathBuf), + /// Bing(), + /// } + /// + /// impl UCustomError for MyError { + /// fn code(&self) -> i32 { + /// match self { + /// MyError::Foo(_) => 2, + /// // All other errors yield the same error code, there's no + /// // need to list them explicitly. + /// _ => 1, + /// } + /// } + /// } + /// + /// impl Error for MyError {} + /// + /// impl Display for MyError { + /// fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + /// use MyError as ME; + /// match self { + /// ME::Foo(s) => write!(f, "Unknown Foo: '{}'", s), + /// ME::Bar(p) => write!(f, "Couldn't find Bar: '{}'", p.display()), + /// ME::Bing() => write!(f, "Exterminate!"), + /// } + /// } + /// } + /// ``` fn code(&self) -> i32 { 1 } + /// Print usage help to a custom error. + /// + /// Return true or false to control whether a short usage help is printed + /// below the error message. The usage help is in the format: "Try '{name} + /// --help' for more information." and printed only if `true` is returned. + /// + /// # Example + /// + /// ``` + /// use uucore::error::{UCustomError}; + /// use std::{ + /// error::Error, + /// fmt::{Display, Debug}, + /// path::PathBuf + /// }; + /// + /// #[derive(Debug)] + /// enum MyError { + /// Foo(String), + /// Bar(PathBuf), + /// Bing(), + /// } + /// + /// impl UCustomError for MyError { + /// fn usage(&self) -> bool { + /// match self { + /// // This will have a short usage help appended + /// MyError::Bar(_) => true, + /// // These matches won't have a short usage help appended + /// _ => false, + /// } + /// } + /// } + /// + /// impl Error for MyError {} + /// + /// impl Display for MyError { + /// fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + /// use MyError as ME; + /// match self { + /// ME::Foo(s) => write!(f, "Unknown Foo: '{}'", s), + /// ME::Bar(p) => write!(f, "Couldn't find Bar: '{}'", p.display()), + /// ME::Bing() => write!(f, "Exterminate!"), + /// } + /// } + /// } + /// ``` fn usage(&self) -> bool { false } diff --git a/tests/by-util/test_du.rs b/tests/by-util/test_du.rs index 607d5dc45..efc097773 100644 --- a/tests/by-util/test_du.rs +++ b/tests/by-util/test_du.rs @@ -398,8 +398,7 @@ fn test_du_time() { let result = ts.ucmd().arg("--time=ctime").arg("date_test").succeeds(); result.stdout_only("0\t2016-06-16 00:00\tdate_test\n"); - #[cfg(not(target_env = "musl"))] - { + if birth_supported() { use regex::Regex; let re_birth = @@ -409,6 +408,16 @@ fn test_du_time() { } } +#[cfg(feature = "touch")] +fn birth_supported() -> bool { + let ts = TestScenario::new(util_name!()); + let m = match std::fs::metadata(ts.fixtures.subdir) { + Ok(m) => m, + Err(e) => panic!("{}", e), + }; + m.created().is_ok() +} + #[cfg(not(target_os = "windows"))] #[cfg(feature = "chmod")] #[test] diff --git a/tests/by-util/test_sleep.rs b/tests/by-util/test_sleep.rs index a17beddf6..94a0a6896 100644 --- a/tests/by-util/test_sleep.rs +++ b/tests/by-util/test_sleep.rs @@ -110,3 +110,8 @@ fn test_sleep_sum_duration_many() { let duration = before_test.elapsed(); assert!(duration >= millis_900); } + +#[test] +fn test_sleep_wrong_time() { + new_ucmd!().args(&["0.1s", "abc"]).fails(); +}