From 3b8f1358428eab69a232b71fe8450b9e03142d93 Mon Sep 17 00:00:00 2001 From: Andreas Hartmann Date: Sat, 26 Jun 2021 20:09:01 +0200 Subject: [PATCH 01/10] 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/10] 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/10] 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/10] 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/10] 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/10] 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 3a0164310a7f31da95a9d5f81920bdcbc9f3e6dd Mon Sep 17 00:00:00 2001 From: Andreas Hartmann Date: Wed, 30 Jun 2021 11:14:32 +0200 Subject: [PATCH 07/10] 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 08/10] 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 09/10] 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 10/10] 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(),