From c1f67ed775bb33cdf17d563c20ebce8cdf886cde Mon Sep 17 00:00:00 2001 From: Michael Debertol Date: Sat, 22 May 2021 23:06:30 +0200 Subject: [PATCH 1/4] sort: support --sort flag and check for conflicts `sort` supports three ways to specify the sort mode: a long option (e.g. --numeric-sort), a short option (e.g. -n) and the sort flag (e.g. --sort=numeric). This adds support for the sort flag. Additionally, sort modes now conflict, which means that an error is shown when multiple modes are passed, instead of silently picking a mode. For consistency, I added the `random` sort mode to the `SortMode` enum, instead of it being a bool flag. --- src/uu/sort/src/sort.rs | 178 +++++++++++++++++++++++-------------- tests/by-util/test_sort.rs | 139 +++++++++++++++++++---------- 2 files changed, 202 insertions(+), 115 deletions(-) diff --git a/src/uu/sort/src/sort.rs b/src/uu/sort/src/sort.rs index bc3b65492..81787ece6 100644 --- a/src/uu/sort/src/sort.rs +++ b/src/uu/sort/src/sort.rs @@ -64,6 +64,17 @@ static OPT_NUMERIC_SORT: &str = "numeric-sort"; static OPT_GENERAL_NUMERIC_SORT: &str = "general-numeric-sort"; static OPT_VERSION_SORT: &str = "version-sort"; +static OPT_SORT: &str = "sort"; + +static ALL_SORT_MODES: &[&str] = &[ + OPT_GENERAL_NUMERIC_SORT, + OPT_HUMAN_NUMERIC_SORT, + OPT_MONTH_SORT, + OPT_NUMERIC_SORT, + OPT_VERSION_SORT, + OPT_RANDOM, +]; + static OPT_DICTIONARY_ORDER: &str = "dictionary-order"; static OPT_MERGE: &str = "merge"; static OPT_CHECK: &str = "check"; @@ -105,6 +116,7 @@ enum SortMode { GeneralNumeric, Month, Version, + Random, Default, } #[derive(Clone)] @@ -122,7 +134,6 @@ pub struct GlobalSettings { unique: bool, check: bool, check_silent: bool, - random: bool, salt: String, selectors: Vec, separator: Option, @@ -191,7 +202,6 @@ impl Default for GlobalSettings { unique: false, check: false, check_silent: false, - random: false, salt: String::new(), selectors: vec![], separator: None, @@ -209,7 +219,6 @@ struct KeySettings { ignore_case: bool, dictionary_order: bool, ignore_non_printing: bool, - random: bool, reverse: bool, } @@ -220,7 +229,6 @@ impl From<&GlobalSettings> for KeySettings { ignore_blanks: settings.ignore_blanks, ignore_case: settings.ignore_case, ignore_non_printing: settings.ignore_non_printing, - random: settings.random, reverse: settings.reverse, dictionary_order: settings.dictionary_order, } @@ -398,7 +406,7 @@ impl<'a> Line<'a> { } } } - if !(settings.random + if !(settings.mode == SortMode::Random || settings.stable || settings.unique || !(settings.dictionary_order @@ -502,7 +510,7 @@ impl KeyPosition { 'h' => settings.mode = SortMode::HumanNumeric, 'i' => settings.ignore_non_printing = true, 'n' => settings.mode = SortMode::Numeric, - 'R' => settings.random = true, + 'R' => settings.mode = SortMode::Random, 'r' => settings.reverse = true, 'V' => settings.mode = SortMode::Version, c => { @@ -526,7 +534,9 @@ impl KeyPosition { | SortMode::GeneralNumeric | SortMode::Month => SortMode::Default, // Only SortMode::Default and SortMode::Version work with dictionary_order and ignore_non_printing - m @ SortMode::Default | m @ SortMode::Version => m, + m @ SortMode::Default + | m @ SortMode::Version + | m @ SortMode::Random => m, } } _ => {} @@ -720,6 +730,16 @@ With no FILE, or when FILE is -, read standard input.", ) } +fn make_sort_mode_arg<'a, 'b>(mode: &'a str, short: &'b str, help: &'b str) -> Arg<'a, 'b> { + let mut arg = Arg::with_name(mode).short(short).long(mode).help(help); + for possible_mode in ALL_SORT_MODES { + if *possible_mode != mode { + arg = arg.conflicts_with(possible_mode); + } + } + arg +} + pub fn uumain(args: impl uucore::Args) -> i32 { let args = args .collect_str(InvalidEncodingHandling::Ignore) @@ -732,34 +752,62 @@ pub fn uumain(args: impl uucore::Args) -> i32 { .about(ABOUT) .usage(&usage[..]) .arg( - Arg::with_name(OPT_HUMAN_NUMERIC_SORT) - .short("h") - .long(OPT_HUMAN_NUMERIC_SORT) - .help("compare according to human readable sizes, eg 1M > 100k"), + Arg::with_name(OPT_SORT) + .long(OPT_SORT) + .takes_value(true) + .possible_values( + &[ + "general-numeric", + "human-numeric", + "month", + "numeric", + "version", + "random", + ] + ) + .conflicts_with_all(ALL_SORT_MODES) ) .arg( - Arg::with_name(OPT_MONTH_SORT) - .short("M") - .long(OPT_MONTH_SORT) - .help("compare according to month name abbreviation"), + make_sort_mode_arg( + OPT_HUMAN_NUMERIC_SORT, + "h", + "compare according to human readable sizes, eg 1M > 100k" + ), ) .arg( - Arg::with_name(OPT_NUMERIC_SORT) - .short("n") - .long(OPT_NUMERIC_SORT) - .help("compare according to string numerical value"), + make_sort_mode_arg( + OPT_MONTH_SORT, + "M", + "compare according to month name abbreviation" + ), ) .arg( - Arg::with_name(OPT_GENERAL_NUMERIC_SORT) - .short("g") - .long(OPT_GENERAL_NUMERIC_SORT) - .help("compare according to string general numerical value"), + make_sort_mode_arg( + OPT_NUMERIC_SORT, + "n", + "compare according to string numerical value" + ), ) .arg( - Arg::with_name(OPT_VERSION_SORT) - .short("V") - .long(OPT_VERSION_SORT) - .help("Sort by SemVer version number, eg 1.12.2 > 1.1.2"), + make_sort_mode_arg( + OPT_GENERAL_NUMERIC_SORT, + "g", + "compare according to string general numerical value" + ), + ) + .arg( + make_sort_mode_arg( + OPT_VERSION_SORT, + "V", + "Sort by SemVer version number, eg 1.12.2 > 1.1.2", + ), + ) + .arg( + make_sort_mode_arg( + OPT_RANDOM, + "R", + "shuffle in random order", + ), ) .arg( Arg::with_name(OPT_DICTIONARY_ORDER) @@ -813,12 +861,6 @@ pub fn uumain(args: impl uucore::Args) -> i32 { .takes_value(true) .value_name("FILENAME"), ) - .arg( - Arg::with_name(OPT_RANDOM) - .short("R") - .long(OPT_RANDOM) - .help("shuffle in random order"), - ) .arg( Arg::with_name(OPT_REVERSE) .short("r") @@ -925,16 +967,25 @@ pub fn uumain(args: impl uucore::Args) -> i32 { .unwrap_or_default() }; - settings.mode = if matches.is_present(OPT_HUMAN_NUMERIC_SORT) { + settings.mode = if matches.is_present(OPT_HUMAN_NUMERIC_SORT) + || matches.value_of(OPT_SORT) == Some("human-numeric") + { SortMode::HumanNumeric - } else if matches.is_present(OPT_MONTH_SORT) { + } else if matches.is_present(OPT_MONTH_SORT) || matches.value_of(OPT_SORT) == Some("month") { SortMode::Month - } else if matches.is_present(OPT_GENERAL_NUMERIC_SORT) { + } else if matches.is_present(OPT_GENERAL_NUMERIC_SORT) + || matches.value_of(OPT_SORT) == Some("general-numeric") + { SortMode::GeneralNumeric - } else if matches.is_present(OPT_NUMERIC_SORT) { + } else if matches.is_present(OPT_NUMERIC_SORT) || matches.value_of(OPT_SORT) == Some("numeric") + { SortMode::Numeric - } else if matches.is_present(OPT_VERSION_SORT) { + } else if matches.is_present(OPT_VERSION_SORT) || matches.value_of(OPT_SORT) == Some("version") + { SortMode::Version + } else if matches.is_present(OPT_RANDOM) || matches.value_of(OPT_SORT) == Some("random") { + settings.salt = get_rand_string(); + SortMode::Random } else { SortMode::Default }; @@ -978,11 +1029,6 @@ pub fn uumain(args: impl uucore::Args) -> i32 { settings.stable = matches.is_present(OPT_STABLE); settings.unique = matches.is_present(OPT_UNIQUE); - if matches.is_present(OPT_RANDOM) { - settings.random = matches.is_present(OPT_RANDOM); - settings.salt = get_rand_string(); - } - if files.is_empty() { /* if no file, default to stdin */ files.push("-".to_owned()); @@ -1110,28 +1156,25 @@ fn compare_by<'a>(a: &Line<'a>, b: &Line<'a>, global_settings: &GlobalSettings) let b_str = b_selection.slice; let settings = &selector.settings; - let cmp: Ordering = if settings.random { - random_shuffle(a_str, b_str, &global_settings.salt) - } else { - match settings.mode { - SortMode::Numeric | SortMode::HumanNumeric => numeric_str_cmp( - (a_str, a_selection.num_cache.as_ref().unwrap().as_num_info()), - (b_str, b_selection.num_cache.as_ref().unwrap().as_num_info()), - ), - SortMode::GeneralNumeric => general_numeric_compare( - a_selection.num_cache.as_ref().unwrap().as_f64(), - b_selection.num_cache.as_ref().unwrap().as_f64(), - ), - SortMode::Month => month_compare(a_str, b_str), - SortMode::Version => version_compare(a_str, b_str), - SortMode::Default => custom_str_cmp( - a_str, - b_str, - settings.ignore_non_printing, - settings.dictionary_order, - settings.ignore_case, - ), - } + let cmp: Ordering = match settings.mode { + SortMode::Random => random_shuffle(a_str, b_str, &global_settings.salt), + SortMode::Numeric | SortMode::HumanNumeric => numeric_str_cmp( + (a_str, a_selection.num_cache.as_ref().unwrap().as_num_info()), + (b_str, b_selection.num_cache.as_ref().unwrap().as_num_info()), + ), + SortMode::GeneralNumeric => general_numeric_compare( + a_selection.num_cache.as_ref().unwrap().as_f64(), + b_selection.num_cache.as_ref().unwrap().as_f64(), + ), + SortMode::Month => month_compare(a_str, b_str), + SortMode::Version => version_compare(a_str, b_str), + SortMode::Default => custom_str_cmp( + a_str, + b_str, + settings.ignore_non_printing, + settings.dictionary_order, + settings.ignore_case, + ), }; if cmp != Ordering::Equal { return if settings.reverse { cmp.reverse() } else { cmp }; @@ -1139,7 +1182,10 @@ fn compare_by<'a>(a: &Line<'a>, b: &Line<'a>, global_settings: &GlobalSettings) } // Call "last resort compare" if all selectors returned Equal - let cmp = if global_settings.random || global_settings.stable || global_settings.unique { + let cmp = if global_settings.mode == SortMode::Random + || global_settings.stable + || global_settings.unique + { Ordering::Equal } else { a.line.cmp(b.line) diff --git a/tests/by-util/test_sort.rs b/tests/by-util/test_sort.rs index 23705d2ee..e4676b379 100644 --- a/tests/by-util/test_sort.rs +++ b/tests/by-util/test_sort.rs @@ -1,18 +1,20 @@ use crate::common::util::*; -fn test_helper(file_name: &str, args: &str) { - new_ucmd!() - .arg(format!("{}.txt", file_name)) - .args(&args.split(' ').collect::>()) - .succeeds() - .stdout_is_fixture(format!("{}.expected", file_name)); +fn test_helper(file_name: &str, possible_args: &[&str]) { + for args in possible_args { + new_ucmd!() + .arg(format!("{}.txt", file_name)) + .args(&args.split(' ').collect::>()) + .succeeds() + .stdout_is_fixture(format!("{}.expected", file_name)); - new_ucmd!() - .arg(format!("{}.txt", file_name)) - .arg("--debug") - .args(&args.split(' ').collect::>()) - .succeeds() - .stdout_is_fixture(format!("{}.expected.debug", file_name)); + new_ucmd!() + .arg(format!("{}.txt", file_name)) + .arg("--debug") + .args(&args.split(' ').collect::>()) + .succeeds() + .stdout_is_fixture(format!("{}.expected.debug", file_name)); + } } #[test] @@ -71,7 +73,7 @@ fn test_extsort_zero_terminated() { #[test] fn test_months_whitespace() { - test_helper("months-whitespace", "-M"); + test_helper("months-whitespace", &["-M", "--month-sort", "--sort=month"]); } #[test] @@ -85,7 +87,10 @@ fn test_version_empty_lines() { #[test] fn test_human_numeric_whitespace() { - test_helper("human-numeric-whitespace", "-h"); + test_helper( + "human-numeric-whitespace", + &["-h", "--human-numeric-sort", "--sort=human-numeric"], + ); } // This tests where serde often fails when reading back JSON @@ -102,12 +107,18 @@ fn test_extsort_as64_bailout() { #[test] fn test_multiple_decimals_general() { - test_helper("multiple_decimals_general", "-g") + test_helper( + "multiple_decimals_general", + &["-g", "--general-numeric-sort", "--sort=general-numeric"], + ) } #[test] fn test_multiple_decimals_numeric() { - test_helper("multiple_decimals_numeric", "-n") + test_helper( + "multiple_decimals_numeric", + &["-n", "--numeric-sort", "--sort=numeric"], + ) } #[test] @@ -186,72 +197,93 @@ fn test_random_shuffle_contains_two_runs_not_the_same() { #[test] fn test_numeric_floats_and_ints() { - test_helper("numeric_floats_and_ints", "-n"); + test_helper( + "numeric_floats_and_ints", + &["-n", "--numeric-sort", "--sort=numeric"], + ); } #[test] fn test_numeric_floats() { - test_helper("numeric_floats", "-n"); + test_helper( + "numeric_floats", + &["-n", "--numeric-sort", "--sort=numeric"], + ); } #[test] fn test_numeric_floats_with_nan() { - test_helper("numeric_floats_with_nan", "-n"); + test_helper( + "numeric_floats_with_nan", + &["-n", "--numeric-sort", "--sort=numeric"], + ); } #[test] fn test_numeric_unfixed_floats() { - test_helper("numeric_unfixed_floats", "-n"); + test_helper( + "numeric_unfixed_floats", + &["-n", "--numeric-sort", "--sort=numeric"], + ); } #[test] fn test_numeric_fixed_floats() { - test_helper("numeric_fixed_floats", "-n"); + test_helper( + "numeric_fixed_floats", + &["-n", "--numeric-sort", "--sort=numeric"], + ); } #[test] fn test_numeric_unsorted_ints() { - test_helper("numeric_unsorted_ints", "-n"); + test_helper( + "numeric_unsorted_ints", + &["-n", "--numeric-sort", "--sort=numeric"], + ); } #[test] fn test_human_block_sizes() { - test_helper("human_block_sizes", "-h"); + test_helper( + "human_block_sizes", + &["-h", "--human-numeric-sort", "--sort=human-numeric"], + ); } #[test] fn test_month_default() { - test_helper("month_default", "-M"); + test_helper("month_default", &["-M", "--month-sort", "--sort=month"]); } #[test] fn test_month_stable() { - test_helper("month_stable", "-Ms"); + test_helper("month_stable", &["-Ms"]); } #[test] fn test_default_unsorted_ints() { - test_helper("default_unsorted_ints", ""); + test_helper("default_unsorted_ints", &[""]); } #[test] fn test_numeric_unique_ints() { - test_helper("numeric_unsorted_ints_unique", "-nu"); + test_helper("numeric_unsorted_ints_unique", &["-nu"]); } #[test] fn test_version() { - test_helper("version", "-V"); + test_helper("version", &["-V"]); } #[test] fn test_ignore_case() { - test_helper("ignore_case", "-f"); + test_helper("ignore_case", &["-f"]); } #[test] fn test_dictionary_order() { - test_helper("dictionary_order", "-d"); + test_helper("dictionary_order", &["-d"]); } #[test] @@ -278,47 +310,53 @@ fn test_non_printing_chars() { #[test] fn test_exponents_positive_general_fixed() { - test_helper("exponents_general", "-g"); + test_helper("exponents_general", &["-g"]); } #[test] fn test_exponents_positive_numeric() { - test_helper("exponents-positive-numeric", "-n"); + test_helper( + "exponents-positive-numeric", + &["-n", "--numeric-sort", "--sort=numeric"], + ); } #[test] fn test_months_dedup() { - test_helper("months-dedup", "-Mu"); + test_helper("months-dedup", &["-Mu"]); } #[test] fn test_mixed_floats_ints_chars_numeric() { - test_helper("mixed_floats_ints_chars_numeric", "-n"); + test_helper( + "mixed_floats_ints_chars_numeric", + &["-n", "--numeric-sort", "--sort=numeric"], + ); } #[test] fn test_mixed_floats_ints_chars_numeric_unique() { - test_helper("mixed_floats_ints_chars_numeric_unique", "-nu"); + test_helper("mixed_floats_ints_chars_numeric_unique", &["-nu"]); } #[test] fn test_words_unique() { - test_helper("words_unique", "-u"); + test_helper("words_unique", &["-u"]); } #[test] fn test_numeric_unique() { - test_helper("numeric_unique", "-nu"); + test_helper("numeric_unique", &["-nu"]); } #[test] fn test_mixed_floats_ints_chars_numeric_reverse() { - test_helper("mixed_floats_ints_chars_numeric_unique_reverse", "-nur"); + test_helper("mixed_floats_ints_chars_numeric_unique_reverse", &["-nur"]); } #[test] fn test_mixed_floats_ints_chars_numeric_stable() { - test_helper("mixed_floats_ints_chars_numeric_stable", "-ns"); + test_helper("mixed_floats_ints_chars_numeric_stable", &["-ns"]); } #[test] @@ -347,12 +385,15 @@ fn test_numeric_floats2() { #[test] fn test_numeric_floats_with_nan2() { - test_helper("numeric-floats-with-nan2", "-n"); + test_helper( + "numeric-floats-with-nan2", + &["-n", "--numeric-sort", "--sort=numeric"], + ); } #[test] fn test_human_block_sizes2() { - for human_numeric_sort_param in vec!["-h", "--human-numeric-sort"] { + for human_numeric_sort_param in &["-h", "--human-numeric-sort", "--sort=human-numeric"] { let input = "8981K\n909991M\n-8T\n21G\n0.8M"; new_ucmd!() .arg(human_numeric_sort_param) @@ -364,7 +405,7 @@ fn test_human_block_sizes2() { #[test] fn test_month_default2() { - for month_sort_param in vec!["-M", "--month-sort"] { + for month_sort_param in &["-M", "--month-sort", "--sort=month"] { let input = "JAn\nMAY\n000may\nJun\nFeb"; new_ucmd!() .arg(month_sort_param) @@ -397,32 +438,32 @@ fn test_numeric_unique_ints2() { #[test] fn test_keys_open_ended() { - test_helper("keys_open_ended", "-k 2.3"); + test_helper("keys_open_ended", &["-k 2.3"]); } #[test] fn test_keys_closed_range() { - test_helper("keys_closed_range", "-k 2.2,2.2"); + test_helper("keys_closed_range", &["-k 2.2,2.2"]); } #[test] fn test_keys_multiple_ranges() { - test_helper("keys_multiple_ranges", "-k 2,2 -k 3,3"); + test_helper("keys_multiple_ranges", &["-k 2,2 -k 3,3"]); } #[test] fn test_keys_no_field_match() { - test_helper("keys_no_field_match", "-k 4,4"); + test_helper("keys_no_field_match", &["-k 4,4"]); } #[test] fn test_keys_no_char_match() { - test_helper("keys_no_char_match", "-k 1.2"); + test_helper("keys_no_char_match", &["-k 1.2"]); } #[test] fn test_keys_custom_separator() { - test_helper("keys_custom_separator", "-k 2.2,2.2 -t x"); + test_helper("keys_custom_separator", &["-k 2.2,2.2 -t x"]); } #[test] @@ -534,7 +575,7 @@ aaaa #[test] fn test_zero_terminated() { - test_helper("zero-terminated", "-z"); + test_helper("zero-terminated", &["-z"]); } #[test] From 4aaeede3d8475058531daf740059ed44c3a12850 Mon Sep 17 00:00:00 2001 From: Sylvestre Ledru Date: Sun, 23 May 2021 00:13:53 +0200 Subject: [PATCH 2/4] rustfmt the recent change --- src/uu/pinky/src/pinky.rs | 2 +- src/uu/sort/src/chunks.rs | 4 +--- src/uu/sort/src/sort.rs | 4 +--- src/uu/stdbuf/src/stdbuf.rs | 25 +++++++++++++------------ tests/by-util/test_stdbuf.rs | 12 ++++++------ tests/by-util/test_tail.rs | 4 ---- tests/by-util/test_who.rs | 8 ++------ 7 files changed, 24 insertions(+), 35 deletions(-) diff --git a/src/uu/pinky/src/pinky.rs b/src/uu/pinky/src/pinky.rs index f0ab44e5f..d65775c2d 100644 --- a/src/uu/pinky/src/pinky.rs +++ b/src/uu/pinky/src/pinky.rs @@ -48,7 +48,7 @@ fn get_usage() -> String { fn get_long_usage() -> String { format!( "A lightweight 'finger' program; print user information.\n\ - The utmp file will be {}.", + The utmp file will be {}.", utmpx::DEFAULT_FILE ) } diff --git a/src/uu/sort/src/chunks.rs b/src/uu/sort/src/chunks.rs index 7a7749003..6ec759211 100644 --- a/src/uu/sort/src/chunks.rs +++ b/src/uu/sort/src/chunks.rs @@ -223,9 +223,7 @@ fn read_to_buffer( Err(e) if e.kind() == ErrorKind::Interrupted => { // retry } - Err(e) => { - crash!(1, "{}", e) - } + Err(e) => crash!(1, "{}", e), } } } diff --git a/src/uu/sort/src/sort.rs b/src/uu/sort/src/sort.rs index bc3b65492..1bbfdc5c5 100644 --- a/src/uu/sort/src/sort.rs +++ b/src/uu/sort/src/sort.rs @@ -505,9 +505,7 @@ impl KeyPosition { 'R' => settings.random = true, 'r' => settings.reverse = true, 'V' => settings.mode = SortMode::Version, - c => { - crash!(1, "invalid option for key: `{}`", c) - } + c => crash!(1, "invalid option for key: `{}`", c), } // All numeric sorts and month sort conflict with dictionary_order and ignore_non_printing. // Instad of reporting an error, let them overwrite each other. diff --git a/src/uu/stdbuf/src/stdbuf.rs b/src/uu/stdbuf/src/stdbuf.rs index 77f6d9dad..485b3c70e 100644 --- a/src/uu/stdbuf/src/stdbuf.rs +++ b/src/uu/stdbuf/src/stdbuf.rs @@ -24,18 +24,19 @@ use uucore::InvalidEncodingHandling; static VERSION: &str = env!("CARGO_PKG_VERSION"); static ABOUT: &str = "Run COMMAND, with modified buffering operations for its standard streams.\n\n\ - Mandatory arguments to long options are mandatory for short options too."; -static LONG_HELP: &str = "If MODE is 'L' the corresponding stream will be line buffered.\n\ - This option is invalid with standard input.\n\n\ - If MODE is '0' the corresponding stream will be unbuffered.\n\n\ - Otherwise MODE is a number which may be followed by one of the following:\n\n\ - KB 1000, K 1024, MB 1000*1000, M 1024*1024, and so on for G, T, P, E, Z, Y.\n\ - In this case the corresponding stream will be fully buffered with the buffer size set to \ - MODE bytes.\n\n\ - NOTE: If COMMAND adjusts the buffering of its standard streams ('tee' does for e.g.) then \ - that will override corresponding settings changed by 'stdbuf'.\n\ - Also some filters (like 'dd' and 'cat' etc.) don't use streams for I/O, \ - and are thus unaffected by 'stdbuf' settings.\n"; + Mandatory arguments to long options are mandatory for short options too."; +static LONG_HELP: &str = + "If MODE is 'L' the corresponding stream will be line buffered.\n\ + This option is invalid with standard input.\n\n\ + If MODE is '0' the corresponding stream will be unbuffered.\n\n\ + Otherwise MODE is a number which may be followed by one of the following:\n\n\ + KB 1000, K 1024, MB 1000*1000, M 1024*1024, and so on for G, T, P, E, Z, Y.\n\ + In this case the corresponding stream will be fully buffered with the buffer size set to \ + MODE bytes.\n\n\ + NOTE: If COMMAND adjusts the buffering of its standard streams ('tee' does for e.g.) then \ + that will override corresponding settings changed by 'stdbuf'.\n\ + Also some filters (like 'dd' and 'cat' etc.) don't use streams for I/O, \ + and are thus unaffected by 'stdbuf' settings.\n"; mod options { pub const INPUT: &str = "input"; diff --git a/tests/by-util/test_stdbuf.rs b/tests/by-util/test_stdbuf.rs index 808b7382a..4105cb7a2 100644 --- a/tests/by-util/test_stdbuf.rs +++ b/tests/by-util/test_stdbuf.rs @@ -27,12 +27,12 @@ fn test_stdbuf_line_buffered_stdout() { fn test_stdbuf_no_buffer_option_fails() { new_ucmd!().args(&["head"]).fails().stderr_is( "error: The following required arguments were not provided:\n \ - --error \n \ - --input \n \ - --output \n\n\ - USAGE:\n \ - stdbuf OPTION... COMMAND\n\n\ - For more information try --help", + --error \n \ + --input \n \ + --output \n\n\ + USAGE:\n \ + stdbuf OPTION... COMMAND\n\n\ + For more information try --help", ); } diff --git a/tests/by-util/test_tail.rs b/tests/by-util/test_tail.rs index dddbb9c31..f3c9a7b11 100644 --- a/tests/by-util/test_tail.rs +++ b/tests/by-util/test_tail.rs @@ -349,7 +349,6 @@ fn test_sleep_interval() { new_ucmd!().arg("-s").arg("10").arg(FOOBAR_TXT).succeeds(); } - /// Test for reading all but the first NUM bytes: `tail -c +3`. #[test] fn test_positive_bytes() { @@ -360,7 +359,6 @@ fn test_positive_bytes() { .stdout_is("cde"); } - /// Test for reading all bytes, specified by `tail -c +0`. #[test] fn test_positive_zero_bytes() { @@ -371,7 +369,6 @@ fn test_positive_zero_bytes() { .stdout_is("abcde"); } - /// Test for reading all but the first NUM lines: `tail -n +3`. #[test] fn test_positive_lines() { @@ -382,7 +379,6 @@ fn test_positive_lines() { .stdout_is("c\nd\ne\n"); } - /// Test for reading all lines, specified by `tail -n +0`. #[test] fn test_positive_zero_lines() { diff --git a/tests/by-util/test_who.rs b/tests/by-util/test_who.rs index 1aa8d604d..df023bb0a 100644 --- a/tests/by-util/test_who.rs +++ b/tests/by-util/test_who.rs @@ -97,13 +97,9 @@ fn test_runlevel() { #[cfg(any(target_vendor = "apple", target_os = "freebsd"))] #[test] fn test_runlevel() { - let expected = - "error: Found argument"; + let expected = "error: Found argument"; for opt in vec!["-r", "--runlevel"] { - new_ucmd!() - .arg(opt) - .fails() - .stderr_contains(expected); + new_ucmd!().arg(opt).fails().stderr_contains(expected); } } From 95092e64402cf5feadeb4d5f496cc1f8cbdd239e Mon Sep 17 00:00:00 2001 From: Sylvestre Ledru Date: Sun, 23 May 2021 00:33:54 +0200 Subject: [PATCH 3/4] ignore test_should_calculate_implicit_padding_per_free_argument Fails from time to time with ``` ---- test_numfmt::test_should_calculate_implicit_padding_per_free_argument stdout ---- current_directory_resolved: run: /target/x86_64-unknown-linux-musl/debug/coreutils numfmt --from=auto 1Ki 2K thread 'test_numfmt::test_should_calculate_implicit_padding_per_free_argument' panicked at 'failed to write to stdin of child: Broken pipe (os error 32)', tests/common/util.rs:859:21 ``` --- tests/by-util/test_numfmt.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/by-util/test_numfmt.rs b/tests/by-util/test_numfmt.rs index 64fc5360d..b52dbc359 100644 --- a/tests/by-util/test_numfmt.rs +++ b/tests/by-util/test_numfmt.rs @@ -281,6 +281,7 @@ fn test_leading_whitespace_in_free_argument_should_imply_padding() { } #[test] +#[ignore] fn test_should_calculate_implicit_padding_per_free_argument() { new_ucmd!() .args(&["--from=auto", " 1Ki", " 2K"]) From bc9db289e8edad21de4b2e57a457542d6b1280ee Mon Sep 17 00:00:00 2001 From: Jeffrey Finkelstein Date: Tue, 11 May 2021 23:48:06 -0400 Subject: [PATCH 4/4] head: add abstractions for "all but last n lines" Add some abstractions to simplify the `rbuf_but_last_n_lines()` function, which implements the "take all but the last `n` lines" functionality of the `head` program. This commit adds - `RingBuffer`, a fixed-size ring buffer, - `ZLines`, an iterator over zero-terminated "lines", - `TakeAllBut`, an iterator over all but the last `n` elements of an iterator. These three together make the implementation of `rbuf_but_last_n_lines()` concise. --- src/uu/head/Cargo.toml | 2 +- src/uu/head/src/head.rs | 42 +++---- src/uu/head/src/lines.rs | 73 ++++++++++++ src/uu/head/src/take.rs | 93 +++++++++++++++ src/uu/tail/Cargo.toml | 2 +- src/uu/tail/src/ringbuffer.rs | 61 ---------- src/uu/tail/src/tail.rs | 3 +- src/uucore/Cargo.toml | 1 + src/uucore/src/lib/features.rs | 2 + src/uucore/src/lib/features/ringbuffer.rs | 134 ++++++++++++++++++++++ src/uucore/src/lib/lib.rs | 2 + tests/by-util/test_head.rs | 9 ++ 12 files changed, 333 insertions(+), 91 deletions(-) create mode 100644 src/uu/head/src/lines.rs create mode 100644 src/uu/head/src/take.rs delete mode 100644 src/uu/tail/src/ringbuffer.rs create mode 100644 src/uucore/src/lib/features/ringbuffer.rs diff --git a/src/uu/head/Cargo.toml b/src/uu/head/Cargo.toml index 3c383cb6f..661052f58 100644 --- a/src/uu/head/Cargo.toml +++ b/src/uu/head/Cargo.toml @@ -16,7 +16,7 @@ path = "src/head.rs" [dependencies] clap = "2.33" -uucore = { version=">=0.0.8", package="uucore", path="../../uucore" } +uucore = { version=">=0.0.8", package="uucore", path="../../uucore", features=["ringbuffer"] } uucore_procs = { version=">=0.0.5", package="uucore_procs", path="../../uucore_procs" } [[bin]] diff --git a/src/uu/head/src/head.rs b/src/uu/head/src/head.rs index 0c8b3bc88..3602b4a73 100644 --- a/src/uu/head/src/head.rs +++ b/src/uu/head/src/head.rs @@ -27,8 +27,12 @@ mod options { pub const ZERO_NAME: &str = "ZERO"; pub const FILES_NAME: &str = "FILE"; } +mod lines; mod parse; mod split; +mod take; +use lines::zlines; +use take::take_all_but; fn app<'a>() -> App<'a, 'a> { App::new(executable!()) @@ -293,36 +297,22 @@ fn rbuf_but_last_n_bytes(input: &mut impl std::io::BufRead, n: usize) -> std::io } fn rbuf_but_last_n_lines( - input: &mut impl std::io::BufRead, + input: impl std::io::BufRead, n: usize, zero: bool, ) -> std::io::Result<()> { - if n == 0 { - //prints everything - return rbuf_n_bytes(input, std::usize::MAX); + if zero { + let stdout = std::io::stdout(); + let mut stdout = stdout.lock(); + for bytes in take_all_but(zlines(input), n) { + stdout.write_all(&bytes?)?; + } + } else { + for line in take_all_but(input.lines(), n) { + println!("{}", line?); + } } - let mut ringbuf = vec![Vec::new(); n]; - let stdout = std::io::stdout(); - let mut stdout = stdout.lock(); - let mut line = Vec::new(); - let mut lines = 0usize; - split::walk_lines(input, zero, |e| match e { - split::Event::Data(dat) => { - line.extend_from_slice(dat); - Ok(true) - } - split::Event::Line => { - if lines < n { - ringbuf[lines] = std::mem::replace(&mut line, Vec::new()); - lines += 1; - } else { - stdout.write_all(&ringbuf[0])?; - ringbuf.rotate_left(1); - ringbuf[n - 1] = std::mem::replace(&mut line, Vec::new()); - } - Ok(true) - } - }) + Ok(()) } fn head_backwards_file(input: &mut std::fs::File, options: &HeadOptions) -> std::io::Result<()> { diff --git a/src/uu/head/src/lines.rs b/src/uu/head/src/lines.rs new file mode 100644 index 000000000..dcae27bc8 --- /dev/null +++ b/src/uu/head/src/lines.rs @@ -0,0 +1,73 @@ +//! Iterate over zero-terminated lines. +use std::io::BufRead; + +/// The zero byte, representing the null character. +const ZERO: u8 = 0; + +/// Returns an iterator over the lines of the given reader. +/// +/// The iterator returned from this function will yield instances of +/// [`io::Result`]<[`Vec`]<[`u8`]>>, representing the bytes of the line +/// *including* the null character (with the possible exception of the +/// last line, which may not have one). +/// +/// # Examples +/// +/// ```rust,ignore +/// use std::io::Cursor; +/// +/// let cursor = Cursor::new(b"x\0y\0z\0"); +/// let mut iter = zlines(cursor).map(|l| l.unwrap()); +/// assert_eq!(iter.next(), Some(b"x\0".to_vec())); +/// assert_eq!(iter.next(), Some(b"y\0".to_vec())); +/// assert_eq!(iter.next(), Some(b"z\0".to_vec())); +/// assert_eq!(iter.next(), None); +/// ``` +pub fn zlines(buf: B) -> ZLines { + ZLines { buf } +} + +/// An iterator over the zero-terminated lines of an instance of `BufRead`. +pub struct ZLines { + buf: B, +} + +impl Iterator for ZLines { + type Item = std::io::Result>; + + fn next(&mut self) -> Option>> { + let mut buf = Vec::new(); + match self.buf.read_until(ZERO, &mut buf) { + Ok(0) => None, + Ok(_) => Some(Ok(buf)), + Err(e) => Some(Err(e)), + } + } +} + +#[cfg(test)] +mod tests { + + use crate::lines::zlines; + use std::io::Cursor; + + #[test] + fn test_null_terminated() { + let cursor = Cursor::new(b"x\0y\0z\0"); + let mut iter = zlines(cursor).map(|l| l.unwrap()); + assert_eq!(iter.next(), Some(b"x\0".to_vec())); + assert_eq!(iter.next(), Some(b"y\0".to_vec())); + assert_eq!(iter.next(), Some(b"z\0".to_vec())); + assert_eq!(iter.next(), None); + } + + #[test] + fn test_not_null_terminated() { + let cursor = Cursor::new(b"x\0y\0z"); + let mut iter = zlines(cursor).map(|l| l.unwrap()); + assert_eq!(iter.next(), Some(b"x\0".to_vec())); + assert_eq!(iter.next(), Some(b"y\0".to_vec())); + assert_eq!(iter.next(), Some(b"z".to_vec())); + assert_eq!(iter.next(), None); + } +} diff --git a/src/uu/head/src/take.rs b/src/uu/head/src/take.rs new file mode 100644 index 000000000..94fa012be --- /dev/null +++ b/src/uu/head/src/take.rs @@ -0,0 +1,93 @@ +//! Take all but the last elements of an iterator. +use uucore::ringbuffer::RingBuffer; + +/// Create an iterator over all but the last `n` elements of `iter`. +/// +/// # Examples +/// +/// ```rust,ignore +/// let data = [1, 2, 3, 4, 5]; +/// let n = 2; +/// let mut iter = take_all_but(data.iter(), n); +/// assert_eq!(Some(4), iter.next()); +/// assert_eq!(Some(5), iter.next()); +/// assert_eq!(None, iter.next()); +/// ``` +pub fn take_all_but(iter: I, n: usize) -> TakeAllBut { + TakeAllBut::new(iter, n) +} + +/// An iterator that only iterates over the last elements of another iterator. +pub struct TakeAllBut { + iter: I, + buf: RingBuffer<::Item>, +} + +impl TakeAllBut { + pub fn new(mut iter: I, n: usize) -> TakeAllBut { + // Create a new ring buffer and fill it up. + // + // If there are fewer than `n` elements in `iter`, then we + // exhaust the iterator so that whenever `TakeAllBut::next()` is + // called, it will return `None`, as expected. + let mut buf = RingBuffer::new(n); + for _ in 0..n { + let value = match iter.next() { + None => { + break; + } + Some(x) => x, + }; + buf.push_back(value); + } + TakeAllBut { iter, buf } + } +} + +impl Iterator for TakeAllBut +where + I: Iterator, +{ + type Item = ::Item; + + fn next(&mut self) -> Option<::Item> { + match self.iter.next() { + Some(value) => self.buf.push_back(value), + None => None, + } + } +} + +#[cfg(test)] +mod tests { + + use crate::take::take_all_but; + + #[test] + fn test_fewer_elements() { + let mut iter = take_all_but([0, 1, 2].iter(), 2); + assert_eq!(Some(&0), iter.next()); + assert_eq!(None, iter.next()); + } + + #[test] + fn test_same_number_of_elements() { + let mut iter = take_all_but([0, 1].iter(), 2); + assert_eq!(None, iter.next()); + } + + #[test] + fn test_more_elements() { + let mut iter = take_all_but([0].iter(), 2); + assert_eq!(None, iter.next()); + } + + #[test] + fn test_zero_elements() { + let mut iter = take_all_but([0, 1, 2].iter(), 0); + assert_eq!(Some(&0), iter.next()); + assert_eq!(Some(&1), iter.next()); + assert_eq!(Some(&2), iter.next()); + assert_eq!(None, iter.next()); + } +} diff --git a/src/uu/tail/Cargo.toml b/src/uu/tail/Cargo.toml index d3f60e09b..273c67bb3 100644 --- a/src/uu/tail/Cargo.toml +++ b/src/uu/tail/Cargo.toml @@ -17,7 +17,7 @@ path = "src/tail.rs" [dependencies] clap = "2.33" libc = "0.2.42" -uucore = { version=">=0.0.8", package="uucore", path="../../uucore" } +uucore = { version=">=0.0.8", package="uucore", path="../../uucore", features=["ringbuffer"] } uucore_procs = { version=">=0.0.5", package="uucore_procs", path="../../uucore_procs" } winapi = { version="0.3", features=["fileapi", "handleapi", "processthreadsapi", "synchapi", "winbase"] } diff --git a/src/uu/tail/src/ringbuffer.rs b/src/uu/tail/src/ringbuffer.rs deleted file mode 100644 index 86483b8ed..000000000 --- a/src/uu/tail/src/ringbuffer.rs +++ /dev/null @@ -1,61 +0,0 @@ -//! A fixed-size ring buffer. -use std::collections::VecDeque; - -/// A fixed-size ring buffer backed by a `VecDeque`. -/// -/// If the ring buffer is not full, then calling the [`push_back`] -/// method appends elements, as in a [`VecDeque`]. If the ring buffer -/// is full, then calling [`push_back`] removes the element at the -/// front of the buffer (in a first-in, first-out manner) before -/// appending the new element to the back of the buffer. -/// -/// Use [`from_iter`] to take the last `size` elements from an -/// iterator. -/// -/// # Examples -/// -/// After exceeding the size limit, the oldest elements are dropped in -/// favor of the newest element: -/// -/// ```rust,ignore -/// let buffer: RingBuffer = RingBuffer::new(2); -/// buffer.push_back(0); -/// buffer.push_back(1); -/// buffer.push_back(2); -/// assert_eq!(vec![1, 2], buffer.data); -/// ``` -/// -/// Take the last `n` elements from an iterator: -/// -/// ```rust,ignore -/// let iter = vec![0, 1, 2, 3].iter(); -/// assert_eq!(vec![2, 3], RingBuffer::from_iter(iter, 2).data); -/// ``` -pub struct RingBuffer { - pub data: VecDeque, - size: usize, -} - -impl RingBuffer { - pub fn new(size: usize) -> RingBuffer { - RingBuffer { - data: VecDeque::new(), - size, - } - } - - pub fn from_iter(iter: impl Iterator, size: usize) -> RingBuffer { - let mut ringbuf = RingBuffer::new(size); - for value in iter { - ringbuf.push_back(value); - } - ringbuf - } - - pub fn push_back(&mut self, value: T) { - if self.size <= self.data.len() { - self.data.pop_front(); - } - self.data.push_back(value) - } -} diff --git a/src/uu/tail/src/tail.rs b/src/uu/tail/src/tail.rs index 06d0e6fdb..15a819d35 100644 --- a/src/uu/tail/src/tail.rs +++ b/src/uu/tail/src/tail.rs @@ -17,9 +17,7 @@ extern crate uucore; mod chunks; mod platform; -mod ringbuffer; use chunks::ReverseChunks; -use ringbuffer::RingBuffer; use clap::{App, Arg}; use std::collections::VecDeque; @@ -30,6 +28,7 @@ use std::io::{stdin, stdout, BufRead, BufReader, Read, Seek, SeekFrom, Write}; use std::path::Path; use std::thread::sleep; use std::time::Duration; +use uucore::ringbuffer::RingBuffer; pub mod options { pub mod verbosity { diff --git a/src/uucore/Cargo.toml b/src/uucore/Cargo.toml index 85efe0434..482252680 100644 --- a/src/uucore/Cargo.toml +++ b/src/uucore/Cargo.toml @@ -47,6 +47,7 @@ mode = ["libc"] parse_time = [] perms = ["libc"] process = ["libc"] +ringbuffer = [] signals = [] utf8 = [] utmpx = ["time", "libc"] diff --git a/src/uucore/src/lib/features.rs b/src/uucore/src/lib/features.rs index 0287b9675..310a41fe1 100644 --- a/src/uucore/src/lib/features.rs +++ b/src/uucore/src/lib/features.rs @@ -8,6 +8,8 @@ pub mod fs; pub mod fsext; #[cfg(feature = "parse_time")] pub mod parse_time; +#[cfg(feature = "ringbuffer")] +pub mod ringbuffer; #[cfg(feature = "zero-copy")] pub mod zero_copy; diff --git a/src/uucore/src/lib/features/ringbuffer.rs b/src/uucore/src/lib/features/ringbuffer.rs new file mode 100644 index 000000000..60847df8f --- /dev/null +++ b/src/uucore/src/lib/features/ringbuffer.rs @@ -0,0 +1,134 @@ +//! A fixed-size ring buffer. +use std::collections::VecDeque; + +/// A fixed-size ring buffer backed by a `VecDeque`. +/// +/// If the ring buffer is not full, then calling the [`push_back`] +/// method appends elements, as in a [`VecDeque`]. If the ring buffer +/// is full, then calling [`push_back`] removes the element at the +/// front of the buffer (in a first-in, first-out manner) before +/// appending the new element to the back of the buffer. +/// +/// Use [`from_iter`] to take the last `size` elements from an +/// iterator. +/// +/// # Examples +/// +/// After exceeding the size limit, the oldest elements are dropped in +/// favor of the newest element: +/// +/// ```rust,ignore +/// let mut buffer: RingBuffer = RingBuffer::new(2); +/// buffer.push_back(0); +/// buffer.push_back(1); +/// buffer.push_back(2); +/// assert_eq!(vec![1, 2], buffer.data); +/// ``` +/// +/// Take the last `n` elements from an iterator: +/// +/// ```rust,ignore +/// let iter = [0, 1, 2].iter(); +/// let actual = RingBuffer::from_iter(iter, 2).data; +/// let expected = VecDeque::from_iter([1, 2].iter()); +/// assert_eq!(expected, actual); +/// ``` +pub struct RingBuffer { + pub data: VecDeque, + size: usize, +} + +impl RingBuffer { + pub fn new(size: usize) -> RingBuffer { + RingBuffer { + data: VecDeque::new(), + size, + } + } + + pub fn from_iter(iter: impl Iterator, size: usize) -> RingBuffer { + let mut ringbuf = RingBuffer::new(size); + for value in iter { + ringbuf.push_back(value); + } + ringbuf + } + + /// Append a value to the end of the ring buffer. + /// + /// If the ring buffer is not full, this method return [`None`]. If + /// the ring buffer is full, appending a new element will cause the + /// oldest element to be evicted. In that case this method returns + /// that element, or `None`. + /// + /// In the special case where the size limit is zero, each call to + /// this method with input `value` returns `Some(value)`, because + /// the input is immediately evicted. + /// + /// # Examples + /// + /// Appending an element when the buffer is full returns the oldest + /// element: + /// + /// ```rust,ignore + /// let mut buf = RingBuffer::new(3); + /// assert_eq!(None, buf.push_back(0)); + /// assert_eq!(None, buf.push_back(1)); + /// assert_eq!(None, buf.push_back(2)); + /// assert_eq!(Some(0), buf.push_back(3)); + /// ``` + /// + /// If the size limit is zero, then this method always returns the + /// input value: + /// + /// ```rust,ignore + /// let mut buf = RingBuffer::new(0); + /// assert_eq!(Some(0), buf.push_back(0)); + /// assert_eq!(Some(1), buf.push_back(1)); + /// assert_eq!(Some(2), buf.push_back(2)); + /// ``` + pub fn push_back(&mut self, value: T) -> Option { + if self.size == 0 { + return Some(value); + } + let result = if self.size <= self.data.len() { + self.data.pop_front() + } else { + None + }; + self.data.push_back(value); + result + } +} + +#[cfg(test)] +mod tests { + + use crate::ringbuffer::RingBuffer; + use std::collections::VecDeque; + use std::iter::FromIterator; + + #[test] + fn test_size_limit_zero() { + let mut buf = RingBuffer::new(0); + assert_eq!(Some(0), buf.push_back(0)); + assert_eq!(Some(1), buf.push_back(1)); + assert_eq!(Some(2), buf.push_back(2)); + } + + #[test] + fn test_evict_oldest() { + let mut buf = RingBuffer::new(2); + assert_eq!(None, buf.push_back(0)); + assert_eq!(None, buf.push_back(1)); + assert_eq!(Some(0), buf.push_back(2)); + } + + #[test] + fn test_from_iter() { + let iter = [0, 1, 2].iter(); + let actual = RingBuffer::from_iter(iter, 2).data; + let expected = VecDeque::from_iter([1, 2].iter()); + assert_eq!(expected, actual); + } +} diff --git a/src/uucore/src/lib/lib.rs b/src/uucore/src/lib/lib.rs index 28bae08cb..eb630f53a 100644 --- a/src/uucore/src/lib/lib.rs +++ b/src/uucore/src/lib/lib.rs @@ -39,6 +39,8 @@ pub use crate::features::fs; pub use crate::features::fsext; #[cfg(feature = "parse_time")] pub use crate::features::parse_time; +#[cfg(feature = "ringbuffer")] +pub use crate::features::ringbuffer; #[cfg(feature = "zero-copy")] pub use crate::features::zero_copy; diff --git a/tests/by-util/test_head.rs b/tests/by-util/test_head.rs index 88df1f068..b2a3cf0cb 100755 --- a/tests/by-util/test_head.rs +++ b/tests/by-util/test_head.rs @@ -129,6 +129,15 @@ fn test_zero_terminated_syntax_2() { .stdout_is("x\0y"); } +#[test] +fn test_zero_terminated_negative_lines() { + new_ucmd!() + .args(&["-z", "-n", "-1"]) + .pipe_in("x\0y\0z\0") + .run() + .stdout_is("x\0y\0"); +} + #[test] fn test_negative_byte_syntax() { new_ucmd!()