From 3b8f1358428eab69a232b71fe8450b9e03142d93 Mon Sep 17 00:00:00 2001 From: Andreas Hartmann Date: Sat, 26 Jun 2021 20:09:01 +0200 Subject: [PATCH 01/24] backup_control: Add backup help string from GNU utils The previous help string for the backup subroutines didn't comply with the formatting found in the `--help` output of e.g. `mv` or `ln`. Use the exact help string from these utilities instead. --- src/uucore/src/lib/mods/backup_control.rs | 20 ++++++++------------ 1 file changed, 8 insertions(+), 12 deletions(-) diff --git a/src/uucore/src/lib/mods/backup_control.rs b/src/uucore/src/lib/mods/backup_control.rs index b8f389c83..4f1e2d00a 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 { From fa0b4861b91c53333dd1de72247d95c76caf1c0d Mon Sep 17 00:00:00 2001 From: Andreas Hartmann Date: Sat, 26 Jun 2021 20:13:51 +0200 Subject: [PATCH 02/24] backup_control: Match abbreviated backup options Add a function that takes a user-supplied backup option and checks if it matches any of the valid backup options. This is because GNU allows to abbreviate backup options, as long as they are valid and unambiguous. In case a backup option is either invalid or ambiguous, an error type is returned that contains a formatted error string for output to the user. --- src/uucore/src/lib/mods/backup_control.rs | 46 ++++++++++++++++++++++- 1 file changed, 45 insertions(+), 1 deletion(-) diff --git a/src/uucore/src/lib/mods/backup_control.rs b/src/uucore/src/lib/mods/backup_control.rs index 4f1e2d00a..6af2b3534 100644 --- a/src/uucore/src/lib/mods/backup_control.rs +++ b/src/uucore/src/lib/mods/backup_control.rs @@ -7,7 +7,7 @@ pub static BACKUP_CONTROL_VALUES: &[&str] = &[ "simple", "never", "numbered", "t", "existing", "nil", "none", "off", ]; -pub static BACKUP_CONTROL_LONG_HELP: &str = +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: @@ -65,6 +65,50 @@ pub fn determine_backup_mode(backup_opt_exists: bool, backup_opt: Option<&str>) } } +/// 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 x = vec!["simple", "never", "numbered", "t", + "existing", "nil", "none", "off"]; + + let matches: Vec<&&str> = x.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), + _ => panic!(), // cannot happen as we must have exactly one match + // from the list above. + } + } else { + let error_type = if matches.len() == 0 { "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)) + } +} + pub fn get_backup_path( backup_mode: BackupMode, backup_path: &Path, From 1309757d4d677a822dee4d05cb3523df7a2277f1 Mon Sep 17 00:00:00 2001 From: Andreas Hartmann Date: Sat, 26 Jun 2021 20:16:02 +0200 Subject: [PATCH 03/24] backup_control: Make utility functions private --- src/uucore/src/lib/mods/backup_control.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/uucore/src/lib/mods/backup_control.rs b/src/uucore/src/lib/mods/backup_control.rs index 6af2b3534..79dab9c9e 100644 --- a/src/uucore/src/lib/mods/backup_control.rs +++ b/src/uucore/src/lib/mods/backup_control.rs @@ -122,13 +122,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); @@ -139,7 +139,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() { From 89c6d32a20efa7a6dd7123a2f263aebc0ee83df7 Mon Sep 17 00:00:00 2001 From: Andreas Hartmann Date: Sat, 26 Jun 2021 20:16:42 +0200 Subject: [PATCH 04/24] backup_control: Refactor backup mode determination Refactor the function that determines which backup mode to select based on user input. It now complies with what the [GNU manual][1] specifies. [1]: https://www.gnu.org/software/coreutils/manual/html_node/Backup-options.html --- src/uucore/src/lib/mods/backup_control.rs | 150 +++++++++++++++++----- 1 file changed, 117 insertions(+), 33 deletions(-) diff --git a/src/uucore/src/lib/mods/backup_control.rs b/src/uucore/src/lib/mods/backup_control.rs index 79dab9c9e..4af4db636 100644 --- a/src/uucore/src/lib/mods/backup_control.rs +++ b/src/uucore/src/lib/mods/backup_control.rs @@ -8,7 +8,7 @@ pub static BACKUP_CONTROL_VALUES: &[&str] = &[ ]; pub static BACKUP_CONTROL_LONG_HELP: &str = -"The backup suffix is '~', unless set with --suffix or SIMPLE_BACKUP_SUFFIX. + "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: @@ -33,35 +33,115 @@ 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. +/// +/// ``` +/// let OPT_BACKUP: &str = "backup"; +/// let OPT_BACKUP_NO_ARG: &str = "b"; +/// let matches = App::new("myprog") +/// .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![ +/// "myprog", "-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 1; +/// }, +/// Ok(mode) => mode, +/// }; +/// ``` +/// +/// This example shows an ambiguous imput, as 'n' may resolve to 4 different +/// backup modes. +/// +/// +/// ``` +/// let OPT_BACKUP: &str = "backup"; +/// let OPT_BACKUP_NO_ARG: &str = "b"; +/// let matches = App::new("myprog") +/// .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![ +/// "myprog", "-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 1; +/// }, +/// 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 { - BackupMode::NoBackup + // No option was present at all + Ok(BackupMode::NoBackup) } } @@ -82,10 +162,8 @@ pub fn determine_backup_mode(backup_opt_exists: bool, backup_opt: Option<&str>) /// 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 x = vec!["simple", "never", "numbered", "t", - "existing", "nil", "none", "off"]; - - let matches: Vec<&&str> = x.iter() + let matches: Vec<&&str> = BACKUP_CONTROL_VALUES + .iter() .filter(|val| val.starts_with(method)) .collect(); if matches.len() == 1 { @@ -94,18 +172,24 @@ fn match_method(method: &str, origin: &str) -> Result { "numbered" | "t" => Ok(BackupMode::NumberedBackup), "existing" | "nil" => Ok(BackupMode::ExistingBackup), "none" | "off" => Ok(BackupMode::NoBackup), - _ => panic!(), // cannot happen as we must have exactly one match - // from the list above. + _ => unreachable!(), // cannot happen as we must have exactly one match + // from the list above. } } else { - let error_type = if matches.len() == 0 { "invalid" } else { "ambiguous" }; + let error_type = if matches.is_empty() { + "invalid" + } else { + "ambiguous" + }; Err(format!( -"{0} argument ‘{1}’ for ‘{2}’ + "{0} argument ‘{1}’ for ‘{2}’ Valid arguments are: - ‘none’, ‘off’ - ‘simple’, ‘never’ - ‘existing’, ‘nil’ - - ‘numbered’, ‘t’", error_type, method, origin)) + - ‘numbered’, ‘t’", + error_type, method, origin + )) } } From a783d051017711318441c30d70791be1d075fa66 Mon Sep 17 00:00:00 2001 From: Andreas Hartmann Date: Wed, 30 Jun 2021 11:16:44 +0200 Subject: [PATCH 05/24] backup_control: Add module tests Adds a tests submodule that performs tests on the `determine_backup_mode` function to ensure it handles backup options like specified by [GNU][1]. [1]: https://www.gnu.org/software/coreutils/manual/html_node/Backup-options.html --- src/uucore/src/lib/mods/backup_control.rs | 292 ++++++++++++++++++---- 1 file changed, 244 insertions(+), 48 deletions(-) diff --git a/src/uucore/src/lib/mods/backup_control.rs b/src/uucore/src/lib/mods/backup_control.rs index 4af4db636..a36988048 100644 --- a/src/uucore/src/lib/mods/backup_control.rs +++ b/src/uucore/src/lib/mods/backup_control.rs @@ -58,31 +58,38 @@ pub fn determine_backup_suffix(supplied_suffix: Option<&str>) -> String { /// application. /// /// ``` -/// let OPT_BACKUP: &str = "backup"; -/// let OPT_BACKUP_NO_ARG: &str = "b"; -/// let matches = App::new("myprog") -/// .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![ -/// "myprog", "-b", "--backup=t" -/// ]); +/// #[macro_use] +/// extern crate uucore; +/// use uucore::backup_control::{self, BackupMode}; +/// use clap::{App, Arg}; /// -/// 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 1; -/// }, -/// Ok(mode) => mode, -/// }; +/// fn main() { +/// let OPT_BACKUP: &str = "backup"; +/// let OPT_BACKUP_NO_ARG: &str = "b"; +/// let matches = App::new("myprog") +/// .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![ +/// "myprog", "-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 imput, as 'n' may resolve to 4 different @@ -90,31 +97,38 @@ pub fn determine_backup_suffix(supplied_suffix: Option<&str>) -> String { /// /// /// ``` -/// let OPT_BACKUP: &str = "backup"; -/// let OPT_BACKUP_NO_ARG: &str = "b"; -/// let matches = App::new("myprog") -/// .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![ -/// "myprog", "-b", "--backup=n" -/// ]); +/// #[macro_use] +/// extern crate uucore; +/// use uucore::backup_control::{self, BackupMode}; +/// use clap::{crate_version, App, Arg, ArgMatches}; /// -/// 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 1; -/// }, -/// Ok(mode) => mode, -/// }; +/// fn main() { +/// let OPT_BACKUP: &str = "backup"; +/// let OPT_BACKUP_NO_ARG: &str = "b"; +/// let matches = App::new("myprog") +/// .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![ +/// "myprog", "-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, @@ -232,3 +246,185 @@ 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; + + // 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 result = + determine_backup_mode(short_opt_present, long_opt_present, long_opt_value).unwrap(); + + assert!(result == BackupMode::ExistingBackup); + } + + // -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; + env::set_var(ENV_VERSION_CONTROL, "none"); + + let result = + determine_backup_mode(short_opt_present, long_opt_present, long_opt_value).unwrap(); + + assert!(result == BackupMode::ExistingBackup); + env::remove_var(ENV_VERSION_CONTROL); + } + + // --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 result = + determine_backup_mode(short_opt_present, long_opt_present, long_opt_value).unwrap(); + + assert!(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 result = + determine_backup_mode(short_opt_present, long_opt_present, long_opt_value).unwrap(); + + assert!(result == BackupMode::ExistingBackup); + } + + // --backup can be passed without an argument, but reads env var if existant + #[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; + env::set_var(ENV_VERSION_CONTROL, "none"); + + let result = + determine_backup_mode(short_opt_present, long_opt_present, long_opt_value).unwrap(); + + assert!(result == BackupMode::NoBackup); + env::remove_var(ENV_VERSION_CONTROL); + } + + // --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 result = + determine_backup_mode(short_opt_present, long_opt_present, long_opt_value).unwrap(); + + assert!(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 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 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 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; + 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; + 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 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 result = + determine_backup_mode(short_opt_present, long_opt_present, long_opt_value).unwrap(); + + assert!(result == BackupMode::SimpleBackup); + } + + // --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; + env::set_var(ENV_VERSION_CONTROL, "si"); + + let result = + determine_backup_mode(short_opt_present, long_opt_present, long_opt_value).unwrap(); + + assert!(result == BackupMode::SimpleBackup); + env::remove_var(ENV_VERSION_CONTROL); + } +} From 9d94307880b4dd51b9424461fe8913e7d50c18fd Mon Sep 17 00:00:00 2001 From: Andreas Hartmann Date: Sat, 26 Jun 2021 20:18:07 +0200 Subject: [PATCH 06/24] mv: Adapt for new backup_control utilities --- src/uu/mv/src/mv.rs | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) 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( From 6c26976edb0c31218edd103ef80a451fd6b62250 Mon Sep 17 00:00:00 2001 From: Son Nguyen Date: Mon, 12 Jul 2021 11:20:23 -0700 Subject: [PATCH 07/24] sleep: use UResult (#2492) * sleep: use UResult in util * sleep: add in error + test for it * sleep: UResult - removed some verbosity --- src/uu/sleep/src/sleep.rs | 21 ++++++++++++--------- tests/by-util/test_sleep.rs | 7 +++++++ 2 files changed, 19 insertions(+), 9 deletions(-) diff --git a/src/uu/sleep/src/sleep.rs b/src/uu/sleep/src/sleep.rs index ada3336df..684f98c95 100644 --- a/src/uu/sleep/src/sleep.rs +++ b/src/uu/sleep/src/sleep.rs @@ -11,6 +11,8 @@ 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 +34,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 +64,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, format!("{}", f))), }, - ); + )?; - thread::sleep(sleep_dur); + Ok(thread::sleep(sleep_dur)) } diff --git a/tests/by-util/test_sleep.rs b/tests/by-util/test_sleep.rs index a17beddf6..bb42d7974 100644 --- a/tests/by-util/test_sleep.rs +++ b/tests/by-util/test_sleep.rs @@ -110,3 +110,10 @@ 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(); +} From 337d257e8dbb4b1b58f6c7bbbe7bb02e6ad1179d Mon Sep 17 00:00:00 2001 From: Terts Diepraam Date: Mon, 12 Jul 2021 20:21:20 +0200 Subject: [PATCH 08/24] tests/sleep: fmt --- tests/by-util/test_sleep.rs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/tests/by-util/test_sleep.rs b/tests/by-util/test_sleep.rs index bb42d7974..94a0a6896 100644 --- a/tests/by-util/test_sleep.rs +++ b/tests/by-util/test_sleep.rs @@ -113,7 +113,5 @@ fn test_sleep_sum_duration_many() { #[test] fn test_sleep_wrong_time() { - new_ucmd!() - .args(&["0.1s", "abc"]) - .fails(); + new_ucmd!().args(&["0.1s", "abc"]).fails(); } From c48e623a80dac9348071a172a210e9e674b01b7a Mon Sep 17 00:00:00 2001 From: 353fc443 <353fc443@pm.me> Date: Tue, 13 Jul 2021 06:56:27 +0000 Subject: [PATCH 09/24] false: addding uresult Related to #2464 --- src/uu/false/src/false.rs | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) 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> { From 5730c8693f2bb01289adc8a1deaae49902df353e Mon Sep 17 00:00:00 2001 From: 353fc443 <353fc443@pm.me> Date: Tue, 13 Jul 2021 07:25:18 +0000 Subject: [PATCH 10/24] sleep: fixing clippy warnings --- src/uu/sleep/src/sleep.rs | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/src/uu/sleep/src/sleep.rs b/src/uu/sleep/src/sleep.rs index 684f98c95..1cb858a34 100644 --- a/src/uu/sleep/src/sleep.rs +++ b/src/uu/sleep/src/sleep.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; @@ -70,9 +73,9 @@ fn sleep(args: Vec<&str>) -> UResult<()> { Duration::new(0, 0), |result, arg| match uucore::parse_time::from_str(&arg[..]) { Ok(m) => Ok(m + result), - Err(f) => Err(USimpleError::new(1, format!("{}", f))), + Err(f) => Err(USimpleError::new(1, f)), }, )?; - - Ok(thread::sleep(sleep_dur)) + thread::sleep(sleep_dur); + Ok(()) } From 2845dd3140e37748cfa0a72a386108a5108d0a76 Mon Sep 17 00:00:00 2001 From: 353fc443 <353fc443@pm.me> Date: Tue, 13 Jul 2021 08:55:33 +0000 Subject: [PATCH 11/24] echo: adding uresult Related to #2464 --- src/uu/echo/src/echo.rs | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) 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> { From 425de97650a35034d215ed9dfef2364f4af2175d Mon Sep 17 00:00:00 2001 From: 353fc443 <353fc443@pm.me> Date: Tue, 13 Jul 2021 09:10:47 +0000 Subject: [PATCH 12/24] pwd: added uresult Related to #2464 --- src/uu/pwd/src/pwd.rs | 24 +++++++++++++++++------- 1 file changed, 17 insertions(+), 7 deletions(-) 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> { From 7d94121b95865cfef74c9480b5c8d0ba4af1cff9 Mon Sep 17 00:00:00 2001 From: 353fc443 <353fc443@pm.me> Date: Tue, 13 Jul 2021 09:49:39 +0000 Subject: [PATCH 13/24] kill: added uresult Related to #2464 --- src/uu/kill/src/kill.rs | 58 +++++++++++++++++++++++++---------------- 1 file changed, 36 insertions(+), 22 deletions(-) diff --git a/src/uu/kill/src/kill.rs b/src/uu/kill/src/kill.rs index 92868efdb..672703f62 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(); @@ -69,10 +71,10 @@ pub fn uumain(args: impl uucore::Args) -> i32 { return kill(&sig, &pids_or_signals); } Mode::Table => table(), - Mode::List => list(pids_or_signals.get(0).cloned()), + Mode::List => list(pids_or_signals.get(0).cloned())?, } - EXIT_OK + Ok(()) } pub fn uu_app() -> App<'static, 'static> { @@ -142,20 +144,23 @@ fn table() { 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() { +fn print_signals() -> UResult<()> { let mut pos = 0; for (idx, signal) in ALL_SIGNALS.iter().enumerate() { pos += signal.len(); @@ -168,32 +173,41 @@ fn print_signals() { print!(" "); } } + Ok(()) } -fn list(arg: Option) { +fn list(arg: Option) -> UResult<()> { match arg { - Some(ref x) => print_signal(x), - None => print_signals(), - }; + Some(ref x) => Ok(print_signal(x)?), + None => Ok(print_signals()?), + } } -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; + return Err(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(()) } From bd45fe26dcce50b6de00bc81a112249a8eaf5806 Mon Sep 17 00:00:00 2001 From: 353fc443 <353fc443@pm.me> Date: Tue, 13 Jul 2021 13:47:06 +0000 Subject: [PATCH 14/24] true: added uresult Related to #2464 --- src/uu/true/src/true.rs | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) 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> { From a6b0cf29e047934ad50382d8259cadf4e12b4b17 Mon Sep 17 00:00:00 2001 From: 353fc443 <353fc443@pm.me> Date: Tue, 13 Jul 2021 14:27:00 +0000 Subject: [PATCH 15/24] kill: added suggestions --- src/uu/kill/src/kill.rs | 25 ++++++++++++++----------- 1 file changed, 14 insertions(+), 11 deletions(-) diff --git a/src/uu/kill/src/kill.rs b/src/uu/kill/src/kill.rs index 672703f62..88eb08fa7 100644 --- a/src/uu/kill/src/kill.rs +++ b/src/uu/kill/src/kill.rs @@ -68,13 +68,14 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> { (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(), - Mode::List => list(pids_or_signals.get(0).cloned())?, + Mode::Table => { + table(); + Ok(()) + } + Mode::List => list(pids_or_signals.get(0).cloned()), } - - Ok(()) } pub fn uu_app() -> App<'static, 'static> { @@ -141,7 +142,7 @@ fn table() { println!(); } } - println!() + println!(); } fn print_signal(signal_name_or_value: &str) -> UResult<()> { @@ -160,7 +161,7 @@ fn print_signal(signal_name_or_value: &str) -> UResult<()> { )) } -fn print_signals() -> UResult<()> { +fn print_signals() { let mut pos = 0; for (idx, signal) in ALL_SIGNALS.iter().enumerate() { pos += signal.len(); @@ -173,13 +174,15 @@ fn print_signals() -> UResult<()> { print!(" "); } } - Ok(()) } fn list(arg: Option) -> UResult<()> { match arg { - Some(ref x) => Ok(print_signal(x)?), - None => Ok(print_signals()?), + Some(ref x) => print_signal(x), + None => { + print_signals(); + Ok(()) + } } } @@ -198,7 +201,7 @@ fn kill(signalname: &str, pids: &[String]) -> UResult<()> { match pid.parse::() { Ok(x) => { if unsafe { libc::kill(x as pid_t, signal_value as c_int) } != 0 { - return Err(USimpleError::new(1, format!("{}", Error::last_os_error()))); + show!(USimpleError::new(1, format!("{}", Error::last_os_error()))); } } Err(e) => { From 2d74d052fc4d1af79bbdcadcb1979b436cbd0369 Mon Sep 17 00:00:00 2001 From: Michael Debertol Date: Fri, 16 Jul 2021 19:54:41 +0200 Subject: [PATCH 16/24] Mention the GNU manual in the contributing guide --- CONTRIBUTING.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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. From 3c3daf502317b741e7ceae3ccfeeff118b0ee907 Mon Sep 17 00:00:00 2001 From: 353fc443 <353fc443@pm.me> Date: Sat, 17 Jul 2021 08:40:27 +0000 Subject: [PATCH 17/24] chown: added UResult --- src/uu/chown/src/chown.rs | 45 +++++++++++++++++++++++---------------- 1 file changed, 27 insertions(+), 18 deletions(-) diff --git a/src/uu/chown/src/chown.rs b/src/uu/chown/src/chown.rs index e1d3ff22b..68e7e24aa 100644 --- a/src/uu/chown/src/chown.rs +++ b/src/uu/chown/src/chown.rs @@ -7,6 +7,9 @@ // spell-checker:ignore (ToDO) COMFOLLOW Chowner Passwd RFILE RFILE's derefer dgid duid +// Clippy bug: https://github.com/rust-lang/rust-clippy/issues/7422 +#![allow(clippy::nonstandard_macro_braces)] + #[macro_use] extern crate uucore; pub use uucore::entries::{self, Group, Locate, Passwd}; @@ -14,6 +17,8 @@ use uucore::fs::resolve_relative_path; use uucore::libc::{gid_t, uid_t}; use uucore::perms::{wrap_chown, Verbosity}; +use uucore::error::{UError, UResult, USimpleError}; + use clap::{crate_version, App, Arg}; use walkdir::WalkDir; @@ -66,7 +71,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(); @@ -87,9 +93,9 @@ pub fn uumain(args: impl uucore::Args) -> i32 { let preserve_root = matches.is_present(options::preserve_root::PRESERVE); let mut derefer = if matches.is_present(options::dereference::NO_DEREFERENCE) { - 1 + Err(UError::from(1)) } else { - 0 + Ok(()) }; let mut bit_flag = if matches.is_present(options::traverse::TRAVERSE) { @@ -103,11 +109,13 @@ pub fn uumain(args: impl uucore::Args) -> i32 { let recursive = matches.is_present(options::RECURSIVE); if recursive { if bit_flag == FTS_PHYSICAL { - if derefer == 1 { - show_error!("-R --dereference requires -H or -L"); - return 1; + if derefer.is_err() { + return Err(USimpleError::new( + 1, + "-R --dereference requires -H or -L".to_string(), + )); } - derefer = 0; + derefer = Ok(()); } } else { bit_flag = FTS_PHYSICAL; @@ -131,10 +139,7 @@ pub fn uumain(args: impl uucore::Args) -> i32 { 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; - } + Err(e) => return Err(USimpleError::new(1, e)), } } else { IfFrom::All @@ -149,8 +154,10 @@ pub fn uumain(args: impl uucore::Args) -> i32 { dest_uid = Some(meta.uid()); } Err(e) => { - show_error!("failed to get attributes of '{}': {}", file, e); - return 1; + return Err(USimpleError::new( + 1, + format!("failed to get attributes of '{}': {}", file, e), + )); } } } else { @@ -160,8 +167,7 @@ pub fn uumain(args: impl uucore::Args) -> i32 { dest_gid = g; } Err(e) => { - show_error!("{}", e); - return 1; + return Err(USimpleError::new(1, e)); } } } @@ -171,7 +177,7 @@ pub fn uumain(args: impl uucore::Args) -> i32 { dest_gid, verbosity, recursive, - dereference: derefer != 0, + dereference: derefer.is_ok(), filter, preserve_root, files, @@ -330,12 +336,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 { From e9174f7d820c6ead4dcb18e74069d9e30921b8b9 Mon Sep 17 00:00:00 2001 From: 353fc443 <353fc443@pm.me> Date: Sat, 17 Jul 2021 14:14:23 +0000 Subject: [PATCH 18/24] chown: code cleanup --- src/uu/chown/src/chown.rs | 60 ++++++++++++++------------------------- 1 file changed, 21 insertions(+), 39 deletions(-) diff --git a/src/uu/chown/src/chown.rs b/src/uu/chown/src/chown.rs index 68e7e24aa..081d3a148 100644 --- a/src/uu/chown/src/chown.rs +++ b/src/uu/chown/src/chown.rs @@ -7,9 +7,6 @@ // spell-checker:ignore (ToDO) COMFOLLOW Chowner Passwd RFILE RFILE's derefer dgid duid -// Clippy bug: https://github.com/rust-lang/rust-clippy/issues/7422 -#![allow(clippy::nonstandard_macro_braces)] - #[macro_use] extern crate uucore; pub use uucore::entries::{self, Group, Locate, Passwd}; @@ -17,7 +14,7 @@ use uucore::fs::resolve_relative_path; use uucore::libc::{gid_t, uid_t}; use uucore::perms::{wrap_chown, Verbosity}; -use uucore::error::{UError, UResult, USimpleError}; +use uucore::error::{FromIo, UError, UResult, USimpleError}; use clap::{crate_version, App, Arg}; @@ -93,9 +90,9 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> { let preserve_root = matches.is_present(options::preserve_root::PRESERVE); let mut derefer = if matches.is_present(options::dereference::NO_DEREFERENCE) { - Err(UError::from(1)) + 1 } else { - Ok(()) + 0 }; let mut bit_flag = if matches.is_present(options::traverse::TRAVERSE) { @@ -109,13 +106,13 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> { let recursive = matches.is_present(options::RECURSIVE); if recursive { if bit_flag == FTS_PHYSICAL { - if derefer.is_err() { + if derefer == 1 { return Err(USimpleError::new( 1, "-R --dereference requires -H or -L".to_string(), )); } - derefer = Ok(()); + derefer = 0; } } else { bit_flag = FTS_PHYSICAL; @@ -134,12 +131,11 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> { }; 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) => return Err(USimpleError::new(1, e)), + 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 @@ -148,28 +144,14 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> { 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) => { - return Err(USimpleError::new( - 1, - format!("failed to get attributes of '{}': {}", file, e), - )); - } - } + 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) => { - return Err(USimpleError::new(1, e)); - } - } + let (u, g) = parse_spec(owner)?; + dest_uid = u; + dest_gid = g; } let executor = Chowner { bit_flag, @@ -177,7 +159,7 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> { dest_gid, verbosity, recursive, - dereference: derefer.is_ok(), + dereference: derefer != 0, filter, preserve_root, files, @@ -281,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(); @@ -289,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 { @@ -298,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 { From 37c986b005d772a7ebb117db7c84a341c4fecf79 Mon Sep 17 00:00:00 2001 From: Syukron Rifail M Date: Mon, 19 Jul 2021 06:55:11 +0700 Subject: [PATCH 19/24] test_du: ignore unsupported --time=birth --- tests/by-util/test_du.rs | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) 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] From 3a5caec5c2a5a39ec66b3adf6b0bca959f4348f6 Mon Sep 17 00:00:00 2001 From: Andreas Hartmann Date: Tue, 13 Jul 2021 16:59:56 +0200 Subject: [PATCH 20/24] uucore: error: Add docs to the `code` and `usage` functions Make it plain what each of the functions is meant to implement and how they are supposed to work. --- src/uucore/src/lib/mods/error.rs | 132 +++++++++++++++++++++++++++++-- 1 file changed, 127 insertions(+), 5 deletions(-) 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 } From 3a0164310a7f31da95a9d5f81920bdcbc9f3e6dd Mon Sep 17 00:00:00 2001 From: Andreas Hartmann Date: Wed, 30 Jun 2021 11:14:32 +0200 Subject: [PATCH 21/24] cp: Adapt to modified backup mode determination --- src/uu/cp/src/cp.rs | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) 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)); From 2db1ec99f13335d636f0755b64dc2dd2670327ea Mon Sep 17 00:00:00 2001 From: Andreas Hartmann Date: Wed, 30 Jun 2021 11:16:07 +0200 Subject: [PATCH 22/24] ln: Adapt to modified backup mode determination --- src/uu/ln/src/ln.rs | 70 ++++++++++++++++++--------------------------- 1 file changed, 28 insertions(+), 42 deletions(-) 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 \ From 250bcaf7c5d0ab6be15599eb200726ec0d613d02 Mon Sep 17 00:00:00 2001 From: Andreas Hartmann Date: Mon, 5 Jul 2021 14:11:31 +0200 Subject: [PATCH 23/24] backup_control: Run tests in series Make all tests lock a mutex to ensure that they're run in series rather than parallel. We must take this precaution due to the fact that all tests are run in parallel as threads of one parent process. As all threads in a process share e.g. environment variables, we use the Mutex to ensure they're run one after another. This way we can guarantee that tests that rely on environment variables to have specific values will see these variables, too. An alternative implementation could have used the [rusty fork][1] crate to run all tests that need env variables in separate processes rather than threads. However, rusty fork likely wouldn't run on all platforms that the utilities are supposed to run on. --- Cargo.lock | 3 + src/uucore/Cargo.toml | 4 + src/uucore/src/lib/mods/backup_control.rs | 131 +++++++++++++--------- 3 files changed, 85 insertions(+), 53 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 993fe3e39..2b1d8b8c2 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" @@ -2758,6 +2760,7 @@ dependencies = [ name = "uucore" version = "0.0.8" dependencies = [ + "clap", "data-encoding", "dns-lookup", "dunce", diff --git a/src/uucore/Cargo.toml b/src/uucore/Cargo.toml index 0c11d2c15..69e724765 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 a36988048..6fa48d308 100644 --- a/src/uucore/src/lib/mods/backup_control.rs +++ b/src/uucore/src/lib/mods/backup_control.rs @@ -66,7 +66,7 @@ pub fn determine_backup_suffix(supplied_suffix: Option<&str>) -> String { /// fn main() { /// let OPT_BACKUP: &str = "backup"; /// let OPT_BACKUP_NO_ARG: &str = "b"; -/// let matches = App::new("myprog") +/// let matches = App::new("app") /// .arg(Arg::with_name(OPT_BACKUP_NO_ARG) /// .short(OPT_BACKUP_NO_ARG)) /// .arg(Arg::with_name(OPT_BACKUP) @@ -75,7 +75,7 @@ pub fn determine_backup_suffix(supplied_suffix: Option<&str>) -> String { /// .require_equals(true) /// .min_values(0)) /// .get_matches_from(vec![ -/// "myprog", "-b", "--backup=t" +/// "app", "-b", "--backup=t" /// ]); /// /// let backup_mode = backup_control::determine_backup_mode( @@ -92,7 +92,7 @@ pub fn determine_backup_suffix(supplied_suffix: Option<&str>) -> String { /// } /// ``` /// -/// This example shows an ambiguous imput, as 'n' may resolve to 4 different +/// This example shows an ambiguous input, as 'n' may resolve to 4 different /// backup modes. /// /// @@ -105,7 +105,7 @@ pub fn determine_backup_suffix(supplied_suffix: Option<&str>) -> String { /// fn main() { /// let OPT_BACKUP: &str = "backup"; /// let OPT_BACKUP_NO_ARG: &str = "b"; -/// let matches = App::new("myprog") +/// let matches = App::new("app") /// .arg(Arg::with_name(OPT_BACKUP_NO_ARG) /// .short(OPT_BACKUP_NO_ARG)) /// .arg(Arg::with_name(OPT_BACKUP) @@ -114,7 +114,7 @@ pub fn determine_backup_suffix(supplied_suffix: Option<&str>) -> String { /// .require_equals(true) /// .min_values(0)) /// .get_matches_from(vec![ -/// "myprog", "-b", "--backup=n" +/// "app", "-b", "--backup=n" /// ]); /// /// let backup_mode = backup_control::determine_backup_mode( @@ -254,6 +254,19 @@ fn existing_backup_path(path: &Path, suffix: &str) -> PathBuf { 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"; @@ -264,26 +277,12 @@ mod tests { 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!(result == BackupMode::ExistingBackup); - } - - // -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; - env::set_var(ENV_VERSION_CONTROL, "none"); - - let result = - determine_backup_mode(short_opt_present, long_opt_present, long_opt_value).unwrap(); - - assert!(result == BackupMode::ExistingBackup); - env::remove_var(ENV_VERSION_CONTROL); + assert_eq!(result, BackupMode::ExistingBackup); } // --backup takes precedence over -b @@ -292,11 +291,12 @@ mod tests { 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!(result == BackupMode::NoBackup); + assert_eq!(result, BackupMode::NoBackup); } // --backup can be passed without an argument @@ -305,26 +305,12 @@ mod tests { 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!(result == BackupMode::ExistingBackup); - } - - // --backup can be passed without an argument, but reads env var if existant - #[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; - env::set_var(ENV_VERSION_CONTROL, "none"); - - let result = - determine_backup_mode(short_opt_present, long_opt_present, long_opt_value).unwrap(); - - assert!(result == BackupMode::NoBackup); - env::remove_var(ENV_VERSION_CONTROL); + assert_eq!(result, BackupMode::ExistingBackup); } // --backup can be passed with an argument only @@ -333,11 +319,12 @@ mod tests { 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!(result == BackupMode::SimpleBackup); + assert_eq!(result, BackupMode::SimpleBackup); } // --backup errors on invalid argument @@ -346,6 +333,7 @@ mod tests { 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); @@ -360,6 +348,7 @@ mod tests { 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); @@ -368,12 +357,59 @@ mod tests { 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); @@ -390,6 +426,7 @@ mod tests { 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); @@ -400,31 +437,19 @@ mod tests { env::remove_var(ENV_VERSION_CONTROL); } - // --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 result = - determine_backup_mode(short_opt_present, long_opt_present, long_opt_value).unwrap(); - - assert!(result == BackupMode::SimpleBackup); - } - // --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!(result == BackupMode::SimpleBackup); + assert_eq!(result, BackupMode::SimpleBackup); env::remove_var(ENV_VERSION_CONTROL); } } From e07b4e9f59969fc2080fbed5dbb84d28b5a9c5f5 Mon Sep 17 00:00:00 2001 From: Andreas Hartmann Date: Mon, 5 Jul 2021 14:20:28 +0200 Subject: [PATCH 24/24] install: Adapt to modified backup mode determination --- src/uu/install/src/install.rs | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) 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(),