From 3b8f1358428eab69a232b71fe8450b9e03142d93 Mon Sep 17 00:00:00 2001 From: Andreas Hartmann Date: Sat, 26 Jun 2021 20:09:01 +0200 Subject: [PATCH 01/64] 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/64] 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/64] 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/64] 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/64] 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/64] 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/64] 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/64] 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/64] 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/64] 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/64] 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/64] 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/64] 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/64] 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/64] 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/64] 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/64] 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/64] 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/64] 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/64] 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/64] 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/64] 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/64] 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/64] 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(), From 38a47b4358b2e149dcaaa02a417dc527cabb02ec Mon Sep 17 00:00:00 2001 From: Dave Hodder Date: Thu, 22 Jul 2021 17:38:43 +0100 Subject: [PATCH 25/64] CONTRIBUTING: Add Apache License note (#2086) Add a note the licensing section, stating that references using the Apache License are acceptable on a case-by-case basis when there is no MIT-licensed alternative. * Follow-up to #1994 / https://github.com/uutils/coreutils/pull/2493 * Intended to resolve issue #2086 --- CONTRIBUTING.md | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index c9a50cd1e..1a78d8bf0 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -103,6 +103,13 @@ The following types of license are acceptable: * "MIT equivalent" license (2-clause BSD, 3-clause BSD, ISC) * License less restrictive than the MIT License (CC0 1.0 Universal) +Some licenses are similar to the above, but are not preferable because they +contain additional conditions not present in the MIT License. The following are +accepted in referenced dependencies, on a case-by-case basis, when there is no +MIT-licensed alternative: + +* Apache License version 2.0 + Licenses we will not use: * An ambiguous license, or no license From 2dc9dfe62f15eed62b1a4f66a8606e68159290f8 Mon Sep 17 00:00:00 2001 From: Dave Hodder Date: Thu, 22 Jul 2021 18:02:51 +0100 Subject: [PATCH 26/64] CONTRIBUTING: adjust formatting --- CONTRIBUTING.md | 24 +++++++++++++++++------- 1 file changed, 17 insertions(+), 7 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 1a78d8bf0..41733946b 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -4,9 +4,10 @@ Contributions are very welcome, and should target Rust's master branch until the standard libraries are stabilized. You may *claim* an item on the to-do list by following these steps: -1. Open an issue named "Implement [the utility of your choice]", e.g. "Implement ls" +1. Open an issue named "Implement [the utility of your choice]", e.g. "Implement + ls". 1. State that you are working on this utility. -1. Develop the utility +1. Develop the utility. 1. Add integration tests. 1. Add the reference to your utility into Cargo.toml and Makefile. 1. Remove utility from the to-do list in the README. @@ -17,12 +18,20 @@ search the issues to make sure no one else is working on it. ## Best practices -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. 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. +1. Make sure that the code coverage is covering all of the cases, including + errors. 1. The code must be clippy-warning-free and rustfmt-compliant. -1. Don't hesitate to move common functions into uucore if they can be reused by other binaries. +1. Don't hesitate to move common functions into uucore if they can be reused by + other binaries. 1. Unsafe code should be documented with Safety comments. 1. uutils is original code. It cannot contain code from existing GNU or Unix-like utilities, nor should it link to or reference GNU libraries. @@ -99,7 +108,8 @@ project, a tool like `cargo-license` can be used to show their license details. The following types of license are acceptable: * MIT License -* Dual- or tri-license with an MIT License option ("Apache-2.0 or MIT" is a popular combination) +* Dual- or tri-license with an MIT License option ("Apache-2.0 or MIT" is a + popular combination) * "MIT equivalent" license (2-clause BSD, 3-clause BSD, ISC) * License less restrictive than the MIT License (CC0 1.0 Universal) From 46a2491727bda53241698b0da153c726459d6c70 Mon Sep 17 00:00:00 2001 From: Tuomas Tynkkynen Date: Fri, 23 Jul 2021 19:04:18 +0300 Subject: [PATCH 27/64] stat: Support \t in --printf format --- src/uu/stat/src/stat.rs | 1 + tests/by-util/test_stat.rs | 3 ++- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/src/uu/stat/src/stat.rs b/src/uu/stat/src/stat.rs index 70c06bdf6..c56971f6b 100644 --- a/src/uu/stat/src/stat.rs +++ b/src/uu/stat/src/stat.rs @@ -436,6 +436,7 @@ impl Stater { 'f' => tokens.push(Token::Char('\x0C')), 'n' => tokens.push(Token::Char('\n')), 'r' => tokens.push(Token::Char('\r')), + 't' => tokens.push(Token::Char('\t')), 'v' => tokens.push(Token::Char('\x0B')), c => { show_warning!("unrecognized escape '\\{}'", c); diff --git a/tests/by-util/test_stat.rs b/tests/by-util/test_stat.rs index 7cff0d89c..af9e3de45 100644 --- a/tests/by-util/test_stat.rs +++ b/tests/by-util/test_stat.rs @@ -64,7 +64,7 @@ mod test_generate_tokens { #[test] fn printf_format() { - let s = "%-# 15a\\r\\\"\\\\\\a\\b\\e\\f\\v%+020.-23w\\x12\\167\\132\\112\\n"; + let s = "%-# 15a\\t\\r\\\"\\\\\\a\\b\\e\\f\\v%+020.-23w\\x12\\167\\132\\112\\n"; let expected = vec![ Token::Directive { flag: F_LEFT | F_ALTER | F_SPACE, @@ -72,6 +72,7 @@ mod test_generate_tokens { precision: -1, format: 'a', }, + Token::Char('\t'), Token::Char('\r'), Token::Char('"'), Token::Char('\\'), From 5c7afe7a6b2cb3b062e6ea64de102ea09736027b Mon Sep 17 00:00:00 2001 From: Syukron Rifail M Date: Sat, 24 Jul 2021 18:17:28 +0700 Subject: [PATCH 28/64] df: add UResult --- src/uu/df/src/df.rs | 59 +++++++++++++++++++++++++++++++++------------ 1 file changed, 43 insertions(+), 16 deletions(-) diff --git a/src/uu/df/src/df.rs b/src/uu/df/src/df.rs index 1092938df..baa5fe292 100644 --- a/src/uu/df/src/df.rs +++ b/src/uu/df/src/df.rs @@ -8,6 +8,8 @@ #[macro_use] extern crate uucore; +use uucore::error::UCustomError; +use uucore::error::UResult; #[cfg(unix)] use uucore::fsext::statfs_fn; use uucore::fsext::{read_fs_list, FsUsage, MountInfo}; @@ -19,8 +21,10 @@ use std::cell::Cell; use std::collections::HashMap; use std::collections::HashSet; +use std::error::Error; #[cfg(unix)] use std::ffi::CString; +use std::fmt::Display; #[cfg(unix)] use std::mem; @@ -33,9 +37,6 @@ use std::path::Path; static ABOUT: &str = "Show information about the file system on which each FILE resides,\n\ or all file systems by default."; -static EXIT_OK: i32 = 0; -static EXIT_ERR: i32 = 1; - static OPT_ALL: &str = "all"; static OPT_BLOCKSIZE: &str = "blocksize"; static OPT_DIRECT: &str = "direct"; @@ -226,8 +227,8 @@ fn filter_mount_list(vmi: Vec, paths: &[String], opt: &Options) -> Ve /// Convert `value` to a human readable string based on `base`. /// e.g. It returns 1G when value is 1 * 1024 * 1024 * 1024 and base is 1024. /// Note: It returns `value` if `base` isn't positive. -fn human_readable(value: u64, base: i64) -> String { - match base { +fn human_readable(value: u64, base: i64) -> UResult { + let base_str = match base { d if d < 0 => value.to_string(), // ref: [Binary prefix](https://en.wikipedia.org/wiki/Binary_prefix) @@ @@ -242,8 +243,10 @@ fn human_readable(value: u64, base: i64) -> String { NumberPrefix::Prefixed(prefix, bytes) => format!("{:.1}{}", bytes, prefix.symbol()), }, - _ => crash!(EXIT_ERR, "Internal error: Unknown base value {}", base), - } + _ => return Err(DfError::InvalidBaseValue(base.to_string()).into()), + }; + + Ok(base_str) } fn use_size(free_size: u64, total_size: u64) -> String { @@ -256,7 +259,31 @@ fn use_size(free_size: u64, total_size: u64) -> String { ); } -pub fn uumain(args: impl uucore::Args) -> i32 { +#[derive(Debug)] +enum DfError { + InvalidBaseValue(String), +} + +impl Display for DfError { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + DfError::InvalidBaseValue(s) => write!(f, "Internal error: Unknown base value {}", s), + } + } +} + +impl Error for DfError {} + +impl UCustomError for DfError { + fn code(&self) -> i32 { + match self { + DfError::InvalidBaseValue(_) => 1, + } + } +} + +#[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); @@ -269,7 +296,7 @@ pub fn uumain(args: impl uucore::Args) -> i32 { { if matches.is_present(OPT_INODES) { println!("{}: doesn't support -i option", executable!()); - return EXIT_OK; + return Ok(()); } } @@ -353,15 +380,15 @@ pub fn uumain(args: impl uucore::Args) -> i32 { if opt.show_inode_instead { print!( "{0: >12} ", - human_readable(fs.usage.files, opt.human_readable_base) + human_readable(fs.usage.files, opt.human_readable_base)? ); print!( "{0: >12} ", - human_readable(fs.usage.files - fs.usage.ffree, opt.human_readable_base) + human_readable(fs.usage.files - fs.usage.ffree, opt.human_readable_base)? ); print!( "{0: >12} ", - human_readable(fs.usage.ffree, opt.human_readable_base) + human_readable(fs.usage.ffree, opt.human_readable_base)? ); print!( "{0: >5} ", @@ -375,15 +402,15 @@ pub fn uumain(args: impl uucore::Args) -> i32 { let free_size = fs.usage.blocksize * fs.usage.bfree; print!( "{0: >12} ", - human_readable(total_size, opt.human_readable_base) + human_readable(total_size, opt.human_readable_base)? ); print!( "{0: >12} ", - human_readable(total_size - free_size, opt.human_readable_base) + human_readable(total_size - free_size, opt.human_readable_base)? ); print!( "{0: >12} ", - human_readable(free_size, opt.human_readable_base) + human_readable(free_size, opt.human_readable_base)? ); if cfg!(target_os = "macos") { let used = fs.usage.blocks - fs.usage.bfree; @@ -396,7 +423,7 @@ pub fn uumain(args: impl uucore::Args) -> i32 { println!(); } - EXIT_OK + Ok(()) } pub fn uu_app() -> App<'static, 'static> { From 3af8a89bce196e43c21c4733868d2195b626a2d0 Mon Sep 17 00:00:00 2001 From: Dave Hodder Date: Sun, 25 Jul 2021 12:27:26 +0100 Subject: [PATCH 29/64] CONTRIBUTING: Add Apache as valid license (#2086) --- CONTRIBUTING.md | 6 ------ 1 file changed, 6 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 41733946b..2a8a80315 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -112,12 +112,6 @@ The following types of license are acceptable: popular combination) * "MIT equivalent" license (2-clause BSD, 3-clause BSD, ISC) * License less restrictive than the MIT License (CC0 1.0 Universal) - -Some licenses are similar to the above, but are not preferable because they -contain additional conditions not present in the MIT License. The following are -accepted in referenced dependencies, on a case-by-case basis, when there is no -MIT-licensed alternative: - * Apache License version 2.0 Licenses we will not use: From 9702aa6414ae2e67c781c1038d09c64791d5f486 Mon Sep 17 00:00:00 2001 From: sagu <16504129+sagudev@users.noreply.github.com> Date: Sun, 25 Jul 2021 18:06:41 +0200 Subject: [PATCH 30/64] Revert "silent buggy clippy warning" --- src/uu/arch/src/arch.rs | 3 --- src/uu/cat/src/cat.rs | 3 --- src/uu/csplit/src/csplit_error.rs | 3 --- src/uu/ls/src/ls.rs | 3 --- src/uu/mkdir/src/mkdir.rs | 3 --- src/uu/mktemp/src/mktemp.rs | 3 --- src/uu/touch/src/touch.rs | 3 --- src/uu/wc/src/wc.rs | 2 -- src/uucore/src/lib/features/encoding.rs | 3 --- 9 files changed, 26 deletions(-) diff --git a/src/uu/arch/src/arch.rs b/src/uu/arch/src/arch.rs index ef12eb82a..94ec97e98 100644 --- a/src/uu/arch/src/arch.rs +++ b/src/uu/arch/src/arch.rs @@ -6,9 +6,6 @@ // 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; diff --git a/src/uu/cat/src/cat.rs b/src/uu/cat/src/cat.rs index 8ad563c5d..35a5308ed 100644 --- a/src/uu/cat/src/cat.rs +++ b/src/uu/cat/src/cat.rs @@ -10,9 +10,6 @@ // spell-checker:ignore (ToDO) nonprint nonblank nonprinting -// clippy bug https://github.com/rust-lang/rust-clippy/issues/7422 -#![allow(clippy::nonstandard_macro_braces)] - #[cfg(unix)] extern crate unix_socket; #[macro_use] diff --git a/src/uu/csplit/src/csplit_error.rs b/src/uu/csplit/src/csplit_error.rs index e2f514ea9..637cf8890 100644 --- a/src/uu/csplit/src/csplit_error.rs +++ b/src/uu/csplit/src/csplit_error.rs @@ -1,6 +1,3 @@ -// clippy bug https://github.com/rust-lang/rust-clippy/issues/7422 -#![allow(clippy::nonstandard_macro_braces)] - use std::io; use thiserror::Error; diff --git a/src/uu/ls/src/ls.rs b/src/uu/ls/src/ls.rs index a2c6f3481..8fcd34bed 100644 --- a/src/uu/ls/src/ls.rs +++ b/src/uu/ls/src/ls.rs @@ -7,9 +7,6 @@ // spell-checker:ignore (ToDO) cpio svgz webm somegroup nlink rmvb xspf -// clippy bug https://github.com/rust-lang/rust-clippy/issues/7422 -#![allow(clippy::nonstandard_macro_braces)] - #[macro_use] extern crate uucore; #[cfg(unix)] diff --git a/src/uu/mkdir/src/mkdir.rs b/src/uu/mkdir/src/mkdir.rs index 7362601ba..a99867570 100644 --- a/src/uu/mkdir/src/mkdir.rs +++ b/src/uu/mkdir/src/mkdir.rs @@ -5,9 +5,6 @@ // * 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; diff --git a/src/uu/mktemp/src/mktemp.rs b/src/uu/mktemp/src/mktemp.rs index ef5c41abf..8a4b472aa 100644 --- a/src/uu/mktemp/src/mktemp.rs +++ b/src/uu/mktemp/src/mktemp.rs @@ -8,9 +8,6 @@ // spell-checker:ignore (paths) GPGHome -// clippy bug https://github.com/rust-lang/rust-clippy/issues/7422 -#![allow(clippy::nonstandard_macro_braces)] - #[macro_use] extern crate uucore; diff --git a/src/uu/touch/src/touch.rs b/src/uu/touch/src/touch.rs index bfc7a4197..dd2b05d0e 100644 --- a/src/uu/touch/src/touch.rs +++ b/src/uu/touch/src/touch.rs @@ -8,9 +8,6 @@ // spell-checker:ignore (ToDO) filetime strptime utcoff strs datetime MMDDhhmm -// clippy bug https://github.com/rust-lang/rust-clippy/issues/7422 -#![allow(clippy::nonstandard_macro_braces)] - pub extern crate filetime; #[macro_use] diff --git a/src/uu/wc/src/wc.rs b/src/uu/wc/src/wc.rs index 95d71e77a..0bcc66664 100644 --- a/src/uu/wc/src/wc.rs +++ b/src/uu/wc/src/wc.rs @@ -4,8 +4,6 @@ // * // * 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; diff --git a/src/uucore/src/lib/features/encoding.rs b/src/uucore/src/lib/features/encoding.rs index 08c0d27e9..03fa0ed8b 100644 --- a/src/uucore/src/lib/features/encoding.rs +++ b/src/uucore/src/lib/features/encoding.rs @@ -7,9 +7,6 @@ // spell-checker:ignore (strings) ABCDEFGHIJKLMNOPQRSTUVWXYZ -// clippy bug https://github.com/rust-lang/rust-clippy/issues/7422 -#![allow(clippy::nonstandard_macro_braces)] - extern crate data_encoding; use self::data_encoding::{DecodeError, BASE32, BASE64}; From 4f4338f1c0290448f7e00de62bf258eb8e8ffd93 Mon Sep 17 00:00:00 2001 From: sagudev Date: Sun, 25 Jul 2021 18:51:16 +0200 Subject: [PATCH 31/64] Delete all allow(nonstandard_macro_braces) and fix other clippy warnings --- src/uu/cp/src/cp.rs | 1 - src/uu/dirname/src/dirname.rs | 3 --- src/uu/du/src/du.rs | 3 --- src/uu/echo/src/echo.rs | 3 --- src/uu/false/src/false.rs | 3 --- src/uu/hostid/src/hostid.rs | 3 --- src/uu/hostname/src/hostname.rs | 3 --- src/uu/kill/src/kill.rs | 3 --- src/uu/pwd/src/pwd.rs | 3 --- src/uu/sleep/src/sleep.rs | 3 --- src/uu/true/src/true.rs | 3 --- 11 files changed, 31 deletions(-) diff --git a/src/uu/cp/src/cp.rs b/src/uu/cp/src/cp.rs index 91ea7ef37..7c67649c2 100644 --- a/src/uu/cp/src/cp.rs +++ b/src/uu/cp/src/cp.rs @@ -55,7 +55,6 @@ use walkdir::WalkDir; use std::os::unix::fs::PermissionsExt; #[cfg(target_os = "linux")] -#[allow(clippy::missing_safety_doc)] ioctl!(write ficlone with 0x94, 9; std::os::raw::c_int); quick_error! { diff --git a/src/uu/dirname/src/dirname.rs b/src/uu/dirname/src/dirname.rs index 8d85dc85e..63ee57272 100644 --- a/src/uu/dirname/src/dirname.rs +++ b/src/uu/dirname/src/dirname.rs @@ -5,9 +5,6 @@ // 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; diff --git a/src/uu/du/src/du.rs b/src/uu/du/src/du.rs index 61a3b8c29..9c05eb982 100644 --- a/src/uu/du/src/du.rs +++ b/src/uu/du/src/du.rs @@ -5,9 +5,6 @@ // * 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; diff --git a/src/uu/echo/src/echo.rs b/src/uu/echo/src/echo.rs index acdd22948..aae1ad10d 100644 --- a/src/uu/echo/src/echo.rs +++ b/src/uu/echo/src/echo.rs @@ -6,9 +6,6 @@ // 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; diff --git a/src/uu/false/src/false.rs b/src/uu/false/src/false.rs index 232431142..2d64c8376 100644 --- a/src/uu/false/src/false.rs +++ b/src/uu/false/src/false.rs @@ -5,9 +5,6 @@ // * 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; diff --git a/src/uu/hostid/src/hostid.rs b/src/uu/hostid/src/hostid.rs index 180c4d2e5..b0f68968d 100644 --- a/src/uu/hostid/src/hostid.rs +++ b/src/uu/hostid/src/hostid.rs @@ -7,9 +7,6 @@ // spell-checker:ignore (ToDO) gethostid -// Clippy bug: https://github.com/rust-lang/rust-clippy/issues/7422 -#![allow(clippy::nonstandard_macro_braces)] - #[macro_use] extern crate uucore; diff --git a/src/uu/hostname/src/hostname.rs b/src/uu/hostname/src/hostname.rs index 14f8b9df2..045e43045 100644 --- a/src/uu/hostname/src/hostname.rs +++ b/src/uu/hostname/src/hostname.rs @@ -7,9 +7,6 @@ // spell-checker:ignore (ToDO) MAKEWORD addrs hashset -// Clippy bug: https://github.com/rust-lang/rust-clippy/issues/7422 -#![allow(clippy::nonstandard_macro_braces)] - #[macro_use] extern crate uucore; diff --git a/src/uu/kill/src/kill.rs b/src/uu/kill/src/kill.rs index 88eb08fa7..b3f5010ca 100644 --- a/src/uu/kill/src/kill.rs +++ b/src/uu/kill/src/kill.rs @@ -7,9 +7,6 @@ // 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; diff --git a/src/uu/pwd/src/pwd.rs b/src/uu/pwd/src/pwd.rs index c72cc64e2..37effe618 100644 --- a/src/uu/pwd/src/pwd.rs +++ b/src/uu/pwd/src/pwd.rs @@ -5,9 +5,6 @@ // * 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; diff --git a/src/uu/sleep/src/sleep.rs b/src/uu/sleep/src/sleep.rs index 1cb858a34..127804a9f 100644 --- a/src/uu/sleep/src/sleep.rs +++ b/src/uu/sleep/src/sleep.rs @@ -5,9 +5,6 @@ // * 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; diff --git a/src/uu/true/src/true.rs b/src/uu/true/src/true.rs index e6b7b9025..f84a89176 100644 --- a/src/uu/true/src/true.rs +++ b/src/uu/true/src/true.rs @@ -5,9 +5,6 @@ // * 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; From abf4c69b281dfd7f275c2a0101de09c0df41c947 Mon Sep 17 00:00:00 2001 From: Jeffrey Finkelstein Date: Sun, 25 Jul 2021 13:55:24 -0400 Subject: [PATCH 32/64] tac: correct error message when reading from dir. Correct the error message produced by `tac` when trying to read from a directory. Previously if the path 'a' referred to a directory, then running `tac a` would produce the error message dir: read error: Invalid argument after this commit it produces a: read error: Invalid argument which matches GNU `tac`. --- src/uu/tac/src/tac.rs | 2 +- tests/by-util/test_tac.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/uu/tac/src/tac.rs b/src/uu/tac/src/tac.rs index ae1fd9bc5..ad3badff4 100644 --- a/src/uu/tac/src/tac.rs +++ b/src/uu/tac/src/tac.rs @@ -97,7 +97,7 @@ fn tac(filenames: Vec, before: bool, _: bool, separator: &str) -> i32 { let path = Path::new(filename); if path.is_dir() || path.metadata().is_err() { if path.is_dir() { - show_error!("dir: read error: Invalid argument"); + show_error!("{}: read error: Invalid argument", filename); } else { show_error!( "failed to open '{}' for reading: No such file or directory", diff --git a/tests/by-util/test_tac.rs b/tests/by-util/test_tac.rs index a8adbb28e..a16988acf 100644 --- a/tests/by-util/test_tac.rs +++ b/tests/by-util/test_tac.rs @@ -66,5 +66,5 @@ fn test_invalid_input() { .ucmd() .arg("a") .fails() - .stderr_contains("dir: read error: Invalid argument"); + .stderr_contains("a: read error: Invalid argument"); } From 2648f330e4403734c5cce4795f2b700c6374d269 Mon Sep 17 00:00:00 2001 From: Jeffrey Finkelstein Date: Sun, 25 Jul 2021 15:19:29 -0400 Subject: [PATCH 33/64] tac: handle no line separators in file Change the behavior of `tac` when there are no line separators in the input. Previously, it was appending an extra line separator; this commit prevents that from happening. The input is now writted directly to stdout. --- src/uu/tac/src/tac.rs | 9 ++++++++- tests/by-util/test_tac.rs | 5 +++++ 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/src/uu/tac/src/tac.rs b/src/uu/tac/src/tac.rs index ae1fd9bc5..2622333e1 100644 --- a/src/uu/tac/src/tac.rs +++ b/src/uu/tac/src/tac.rs @@ -139,9 +139,16 @@ fn tac(filenames: Vec, before: bool, _: bool, separator: &str) -> i32 { i += 1; } } + // If the file contains no line separators, then simply write + // the contents of the file directly to stdout. + if offsets.is_empty() { + out.write_all(&data) + .unwrap_or_else(|e| crash!(1, "failed to write to stdout: {}", e)); + return exit_code; + } // if there isn't a separator at the end of the file, fake it - if offsets.is_empty() || *offsets.last().unwrap() < data.len() - slen { + if *offsets.last().unwrap() < data.len() - slen { offsets.push(data.len()); } diff --git a/tests/by-util/test_tac.rs b/tests/by-util/test_tac.rs index a8adbb28e..54054db53 100644 --- a/tests/by-util/test_tac.rs +++ b/tests/by-util/test_tac.rs @@ -68,3 +68,8 @@ fn test_invalid_input() { .fails() .stderr_contains("dir: read error: Invalid argument"); } + +#[test] +fn test_no_line_separators() { + new_ucmd!().pipe_in("a").succeeds().stdout_is("a"); +} From 3c7940ddfd1ac751bc1bf4c71d09706437294b14 Mon Sep 17 00:00:00 2001 From: Jeffrey Finkelstein Date: Sun, 25 Jul 2021 16:22:22 -0400 Subject: [PATCH 34/64] pathchk: remove double negation --- src/uu/pathchk/src/pathchk.rs | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/src/uu/pathchk/src/pathchk.rs b/src/uu/pathchk/src/pathchk.rs index 335266456..7f728667f 100644 --- a/src/uu/pathchk/src/pathchk.rs +++ b/src/uu/pathchk/src/pathchk.rs @@ -170,7 +170,7 @@ fn check_basic(path: &[String]) -> bool { fn check_extra(path: &[String]) -> bool { // components: leading hyphens for p in path { - if !no_leading_hyphen(p) { + if p.starts_with('-') { writeln!( &mut std::io::stderr(), "leading hyphen in file name component '{}'", @@ -236,11 +236,6 @@ fn check_searchable(path: &str) -> bool { } } -// check for a hyphen at the beginning of a path segment -fn no_leading_hyphen(path_segment: &str) -> bool { - !path_segment.starts_with('-') -} - // check whether a path segment contains only valid (read: portable) characters fn check_portable_chars(path_segment: &str) -> bool { const VALID_CHARS: &[u8] = b"ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789._-"; From bd7d8fdde7cbcb5bf19279a6151515c764332145 Mon Sep 17 00:00:00 2001 From: Michael Debertol Date: Tue, 27 Jul 2021 23:54:29 +0200 Subject: [PATCH 35/64] sort: remove duplication from usage string The custom usage string does not have to include the "sort\nUsage:" part, because this part is already printed by clap. It prevents the following duplication: USAGE: sort Usage: sort [OPTION]... [FILE].. Now, only the following is printed: USAGE: sort [OPTION]... [FILE]... --- src/uu/sort/src/sort.rs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/uu/sort/src/sort.rs b/src/uu/sort/src/sort.rs index 55bcdb77b..91365b603 100644 --- a/src/uu/sort/src/sort.rs +++ b/src/uu/sort/src/sort.rs @@ -916,9 +916,7 @@ impl FieldSelector { fn get_usage() -> String { format!( - "{0} -Usage: - {0} [OPTION]... [FILE]... + "{0} [OPTION]... [FILE]... Write the sorted concatenation of all FILE(s) to standard output. Mandatory arguments for long options are mandatory for short options too. With no FILE, or when FILE is -, read standard input.", From f34505df54acf93ab617bbd1a283b01101b57e38 Mon Sep 17 00:00:00 2001 From: Sylvestre Ledru Date: Fri, 30 Jul 2021 14:41:47 +0200 Subject: [PATCH 36/64] bump the minimal version for coverage to 1.52 Drivers: https://github.com/rust-lang/rust/issues/71395 https://github.com/rust-lang/rust/pull/80470 needed by grcov --- .github/workflows/CICD.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/CICD.yml b/.github/workflows/CICD.yml index b2d77a5a4..012c14264 100644 --- a/.github/workflows/CICD.yml +++ b/.github/workflows/CICD.yml @@ -14,7 +14,7 @@ env: PROJECT_DESC: "Core universal (cross-platform) utilities" PROJECT_AUTH: "uutils" RUST_MIN_SRV: "1.43.1" ## v1.43.0 - RUST_COV_SRV: "2020-08-01" ## (~v1.47.0) supported rust version for code coverage; (date required/used by 'coverage') ## !maint: refactor when code coverage support is included in the stable channel + RUST_COV_SRV: "2021-05-06" ## (~v1.52.0) supported rust version for code coverage; (date required/used by 'coverage') ## !maint: refactor when code coverage support is included in the stable channel on: [push, pull_request] From 3e096491f3694ae420a43e50d63388c81e543b85 Mon Sep 17 00:00:00 2001 From: Sylvestre Ledru Date: Fri, 30 Jul 2021 15:22:20 +0200 Subject: [PATCH 37/64] Remove clippy allow was causing unused attribute `allow` --- src/uu/cp/src/cp.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/src/uu/cp/src/cp.rs b/src/uu/cp/src/cp.rs index 4deaefa98..031890b5a 100644 --- a/src/uu/cp/src/cp.rs +++ b/src/uu/cp/src/cp.rs @@ -55,7 +55,6 @@ use walkdir::WalkDir; use std::os::unix::fs::PermissionsExt; #[cfg(target_os = "linux")] -#[allow(clippy::missing_safety_doc)] ioctl!(write ficlone with 0x94, 9; std::os::raw::c_int); quick_error! { From a214ca60bdf03f450b342f24187e080b81ce340c Mon Sep 17 00:00:00 2001 From: Michael Debertol Date: Tue, 27 Jul 2021 15:49:38 +0200 Subject: [PATCH 38/64] sort: allow null bytes for -t --- src/uu/sort/src/sort.rs | 5 ++++- tests/by-util/test_sort.rs | 9 +++++++++ 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/src/uu/sort/src/sort.rs b/src/uu/sort/src/sort.rs index 91365b603..f779bca38 100644 --- a/src/uu/sort/src/sort.rs +++ b/src/uu/sort/src/sort.rs @@ -1065,7 +1065,10 @@ pub fn uumain(args: impl uucore::Args) -> i32 { if let Some(arg) = matches.args.get(options::SEPARATOR) { let separator = arg.vals[0].to_string_lossy(); - let separator = separator; + let mut separator = separator.as_ref(); + if separator == "\\0" { + separator = "\0"; + } if separator.len() != 1 { crash!(1, "separator must be exactly one character long"); } diff --git a/tests/by-util/test_sort.rs b/tests/by-util/test_sort.rs index 4a1cc3fa9..a9519e153 100644 --- a/tests/by-util/test_sort.rs +++ b/tests/by-util/test_sort.rs @@ -959,3 +959,12 @@ fn test_key_takes_one_arg() { .succeeds() .stdout_is_fixture("keys_open_ended.expected"); } + +#[test] +fn test_separator_null() { + new_ucmd!() + .args(&["-k1,1", "-k3,3", "-t", "\\0"]) + .pipe_in("z\0a\0b\nz\0b\0a\na\0z\0z\n") + .succeeds() + .stdout_only("a\0z\0z\nz\0b\0a\nz\0a\0b\n"); +} From 891d25cebd7e888540ba2bfbfea018f00d1cc434 Mon Sep 17 00:00:00 2001 From: Michael Debertol Date: Sun, 25 Jul 2021 22:24:37 +0200 Subject: [PATCH 39/64] sort: fix exit codes and error messages --- src/uu/sort/src/chunks.rs | 4 ++-- src/uu/sort/src/ext_sort.rs | 2 +- src/uu/sort/src/sort.rs | 29 +++++++++++++++++++++-------- tests/by-util/test_sort.rs | 4 ++-- 4 files changed, 26 insertions(+), 13 deletions(-) diff --git a/src/uu/sort/src/chunks.rs b/src/uu/sort/src/chunks.rs index 9e9d212c2..5ab98392d 100644 --- a/src/uu/sort/src/chunks.rs +++ b/src/uu/sort/src/chunks.rs @@ -175,7 +175,7 @@ pub fn read( // because it was only temporarily transmuted to a Vec> to make recycling possible. std::mem::transmute::>, Vec>>(lines) }; - let read = crash_if_err!(1, std::str::from_utf8(&buffer[..read])); + let read = crash_if_err!(2, std::str::from_utf8(&buffer[..read])); let mut line_data = LineData { selections, num_infos, @@ -313,7 +313,7 @@ fn read_to_buffer( Err(e) if e.kind() == ErrorKind::Interrupted => { // retry } - Err(e) => crash!(1, "{}", e), + Err(e) => crash!(2, "{}", e), } } } diff --git a/src/uu/sort/src/ext_sort.rs b/src/uu/sort/src/ext_sort.rs index e0814b7a2..201dda8bb 100644 --- a/src/uu/sort/src/ext_sort.rs +++ b/src/uu/sort/src/ext_sort.rs @@ -211,7 +211,7 @@ fn read_write_loop( } let tmp_dir = crash_if_err!( - 1, + 2, tempfile::Builder::new() .prefix("uutils_sort") .tempdir_in(tmp_dir_parent) diff --git a/src/uu/sort/src/sort.rs b/src/uu/sort/src/sort.rs index 91365b603..3d7e890b9 100644 --- a/src/uu/sort/src/sort.rs +++ b/src/uu/sort/src/sort.rs @@ -215,7 +215,7 @@ impl GlobalSettings { Ok(f) => BufWriter::new(Box::new(f) as Box), Err(e) => { show_error!("{0}: {1}", filename, e.to_string()); - panic!("Could not open output file"); + crash!(2, "Could not open output file"); } }, None => BufWriter::new(Box::new(stdout()) as Box), @@ -942,7 +942,9 @@ pub fn uumain(args: impl uucore::Args) -> i32 { let usage = get_usage(); let mut settings: GlobalSettings = Default::default(); - let matches = uu_app().usage(&usage[..]).get_matches_from(args); + let matches = uu_app().usage(&usage[..]).get_matches_from_safe(args); + + let matches = crash_if_err!(2, matches); settings.debug = matches.is_present(options::DEBUG); @@ -1060,14 +1062,14 @@ pub fn uumain(args: impl uucore::Args) -> i32 { /* if no file, default to stdin */ files.push("-".to_owned()); } else if settings.check && files.len() != 1 { - crash!(1, "extra operand `{}' not allowed with -c", files[1]) + crash!(2, "extra operand `{}' not allowed with -c", files[1]) } if let Some(arg) = matches.args.get(options::SEPARATOR) { let separator = arg.vals[0].to_string_lossy(); let separator = separator; if separator.len() != 1 { - crash!(1, "separator must be exactly one character long"); + crash!(2, "separator must be exactly one character long"); } settings.separator = Some(separator.chars().next().unwrap()) } @@ -1338,7 +1340,7 @@ fn exec(files: &[String], settings: &GlobalSettings) -> i32 { file_merger.write_all(settings); } else if settings.check { if files.len() > 1 { - crash!(1, "only one file allowed with -c"); + crash!(2, "only one file allowed with -c"); } return check::check(files.first().unwrap(), settings); } else { @@ -1623,7 +1625,11 @@ fn print_sorted<'a, T: Iterator>>(iter: T, settings: &Global } } -// from cat.rs +/// Strips the trailing " (os error XX)" from io error strings. +fn strip_errno(err: &str) -> &str { + &err[..err.find(" (os error ").unwrap_or(err.len())] +} + fn open(path: impl AsRef) -> Box { let path = path.as_ref(); if path == "-" { @@ -1631,10 +1637,17 @@ fn open(path: impl AsRef) -> Box { return Box::new(stdin) as Box; } - match File::open(Path::new(path)) { + let path = Path::new(path); + + match File::open(path) { Ok(f) => Box::new(f) as Box, Err(e) => { - crash!(2, "cannot read: {0:?}: {1}", path, e); + crash!( + 2, + "cannot read: {0}: {1}", + path.to_string_lossy(), + strip_errno(&e.to_string()) + ); } } } diff --git a/tests/by-util/test_sort.rs b/tests/by-util/test_sort.rs index 4a1cc3fa9..e594de0a7 100644 --- a/tests/by-util/test_sort.rs +++ b/tests/by-util/test_sort.rs @@ -839,9 +839,9 @@ fn test_nonexistent_file() { .status_code(2) .stderr_only( #[cfg(not(windows))] - "sort: cannot read: \"nonexistent.txt\": No such file or directory (os error 2)", + "sort: cannot read: nonexistent.txt: No such file or directory", #[cfg(windows)] - "sort: cannot read: \"nonexistent.txt\": The system cannot find the file specified. (os error 2)", + "sort: cannot read: nonexistent.txt: The system cannot find the file specified.", ); } From c6e044927c95fbeab5364349e9e1e7d1e09158e9 Mon Sep 17 00:00:00 2001 From: Michael Debertol Date: Sun, 25 Jul 2021 22:25:54 +0200 Subject: [PATCH 40/64] sort: print check messages to stderr, not stdout --- src/uu/sort/src/check.rs | 4 ++-- tests/by-util/test_sort.rs | 6 +++--- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/uu/sort/src/check.rs b/src/uu/sort/src/check.rs index f1cd22686..44c71674e 100644 --- a/src/uu/sort/src/check.rs +++ b/src/uu/sort/src/check.rs @@ -56,7 +56,7 @@ pub fn check(path: &str, settings: &GlobalSettings) -> i32 { ) == Ordering::Greater { if !settings.check_silent { - println!("sort: {}:{}: disorder: {}", path, line_idx, new_first.line); + eprintln!("sort: {}:{}: disorder: {}", path, line_idx, new_first.line); } return 1; } @@ -68,7 +68,7 @@ pub fn check(path: &str, settings: &GlobalSettings) -> i32 { if compare_by(a, b, settings, chunk.line_data(), chunk.line_data()) == Ordering::Greater { if !settings.check_silent { - println!("sort: {}:{}: disorder: {}", path, line_idx, b.line); + eprintln!("sort: {}:{}: disorder: {}", path, line_idx, b.line); } return 1; } diff --git a/tests/by-util/test_sort.rs b/tests/by-util/test_sort.rs index e594de0a7..1add0bb54 100644 --- a/tests/by-util/test_sort.rs +++ b/tests/by-util/test_sort.rs @@ -181,7 +181,7 @@ fn test_check_zero_terminated_failure() { .arg("-c") .arg("zero-terminated.txt") .fails() - .stdout_is("sort: zero-terminated.txt:2: disorder: ../../fixtures/du\n"); + .stderr_only("sort: zero-terminated.txt:2: disorder: ../../fixtures/du\n"); } #[test] @@ -775,13 +775,13 @@ fn test_check() { .arg(diagnose_arg) .arg("check_fail.txt") .fails() - .stdout_is("sort: check_fail.txt:6: disorder: 5\n"); + .stderr_only("sort: check_fail.txt:6: disorder: 5\n"); new_ucmd!() .arg(diagnose_arg) .arg("multiple_files.expected") .succeeds() - .stdout_is(""); + .stderr_is(""); } } From a33b6d87b578990893257c2b063f8579a3f960c4 Mon Sep 17 00:00:00 2001 From: Michael Debertol Date: Fri, 30 Jul 2021 20:48:07 +0200 Subject: [PATCH 41/64] sort: open the output file at the beginning This causes us to print an error message about an invalid output file right at the start of the invocation, but *after* verifying other arguments. --- src/uu/sort/src/ext_sort.rs | 18 ++++++++--- src/uu/sort/src/merge.rs | 6 ++-- src/uu/sort/src/sort.rs | 59 ++++++++++++++++++++++--------------- tests/by-util/test_sort.rs | 27 +++++++++++++++++ 4 files changed, 80 insertions(+), 30 deletions(-) diff --git a/src/uu/sort/src/ext_sort.rs b/src/uu/sort/src/ext_sort.rs index 201dda8bb..ea53900e2 100644 --- a/src/uu/sort/src/ext_sort.rs +++ b/src/uu/sort/src/ext_sort.rs @@ -28,6 +28,7 @@ use crate::merge::ClosedTmpFile; use crate::merge::WriteableCompressedTmpFile; use crate::merge::WriteablePlainTmpFile; use crate::merge::WriteableTmpFile; +use crate::Output; use crate::{ chunks::{self, Chunk}, compare_by, merge, sort_by, GlobalSettings, @@ -38,7 +39,11 @@ use tempfile::TempDir; const START_BUFFER_SIZE: usize = 8_000; /// Sort files by using auxiliary files for storing intermediate chunks (if needed), and output the result. -pub fn ext_sort(files: &mut impl Iterator>, settings: &GlobalSettings) { +pub fn ext_sort( + files: &mut impl Iterator>, + settings: &GlobalSettings, + output: Output, +) { let (sorted_sender, sorted_receiver) = std::sync::mpsc::sync_channel(1); let (recycled_sender, recycled_receiver) = std::sync::mpsc::sync_channel(1); thread::spawn({ @@ -51,6 +56,7 @@ pub fn ext_sort(files: &mut impl Iterator>, settings settings, sorted_receiver, recycled_sender, + output, ); } else { reader_writer::<_, WriteablePlainTmpFile>( @@ -58,6 +64,7 @@ pub fn ext_sort(files: &mut impl Iterator>, settings settings, sorted_receiver, recycled_sender, + output, ); } } @@ -67,6 +74,7 @@ fn reader_writer>, Tmp: WriteableTmpFile settings: &GlobalSettings, receiver: Receiver, sender: SyncSender, + output: Output, ) { let separator = if settings.zero_terminated { b'\0' @@ -96,7 +104,7 @@ fn reader_writer>, Tmp: WriteableTmpFile settings, Some((tmp_dir, tmp_dir_size)), ); - merger.write_all(settings); + merger.write_all(settings, output); } ReadResult::SortedSingleChunk(chunk) => { if settings.unique { @@ -106,9 +114,10 @@ fn reader_writer>, Tmp: WriteableTmpFile == Ordering::Equal }), settings, + output, ); } else { - print_sorted(chunk.lines().iter(), settings); + print_sorted(chunk.lines().iter(), settings, output); } } ReadResult::SortedTwoChunks([a, b]) => { @@ -128,9 +137,10 @@ fn reader_writer>, Tmp: WriteableTmpFile }) .map(|(line, _)| line), settings, + output, ); } else { - print_sorted(merged_iter.map(|(line, _)| line), settings); + print_sorted(merged_iter.map(|(line, _)| line), settings, output); } } ReadResult::EmptyInput => { diff --git a/src/uu/sort/src/merge.rs b/src/uu/sort/src/merge.rs index 12d7a9b9b..b8d69fb14 100644 --- a/src/uu/sort/src/merge.rs +++ b/src/uu/sort/src/merge.rs @@ -25,7 +25,7 @@ use tempfile::TempDir; use crate::{ chunks::{self, Chunk, RecycledChunk}, - compare_by, GlobalSettings, + compare_by, GlobalSettings, Output, }; /// Merge pre-sorted `Box`s. @@ -238,8 +238,8 @@ pub struct FileMerger<'a> { impl<'a> FileMerger<'a> { /// Write the merged contents to the output file. - pub fn write_all(&mut self, settings: &GlobalSettings) { - let mut out = settings.out_writer(); + pub fn write_all(&mut self, settings: &GlobalSettings, output: Output) { + let mut out = output.into_write(); self.write_all_to(settings, &mut out); } diff --git a/src/uu/sort/src/sort.rs b/src/uu/sort/src/sort.rs index 3d7e890b9..6e4cd8a23 100644 --- a/src/uu/sort/src/sort.rs +++ b/src/uu/sort/src/sort.rs @@ -37,7 +37,7 @@ use std::env; use std::ffi::OsStr; use std::fs::File; use std::hash::{Hash, Hasher}; -use std::io::{stdin, stdout, BufRead, BufReader, BufWriter, Read, Write}; +use std::io::{stdin, stdout, BufRead, BufReader, Read, Write}; use std::ops::Range; use std::path::Path; use std::path::PathBuf; @@ -146,6 +146,29 @@ impl SortMode { } } +pub struct Output { + file: Option, +} + +impl Output { + fn new(name: Option<&str>) -> Self { + Self { + file: name.map(|name| { + File::create(name).unwrap_or_else(|e| { + crash!(2, "open failed: {}: {}", name, strip_errno(&e.to_string())) + }) + }), + } + } + + fn into_write(self) -> Box { + match self.file { + Some(file) => Box::new(file), + None => Box::new(stdout()), + } + } +} + #[derive(Clone)] pub struct GlobalSettings { mode: SortMode, @@ -156,7 +179,6 @@ pub struct GlobalSettings { ignore_non_printing: bool, merge: bool, reverse: bool, - output_file: Option, stable: bool, unique: bool, check: bool, @@ -209,19 +231,6 @@ impl GlobalSettings { } } - fn out_writer(&self) -> BufWriter> { - match self.output_file { - Some(ref filename) => match File::create(Path::new(&filename)) { - Ok(f) => BufWriter::new(Box::new(f) as Box), - Err(e) => { - show_error!("{0}: {1}", filename, e.to_string()); - crash!(2, "Could not open output file"); - } - }, - None => BufWriter::new(Box::new(stdout()) as Box), - } - } - /// Precompute some data needed for sorting. /// This function **must** be called before starting to sort, and `GlobalSettings` may not be altered /// afterwards. @@ -253,7 +262,6 @@ impl Default for GlobalSettings { ignore_non_printing: false, merge: false, reverse: false, - output_file: None, stable: false, unique: false, check: false, @@ -1053,7 +1061,6 @@ pub fn uumain(args: impl uucore::Args) -> i32 { settings.ignore_leading_blanks = matches.is_present(options::IGNORE_LEADING_BLANKS); - settings.output_file = matches.value_of(options::OUTPUT).map(String::from); settings.reverse = matches.is_present(options::REVERSE); settings.stable = matches.is_present(options::STABLE); settings.unique = matches.is_present(options::UNIQUE); @@ -1099,9 +1106,11 @@ pub fn uumain(args: impl uucore::Args) -> i32 { ); } + let output = Output::new(matches.value_of(options::OUTPUT)); + settings.init_precomputed(); - exec(&files, &settings) + exec(&files, &settings, output) } pub fn uu_app() -> App<'static, 'static> { @@ -1334,10 +1343,10 @@ pub fn uu_app() -> App<'static, 'static> { .arg(Arg::with_name(options::FILES).multiple(true).takes_value(true)) } -fn exec(files: &[String], settings: &GlobalSettings) -> i32 { +fn exec(files: &[String], settings: &GlobalSettings, output: Output) -> i32 { if settings.merge { let mut file_merger = merge::merge(files.iter().map(open), settings); - file_merger.write_all(settings); + file_merger.write_all(settings, output); } else if settings.check { if files.len() > 1 { crash!(2, "only one file allowed with -c"); @@ -1346,7 +1355,7 @@ fn exec(files: &[String], settings: &GlobalSettings) -> i32 { } else { let mut lines = files.iter().map(open); - ext_sort(&mut lines, settings); + ext_sort(&mut lines, settings, output); } 0 } @@ -1618,8 +1627,12 @@ fn month_compare(a: &str, b: &str) -> Ordering { } } -fn print_sorted<'a, T: Iterator>>(iter: T, settings: &GlobalSettings) { - let mut writer = settings.out_writer(); +fn print_sorted<'a, T: Iterator>>( + iter: T, + settings: &GlobalSettings, + output: Output, +) { + let mut writer = output.into_write(); for line in iter { line.print(&mut writer, settings); } diff --git a/tests/by-util/test_sort.rs b/tests/by-util/test_sort.rs index 1add0bb54..b4b4ba41c 100644 --- a/tests/by-util/test_sort.rs +++ b/tests/by-util/test_sort.rs @@ -959,3 +959,30 @@ fn test_key_takes_one_arg() { .succeeds() .stdout_is_fixture("keys_open_ended.expected"); } + +#[test] +fn test_verifies_out_file() { + let inputs = ["" /* no input */, "some input"]; + for &input in &inputs { + new_ucmd!() + .args(&["-o", "nonexistent_dir/nonexistent_file"]) + .pipe_in(input) + .fails() + .status_code(2) + .stderr_only( + #[cfg(not(windows))] + "sort: open failed: nonexistent_dir/nonexistent_file: No such file or directory", + #[cfg(windows)] + "sort: open failed: nonexistent_dir/nonexistent_file: The system cannot find the path specified.", + ); + } +} + +#[test] +fn test_verifies_out_file_after_keys() { + new_ucmd!() + .args(&["-o", "nonexistent_dir/nonexistent_file", "-k", "0"]) + .fails() + .status_code(2) + .stderr_contains("failed to parse key"); +} From 1bb530eebb84b5ea8ef60f40671d46f15a91df4f Mon Sep 17 00:00:00 2001 From: Michael Debertol Date: Fri, 30 Jul 2021 21:02:49 +0200 Subject: [PATCH 42/64] sort: validate input files at startup --- src/uu/sort/src/sort.rs | 8 ++++++++ tests/by-util/test_sort.rs | 20 ++++++++++++++++++-- 2 files changed, 26 insertions(+), 2 deletions(-) diff --git a/src/uu/sort/src/sort.rs b/src/uu/sort/src/sort.rs index 6e4cd8a23..186b7dc67 100644 --- a/src/uu/sort/src/sort.rs +++ b/src/uu/sort/src/sort.rs @@ -1106,6 +1106,14 @@ pub fn uumain(args: impl uucore::Args) -> i32 { ); } + // Verify that we can open all input files. + // It is the correct behavior to close all files afterwards, + // and to reopen them at a later point. This is different from how the output file is handled, + // probably to prevent running out of file descriptors. + for file in &files { + open(file); + } + let output = Output::new(matches.value_of(options::OUTPUT)); settings.init_precomputed(); diff --git a/tests/by-util/test_sort.rs b/tests/by-util/test_sort.rs index b4b4ba41c..74241ef93 100644 --- a/tests/by-util/test_sort.rs +++ b/tests/by-util/test_sort.rs @@ -979,10 +979,26 @@ fn test_verifies_out_file() { } #[test] -fn test_verifies_out_file_after_keys() { +fn test_verifies_files_after_keys() { new_ucmd!() - .args(&["-o", "nonexistent_dir/nonexistent_file", "-k", "0"]) + .args(&[ + "-o", + "nonexistent_dir/nonexistent_file", + "-k", + "0", + "nonexistent_dir/input_file", + ]) .fails() .status_code(2) .stderr_contains("failed to parse key"); } + +#[test] +#[cfg(unix)] +fn test_verifies_input_files() { + new_ucmd!() + .args(&["/dev/random", "nonexistent_file"]) + .fails() + .status_code(2) + .stderr_is("sort: cannot read: nonexistent_file: No such file or directory"); +} From 47595050245d79cd5e54c4b5966e1be3fb4405b0 Mon Sep 17 00:00:00 2001 From: Michael Debertol Date: Fri, 30 Jul 2021 21:03:50 +0200 Subject: [PATCH 43/64] sort: split a line to make rustfmt work again --- src/uu/sort/src/sort.rs | 138 +++++++++++++++++++--------------------- 1 file changed, 64 insertions(+), 74 deletions(-) diff --git a/src/uu/sort/src/sort.rs b/src/uu/sort/src/sort.rs index 186b7dc67..3ed253ba4 100644 --- a/src/uu/sort/src/sort.rs +++ b/src/uu/sort/src/sort.rs @@ -1129,73 +1129,57 @@ pub fn uu_app() -> App<'static, 'static> { Arg::with_name(options::modes::SORT) .long(options::modes::SORT) .takes_value(true) - .possible_values( - &[ - "general-numeric", - "human-numeric", - "month", - "numeric", - "version", - "random", - ] - ) - .conflicts_with_all(&options::modes::ALL_SORT_MODES) - ) - .arg( - make_sort_mode_arg( - options::modes::HUMAN_NUMERIC, - "h", - "compare according to human readable sizes, eg 1M > 100k" - ), - ) - .arg( - make_sort_mode_arg( - options::modes::MONTH, - "M", - "compare according to month name abbreviation" - ), - ) - .arg( - make_sort_mode_arg( - options::modes::NUMERIC, - "n", - "compare according to string numerical value" - ), - ) - .arg( - make_sort_mode_arg( - options::modes::GENERAL_NUMERIC, - "g", - "compare according to string general numerical value" - ), - ) - .arg( - make_sort_mode_arg( - options::modes::VERSION, - "V", - "Sort by SemVer version number, eg 1.12.2 > 1.1.2", - ), - ) - .arg( - make_sort_mode_arg( - options::modes::RANDOM, - "R", - "shuffle in random order", - ), + .possible_values(&[ + "general-numeric", + "human-numeric", + "month", + "numeric", + "version", + "random", + ]) + .conflicts_with_all(&options::modes::ALL_SORT_MODES), ) + .arg(make_sort_mode_arg( + options::modes::HUMAN_NUMERIC, + "h", + "compare according to human readable sizes, eg 1M > 100k", + )) + .arg(make_sort_mode_arg( + options::modes::MONTH, + "M", + "compare according to month name abbreviation", + )) + .arg(make_sort_mode_arg( + options::modes::NUMERIC, + "n", + "compare according to string numerical value", + )) + .arg(make_sort_mode_arg( + options::modes::GENERAL_NUMERIC, + "g", + "compare according to string general numerical value", + )) + .arg(make_sort_mode_arg( + options::modes::VERSION, + "V", + "Sort by SemVer version number, eg 1.12.2 > 1.1.2", + )) + .arg(make_sort_mode_arg( + options::modes::RANDOM, + "R", + "shuffle in random order", + )) .arg( Arg::with_name(options::DICTIONARY_ORDER) .short("d") .long(options::DICTIONARY_ORDER) .help("consider only blanks and alphanumeric characters") - .conflicts_with_all( - &[ - options::modes::NUMERIC, - options::modes::GENERAL_NUMERIC, - options::modes::HUMAN_NUMERIC, - options::modes::MONTH, - ] - ), + .conflicts_with_all(&[ + options::modes::NUMERIC, + options::modes::GENERAL_NUMERIC, + options::modes::HUMAN_NUMERIC, + options::modes::MONTH, + ]), ) .arg( Arg::with_name(options::MERGE) @@ -1223,7 +1207,10 @@ pub fn uu_app() -> App<'static, 'static> { .short("C") .long(options::check::CHECK_SILENT) .conflicts_with(options::OUTPUT) - .help("exit successfully if the given file is already sorted, and exit with status 1 otherwise."), + .help( + "exit successfully if the given file is already sorted,\ + and exit with status 1 otherwise.", + ), ) .arg( Arg::with_name(options::IGNORE_CASE) @@ -1236,14 +1223,12 @@ pub fn uu_app() -> App<'static, 'static> { .short("i") .long(options::IGNORE_NONPRINTING) .help("ignore nonprinting characters") - .conflicts_with_all( - &[ - options::modes::NUMERIC, - options::modes::GENERAL_NUMERIC, - options::modes::HUMAN_NUMERIC, - options::modes::MONTH - ] - ), + .conflicts_with_all(&[ + options::modes::NUMERIC, + options::modes::GENERAL_NUMERIC, + options::modes::HUMAN_NUMERIC, + options::modes::MONTH, + ]), ) .arg( Arg::with_name(options::IGNORE_LEADING_BLANKS) @@ -1292,7 +1277,8 @@ pub fn uu_app() -> App<'static, 'static> { .short("t") .long(options::SEPARATOR) .help("custom separator for -k") - .takes_value(true)) + .takes_value(true), + ) .arg( Arg::with_name(options::ZERO_TERMINATED) .short("z") @@ -1327,13 +1313,13 @@ pub fn uu_app() -> App<'static, 'static> { .long(options::COMPRESS_PROG) .help("compress temporary files with PROG, decompress with PROG -d") .long_help("PROG has to take input from stdin and output to stdout") - .value_name("PROG") + .value_name("PROG"), ) .arg( Arg::with_name(options::BATCH_SIZE) .long(options::BATCH_SIZE) .help("Merge at most N_MERGE inputs at once.") - .value_name("N_MERGE") + .value_name("N_MERGE"), ) .arg( Arg::with_name(options::FILES0_FROM) @@ -1348,7 +1334,11 @@ pub fn uu_app() -> App<'static, 'static> { .long(options::DEBUG) .help("underline the parts of the line that are actually used for sorting"), ) - .arg(Arg::with_name(options::FILES).multiple(true).takes_value(true)) + .arg( + Arg::with_name(options::FILES) + .multiple(true) + .takes_value(true), + ) } fn exec(files: &[String], settings: &GlobalSettings, output: Output) -> i32 { From fb477360b29a2ef6f962e1f57b9c0ddc0804b13f Mon Sep 17 00:00:00 2001 From: Jeffrey Finkelstein Date: Fri, 30 Jul 2021 23:23:58 -0400 Subject: [PATCH 44/64] expand: expand support for --tabs arguments Add support for * space-separated list of tab stops, like `--tabs="2 4 6"`, * mixed comma- and space-separated lists, like `--tabs="2,4 6"`, * the slash specifier in the last tab stop, like `--tabs=1,/5`, * the plus specifier in the last tab stop, like `--tabs=1,+5`. --- src/uu/expand/src/expand.rs | 176 +++++++++++++++++++++++++++++------ tests/by-util/test_expand.rs | 137 +++++++++++++++++++++++++++ 2 files changed, 286 insertions(+), 27 deletions(-) diff --git a/src/uu/expand/src/expand.rs b/src/uu/expand/src/expand.rs index 66c3eb259..d5c37ce21 100644 --- a/src/uu/expand/src/expand.rs +++ b/src/uu/expand/src/expand.rs @@ -36,28 +36,86 @@ fn get_usage() -> String { format!("{0} [OPTION]... [FILE]...", executable!()) } -fn tabstops_parse(s: String) -> Vec { - let words = s.split(','); +/// The mode to use when replacing tabs beyond the last one specified in +/// the `--tabs` argument. +enum RemainingMode { + None, + Slash, + Plus, +} - let nums = words - .map(|sn| { - sn.parse::() - .unwrap_or_else(|_| crash!(1, "{}\n", "tab size contains invalid character(s)")) - }) - .collect::>(); +/// Decide whether the character is either a space or a comma. +/// +/// # Examples +/// +/// ```rust,ignore +/// assert!(is_space_or_comma(' ')) +/// assert!(is_space_or_comma(',')) +/// assert!(!is_space_or_comma('a')) +/// ``` +fn is_space_or_comma(c: char) -> bool { + c == ' ' || c == ',' +} - if nums.iter().any(|&n| n == 0) { - crash!(1, "{}\n", "tab size cannot be 0"); +/// Parse a list of tabstops from a `--tabs` argument. +/// +/// This function returns both the vector of numbers appearing in the +/// comma- or space-separated list, and also an optional mode, specified +/// by either a "/" or a "+" character appearing before the final number +/// in the list. This mode defines the strategy to use for computing the +/// number of spaces to use for columns beyond the end of the tab stop +/// list specified here. +fn tabstops_parse(s: String) -> (RemainingMode, Vec) { + // Leading commas and spaces are ignored. + let s = s.trim_start_matches(is_space_or_comma); + + // If there were only commas and spaces in the string, just use the + // default tabstops. + if s.is_empty() { + return (RemainingMode::None, vec![DEFAULT_TABSTOP]); } - if let (false, _) = nums - .iter() - .fold((true, 0), |(acc, last), &n| (acc && last <= n, n)) - { - crash!(1, "{}\n", "tab sizes must be ascending"); - } + let mut nums = vec![]; + let mut remaining_mode = RemainingMode::None; + for word in s.split(is_space_or_comma) { + let bytes = word.as_bytes(); + for i in 0..bytes.len() { + match bytes[i] { + b'+' => { + remaining_mode = RemainingMode::Plus; + } + b'/' => { + remaining_mode = RemainingMode::Slash; + } + _ => { + // Parse a number from the byte sequence. + let num = from_utf8(&bytes[i..]).unwrap().parse::().unwrap(); - nums + // Tab size must be positive. + if num == 0 { + crash!(1, "{}\n", "tab size cannot be 0"); + } + + // Tab sizes must be ascending. + if let Some(last_stop) = nums.last() { + if *last_stop >= num { + crash!(1, "tab sizes must be ascending"); + } + } + + // Append this tab stop to the list of all tabstops. + nums.push(num); + break; + } + } + } + } + // If no numbers could be parsed (for example, if `s` were "+,+,+"), + // then just use the default tabstops. + if nums.is_empty() { + nums = vec![DEFAULT_TABSTOP]; + } + (remaining_mode, nums) } struct Options { @@ -66,13 +124,17 @@ struct Options { tspaces: String, iflag: bool, uflag: bool, + + /// Strategy for expanding tabs for columns beyond those specified + /// in `tabstops`. + remaining_mode: RemainingMode, } impl Options { fn new(matches: &ArgMatches) -> Options { - let tabstops = match matches.value_of(options::TABS) { + let (remaining_mode, tabstops) = match matches.value_of(options::TABS) { Some(s) => tabstops_parse(s.to_string()), - None => vec![DEFAULT_TABSTOP], + None => (RemainingMode::None, vec![DEFAULT_TABSTOP]), }; let iflag = matches.is_present(options::INITIAL); @@ -102,6 +164,7 @@ impl Options { tspaces, iflag, uflag, + remaining_mode, } } } @@ -159,13 +222,41 @@ fn open(path: String) -> BufReader> { } } -fn next_tabstop(tabstops: &[usize], col: usize) -> usize { - if tabstops.len() == 1 { - tabstops[0] - col % tabstops[0] - } else { - match tabstops.iter().find(|&&t| t > col) { +/// Compute the number of spaces to the next tabstop. +/// +/// `tabstops` is the sequence of tabstop locations. +/// +/// `col` is the index of the current cursor in the line being written. +/// +/// If `remaining_mode` is [`RemainingMode::Plus`], then the last entry +/// in the `tabstops` slice is interpreted as a relative number of +/// spaces, which this function will return for every input value of +/// `col` beyond the end of the second-to-last element of `tabstops`. +/// +/// If `remaining_mode` is [`RemainingMode::Plus`], then the last entry +/// in the `tabstops` slice is interpreted as a relative number of +/// spaces, which this function will return for every input value of +/// `col` beyond the end of the second-to-last element of `tabstops`. +fn next_tabstop(tabstops: &[usize], col: usize, remaining_mode: &RemainingMode) -> usize { + let num_tabstops = tabstops.len(); + match remaining_mode { + RemainingMode::Plus => match tabstops[0..num_tabstops - 1].iter().find(|&&t| t > col) { Some(t) => t - col, - None => 1, + None => tabstops[num_tabstops - 1] - 1, + }, + RemainingMode::Slash => match tabstops[0..num_tabstops - 1].iter().find(|&&t| t > col) { + Some(t) => t - col, + None => tabstops[num_tabstops - 1] - col % tabstops[num_tabstops - 1], + }, + RemainingMode::None => { + if num_tabstops == 1 { + tabstops[0] - col % tabstops[0] + } else { + match tabstops.iter().find(|&&t| t > col) { + Some(t) => t - col, + None => 1, + } + } } } } @@ -232,12 +323,16 @@ fn expand(options: Options) { match ctype { Tab => { // figure out how many spaces to the next tabstop - let nts = next_tabstop(ts, col); + let nts = next_tabstop(ts, col, &options.remaining_mode); col += nts; // now dump out either spaces if we're expanding, or a literal tab if we're not if init || !options.iflag { - safe_unwrap!(output.write_all(options.tspaces[..nts].as_bytes())); + if nts <= options.tspaces.len() { + safe_unwrap!(output.write_all(options.tspaces[..nts].as_bytes())); + } else { + safe_unwrap!(output.write_all(" ".repeat(nts).as_bytes())); + }; } else { safe_unwrap!(output.write_all(&buf[byte..byte + nbytes])); } @@ -269,3 +364,30 @@ fn expand(options: Options) { } } } + +#[cfg(test)] +mod tests { + use super::next_tabstop; + use super::RemainingMode; + + #[test] + fn test_next_tabstop_remaining_mode_none() { + assert_eq!(next_tabstop(&[1, 5], 0, &RemainingMode::None), 1); + assert_eq!(next_tabstop(&[1, 5], 3, &RemainingMode::None), 2); + assert_eq!(next_tabstop(&[1, 5], 6, &RemainingMode::None), 1); + } + + #[test] + fn test_next_tabstop_remaining_mode_plus() { + assert_eq!(next_tabstop(&[1, 5], 0, &RemainingMode::Plus), 1); + assert_eq!(next_tabstop(&[1, 5], 3, &RemainingMode::Plus), 4); + assert_eq!(next_tabstop(&[1, 5], 6, &RemainingMode::Plus), 4); + } + + #[test] + fn test_next_tabstop_remaining_mode_slash() { + assert_eq!(next_tabstop(&[1, 5], 0, &RemainingMode::Slash), 1); + assert_eq!(next_tabstop(&[1, 5], 3, &RemainingMode::Slash), 2); + assert_eq!(next_tabstop(&[1, 5], 6, &RemainingMode::Slash), 4); + } +} diff --git a/tests/by-util/test_expand.rs b/tests/by-util/test_expand.rs index 834a09736..3d4ac71f3 100644 --- a/tests/by-util/test_expand.rs +++ b/tests/by-util/test_expand.rs @@ -53,3 +53,140 @@ fn test_with_multiple_files() { .stdout_contains(" return") .stdout_contains(" "); } + +#[test] +fn test_tabs_space_separated_list() { + new_ucmd!() + .args(&["--tabs", "3 6 9"]) + .pipe_in("a\tb\tc\td\te") + .succeeds() + .stdout_is("a b c d e"); +} + +#[test] +fn test_tabs_mixed_style_list() { + new_ucmd!() + .args(&["--tabs", ", 3,6 9"]) + .pipe_in("a\tb\tc\td\te") + .succeeds() + .stdout_is("a b c d e"); +} + +#[test] +fn test_tabs_empty_string() { + new_ucmd!() + .args(&["--tabs", ""]) + .pipe_in("a\tb\tc") + .succeeds() + .stdout_is("a b c"); +} + +#[test] +fn test_tabs_comma_only() { + new_ucmd!() + .args(&["--tabs", ","]) + .pipe_in("a\tb\tc") + .succeeds() + .stdout_is("a b c"); +} + +#[test] +fn test_tabs_space_only() { + new_ucmd!() + .args(&["--tabs", " "]) + .pipe_in("a\tb\tc") + .succeeds() + .stdout_is("a b c"); +} + +#[test] +fn test_tabs_slash() { + new_ucmd!() + .args(&["--tabs", "/"]) + .pipe_in("a\tb\tc") + .succeeds() + .stdout_is("a b c"); +} + +#[test] +fn test_tabs_plus() { + new_ucmd!() + .args(&["--tabs", "+"]) + .pipe_in("a\tb\tc") + .succeeds() + .stdout_is("a b c"); +} + +#[test] +fn test_tabs_trailing_slash() { + new_ucmd!() + .arg("--tabs=1,/5") + .pipe_in("\ta\tb\tc") + .succeeds() + // 0 1 + // 01234567890 + .stdout_is(" a b c"); +} + +#[test] +fn test_tabs_trailing_slash_long_columns() { + new_ucmd!() + .arg("--tabs=1,/3") + .pipe_in("\taaaa\tbbbb\tcccc") + .succeeds() + // 0 1 + // 01234567890123456 + .stdout_is(" aaaa bbbb cccc"); +} + +#[test] +fn test_tabs_trailing_plus() { + new_ucmd!() + .arg("--tabs=1,+5") + .pipe_in("\ta\tb\tc") + .succeeds() + // 0 1 + // 012345678901 + .stdout_is(" a b c"); +} + +#[test] +fn test_tabs_trailing_plus_long_columns() { + new_ucmd!() + .arg("--tabs=1,+3") + .pipe_in("\taaaa\tbbbb\tcccc") + .succeeds() + // 0 1 + // 012345678901234567 + .stdout_is(" aaaa bbbb cccc"); +} + +#[test] +fn test_tabs_must_be_ascending() { + new_ucmd!() + .arg("--tabs=1,1") + .fails() + .stderr_contains("tab sizes must be ascending"); +} + +#[test] +fn test_tabs_keep_last_trailing_specifier() { + // If there are multiple trailing specifiers, use only the last one + // before the number. + new_ucmd!() + .arg("--tabs=1,+/+/5") + .pipe_in("\ta\tb\tc") + .succeeds() + // 0 1 + // 01234567890 + .stdout_is(" a b c"); +} + +#[test] +fn test_tabs_comma_separated_no_numbers() { + new_ucmd!() + .arg("--tabs=+,/,+,/") + .pipe_in("\ta\tb\tc") + .succeeds() + .stdout_is(" a b c"); +} From 6b73ddcd12bef3f1c911654900454d6ef84826f7 Mon Sep 17 00:00:00 2001 From: Sylvestre Ledru Date: Sat, 31 Jul 2021 10:38:32 +0200 Subject: [PATCH 45/64] Silent the spell checker on a test examples --- tests/by-util/test_expand.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/by-util/test_expand.rs b/tests/by-util/test_expand.rs index 3d4ac71f3..9c06fe904 100644 --- a/tests/by-util/test_expand.rs +++ b/tests/by-util/test_expand.rs @@ -1,4 +1,5 @@ use crate::common::util::*; +// spell-checker:ignore (ToDO) taaaa tbbbb tcccc #[test] fn test_with_tab() { From 095e53aed2ae5b116da857eb382888686234abbf Mon Sep 17 00:00:00 2001 From: Michael Debertol Date: Mon, 26 Jul 2021 17:38:53 +0200 Subject: [PATCH 46/64] sort: disallow equal lines for --check with --unique --- src/uu/sort/src/check.rs | 12 +++++++++--- tests/by-util/test_sort.rs | 12 ++++++++++++ 2 files changed, 21 insertions(+), 3 deletions(-) diff --git a/src/uu/sort/src/check.rs b/src/uu/sort/src/check.rs index 44c71674e..de320ef77 100644 --- a/src/uu/sort/src/check.rs +++ b/src/uu/sort/src/check.rs @@ -26,6 +26,13 @@ use std::{ /// /// The code we should exit with. pub fn check(path: &str, settings: &GlobalSettings) -> i32 { + let max_allowed_cmp = if settings.unique { + // If `unique` is enabled, the previous line must compare _less_ to the next one. + Ordering::Less + } else { + // Otherwise, the line previous line must compare _less or equal_ to the next one. + Ordering::Equal + }; let file = open(path); let (recycled_sender, recycled_receiver) = sync_channel(2); let (loaded_sender, loaded_receiver) = sync_channel(2); @@ -53,7 +60,7 @@ pub fn check(path: &str, settings: &GlobalSettings) -> i32 { settings, prev_chunk.line_data(), chunk.line_data(), - ) == Ordering::Greater + ) > max_allowed_cmp { if !settings.check_silent { eprintln!("sort: {}:{}: disorder: {}", path, line_idx, new_first.line); @@ -65,8 +72,7 @@ pub fn check(path: &str, settings: &GlobalSettings) -> i32 { for (a, b) in chunk.lines().iter().tuple_windows() { line_idx += 1; - if compare_by(a, b, settings, chunk.line_data(), chunk.line_data()) == Ordering::Greater - { + if compare_by(a, b, settings, chunk.line_data(), chunk.line_data()) > max_allowed_cmp { if !settings.check_silent { eprintln!("sort: {}:{}: disorder: {}", path, line_idx, b.line); } diff --git a/tests/by-util/test_sort.rs b/tests/by-util/test_sort.rs index 5f44ce35f..a5ca80de9 100644 --- a/tests/by-util/test_sort.rs +++ b/tests/by-util/test_sort.rs @@ -796,6 +796,18 @@ fn test_check_silent() { } } +#[test] +fn test_check_unique() { + // Due to a clap bug the combination "-cu" does not work. "-c -u" works. + // See https://github.com/clap-rs/clap/issues/2624 + new_ucmd!() + .args(&["-c", "-u"]) + .pipe_in("A\nA\n") + .fails() + .code_is(1) + .stderr_only("sort: -:2: disorder: A"); +} + #[test] fn test_dictionary_and_nonprinting_conflicts() { let conflicting_args = ["n", "h", "g", "M"]; From f851fb64544b83b24d00d1826252c35dedd6c819 Mon Sep 17 00:00:00 2001 From: Michael Debertol Date: Mon, 26 Jul 2021 12:28:29 +0200 Subject: [PATCH 47/64] sort: initialize the salt when the random setting is on a key as well Additionall, change the type of `salt` from `String` to `Option<[u8; 16]>`. --- src/uu/sort/src/sort.rs | 32 ++++++++++++++------------------ tests/by-util/test_sort.rs | 37 ++++++++++++------------------------- 2 files changed, 26 insertions(+), 43 deletions(-) diff --git a/src/uu/sort/src/sort.rs b/src/uu/sort/src/sort.rs index 4362863d5..06bcc7ff8 100644 --- a/src/uu/sort/src/sort.rs +++ b/src/uu/sort/src/sort.rs @@ -29,7 +29,6 @@ use custom_str_cmp::custom_str_cmp; use ext_sort::ext_sort; use fnv::FnvHasher; use numeric_str_cmp::{human_numeric_str_cmp, numeric_str_cmp, NumInfo, NumInfoParseSettings}; -use rand::distributions::Alphanumeric; use rand::{thread_rng, Rng}; use rayon::prelude::*; use std::cmp::Ordering; @@ -183,7 +182,7 @@ pub struct GlobalSettings { unique: bool, check: bool, check_silent: bool, - salt: String, + salt: Option<[u8; 16]>, selectors: Vec, separator: Option, threads: String, @@ -266,7 +265,7 @@ impl Default for GlobalSettings { unique: false, check: false, check_silent: false, - salt: String::new(), + salt: None, selectors: vec![], separator: None, threads: String::new(), @@ -1006,7 +1005,7 @@ pub fn uumain(args: impl uucore::Args) -> i32 { } else if matches.is_present(options::modes::RANDOM) || matches.value_of(options::modes::SORT) == Some("random") { - settings.salt = get_rand_string(); + settings.salt = Some(get_rand_string()); SortMode::Random } else { SortMode::Default @@ -1086,9 +1085,11 @@ pub fn uumain(args: impl uucore::Args) -> i32 { if let Some(values) = matches.values_of(options::KEY) { for value in values { - settings - .selectors - .push(FieldSelector::parse(value, &settings)); + let selector = FieldSelector::parse(value, &settings); + if selector.settings.mode == SortMode::Random && settings.salt.is_none() { + settings.salt = Some(get_rand_string()); + } + settings.selectors.push(selector); } } @@ -1397,7 +1398,7 @@ fn compare_by<'a>( let settings = &selector.settings; let cmp: Ordering = match settings.mode { - SortMode::Random => random_shuffle(a_str, b_str, &global_settings.salt), + SortMode::Random => random_shuffle(a_str, b_str, &global_settings.salt.unwrap()), SortMode::Numeric => { let a_num_info = &a_line_data.num_infos [a.index * global_settings.precomputed.num_infos_per_line + num_info_index]; @@ -1546,12 +1547,8 @@ fn general_numeric_compare(a: &GeneralF64ParseResult, b: &GeneralF64ParseResult) a.partial_cmp(b).unwrap() } -fn get_rand_string() -> String { - thread_rng() - .sample_iter(&Alphanumeric) - .take(16) - .map(char::from) - .collect::() +fn get_rand_string() -> [u8; 16] { + thread_rng().sample(rand::distributions::Standard) } fn get_hash(t: &T) -> u64 { @@ -1560,10 +1557,9 @@ fn get_hash(t: &T) -> u64 { s.finish() } -fn random_shuffle(a: &str, b: &str, salt: &str) -> Ordering { - let da = get_hash(&[a, salt].concat()); - let db = get_hash(&[b, salt].concat()); - +fn random_shuffle(a: &str, b: &str, salt: &[u8]) -> Ordering { + let da = get_hash(&(a, salt)); + let db = get_hash(&(b, salt)); da.cmp(&db) } diff --git a/tests/by-util/test_sort.rs b/tests/by-util/test_sort.rs index 5f44ce35f..16437d526 100644 --- a/tests/by-util/test_sort.rs +++ b/tests/by-util/test_sort.rs @@ -220,32 +220,19 @@ fn test_random_shuffle_contains_all_lines() { #[test] fn test_random_shuffle_two_runs_not_the_same() { - // check to verify that two random shuffles are not equal; this has the - // potential to fail in the very unlikely event that the random order is the same - // as the starting order, or if both random sorts end up having the same order. - const FILE: &str = "default_unsorted_ints.expected"; - let (at, _ucmd) = at_and_ucmd!(); - let result = new_ucmd!().arg("-R").arg(FILE).run().stdout_move_str(); - let expected = at.read(FILE); - let unexpected = new_ucmd!().arg("-R").arg(FILE).run().stdout_move_str(); + for arg in &["-R", "-k1,1R"] { + // check to verify that two random shuffles are not equal; this has the + // potential to fail in the very unlikely event that the random order is the same + // as the starting order, or if both random sorts end up having the same order. + const FILE: &str = "default_unsorted_ints.expected"; + let (at, _ucmd) = at_and_ucmd!(); + let result = new_ucmd!().arg(arg).arg(FILE).run().stdout_move_str(); + let expected = at.read(FILE); + let unexpected = new_ucmd!().arg(arg).arg(FILE).run().stdout_move_str(); - assert_ne!(result, expected); - assert_ne!(result, unexpected); -} - -#[test] -fn test_random_shuffle_contains_two_runs_not_the_same() { - // check to verify that two random shuffles are not equal; this has the - // potential to fail in the unlikely event that random order is the same - // as the starting order, or if both random sorts end up having the same order. - const FILE: &str = "default_unsorted_ints.expected"; - let (at, _ucmd) = at_and_ucmd!(); - let result = new_ucmd!().arg("-R").arg(FILE).run().stdout_move_str(); - let expected = at.read(FILE); - let unexpected = new_ucmd!().arg("-R").arg(FILE).run().stdout_move_str(); - - assert_ne!(result, expected); - assert_ne!(result, unexpected); + assert_ne!(result, expected); + assert_ne!(result, unexpected); + } } #[test] From 3564dc57924acd958a77085836dd34f9fed0fd76 Mon Sep 17 00:00:00 2001 From: Michael Debertol Date: Mon, 26 Jul 2021 16:27:24 +0200 Subject: [PATCH 48/64] sort: compare strings before comparing hashes Since lines that compare equal should be sorted together, we need to first compare the lines (taking settings into account). Only if they do not compare equal we should compare the hashes. --- src/uu/sort/src/sort.rs | 17 ++++++++++++++++- tests/by-util/test_sort.rs | 10 ++++++++++ 2 files changed, 26 insertions(+), 1 deletion(-) diff --git a/src/uu/sort/src/sort.rs b/src/uu/sort/src/sort.rs index 06bcc7ff8..aa4d483c0 100644 --- a/src/uu/sort/src/sort.rs +++ b/src/uu/sort/src/sort.rs @@ -1398,7 +1398,22 @@ fn compare_by<'a>( let settings = &selector.settings; let cmp: Ordering = match settings.mode { - SortMode::Random => random_shuffle(a_str, b_str, &global_settings.salt.unwrap()), + SortMode::Random => { + // check if the two strings are equal + if custom_str_cmp( + a_str, + b_str, + settings.ignore_non_printing, + settings.dictionary_order, + settings.ignore_case, + ) == Ordering::Equal + { + Ordering::Equal + } else { + // Only if they are not equal compare by the hash + random_shuffle(a_str, b_str, &global_settings.salt.unwrap()) + } + } SortMode::Numeric => { let a_num_info = &a_line_data.num_infos [a.index * global_settings.precomputed.num_infos_per_line + num_info_index]; diff --git a/tests/by-util/test_sort.rs b/tests/by-util/test_sort.rs index 16437d526..8573ce03f 100644 --- a/tests/by-util/test_sort.rs +++ b/tests/by-util/test_sort.rs @@ -235,6 +235,16 @@ fn test_random_shuffle_two_runs_not_the_same() { } } +#[test] +fn test_random_ignore_case() { + let input = "ABC\nABc\nAbC\nAbc\naBC\naBc\nabC\nabc\n"; + new_ucmd!() + .args(&["-fR"]) + .pipe_in(input) + .succeeds() + .stdout_is(input); +} + #[test] fn test_numeric_floats_and_ints() { test_helper( From 849086e9c5c2dd24ca956e89a255cafa7c494cb2 Mon Sep 17 00:00:00 2001 From: Michael Debertol Date: Sat, 31 Jul 2021 20:12:38 +0200 Subject: [PATCH 49/64] sort: handle cases where the output file is also an input file In such cases we have to create a temporary copy of the input file to prevent overwriting the input with the output. This only affects merge sort, because it is the only mode where we start writing to the output before having read all inputs. --- src/uu/sort/src/check.rs | 17 ++++++++-- src/uu/sort/src/merge.rs | 59 ++++++++++++++++++++++++++------ src/uu/sort/src/sort.rs | 69 +++++++++++++++++++++++++------------- tests/by-util/test_sort.rs | 10 ++++++ 4 files changed, 119 insertions(+), 36 deletions(-) diff --git a/src/uu/sort/src/check.rs b/src/uu/sort/src/check.rs index de320ef77..350a98e23 100644 --- a/src/uu/sort/src/check.rs +++ b/src/uu/sort/src/check.rs @@ -14,6 +14,7 @@ use crate::{ use itertools::Itertools; use std::{ cmp::Ordering, + ffi::OsStr, io::Read, iter, sync::mpsc::{sync_channel, Receiver, SyncSender}, @@ -25,7 +26,7 @@ use std::{ /// # Returns /// /// The code we should exit with. -pub fn check(path: &str, settings: &GlobalSettings) -> i32 { +pub fn check(path: &OsStr, settings: &GlobalSettings) -> i32 { let max_allowed_cmp = if settings.unique { // If `unique` is enabled, the previous line must compare _less_ to the next one. Ordering::Less @@ -63,7 +64,12 @@ pub fn check(path: &str, settings: &GlobalSettings) -> i32 { ) > max_allowed_cmp { if !settings.check_silent { - eprintln!("sort: {}:{}: disorder: {}", path, line_idx, new_first.line); + eprintln!( + "sort: {}:{}: disorder: {}", + path.to_string_lossy(), + line_idx, + new_first.line + ); } return 1; } @@ -74,7 +80,12 @@ pub fn check(path: &str, settings: &GlobalSettings) -> i32 { line_idx += 1; if compare_by(a, b, settings, chunk.line_data(), chunk.line_data()) > max_allowed_cmp { if !settings.check_silent { - eprintln!("sort: {}:{}: disorder: {}", path, line_idx, b.line); + eprintln!( + "sort: {}:{}: disorder: {}", + path.to_string_lossy(), + line_idx, + b.line + ); } return 1; } diff --git a/src/uu/sort/src/merge.rs b/src/uu/sort/src/merge.rs index b8d69fb14..4a45028f7 100644 --- a/src/uu/sort/src/merge.rs +++ b/src/uu/sort/src/merge.rs @@ -9,10 +9,11 @@ use std::{ cmp::Ordering, + ffi::OsString, fs::{self, File}, io::{BufWriter, Read, Write}, iter, - path::PathBuf, + path::{Path, PathBuf}, process::{Child, ChildStdin, ChildStdout, Command, Stdio}, rc::Rc, sync::mpsc::{channel, sync_channel, Receiver, Sender, SyncSender}, @@ -25,28 +26,66 @@ use tempfile::TempDir; use crate::{ chunks::{self, Chunk, RecycledChunk}, - compare_by, GlobalSettings, Output, + compare_by, open, GlobalSettings, Output, }; +/// If the output file occurs in the input files as well, copy the contents of the output file +/// and replace its occurrences in the inputs with that copy. +fn replace_output_file_in_input_files( + files: &mut [OsString], + settings: &GlobalSettings, + output: Option<&str>, +) -> Option<(TempDir, usize)> { + let mut copy: Option<(TempDir, PathBuf)> = None; + if let Some(Ok(output_path)) = output.map(|path| Path::new(path).canonicalize()) { + for file in files { + if let Ok(file_path) = Path::new(file).canonicalize() { + if file_path == output_path { + if let Some((_dir, copy)) = © { + *file = copy.clone().into_os_string(); + } else { + let tmp_dir = tempfile::Builder::new() + .prefix("uutils_sort") + .tempdir_in(&settings.tmp_dir) + .unwrap(); + let copy_path = tmp_dir.path().join("0"); + std::fs::copy(file_path, ©_path).unwrap(); + *file = copy_path.clone().into_os_string(); + copy = Some((tmp_dir, copy_path)) + } + } + } + } + } + // if we created a TempDir its size must be one. + copy.map(|(dir, _copy)| (dir, 1)) +} + /// Merge pre-sorted `Box`s. /// /// If `settings.merge_batch_size` is greater than the length of `files`, intermediate files will be used. /// If `settings.compress_prog` is `Some`, intermediate files will be compressed with it. -pub fn merge>>( - files: Files, - settings: &GlobalSettings, -) -> FileMerger { +pub fn merge<'a>( + files: &mut [OsString], + settings: &'a GlobalSettings, + output: Option<&str>, +) -> FileMerger<'a> { + let tmp_dir = replace_output_file_in_input_files(files, settings, output); if settings.compress_prog.is_none() { merge_with_file_limit::<_, _, WriteablePlainTmpFile>( - files.map(|file| PlainMergeInput { inner: file }), + files + .iter() + .map(|file| PlainMergeInput { inner: open(file) }), settings, - None, + tmp_dir, ) } else { merge_with_file_limit::<_, _, WriteableCompressedTmpFile>( - files.map(|file| PlainMergeInput { inner: file }), + files + .iter() + .map(|file| PlainMergeInput { inner: open(file) }), settings, - None, + tmp_dir, ) } } diff --git a/src/uu/sort/src/sort.rs b/src/uu/sort/src/sort.rs index aa4d483c0..1f674e549 100644 --- a/src/uu/sort/src/sort.rs +++ b/src/uu/sort/src/sort.rs @@ -33,8 +33,8 @@ use rand::{thread_rng, Rng}; use rayon::prelude::*; use std::cmp::Ordering; use std::env; -use std::ffi::OsStr; -use std::fs::File; +use std::ffi::{OsStr, OsString}; +use std::fs::{File, OpenOptions}; use std::hash::{Hash, Hasher}; use std::io::{stdin, stdout, BufRead, BufReader, Read, Write}; use std::ops::Range; @@ -146,26 +146,46 @@ impl SortMode { } pub struct Output { - file: Option, + file: Option<(String, File)>, } impl Output { fn new(name: Option<&str>) -> Self { Self { file: name.map(|name| { - File::create(name).unwrap_or_else(|e| { - crash!(2, "open failed: {}: {}", name, strip_errno(&e.to_string())) - }) + // This is different from `File::create()` because we don't truncate the output yet. + // This allows using the output file as an input file. + ( + name.to_owned(), + OpenOptions::new() + .write(true) + .create(true) + .open(name) + .unwrap_or_else(|e| { + crash!(2, "open failed: {}: {}", name, strip_errno(&e.to_string())) + }), + ) }), } } fn into_write(self) -> Box { match self.file { - Some(file) => Box::new(file), + Some((_name, file)) => { + // truncate the file + file.set_len(0).unwrap(); + Box::new(file) + } None => Box::new(stdout()), } } + + fn as_output_name(&self) -> Option<&str> { + match &self.file { + Some((name, _file)) => Some(name), + None => None, + } + } } #[derive(Clone)] @@ -956,29 +976,28 @@ pub fn uumain(args: impl uucore::Args) -> i32 { settings.debug = matches.is_present(options::DEBUG); // check whether user specified a zero terminated list of files for input, otherwise read files from args - let mut files: Vec = if matches.is_present(options::FILES0_FROM) { - let files0_from: Vec = matches - .values_of(options::FILES0_FROM) - .map(|v| v.map(ToString::to_string).collect()) + let mut files: Vec = if matches.is_present(options::FILES0_FROM) { + let files0_from: Vec = matches + .values_of_os(options::FILES0_FROM) + .map(|v| v.map(ToOwned::to_owned).collect()) .unwrap_or_default(); let mut files = Vec::new(); for path in &files0_from { - let reader = open(path.as_str()); + let reader = open(&path); let buf_reader = BufReader::new(reader); for line in buf_reader.split(b'\0').flatten() { - files.push( + files.push(OsString::from( std::str::from_utf8(&line) - .expect("Could not parse string from zero terminated input.") - .to_string(), - ); + .expect("Could not parse string from zero terminated input."), + )); } } files } else { matches - .values_of(options::FILES) - .map(|v| v.map(ToString::to_string).collect()) + .values_of_os(options::FILES) + .map(|v| v.map(ToOwned::to_owned).collect()) .unwrap_or_default() }; @@ -1066,9 +1085,13 @@ pub fn uumain(args: impl uucore::Args) -> i32 { if files.is_empty() { /* if no file, default to stdin */ - files.push("-".to_owned()); + files.push("-".to_string().into()); } else if settings.check && files.len() != 1 { - crash!(2, "extra operand `{}' not allowed with -c", files[1]) + crash!( + 2, + "extra operand `{}' not allowed with -c", + files[1].to_string_lossy() + ) } if let Some(arg) = matches.args.get(options::SEPARATOR) { @@ -1122,7 +1145,7 @@ pub fn uumain(args: impl uucore::Args) -> i32 { settings.init_precomputed(); - exec(&files, &settings, output) + exec(&mut files, &settings, output) } pub fn uu_app() -> App<'static, 'static> { @@ -1345,9 +1368,9 @@ pub fn uu_app() -> App<'static, 'static> { ) } -fn exec(files: &[String], settings: &GlobalSettings, output: Output) -> i32 { +fn exec(files: &mut [OsString], settings: &GlobalSettings, output: Output) -> i32 { if settings.merge { - let mut file_merger = merge::merge(files.iter().map(open), settings); + let mut file_merger = merge::merge(files, settings, output.as_output_name()); file_merger.write_all(settings, output); } else if settings.check { if files.len() > 1 { diff --git a/tests/by-util/test_sort.rs b/tests/by-util/test_sort.rs index b9cdb2b80..4c4a7a697 100644 --- a/tests/by-util/test_sort.rs +++ b/tests/by-util/test_sort.rs @@ -1020,3 +1020,13 @@ fn test_separator_null() { .succeeds() .stdout_only("a\0z\0z\nz\0b\0a\nz\0a\0b\n"); } + +#[test] +fn test_output_is_input() { + let input = "a\nb\nc\n"; + let (at, mut cmd) = at_and_ucmd!(); + at.touch("file"); + at.append("file", input); + cmd.args(&["-m", "-o", "file", "file"]).succeeds(); + assert_eq!(at.read("file"), input); +} From f29239beecacadd085ef2f8d4e8bb96a47f20466 Mon Sep 17 00:00:00 2001 From: Michael Debertol Date: Sat, 31 Jul 2021 16:15:22 +0200 Subject: [PATCH 50/64] sort: buffer writes to the output This fixes a regression from a33b6d87b578990893257c2b063f8579a3f960c4 --- src/uu/sort/src/sort.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/uu/sort/src/sort.rs b/src/uu/sort/src/sort.rs index 1f674e549..13c2c3cfa 100644 --- a/src/uu/sort/src/sort.rs +++ b/src/uu/sort/src/sort.rs @@ -36,7 +36,7 @@ use std::env; use std::ffi::{OsStr, OsString}; use std::fs::{File, OpenOptions}; use std::hash::{Hash, Hasher}; -use std::io::{stdin, stdout, BufRead, BufReader, Read, Write}; +use std::io::{stdin, stdout, BufRead, BufReader, BufWriter, Read, Write}; use std::ops::Range; use std::path::Path; use std::path::PathBuf; @@ -169,15 +169,15 @@ impl Output { } } - fn into_write(self) -> Box { - match self.file { + fn into_write(self) -> BufWriter> { + BufWriter::new(match self.file { Some((_name, file)) => { // truncate the file file.set_len(0).unwrap(); Box::new(file) } None => Box::new(stdout()), - } + }) } fn as_output_name(&self) -> Option<&str> { From 5bf4536bdde00f1268e3172d6aad3ef86c56f8e2 Mon Sep 17 00:00:00 2001 From: Michael Debertol Date: Sat, 31 Jul 2021 19:40:38 +0200 Subject: [PATCH 51/64] sort: ignore failure to truncate the output file --- src/uu/sort/src/sort.rs | 2 +- tests/by-util/test_sort.rs | 9 +++++++++ 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/src/uu/sort/src/sort.rs b/src/uu/sort/src/sort.rs index 13c2c3cfa..7c855ab19 100644 --- a/src/uu/sort/src/sort.rs +++ b/src/uu/sort/src/sort.rs @@ -173,7 +173,7 @@ impl Output { BufWriter::new(match self.file { Some((_name, file)) => { // truncate the file - file.set_len(0).unwrap(); + let _ = file.set_len(0); Box::new(file) } None => Box::new(stdout()), diff --git a/tests/by-util/test_sort.rs b/tests/by-util/test_sort.rs index 4c4a7a697..36bed4b94 100644 --- a/tests/by-util/test_sort.rs +++ b/tests/by-util/test_sort.rs @@ -1030,3 +1030,12 @@ fn test_output_is_input() { cmd.args(&["-m", "-o", "file", "file"]).succeeds(); assert_eq!(at.read("file"), input); } + +#[test] +#[cfg(unix)] +fn test_output_device() { + new_ucmd!() + .args(&["-o", "/dev/null"]) + .pipe_in("input") + .succeeds(); +} From 418f5b7692f3e487c875a4b6252e04e8b00a3130 Mon Sep 17 00:00:00 2001 From: Michael Debertol Date: Sat, 31 Jul 2021 20:19:11 +0200 Subject: [PATCH 52/64] sort: handle empty merge inputs --- src/uu/sort/src/merge.rs | 14 ++++++++------ tests/by-util/test_sort.rs | 9 +++++++++ tests/fixtures/sort/empty.txt | 0 3 files changed, 17 insertions(+), 6 deletions(-) create mode 100644 tests/fixtures/sort/empty.txt diff --git a/src/uu/sort/src/merge.rs b/src/uu/sort/src/merge.rs index 4a45028f7..a5ac9411b 100644 --- a/src/uu/sort/src/merge.rs +++ b/src/uu/sort/src/merge.rs @@ -194,12 +194,14 @@ fn merge_without_limit>( let mut mergeable_files = vec![]; for (file_number, receiver) in loaded_receivers.into_iter().enumerate() { - mergeable_files.push(MergeableFile { - current_chunk: Rc::new(receiver.recv().unwrap()), - file_number, - line_idx: 0, - receiver, - }) + if let Ok(chunk) = receiver.recv() { + mergeable_files.push(MergeableFile { + current_chunk: Rc::new(chunk), + file_number, + line_idx: 0, + receiver, + }) + } } FileMerger { diff --git a/tests/by-util/test_sort.rs b/tests/by-util/test_sort.rs index 36bed4b94..8e9861a39 100644 --- a/tests/by-util/test_sort.rs +++ b/tests/by-util/test_sort.rs @@ -1039,3 +1039,12 @@ fn test_output_device() { .pipe_in("input") .succeeds(); } + +#[test] +fn test_merge_empty_input() { + new_ucmd!() + .args(&["-m", "empty.txt"]) + .succeeds() + .no_stderr() + .no_stdout(); +} diff --git a/tests/fixtures/sort/empty.txt b/tests/fixtures/sort/empty.txt new file mode 100644 index 000000000..e69de29bb From 663c9751a164659222859a96a2ff695d3bcced72 Mon Sep 17 00:00:00 2001 From: Michael Debertol Date: Sat, 31 Jul 2021 23:58:33 +0200 Subject: [PATCH 53/64] sort: do not exit with failure for "--version" or "--help" --- src/uu/sort/src/sort.rs | 20 +++++++++++++++++--- tests/by-util/test_sort.rs | 17 +++++++++++++++++ 2 files changed, 34 insertions(+), 3 deletions(-) diff --git a/src/uu/sort/src/sort.rs b/src/uu/sort/src/sort.rs index aa4d483c0..3bb5eea4e 100644 --- a/src/uu/sort/src/sort.rs +++ b/src/uu/sort/src/sort.rs @@ -949,9 +949,23 @@ pub fn uumain(args: impl uucore::Args) -> i32 { let usage = get_usage(); let mut settings: GlobalSettings = Default::default(); - let matches = uu_app().usage(&usage[..]).get_matches_from_safe(args); - - let matches = crash_if_err!(2, matches); + let matches = match uu_app().usage(&usage[..]).get_matches_from_safe(args) { + Ok(t) => t, + Err(e) => { + // not all clap "Errors" are because of a failure to parse arguments. + // "--version" also causes an Error to be returned, but we should not print to stderr + // nor return with a non-zero exit code in this case (we should print to stdout and return 0). + // This logic is similar to the code in clap, but we return 2 as the exit code in case of real failure + // (clap returns 1). + if e.use_stderr() { + eprintln!("{}", e.message); + return 2; + } else { + println!("{}", e.message); + return 0; + } + } + }; settings.debug = matches.is_present(options::DEBUG); diff --git a/tests/by-util/test_sort.rs b/tests/by-util/test_sort.rs index b9cdb2b80..0b7436861 100644 --- a/tests/by-util/test_sort.rs +++ b/tests/by-util/test_sort.rs @@ -1020,3 +1020,20 @@ fn test_separator_null() { .succeeds() .stdout_only("a\0z\0z\nz\0b\0a\nz\0a\0b\n"); } + +#[test] +fn test_no_error_for_version() { + new_ucmd!() + .arg("--version") + .succeeds() + .stdout_contains("sort"); +} + +#[test] +fn test_wrong_args_exit_code() { + new_ucmd!() + .arg("--misspelled") + .fails() + .status_code(2) + .stderr_contains("--misspelled"); +} From 55d1dc78b0ed4b84cdb6a2e4f35f52ea89726e52 Mon Sep 17 00:00:00 2001 From: Yagiz Degirmenci Date: Sun, 1 Aug 2021 01:25:21 +0300 Subject: [PATCH 54/64] feat(ln): use UResult Signed-off-by: Yagiz Degirmenci --- src/uu/ln/src/ln.rs | 84 ++++++++++++++++++++++++++++++++------------- 1 file changed, 61 insertions(+), 23 deletions(-) diff --git a/src/uu/ln/src/ln.rs b/src/uu/ln/src/ln.rs index d354acce9..e5226b97d 100644 --- a/src/uu/ln/src/ln.rs +++ b/src/uu/ln/src/ln.rs @@ -11,9 +11,12 @@ extern crate uucore; use clap::{crate_version, App, Arg}; +use uucore::error::{UCustomError, UResult}; use std::borrow::Cow; +use std::error::Error; use std::ffi::OsStr; +use std::fmt::Display; use std::fs; use std::io::{stdin, Result}; @@ -44,6 +47,51 @@ pub enum OverwriteMode { Force, } +#[derive(Debug)] +enum LnError { + TargetIsDirectory(String), + SomeLinksFailed, + FailedToLink(String), + MissingDestination(String), + ExtraOperand(String), + InvalidBackupMode(String), +} + +impl Display for LnError { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + Self::TargetIsDirectory(s) => write!(f, "target '{}' is not a directory", s), + Self::FailedToLink(s) => write!(f, "failed to link '{}'", s), + Self::SomeLinksFailed => write!(f, "some links failed to create"), + Self::MissingDestination(s) => { + write!(f, "missing destination file operand after '{}'", s) + } + Self::ExtraOperand(s) => write!( + f, + "extra operand '{}'\nTry '{} --help' for more information.", + s, + executable!() + ), + Self::InvalidBackupMode(s) => write!(f, "{}", s), + } + } +} + +impl Error for LnError {} + +impl UCustomError for LnError { + fn code(&self) -> i32 { + match self { + Self::TargetIsDirectory(_) => 1, + Self::SomeLinksFailed => 1, + Self::FailedToLink(_) => 1, + Self::MissingDestination(_) => 1, + Self::ExtraOperand(_) => 1, + Self::InvalidBackupMode(_) => 1, + } + } +} + fn get_usage() -> String { format!( "{0} [OPTION]... [-T] TARGET LINK_NAME (1st form) @@ -86,7 +134,8 @@ mod options { static ARG_FILES: &str = "files"; -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 long_usage = get_long_usage(); @@ -122,8 +171,7 @@ pub fn uumain(args: impl uucore::Args) -> i32 { ); let backup_mode = match backup_mode { Err(err) => { - show_usage_error!("{}", err); - return 1; + return Err(LnError::InvalidBackupMode(err.to_string().into()).into()); } Ok(mode) => mode, }; @@ -246,7 +294,7 @@ pub fn uu_app() -> App<'static, 'static> { ) } -fn exec(files: &[PathBuf], settings: &Settings) -> i32 { +fn exec(files: &[PathBuf], settings: &Settings) -> UResult<()> { // Handle cases where we create links in a directory first. if let Some(ref name) = settings.target_dir { // 4th form: a directory is specified by -t. @@ -267,35 +315,25 @@ fn exec(files: &[PathBuf], settings: &Settings) -> i32 { // 1st form. Now there should be only two operands, but if -T is // specified we may have a wrong number of operands. if files.len() == 1 { - show_error!( - "missing destination file operand after '{}'", - files[0].to_string_lossy() - ); - return 1; + return Err(LnError::MissingDestination(files[0].to_string_lossy().into()).into()); } if files.len() > 2 { - show_error!( - "extra operand '{}'\nTry '{} --help' for more information.", - files[2].display(), - executable!() - ); - return 1; + return Err(LnError::ExtraOperand(files[2].display().to_string().into()).into()); } assert!(!files.is_empty()); match link(&files[0], &files[1], settings) { - Ok(_) => 0, + Ok(_) => Ok(()), Err(e) => { - show_error!("{}", e); - 1 + // show_error!("{}", e); + Err(LnError::FailedToLink(e.to_string().into()).into()) } } } -fn link_files_in_dir(files: &[PathBuf], target_dir: &Path, settings: &Settings) -> i32 { +fn link_files_in_dir(files: &[PathBuf], target_dir: &Path, settings: &Settings) -> UResult<()> { if !target_dir.is_dir() { - show_error!("target '{}' is not a directory", target_dir.display()); - return 1; + return Err(LnError::TargetIsDirectory(target_dir.display().to_string()).into()); } let mut all_successful = true; @@ -354,9 +392,9 @@ fn link_files_in_dir(files: &[PathBuf], target_dir: &Path, settings: &Settings) } } if all_successful { - 0 + Ok(()) } else { - 1 + Err(LnError::SomeLinksFailed.into()) } } From 65dd6afa133a33ced1e777240e80018c26495f41 Mon Sep 17 00:00:00 2001 From: Yagiz Degirmenci Date: Sun, 1 Aug 2021 01:27:25 +0300 Subject: [PATCH 55/64] chore(ln): delete comment Signed-off-by: Yagiz Degirmenci --- src/uu/ln/src/ln.rs | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/uu/ln/src/ln.rs b/src/uu/ln/src/ln.rs index e5226b97d..c7489ade7 100644 --- a/src/uu/ln/src/ln.rs +++ b/src/uu/ln/src/ln.rs @@ -324,10 +324,7 @@ fn exec(files: &[PathBuf], settings: &Settings) -> UResult<()> { match link(&files[0], &files[1], settings) { Ok(_) => Ok(()), - Err(e) => { - // show_error!("{}", e); - Err(LnError::FailedToLink(e.to_string().into()).into()) - } + Err(e) => Err(LnError::FailedToLink(e.to_string().into()).into()), } } From 95a890e5f876a382e8d6fe0633465bcda30e5079 Mon Sep 17 00:00:00 2001 From: Yagiz Degirmenci Date: Sun, 1 Aug 2021 01:42:12 +0300 Subject: [PATCH 56/64] chore(ln): fix clippy errors Signed-off-by: Yagiz Degirmenci --- src/uu/ln/src/ln.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/uu/ln/src/ln.rs b/src/uu/ln/src/ln.rs index c7489ade7..65bdcf36c 100644 --- a/src/uu/ln/src/ln.rs +++ b/src/uu/ln/src/ln.rs @@ -171,7 +171,7 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> { ); let backup_mode = match backup_mode { Err(err) => { - return Err(LnError::InvalidBackupMode(err.to_string().into()).into()); + return Err(LnError::InvalidBackupMode(err).into()); } Ok(mode) => mode, }; @@ -318,13 +318,13 @@ fn exec(files: &[PathBuf], settings: &Settings) -> UResult<()> { return Err(LnError::MissingDestination(files[0].to_string_lossy().into()).into()); } if files.len() > 2 { - return Err(LnError::ExtraOperand(files[2].display().to_string().into()).into()); + return Err(LnError::ExtraOperand(files[2].display().to_string()).into()); } assert!(!files.is_empty()); match link(&files[0], &files[1], settings) { Ok(_) => Ok(()), - Err(e) => Err(LnError::FailedToLink(e.to_string().into()).into()), + Err(e) => Err(LnError::FailedToLink(e.to_string()).into()), } } From 3d23ace9b8c02d0e06311db68ecec434c9ed32ac Mon Sep 17 00:00:00 2001 From: Michael Debertol Date: Sun, 1 Aug 2021 17:12:06 +0200 Subject: [PATCH 57/64] sort: remove redundant comment --- src/uu/sort/src/ext_sort.rs | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/uu/sort/src/ext_sort.rs b/src/uu/sort/src/ext_sort.rs index ea53900e2..816bf1e1d 100644 --- a/src/uu/sort/src/ext_sort.rs +++ b/src/uu/sort/src/ext_sort.rs @@ -89,8 +89,6 @@ fn reader_writer>, Tmp: WriteableTmpFile files, &settings.tmp_dir, separator, - // Heuristically chosen: Dividing by 10 seems to keep our memory usage roughly - // around settings.buffer_size as a whole. buffer_size, settings, receiver, From af47c66a00c0912e58d346cd3a26896a8db075c1 Mon Sep 17 00:00:00 2001 From: Michael Debertol Date: Sun, 1 Aug 2021 17:12:35 +0200 Subject: [PATCH 58/64] sort: improve tests --- src/uu/sort/src/check.rs | 8 +++++++- tests/by-util/test_sort.rs | 27 ++++++++++++++++++++++++++- 2 files changed, 33 insertions(+), 2 deletions(-) diff --git a/src/uu/sort/src/check.rs b/src/uu/sort/src/check.rs index 350a98e23..d82565c3d 100644 --- a/src/uu/sort/src/check.rs +++ b/src/uu/sort/src/check.rs @@ -42,7 +42,13 @@ pub fn check(path: &OsStr, settings: &GlobalSettings) -> i32 { move || reader(file, recycled_receiver, loaded_sender, &settings) }); for _ in 0..2 { - let _ = recycled_sender.send(RecycledChunk::new(100 * 1024)); + let _ = recycled_sender.send(RecycledChunk::new(if settings.buffer_size < 100 * 1024 { + // when the buffer size is smaller than 100KiB we choose it instead of the default. + // this improves testability. + settings.buffer_size + } else { + 100 * 1024 + })); } let mut prev_chunk: Option = None; diff --git a/tests/by-util/test_sort.rs b/tests/by-util/test_sort.rs index 838eb4f98..cac9a5a5d 100644 --- a/tests/by-util/test_sort.rs +++ b/tests/by-util/test_sort.rs @@ -771,6 +771,7 @@ fn test_check() { new_ucmd!() .arg(diagnose_arg) .arg("check_fail.txt") + .arg("--buffer-size=10b") .fails() .stderr_only("sort: check_fail.txt:6: disorder: 5\n"); @@ -892,6 +893,29 @@ fn test_compress() { .stdout_only_fixture("ext_sort.expected"); } +#[test] +#[cfg(target_os = "linux")] +fn test_compress_merge() { + new_ucmd!() + .args(&[ + "--compress-program", + "gzip", + "-S", + "10", + "--batch-size=2", + "-m", + "--unique", + "merge_ints_interleaved_1.txt", + "merge_ints_interleaved_2.txt", + "merge_ints_interleaved_3.txt", + "merge_ints_interleaved_3.txt", + "merge_ints_interleaved_2.txt", + "merge_ints_interleaved_1.txt", + ]) + .succeeds() + .stdout_only_fixture("merge_ints_interleaved.expected"); +} + #[test] fn test_compress_fail() { TestScenario::new(util_name!()) @@ -1027,7 +1051,8 @@ fn test_output_is_input() { let (at, mut cmd) = at_and_ucmd!(); at.touch("file"); at.append("file", input); - cmd.args(&["-m", "-o", "file", "file"]).succeeds(); + cmd.args(&["-m", "-u", "-o", "file", "file", "file", "file"]) + .succeeds(); assert_eq!(at.read("file"), input); } From 450a487d763b299856a1d8ff4dfb2a753c6ec711 Mon Sep 17 00:00:00 2001 From: Michael Debertol Date: Sun, 1 Aug 2021 18:02:52 +0200 Subject: [PATCH 59/64] sort: ignore broken pipes in a test Since sort exits early due to the nonexistent file, it might no longer be around when we try to send it the input. This is "by design" and can be ignored. --- tests/by-util/test_sort.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/by-util/test_sort.rs b/tests/by-util/test_sort.rs index cac9a5a5d..1f21722ed 100644 --- a/tests/by-util/test_sort.rs +++ b/tests/by-util/test_sort.rs @@ -1000,6 +1000,7 @@ fn test_verifies_out_file() { new_ucmd!() .args(&["-o", "nonexistent_dir/nonexistent_file"]) .pipe_in(input) + .ignore_stdin_write_error() .fails() .status_code(2) .stderr_only( From e8eb15f05e5c5ba4de5989f5eb0a9eb390102738 Mon Sep 17 00:00:00 2001 From: Michael Debertol Date: Sun, 1 Aug 2021 16:50:33 +0200 Subject: [PATCH 60/64] core/error: require UCustomError to be Send For multi-threaded programs like sort it is necessary to be able to send errors between threads. --- src/uucore/src/lib/mods/error.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/uucore/src/lib/mods/error.rs b/src/uucore/src/lib/mods/error.rs index c64e7df66..d82da9feb 100644 --- a/src/uucore/src/lib/mods/error.rs +++ b/src/uucore/src/lib/mods/error.rs @@ -251,7 +251,7 @@ 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. -pub trait UCustomError: Error { +pub trait UCustomError: Error + Send { /// Error code of a custom error. /// /// Set a return value for each variant of an enum-type to associate an From 940559f0e1b98c02e470734ac6d9d4b050d0c522 Mon Sep 17 00:00:00 2001 From: Thomas Queiroz Date: Tue, 29 Jun 2021 19:20:01 -0300 Subject: [PATCH 61/64] tail: handle `-` as an alias for stdin --- Cargo.lock | 1 + src/uu/tail/Cargo.toml | 4 ++ src/uu/tail/README.md | 2 +- src/uu/tail/src/platform/mod.rs | 3 +- src/uu/tail/src/platform/unix.rs | 22 ++++++++- src/uu/tail/src/tail.rs | 83 +++++++++++++++++++++++--------- 6 files changed, 90 insertions(+), 25 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 51424332d..f411c5920 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2570,6 +2570,7 @@ version = "0.0.6" dependencies = [ "clap", "libc", + "nix 0.20.0", "redox_syscall 0.1.57", "uucore", "uucore_procs", diff --git a/src/uu/tail/Cargo.toml b/src/uu/tail/Cargo.toml index 273c67bb3..47db8dc6b 100644 --- a/src/uu/tail/Cargo.toml +++ b/src/uu/tail/Cargo.toml @@ -24,6 +24,10 @@ winapi = { version="0.3", features=["fileapi", "handleapi", "processthreadsapi", [target.'cfg(target_os = "redox")'.dependencies] redox_syscall = "0.1" +[target.'cfg(unix)'.dependencies] +nix = "0.20" +libc = "0.2" + [[bin]] name = "tail" path = "src/main.rs" diff --git a/src/uu/tail/README.md b/src/uu/tail/README.md index b7f92f8e4..94b6816af 100644 --- a/src/uu/tail/README.md +++ b/src/uu/tail/README.md @@ -11,7 +11,7 @@ ### Others -- [ ] The current implementation does not handle `-` as an alias for stdin. +- [ ] The current implementation doesn't follow stdin in non-unix platforms ## Possible optimizations diff --git a/src/uu/tail/src/platform/mod.rs b/src/uu/tail/src/platform/mod.rs index 010c5c4ac..4a8982713 100644 --- a/src/uu/tail/src/platform/mod.rs +++ b/src/uu/tail/src/platform/mod.rs @@ -2,13 +2,14 @@ * This file is part of the uutils coreutils package. * * (c) Alexander Batischev + * (c) Thomas Queiroz * * For the full copyright and license information, please view the LICENSE * file that was distributed with this source code. */ #[cfg(unix)] -pub use self::unix::{supports_pid_checks, Pid, ProcessChecker}; +pub use self::unix::{stdin_is_pipe_or_fifo, supports_pid_checks, Pid, ProcessChecker}; #[cfg(windows)] pub use self::windows::{supports_pid_checks, Pid, ProcessChecker}; diff --git a/src/uu/tail/src/platform/unix.rs b/src/uu/tail/src/platform/unix.rs index 167f693e6..580a40135 100644 --- a/src/uu/tail/src/platform/unix.rs +++ b/src/uu/tail/src/platform/unix.rs @@ -2,6 +2,7 @@ * This file is part of the uutils coreutils package. * * (c) Alexander Batischev + * (c) Thomas Queiroz * * For the full copyright and license information, please view the LICENSE * file that was distributed with this source code. @@ -9,7 +10,13 @@ // spell-checker:ignore (ToDO) errno EPERM ENOSYS -use std::io::Error; +use std::io::{stdin, Error}; + +use std::os::unix::prelude::AsRawFd; + +use nix::sys::stat::fstat; + +use libc::{S_IFIFO, S_IFSOCK}; pub type Pid = libc::pid_t; @@ -40,3 +47,16 @@ pub fn supports_pid_checks(pid: self::Pid) -> bool { fn get_errno() -> i32 { Error::last_os_error().raw_os_error().unwrap() } + +pub fn stdin_is_pipe_or_fifo() -> bool { + let fd = stdin().lock().as_raw_fd(); + fd >= 0 // GNU tail checks fd >= 0 + && match fstat(fd) { + Ok(stat) => { + let mode = stat.st_mode; + // NOTE: This is probably not the most correct way to check this + (mode & S_IFIFO != 0) || (mode & S_IFSOCK != 0) + } + Err(err) => panic!("{}", err), + } +} diff --git a/src/uu/tail/src/tail.rs b/src/uu/tail/src/tail.rs index 4970cdcc2..471c1a404 100644 --- a/src/uu/tail/src/tail.rs +++ b/src/uu/tail/src/tail.rs @@ -2,6 +2,7 @@ // * // * (c) Morten Olsen Lysgaard // * (c) Alexander Batischev +// * (c) Thomas Queiroz // * // * For the full copyright and license information, please view the LICENSE // * file that was distributed with this source code. @@ -29,6 +30,9 @@ use std::time::Duration; use uucore::parse_size::{parse_size, ParseSizeError}; use uucore::ringbuffer::RingBuffer; +#[cfg(unix)] +use crate::platform::stdin_is_pipe_or_fifo; + pub mod options { pub mod verbosity { pub static QUIET: &str = "quiet"; @@ -130,25 +134,56 @@ pub fn uumain(args: impl uucore::Args) -> i32 { let files: Vec = matches .values_of(options::ARG_FILES) .map(|v| v.map(ToString::to_string).collect()) - .unwrap_or_default(); + .unwrap_or_else(|| vec![String::from("-")]); - if files.is_empty() { - let mut buffer = BufReader::new(stdin()); - unbounded_tail(&mut buffer, &settings); - } else { - let multiple = files.len() > 1; - let mut first_header = true; - let mut readers = Vec::new(); + let multiple = files.len() > 1; + let mut first_header = true; + let mut readers: Vec<(Box, &String)> = Vec::new(); - for filename in &files { - if (multiple || verbose) && !quiet { - if !first_header { - println!(); - } + #[cfg(unix)] + let stdin_string = String::from("standard input"); + + for filename in &files { + let use_stdin = filename.as_str() == "-"; + if (multiple || verbose) && !quiet { + if !first_header { + println!(); + } + if use_stdin { + println!("==> standard input <=="); + } else { println!("==> {} <==", filename); } - first_header = false; + } + first_header = false; + if use_stdin { + let mut reader = BufReader::new(stdin()); + unbounded_tail(&mut reader, &settings); + + // Don't follow stdin since there are no checks for pipes/FIFOs + // + // FIXME windows has GetFileType which can determine if the file is a pipe/FIFO + // so this check can also be performed + + #[cfg(unix)] + { + /* + POSIX specification regarding tail -f + + If the input file is a regular file or if the file operand specifies a FIFO, do not + terminate after the last line of the input file has been copied, but read and copy + further bytes from the input file when they become available. If no file operand is + specified and standard input is a pipe or FIFO, the -f option shall be ignored. If + the input file is not a FIFO, pipe, or regular file, it is unspecified whether or + not the -f option shall be ignored. + */ + + if settings.follow && !stdin_is_pipe_or_fifo() { + readers.push((Box::new(reader), &stdin_string)); + } + } + } else { let path = Path::new(filename); if path.is_dir() { continue; @@ -158,20 +193,20 @@ pub fn uumain(args: impl uucore::Args) -> i32 { bounded_tail(&mut file, &settings); if settings.follow { let reader = BufReader::new(file); - readers.push(reader); + readers.push((Box::new(reader), filename)); } } else { let mut reader = BufReader::new(file); unbounded_tail(&mut reader, &settings); if settings.follow { - readers.push(reader); + readers.push((Box::new(reader), filename)); } } } + } - if settings.follow { - follow(&mut readers[..], &files[..], &settings); - } + if settings.follow { + follow(&mut readers[..], &settings); } 0 @@ -248,8 +283,12 @@ pub fn uu_app() -> App<'static, 'static> { ) } -fn follow(readers: &mut [BufReader], filenames: &[String], settings: &Settings) { +fn follow(readers: &mut [(T, &String)], settings: &Settings) { assert!(settings.follow); + if readers.is_empty() { + return; + } + let mut last = readers.len() - 1; let mut read_some = false; let mut process = platform::ProcessChecker::new(settings.pid); @@ -260,7 +299,7 @@ fn follow(readers: &mut [BufReader], filenames: &[String], settings: let pid_is_dead = !read_some && settings.pid != 0 && process.is_dead(); read_some = false; - for (i, reader) in readers.iter_mut().enumerate() { + for (i, (reader, filename)) in readers.iter_mut().enumerate() { // Print all new content since the last pass loop { let mut datum = String::new(); @@ -269,7 +308,7 @@ fn follow(readers: &mut [BufReader], filenames: &[String], settings: Ok(_) => { read_some = true; if i != last { - println!("\n==> {} <==", filenames[i]); + println!("\n==> {} <==", filename); last = i; } print!("{}", datum); From 6127ea9642854d3c596292a448ed6d99515452d1 Mon Sep 17 00:00:00 2001 From: Thomas Queiroz Date: Mon, 12 Jul 2021 19:46:43 -0400 Subject: [PATCH 62/64] tests/tail: add test for as an alias for stdin --- tests/by-util/test_tail.rs | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/tests/by-util/test_tail.rs b/tests/by-util/test_tail.rs index e8dd63317..28c3580bb 100644 --- a/tests/by-util/test_tail.rs +++ b/tests/by-util/test_tail.rs @@ -23,6 +23,15 @@ fn test_stdin_default() { .stdout_is_fixture("foobar_stdin_default.expected"); } +#[test] +fn test_stdin_explicit() { + new_ucmd!() + .pipe_in_fixture(FOOBAR_TXT) + .arg("-") + .run() + .stdout_is_fixture("foobar_stdin_default.expected"); +} + #[test] fn test_single_default() { new_ucmd!() From 53b16e9b4387da551fbc1a43ee8b981abcb83b45 Mon Sep 17 00:00:00 2001 From: Thomas Queiroz Date: Tue, 13 Jul 2021 19:17:25 -0400 Subject: [PATCH 63/64] docs/spell: add 'Thomas Queiroz' to spell-checker exceptions word list --- .vscode/cspell.dictionaries/people.wordlist.txt | 3 +++ 1 file changed, 3 insertions(+) diff --git a/.vscode/cspell.dictionaries/people.wordlist.txt b/.vscode/cspell.dictionaries/people.wordlist.txt index d7665585b..405733836 100644 --- a/.vscode/cspell.dictionaries/people.wordlist.txt +++ b/.vscode/cspell.dictionaries/people.wordlist.txt @@ -151,6 +151,9 @@ Sylvestre Ledru T Jameson Little Jameson Little +Thomas Queiroz + Thomas + Queiroz Tobias Bohumir Schottdorf Tobias Bohumir From a4709c805c8468cc0c4f3d45d98d7548a8142148 Mon Sep 17 00:00:00 2001 From: Michael Debertol Date: Sun, 1 Aug 2021 16:51:56 +0200 Subject: [PATCH 64/64] sort: use UResult --- src/uu/sort/src/check.rs | 42 +++--- src/uu/sort/src/chunks.rs | 32 +++-- src/uu/sort/src/ext_sort.rs | 64 +++++---- src/uu/sort/src/merge.rs | 186 +++++++++++++----------- src/uu/sort/src/sort.rs | 273 +++++++++++++++++++++++++----------- 5 files changed, 370 insertions(+), 227 deletions(-) diff --git a/src/uu/sort/src/check.rs b/src/uu/sort/src/check.rs index d82565c3d..5be752be0 100644 --- a/src/uu/sort/src/check.rs +++ b/src/uu/sort/src/check.rs @@ -9,7 +9,7 @@ use crate::{ chunks::{self, Chunk, RecycledChunk}, - compare_by, open, GlobalSettings, + compare_by, open, GlobalSettings, SortError, }; use itertools::Itertools; use std::{ @@ -20,13 +20,14 @@ use std::{ sync::mpsc::{sync_channel, Receiver, SyncSender}, thread, }; +use uucore::error::UResult; /// Check if the file at `path` is ordered. /// /// # Returns /// /// The code we should exit with. -pub fn check(path: &OsStr, settings: &GlobalSettings) -> i32 { +pub fn check(path: &OsStr, settings: &GlobalSettings) -> UResult<()> { let max_allowed_cmp = if settings.unique { // If `unique` is enabled, the previous line must compare _less_ to the next one. Ordering::Less @@ -34,7 +35,7 @@ pub fn check(path: &OsStr, settings: &GlobalSettings) -> i32 { // Otherwise, the line previous line must compare _less or equal_ to the next one. Ordering::Equal }; - let file = open(path); + let file = open(path)?; let (recycled_sender, recycled_receiver) = sync_channel(2); let (loaded_sender, loaded_receiver) = sync_channel(2); thread::spawn({ @@ -69,15 +70,13 @@ pub fn check(path: &OsStr, settings: &GlobalSettings) -> i32 { chunk.line_data(), ) > max_allowed_cmp { - if !settings.check_silent { - eprintln!( - "sort: {}:{}: disorder: {}", - path.to_string_lossy(), - line_idx, - new_first.line - ); + return Err(SortError::Disorder { + file: path.to_owned(), + line_number: line_idx, + line: new_first.line.to_owned(), + silent: settings.check_silent, } - return 1; + .into()); } let _ = recycled_sender.send(prev_chunk.recycle()); } @@ -85,21 +84,19 @@ pub fn check(path: &OsStr, settings: &GlobalSettings) -> i32 { for (a, b) in chunk.lines().iter().tuple_windows() { line_idx += 1; if compare_by(a, b, settings, chunk.line_data(), chunk.line_data()) > max_allowed_cmp { - if !settings.check_silent { - eprintln!( - "sort: {}:{}: disorder: {}", - path.to_string_lossy(), - line_idx, - b.line - ); + return Err(SortError::Disorder { + file: path.to_owned(), + line_number: line_idx, + line: b.line.to_owned(), + silent: settings.check_silent, } - return 1; + .into()); } } prev_chunk = Some(chunk); } - 0 + Ok(()) } /// The function running on the reader thread. @@ -108,7 +105,7 @@ fn reader( receiver: Receiver, sender: SyncSender, settings: &GlobalSettings, -) { +) -> UResult<()> { let mut carry_over = vec![]; for recycled_chunk in receiver.iter() { let should_continue = chunks::read( @@ -124,9 +121,10 @@ fn reader( b'\n' }, settings, - ); + )?; if !should_continue { break; } } + Ok(()) } diff --git a/src/uu/sort/src/chunks.rs b/src/uu/sort/src/chunks.rs index 5ab98392d..80d6060d4 100644 --- a/src/uu/sort/src/chunks.rs +++ b/src/uu/sort/src/chunks.rs @@ -14,8 +14,9 @@ use std::{ use memchr::memchr_iter; use ouroboros::self_referencing; +use uucore::error::{UResult, USimpleError}; -use crate::{numeric_str_cmp::NumInfo, GeneralF64ParseResult, GlobalSettings, Line}; +use crate::{numeric_str_cmp::NumInfo, GeneralF64ParseResult, GlobalSettings, Line, SortError}; /// The chunk that is passed around between threads. /// `lines` consist of slices into `buffer`. @@ -137,10 +138,10 @@ pub fn read( max_buffer_size: Option, carry_over: &mut Vec, file: &mut T, - next_files: &mut impl Iterator, + next_files: &mut impl Iterator>, separator: u8, settings: &GlobalSettings, -) -> bool { +) -> UResult { let RecycledChunk { lines, selections, @@ -159,12 +160,12 @@ pub fn read( max_buffer_size, carry_over.len(), separator, - ); + )?; carry_over.clear(); carry_over.extend_from_slice(&buffer[read..]); if read != 0 { - let payload = Chunk::new(buffer, |buffer| { + let payload: UResult = Chunk::try_new(buffer, |buffer| { let selections = unsafe { // SAFETY: It is safe to transmute to an empty vector of selections with shorter lifetime. // It was only temporarily transmuted to a Vec> to make recycling possible. @@ -175,18 +176,19 @@ pub fn read( // because it was only temporarily transmuted to a Vec> to make recycling possible. std::mem::transmute::>, Vec>>(lines) }; - let read = crash_if_err!(2, std::str::from_utf8(&buffer[..read])); + let read = std::str::from_utf8(&buffer[..read]) + .map_err(|error| SortError::Uft8Error { error })?; let mut line_data = LineData { selections, num_infos, parsed_floats, }; parse_lines(read, &mut lines, &mut line_data, separator, settings); - ChunkContents { lines, line_data } + Ok(ChunkContents { lines, line_data }) }); - sender.send(payload).unwrap(); + sender.send(payload?).unwrap(); } - should_continue + Ok(should_continue) } /// Split `read` into `Line`s, and add them to `lines`. @@ -242,12 +244,12 @@ fn parse_lines<'a>( /// * Whether this function should be called again. fn read_to_buffer( file: &mut T, - next_files: &mut impl Iterator, + next_files: &mut impl Iterator>, buffer: &mut Vec, max_buffer_size: Option, start_offset: usize, separator: u8, -) -> (usize, bool) { +) -> UResult<(usize, bool)> { let mut read_target = &mut buffer[start_offset..]; let mut last_file_target_size = read_target.len(); loop { @@ -274,7 +276,7 @@ fn read_to_buffer( // We read enough lines. let end = last_line_end.unwrap(); // We want to include the separator here, because it shouldn't be carried over. - return (end + 1, true); + return Ok((end + 1, true)); } else { // We need to read more lines let len = buffer.len(); @@ -299,11 +301,11 @@ fn read_to_buffer( if let Some(next_file) = next_files.next() { // There is another file. last_file_target_size = leftover_len; - *file = next_file; + *file = next_file?; } else { // This was the last file. let read_len = buffer.len() - leftover_len; - return (read_len, false); + return Ok((read_len, false)); } } } @@ -313,7 +315,7 @@ fn read_to_buffer( Err(e) if e.kind() == ErrorKind::Interrupted => { // retry } - Err(e) => crash!(2, "{}", e), + Err(e) => return Err(USimpleError::new(2, e.to_string())), } } } diff --git a/src/uu/sort/src/ext_sort.rs b/src/uu/sort/src/ext_sort.rs index 816bf1e1d..8ff5665fd 100644 --- a/src/uu/sort/src/ext_sort.rs +++ b/src/uu/sort/src/ext_sort.rs @@ -22,6 +22,7 @@ use std::{ }; use itertools::Itertools; +use uucore::error::UResult; use crate::chunks::RecycledChunk; use crate::merge::ClosedTmpFile; @@ -29,6 +30,7 @@ use crate::merge::WriteableCompressedTmpFile; use crate::merge::WriteablePlainTmpFile; use crate::merge::WriteableTmpFile; use crate::Output; +use crate::SortError; use crate::{ chunks::{self, Chunk}, compare_by, merge, sort_by, GlobalSettings, @@ -40,10 +42,10 @@ const START_BUFFER_SIZE: usize = 8_000; /// Sort files by using auxiliary files for storing intermediate chunks (if needed), and output the result. pub fn ext_sort( - files: &mut impl Iterator>, + files: &mut impl Iterator>>, settings: &GlobalSettings, output: Output, -) { +) -> UResult<()> { let (sorted_sender, sorted_receiver) = std::sync::mpsc::sync_channel(1); let (recycled_sender, recycled_receiver) = std::sync::mpsc::sync_channel(1); thread::spawn({ @@ -57,7 +59,7 @@ pub fn ext_sort( sorted_receiver, recycled_sender, output, - ); + ) } else { reader_writer::<_, WriteablePlainTmpFile>( files, @@ -65,17 +67,20 @@ pub fn ext_sort( sorted_receiver, recycled_sender, output, - ); + ) } } -fn reader_writer>, Tmp: WriteableTmpFile + 'static>( +fn reader_writer< + F: Iterator>>, + Tmp: WriteableTmpFile + 'static, +>( files: F, settings: &GlobalSettings, receiver: Receiver, sender: SyncSender, output: Output, -) { +) -> UResult<()> { let separator = if settings.zero_terminated { b'\0' } else { @@ -93,16 +98,16 @@ fn reader_writer>, Tmp: WriteableTmpFile settings, receiver, sender, - ); + )?; match read_result { ReadResult::WroteChunksToFile { tmp_files, tmp_dir } => { let tmp_dir_size = tmp_files.len(); - let mut merger = merge::merge_with_file_limit::<_, _, Tmp>( + let merger = merge::merge_with_file_limit::<_, _, Tmp>( tmp_files.into_iter().map(|c| c.reopen()), settings, Some((tmp_dir, tmp_dir_size)), - ); - merger.write_all(settings, output); + )?; + merger.write_all(settings, output)?; } ReadResult::SortedSingleChunk(chunk) => { if settings.unique { @@ -145,6 +150,7 @@ fn reader_writer>, Tmp: WriteableTmpFile // don't output anything } } + Ok(()) } /// The function that is executed on the sorter thread. @@ -153,7 +159,11 @@ fn sorter(receiver: Receiver, sender: SyncSender, settings: Global payload.with_contents_mut(|contents| { sort_by(&mut contents.lines, &settings, &contents.line_data) }); - sender.send(payload).unwrap(); + if sender.send(payload).is_err() { + // The receiver has gone away, likely because the other thread hit an error. + // We stop silently because the actual error is printed by the other thread. + return; + } } } @@ -173,15 +183,15 @@ enum ReadResult { } /// The function that is executed on the reader/writer thread. fn read_write_loop( - mut files: impl Iterator>, + mut files: impl Iterator>>, tmp_dir_parent: &Path, separator: u8, buffer_size: usize, settings: &GlobalSettings, receiver: Receiver, sender: SyncSender, -) -> ReadResult { - let mut file = files.next().unwrap(); +) -> UResult> { + let mut file = files.next().unwrap()?; let mut carry_over = vec![]; // kick things off with two reads @@ -199,14 +209,14 @@ fn read_write_loop( &mut files, separator, settings, - ); + )?; if !should_continue { drop(sender); // We have already read the whole input. Since we are in our first two reads, // this means that we can fit the whole input into memory. Bypass writing below and // handle this case in a more straightforward way. - return if let Ok(first_chunk) = receiver.recv() { + return Ok(if let Ok(first_chunk) = receiver.recv() { if let Ok(second_chunk) = receiver.recv() { ReadResult::SortedTwoChunks([first_chunk, second_chunk]) } else { @@ -214,16 +224,14 @@ fn read_write_loop( } } else { ReadResult::EmptyInput - }; + }); } } - let tmp_dir = crash_if_err!( - 2, - tempfile::Builder::new() - .prefix("uutils_sort") - .tempdir_in(tmp_dir_parent) - ); + let tmp_dir = tempfile::Builder::new() + .prefix("uutils_sort") + .tempdir_in(tmp_dir_parent) + .map_err(|_| SortError::TmpDirCreationFailed)?; let mut sender_option = Some(sender); let mut file_number = 0; @@ -232,7 +240,7 @@ fn read_write_loop( let mut chunk = match receiver.recv() { Ok(it) => it, _ => { - return ReadResult::WroteChunksToFile { tmp_files, tmp_dir }; + return Ok(ReadResult::WroteChunksToFile { tmp_files, tmp_dir }); } }; @@ -241,7 +249,7 @@ fn read_write_loop( tmp_dir.path().join(file_number.to_string()), settings.compress_prog.as_deref(), separator, - ); + )?; tmp_files.push(tmp_file); file_number += 1; @@ -258,7 +266,7 @@ fn read_write_loop( &mut files, separator, settings, - ); + )?; if !should_continue { sender_option = None; } @@ -273,8 +281,8 @@ fn write( file: PathBuf, compress_prog: Option<&str>, separator: u8, -) -> I::Closed { - let mut tmp_file = I::create(file, compress_prog); +) -> UResult { + let mut tmp_file = I::create(file, compress_prog)?; write_lines(chunk.lines(), tmp_file.as_write(), separator); tmp_file.finished_writing() } diff --git a/src/uu/sort/src/merge.rs b/src/uu/sort/src/merge.rs index a5ac9411b..fad966f64 100644 --- a/src/uu/sort/src/merge.rs +++ b/src/uu/sort/src/merge.rs @@ -17,16 +17,17 @@ use std::{ process::{Child, ChildStdin, ChildStdout, Command, Stdio}, rc::Rc, sync::mpsc::{channel, sync_channel, Receiver, Sender, SyncSender}, - thread, + thread::{self, JoinHandle}, }; use compare::Compare; use itertools::Itertools; use tempfile::TempDir; +use uucore::error::UResult; use crate::{ chunks::{self, Chunk, RecycledChunk}, - compare_by, open, GlobalSettings, Output, + compare_by, open, GlobalSettings, Output, SortError, }; /// If the output file occurs in the input files as well, copy the contents of the output file @@ -35,7 +36,7 @@ fn replace_output_file_in_input_files( files: &mut [OsString], settings: &GlobalSettings, output: Option<&str>, -) -> Option<(TempDir, usize)> { +) -> UResult> { let mut copy: Option<(TempDir, PathBuf)> = None; if let Some(Ok(output_path)) = output.map(|path| Path::new(path).canonicalize()) { for file in files { @@ -47,9 +48,10 @@ fn replace_output_file_in_input_files( let tmp_dir = tempfile::Builder::new() .prefix("uutils_sort") .tempdir_in(&settings.tmp_dir) - .unwrap(); + .map_err(|_| SortError::TmpDirCreationFailed)?; let copy_path = tmp_dir.path().join("0"); - std::fs::copy(file_path, ©_path).unwrap(); + std::fs::copy(file_path, ©_path) + .map_err(|error| SortError::OpenTmpFileFailed { error })?; *file = copy_path.clone().into_os_string(); copy = Some((tmp_dir, copy_path)) } @@ -58,7 +60,7 @@ fn replace_output_file_in_input_files( } } // if we created a TempDir its size must be one. - copy.map(|(dir, _copy)| (dir, 1)) + Ok(copy.map(|(dir, _copy)| (dir, 1))) } /// Merge pre-sorted `Box`s. @@ -69,13 +71,13 @@ pub fn merge<'a>( files: &mut [OsString], settings: &'a GlobalSettings, output: Option<&str>, -) -> FileMerger<'a> { - let tmp_dir = replace_output_file_in_input_files(files, settings, output); +) -> UResult> { + let tmp_dir = replace_output_file_in_input_files(files, settings, output)?; if settings.compress_prog.is_none() { merge_with_file_limit::<_, _, WriteablePlainTmpFile>( files .iter() - .map(|file| PlainMergeInput { inner: open(file) }), + .map(|file| open(file).map(|file| PlainMergeInput { inner: file })), settings, tmp_dir, ) @@ -83,7 +85,7 @@ pub fn merge<'a>( merge_with_file_limit::<_, _, WriteableCompressedTmpFile>( files .iter() - .map(|file| PlainMergeInput { inner: open(file) }), + .map(|file| open(file).map(|file| PlainMergeInput { inner: file })), settings, tmp_dir, ) @@ -93,24 +95,25 @@ pub fn merge<'a>( // Merge already sorted `MergeInput`s. pub fn merge_with_file_limit< M: MergeInput + 'static, - F: ExactSizeIterator, + F: ExactSizeIterator>, Tmp: WriteableTmpFile + 'static, >( files: F, settings: &GlobalSettings, tmp_dir: Option<(TempDir, usize)>, -) -> FileMerger { +) -> UResult { if files.len() > settings.merge_batch_size { // If we did not get a tmp_dir, create one. - let (tmp_dir, mut tmp_dir_size) = tmp_dir.unwrap_or_else(|| { - ( + let (tmp_dir, mut tmp_dir_size) = match tmp_dir { + Some(x) => x, + None => ( tempfile::Builder::new() .prefix("uutils_sort") .tempdir_in(&settings.tmp_dir) - .unwrap(), + .map_err(|_| SortError::TmpDirCreationFailed)?, 0, - ) - }); + ), + }; let mut remaining_files = files.len(); let batches = files.chunks(settings.merge_batch_size); let mut batches = batches.into_iter(); @@ -118,14 +121,14 @@ pub fn merge_with_file_limit< while remaining_files != 0 { // Work around the fact that `Chunks` is not an `ExactSizeIterator`. remaining_files = remaining_files.saturating_sub(settings.merge_batch_size); - let mut merger = merge_without_limit(batches.next().unwrap(), settings); + let merger = merge_without_limit(batches.next().unwrap(), settings)?; let mut tmp_file = Tmp::create( tmp_dir.path().join(tmp_dir_size.to_string()), settings.compress_prog.as_deref(), - ); + )?; tmp_dir_size += 1; - merger.write_all_to(settings, tmp_file.as_write()); - temporary_files.push(tmp_file.finished_writing()); + merger.write_all_to(settings, tmp_file.as_write())?; + temporary_files.push(tmp_file.finished_writing()?); } assert!(batches.next().is_none()); merge_with_file_limit::<_, _, Tmp>( @@ -133,7 +136,7 @@ pub fn merge_with_file_limit< .into_iter() .map(Box::new(|c: Tmp::Closed| c.reopen()) as Box< - dyn FnMut(Tmp::Closed) -> ::Reopened, + dyn FnMut(Tmp::Closed) -> UResult<::Reopened>, >), settings, Some((tmp_dir, tmp_dir_size)), @@ -147,10 +150,10 @@ pub fn merge_with_file_limit< /// /// It is the responsibility of the caller to ensure that `files` yields only /// as many files as we are allowed to open concurrently. -fn merge_without_limit>( +fn merge_without_limit>>( files: F, settings: &GlobalSettings, -) -> FileMerger { +) -> UResult { let (request_sender, request_receiver) = channel(); let mut reader_files = Vec::with_capacity(files.size_hint().0); let mut loaded_receivers = Vec::with_capacity(files.size_hint().0); @@ -158,7 +161,7 @@ fn merge_without_limit>( let (sender, receiver) = sync_channel(2); loaded_receivers.push(receiver); reader_files.push(Some(ReaderFile { - file, + file: file?, sender, carry_over: vec![], })); @@ -175,7 +178,7 @@ fn merge_without_limit>( .unwrap(); } - thread::spawn({ + let reader_join_handle = thread::spawn({ let settings = settings.clone(); move || { reader( @@ -204,14 +207,15 @@ fn merge_without_limit>( } } - FileMerger { + Ok(FileMerger { heap: binary_heap_plus::BinaryHeap::from_vec_cmp( mergeable_files, FileComparator { settings }, ), request_sender, prev: None, - } + reader_join_handle, + }) } /// The struct on the reader thread representing an input file struct ReaderFile { @@ -226,7 +230,7 @@ fn reader( files: &mut [Option>], settings: &GlobalSettings, separator: u8, -) { +) -> UResult<()> { for (file_idx, recycled_chunk) in recycled_receiver.iter() { if let Some(ReaderFile { file, @@ -243,15 +247,16 @@ fn reader( &mut iter::empty(), separator, settings, - ); + )?; if !should_continue { // Remove the file from the list by replacing it with `None`. let ReaderFile { file, .. } = files[file_idx].take().unwrap(); // Depending on the kind of the `MergeInput`, this may delete the file: - file.finished_reading(); + file.finished_reading()?; } } } + Ok(()) } /// The struct on the main thread representing an input file pub struct MergeableFile { @@ -275,17 +280,20 @@ pub struct FileMerger<'a> { heap: binary_heap_plus::BinaryHeap>, request_sender: Sender<(usize, RecycledChunk)>, prev: Option, + reader_join_handle: JoinHandle>, } impl<'a> FileMerger<'a> { /// Write the merged contents to the output file. - pub fn write_all(&mut self, settings: &GlobalSettings, output: Output) { + pub fn write_all(self, settings: &GlobalSettings, output: Output) -> UResult<()> { let mut out = output.into_write(); - self.write_all_to(settings, &mut out); + self.write_all_to(settings, &mut out) } - pub fn write_all_to(&mut self, settings: &GlobalSettings, out: &mut impl Write) { + pub fn write_all_to(mut self, settings: &GlobalSettings, out: &mut impl Write) -> UResult<()> { while self.write_next(settings, out) {} + drop(self.request_sender); + self.reader_join_handle.join().unwrap() } fn write_next(&mut self, settings: &GlobalSettings, out: &mut impl Write) -> bool { @@ -369,36 +377,41 @@ impl<'a> Compare for FileComparator<'a> { } // Wait for the child to exit and check its exit code. -fn assert_child_success(mut child: Child, program: &str) { +fn check_child_success(mut child: Child, program: &str) -> UResult<()> { if !matches!( child.wait().map(|e| e.code()), Ok(Some(0)) | Ok(None) | Err(_) ) { - crash!(2, "'{}' terminated abnormally", program) + Err(SortError::CompressProgTerminatedAbnormally { + prog: program.to_owned(), + } + .into()) + } else { + Ok(()) } } /// A temporary file that can be written to. -pub trait WriteableTmpFile { +pub trait WriteableTmpFile: Sized { type Closed: ClosedTmpFile; type InnerWrite: Write; - fn create(path: PathBuf, compress_prog: Option<&str>) -> Self; + fn create(path: PathBuf, compress_prog: Option<&str>) -> UResult; /// Closes the temporary file. - fn finished_writing(self) -> Self::Closed; + fn finished_writing(self) -> UResult; fn as_write(&mut self) -> &mut Self::InnerWrite; } /// A temporary file that is (temporarily) closed, but can be reopened. pub trait ClosedTmpFile { type Reopened: MergeInput; /// Reopens the temporary file. - fn reopen(self) -> Self::Reopened; + fn reopen(self) -> UResult; } /// A pre-sorted input for merging. pub trait MergeInput: Send { type InnerRead: Read; /// Cleans this `MergeInput` up. /// Implementations may delete the backing file. - fn finished_reading(self); + fn finished_reading(self) -> UResult<()>; fn as_read(&mut self) -> &mut Self::InnerRead; } @@ -417,15 +430,17 @@ impl WriteableTmpFile for WriteablePlainTmpFile { type Closed = ClosedPlainTmpFile; type InnerWrite = BufWriter; - fn create(path: PathBuf, _: Option<&str>) -> Self { - WriteablePlainTmpFile { - file: BufWriter::new(File::create(&path).unwrap()), + fn create(path: PathBuf, _: Option<&str>) -> UResult { + Ok(WriteablePlainTmpFile { + file: BufWriter::new( + File::create(&path).map_err(|error| SortError::OpenTmpFileFailed { error })?, + ), path, - } + }) } - fn finished_writing(self) -> Self::Closed { - ClosedPlainTmpFile { path: self.path } + fn finished_writing(self) -> UResult { + Ok(ClosedPlainTmpFile { path: self.path }) } fn as_write(&mut self) -> &mut Self::InnerWrite { @@ -434,18 +449,22 @@ impl WriteableTmpFile for WriteablePlainTmpFile { } impl ClosedTmpFile for ClosedPlainTmpFile { type Reopened = PlainTmpMergeInput; - fn reopen(self) -> Self::Reopened { - PlainTmpMergeInput { - file: File::open(&self.path).unwrap(), + fn reopen(self) -> UResult { + Ok(PlainTmpMergeInput { + file: File::open(&self.path).map_err(|error| SortError::OpenTmpFileFailed { error })?, path: self.path, - } + }) } } impl MergeInput for PlainTmpMergeInput { type InnerRead = File; - fn finished_reading(self) { - fs::remove_file(self.path).ok(); + fn finished_reading(self) -> UResult<()> { + // we ignore failures to delete the temporary file, + // because there is a race at the end of the execution and the whole + // temporary directory might already be gone. + let _ = fs::remove_file(self.path); + Ok(()) } fn as_read(&mut self) -> &mut Self::InnerRead { @@ -473,35 +492,33 @@ impl WriteableTmpFile for WriteableCompressedTmpFile { type Closed = ClosedCompressedTmpFile; type InnerWrite = BufWriter; - fn create(path: PathBuf, compress_prog: Option<&str>) -> Self { + fn create(path: PathBuf, compress_prog: Option<&str>) -> UResult { let compress_prog = compress_prog.unwrap(); let mut command = Command::new(compress_prog); - command - .stdin(Stdio::piped()) - .stdout(File::create(&path).unwrap()); - let mut child = crash_if_err!( - 2, - command.spawn().map_err(|err| format!( - "couldn't execute compress program: errno {}", - err.raw_os_error().unwrap() - )) - ); + let tmp_file = + File::create(&path).map_err(|error| SortError::OpenTmpFileFailed { error })?; + command.stdin(Stdio::piped()).stdout(tmp_file); + let mut child = command + .spawn() + .map_err(|err| SortError::CompressProgExecutionFailed { + code: err.raw_os_error().unwrap(), + })?; let child_stdin = child.stdin.take().unwrap(); - WriteableCompressedTmpFile { + Ok(WriteableCompressedTmpFile { path, compress_prog: compress_prog.to_owned(), child, child_stdin: BufWriter::new(child_stdin), - } + }) } - fn finished_writing(self) -> Self::Closed { + fn finished_writing(self) -> UResult { drop(self.child_stdin); - assert_child_success(self.child, &self.compress_prog); - ClosedCompressedTmpFile { + check_child_success(self.child, &self.compress_prog)?; + Ok(ClosedCompressedTmpFile { path: self.path, compress_prog: self.compress_prog, - } + }) } fn as_write(&mut self) -> &mut Self::InnerWrite { @@ -511,33 +528,32 @@ impl WriteableTmpFile for WriteableCompressedTmpFile { impl ClosedTmpFile for ClosedCompressedTmpFile { type Reopened = CompressedTmpMergeInput; - fn reopen(self) -> Self::Reopened { + fn reopen(self) -> UResult { let mut command = Command::new(&self.compress_prog); let file = File::open(&self.path).unwrap(); command.stdin(file).stdout(Stdio::piped()).arg("-d"); - let mut child = crash_if_err!( - 2, - command.spawn().map_err(|err| format!( - "couldn't execute compress program: errno {}", - err.raw_os_error().unwrap() - )) - ); + let mut child = command + .spawn() + .map_err(|err| SortError::CompressProgExecutionFailed { + code: err.raw_os_error().unwrap(), + })?; let child_stdout = child.stdout.take().unwrap(); - CompressedTmpMergeInput { + Ok(CompressedTmpMergeInput { path: self.path, compress_prog: self.compress_prog, child, child_stdout, - } + }) } } impl MergeInput for CompressedTmpMergeInput { type InnerRead = ChildStdout; - fn finished_reading(self) { + fn finished_reading(self) -> UResult<()> { drop(self.child_stdout); - assert_child_success(self.child, &self.compress_prog); - fs::remove_file(self.path).ok(); + check_child_success(self.child, &self.compress_prog)?; + let _ = fs::remove_file(self.path); + Ok(()) } fn as_read(&mut self) -> &mut Self::InnerRead { @@ -550,7 +566,9 @@ pub struct PlainMergeInput { } impl MergeInput for PlainMergeInput { type InnerRead = R; - fn finished_reading(self) {} + fn finished_reading(self) -> UResult<()> { + Ok(()) + } fn as_read(&mut self) -> &mut Self::InnerRead { &mut self.inner } diff --git a/src/uu/sort/src/sort.rs b/src/uu/sort/src/sort.rs index 77cc4e9e9..be9510aef 100644 --- a/src/uu/sort/src/sort.rs +++ b/src/uu/sort/src/sort.rs @@ -33,14 +33,18 @@ use rand::{thread_rng, Rng}; use rayon::prelude::*; use std::cmp::Ordering; use std::env; +use std::error::Error; use std::ffi::{OsStr, OsString}; +use std::fmt::Display; use std::fs::{File, OpenOptions}; use std::hash::{Hash, Hasher}; use std::io::{stdin, stdout, BufRead, BufReader, BufWriter, Read, Write}; use std::ops::Range; use std::path::Path; use std::path::PathBuf; +use std::str::Utf8Error; use unicode_width::UnicodeWidthStr; +use uucore::error::{set_exit_code, UCustomError, UResult, USimpleError, UUsageError}; use uucore::parse_size::{parse_size, ParseSizeError}; use uucore::version_cmp::version_cmp; use uucore::InvalidEncodingHandling; @@ -120,6 +124,111 @@ const POSITIVE: char = '+'; // available memory into consideration, instead of relying on this constant only. const DEFAULT_BUF_SIZE: usize = 1_000_000_000; // 1 GB +#[derive(Debug)] +enum SortError { + Disorder { + file: OsString, + line_number: usize, + line: String, + silent: bool, + }, + OpenFailed { + path: String, + error: std::io::Error, + }, + ReadFailed { + path: String, + error: std::io::Error, + }, + ParseKeyError { + key: String, + msg: String, + }, + OpenTmpFileFailed { + error: std::io::Error, + }, + CompressProgExecutionFailed { + code: i32, + }, + CompressProgTerminatedAbnormally { + prog: String, + }, + TmpDirCreationFailed, + Uft8Error { + error: Utf8Error, + }, +} + +impl Error for SortError {} + +impl UCustomError for SortError { + fn code(&self) -> i32 { + match self { + SortError::Disorder { .. } => 1, + _ => 2, + } + } + + fn usage(&self) -> bool { + false + } +} + +impl Display for SortError { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + SortError::Disorder { + file, + line_number, + line, + silent, + } => { + if !silent { + write!( + f, + "{}:{}: disorder: {}", + file.to_string_lossy(), + line_number, + line + ) + } else { + Ok(()) + } + } + SortError::OpenFailed { path, error } => write!( + f, + "open failed: {}: {}", + path, + strip_errno(&error.to_string()) + ), + SortError::ParseKeyError { key, msg } => { + write!(f, "failed to parse key `{}`: {}", key, msg) + } + SortError::ReadFailed { path, error } => write!( + f, + "cannot read: {}: {}", + path, + strip_errno(&error.to_string()) + ), + SortError::OpenTmpFileFailed { error } => { + write!( + f, + "failed to open temporary file: {}", + strip_errno(&error.to_string()) + ) + } + SortError::CompressProgExecutionFailed { code } => { + write!(f, "couldn't execute compress program: errno {}", code) + } + SortError::CompressProgTerminatedAbnormally { prog } => { + write!(f, "'{}' terminated abnormally", prog) + } + SortError::TmpDirCreationFailed => write!(f, "could not create temporary directory"), + SortError::Uft8Error { error } => write!(f, "{}", error), + } + } +} + #[derive(Eq, Ord, PartialEq, PartialOrd, Clone, Copy, Debug)] enum SortMode { Numeric, @@ -150,23 +259,23 @@ pub struct Output { } impl Output { - fn new(name: Option<&str>) -> Self { - Self { - file: name.map(|name| { - // This is different from `File::create()` because we don't truncate the output yet. - // This allows using the output file as an input file. - ( - name.to_owned(), - OpenOptions::new() - .write(true) - .create(true) - .open(name) - .unwrap_or_else(|e| { - crash!(2, "open failed: {}: {}", name, strip_errno(&e.to_string())) - }), - ) - }), - } + fn new(name: Option<&str>) -> UResult { + let file = if let Some(name) = name { + // This is different from `File::create()` because we don't truncate the output yet. + // This allows using the output file as an input file. + let file = OpenOptions::new() + .write(true) + .create(true) + .open(name) + .map_err(|e| SortError::OpenFailed { + path: name.to_owned(), + error: e, + })?; + Some((name.to_owned(), file)) + } else { + None + }; + Ok(Self { file }) } fn into_write(self) -> BufWriter> { @@ -724,33 +833,37 @@ impl FieldSelector { } } - fn parse(key: &str, global_settings: &GlobalSettings) -> Self { + fn parse(key: &str, global_settings: &GlobalSettings) -> UResult { let mut from_to = key.split(','); let (from, from_options) = Self::split_key_options(from_to.next().unwrap()); let to = from_to.next().map(|to| Self::split_key_options(to)); let options_are_empty = from_options.is_empty() && matches!(to, None | Some((_, ""))); - crash_if_err!( - 2, - if options_are_empty { - // Inherit the global settings if there are no options attached to this key. - (|| { - // This would be ideal for a try block, I think. In the meantime this closure allows - // to use the `?` operator here. - Self::new( - KeyPosition::new(from, 1, global_settings.ignore_leading_blanks)?, - to.map(|(to, _)| { - KeyPosition::new(to, 0, global_settings.ignore_leading_blanks) - }) - .transpose()?, - KeySettings::from(global_settings), - ) - })() - } else { - // Do not inherit from `global_settings`, as there are options attached to this key. - Self::parse_with_options((from, from_options), to) + + if options_are_empty { + // Inherit the global settings if there are no options attached to this key. + (|| { + // This would be ideal for a try block, I think. In the meantime this closure allows + // to use the `?` operator here. + Self::new( + KeyPosition::new(from, 1, global_settings.ignore_leading_blanks)?, + to.map(|(to, _)| { + KeyPosition::new(to, 0, global_settings.ignore_leading_blanks) + }) + .transpose()?, + KeySettings::from(global_settings), + ) + })() + } else { + // Do not inherit from `global_settings`, as there are options attached to this key. + Self::parse_with_options((from, from_options), to) + } + .map_err(|msg| { + SortError::ParseKeyError { + key: key.to_owned(), + msg, } - .map_err(|e| format!("failed to parse key `{}`: {}", key, e)) - ) + .into() + }) } fn parse_with_options( @@ -962,7 +1075,8 @@ fn make_sort_mode_arg<'a, 'b>(mode: &'a str, short: &'b str, help: &'b str) -> A arg } -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(); @@ -979,11 +1093,11 @@ pub fn uumain(args: impl uucore::Args) -> i32 { // (clap returns 1). if e.use_stderr() { eprintln!("{}", e.message); - return 2; + set_exit_code(2); } else { println!("{}", e.message); - return 0; } + return Ok(()); } }; @@ -998,7 +1112,7 @@ pub fn uumain(args: impl uucore::Args) -> i32 { let mut files = Vec::new(); for path in &files0_from { - let reader = open(&path); + let reader = open(&path)?; let buf_reader = BufReader::new(reader); for line in buf_reader.split(b'\0').flatten() { files.push(OsString::from( @@ -1055,12 +1169,14 @@ pub fn uumain(args: impl uucore::Args) -> i32 { env::set_var("RAYON_NUM_THREADS", &settings.threads); } - settings.buffer_size = matches - .value_of(options::BUF_SIZE) - .map_or(DEFAULT_BUF_SIZE, |s| { - GlobalSettings::parse_byte_count(s) - .unwrap_or_else(|e| crash!(2, "{}", format_error_message(e, s, options::BUF_SIZE))) - }); + settings.buffer_size = + matches + .value_of(options::BUF_SIZE) + .map_or(Ok(DEFAULT_BUF_SIZE), |s| { + GlobalSettings::parse_byte_count(s).map_err(|e| { + USimpleError::new(2, format_error_message(e, s, options::BUF_SIZE)) + }) + })?; settings.tmp_dir = matches .value_of(options::TMP_DIR) @@ -1070,9 +1186,9 @@ pub fn uumain(args: impl uucore::Args) -> i32 { settings.compress_prog = matches.value_of(options::COMPRESS_PROG).map(String::from); if let Some(n_merge) = matches.value_of(options::BATCH_SIZE) { - settings.merge_batch_size = n_merge - .parse() - .unwrap_or_else(|_| crash!(2, "invalid --batch-size argument '{}'", n_merge)); + settings.merge_batch_size = n_merge.parse().map_err(|_| { + UUsageError::new(2, format!("invalid --batch-size argument '{}'", n_merge)) + })?; } settings.zero_terminated = matches.is_present(options::ZERO_TERMINATED); @@ -1101,11 +1217,13 @@ pub fn uumain(args: impl uucore::Args) -> i32 { /* if no file, default to stdin */ files.push("-".to_string().into()); } else if settings.check && files.len() != 1 { - crash!( + return Err(UUsageError::new( 2, - "extra operand `{}' not allowed with -c", - files[1].to_string_lossy() - ) + format!( + "extra operand `{}' not allowed with -c", + files[1].to_string_lossy() + ), + )); } if let Some(arg) = matches.args.get(options::SEPARATOR) { @@ -1115,14 +1233,17 @@ pub fn uumain(args: impl uucore::Args) -> i32 { separator = "\0"; } if separator.len() != 1 { - crash!(2, "separator must be exactly one character long"); + return Err(UUsageError::new( + 2, + "separator must be exactly one character long".into(), + )); } settings.separator = Some(separator.chars().next().unwrap()) } if let Some(values) = matches.values_of(options::KEY) { for value in values { - let selector = FieldSelector::parse(value, &settings); + let selector = FieldSelector::parse(value, &settings)?; if selector.settings.mode == SortMode::Random && settings.salt.is_none() { settings.salt = Some(get_rand_string()); } @@ -1152,10 +1273,10 @@ pub fn uumain(args: impl uucore::Args) -> i32 { // and to reopen them at a later point. This is different from how the output file is handled, // probably to prevent running out of file descriptors. for file in &files { - open(file); + open(file)?; } - let output = Output::new(matches.value_of(options::OUTPUT)); + let output = Output::new(matches.value_of(options::OUTPUT))?; settings.init_precomputed(); @@ -1382,21 +1503,20 @@ pub fn uu_app() -> App<'static, 'static> { ) } -fn exec(files: &mut [OsString], settings: &GlobalSettings, output: Output) -> i32 { +fn exec(files: &mut [OsString], settings: &GlobalSettings, output: Output) -> UResult<()> { if settings.merge { - let mut file_merger = merge::merge(files, settings, output.as_output_name()); - file_merger.write_all(settings, output); + let file_merger = merge::merge(files, settings, output.as_output_name())?; + file_merger.write_all(settings, output) } else if settings.check { if files.len() > 1 { - crash!(2, "only one file allowed with -c"); + Err(UUsageError::new(2, "only one file allowed with -c".into())) + } else { + check::check(files.first().unwrap(), settings) } - return check::check(files.first().unwrap(), settings); } else { let mut lines = files.iter().map(open); - - ext_sort(&mut lines, settings, output); + ext_sort(&mut lines, settings, output) } - 0 } fn sort_by<'a>(unsorted: &mut Vec>, settings: &GlobalSettings, line_data: &LineData<'a>) { @@ -1692,25 +1812,22 @@ fn strip_errno(err: &str) -> &str { &err[..err.find(" (os error ").unwrap_or(err.len())] } -fn open(path: impl AsRef) -> Box { +fn open(path: impl AsRef) -> UResult> { let path = path.as_ref(); if path == "-" { let stdin = stdin(); - return Box::new(stdin) as Box; + return Ok(Box::new(stdin) as Box); } let path = Path::new(path); match File::open(path) { - Ok(f) => Box::new(f) as Box, - Err(e) => { - crash!( - 2, - "cannot read: {0}: {1}", - path.to_string_lossy(), - strip_errno(&e.to_string()) - ); + Ok(f) => Ok(Box::new(f) as Box), + Err(error) => Err(SortError::ReadFailed { + path: path.to_string_lossy().to_string(), + error, } + .into()), } }