From 42df6acca7488cecb9871e1f0c7cbc9c6be776bb Mon Sep 17 00:00:00 2001 From: zhitkoff Date: Wed, 23 Aug 2023 20:21:26 -0400 Subject: [PATCH 1/8] split: better handle numeric and hex suffixes, short and long, with and without values Fixes #5171 --- src/uu/split/src/split.rs | 85 +++++++++++++++---- tests/by-util/test_split.rs | 158 +++++++++++++++++++++++++++++++++++- 2 files changed, 226 insertions(+), 17 deletions(-) diff --git a/src/uu/split/src/split.rs b/src/uu/split/src/split.rs index f1be0c47dc1..24fb5ca2235 100644 --- a/src/uu/split/src/split.rs +++ b/src/uu/split/src/split.rs @@ -5,7 +5,7 @@ // * For the full copyright and license information, please view the LICENSE // * file that was distributed with this source code. -// spell-checker:ignore nbbbb ncccc +// spell-checker:ignore nbbbb ncccc hexdigit mod filenames; mod number; @@ -34,7 +34,9 @@ static OPT_ADDITIONAL_SUFFIX: &str = "additional-suffix"; static OPT_FILTER: &str = "filter"; static OPT_NUMBER: &str = "number"; static OPT_NUMERIC_SUFFIXES: &str = "numeric-suffixes"; +static OPT_NUMERIC_SUFFIXES_SHORT: &str = "-d"; static OPT_HEX_SUFFIXES: &str = "hex-suffixes"; +static OPT_HEX_SUFFIXES_SHORT: &str = "-x"; static OPT_SUFFIX_LENGTH: &str = "suffix-length"; static OPT_DEFAULT_SUFFIX_LENGTH: &str = "0"; static OPT_VERBOSE: &str = "verbose"; @@ -125,28 +127,50 @@ pub fn uu_app() -> Command { ) .arg( Arg::new(OPT_NUMERIC_SUFFIXES) - .short('d') .long(OPT_NUMERIC_SUFFIXES) + .require_equals(true) .default_missing_value("0") .num_args(0..=1) + .overrides_with(OPT_NUMERIC_SUFFIXES) .help("use numeric suffixes instead of alphabetic"), ) .arg( - Arg::new(OPT_SUFFIX_LENGTH) - .short('a') - .long(OPT_SUFFIX_LENGTH) - .value_name("N") - .default_value(OPT_DEFAULT_SUFFIX_LENGTH) - .help("use suffixes of fixed length N. 0 implies dynamic length."), + Arg::new(OPT_NUMERIC_SUFFIXES_SHORT) + .short('d') + .action(clap::ArgAction::SetTrue) + .value_parser(|_: &str| -> Result { + Ok("0".to_string()) // force value to "0" + }) + .overrides_with(OPT_NUMERIC_SUFFIXES_SHORT) + .help("use numeric suffixes instead of alphabetic"), ) .arg( Arg::new(OPT_HEX_SUFFIXES) - .short('x') .long(OPT_HEX_SUFFIXES) .default_missing_value("0") + .require_equals(true) .num_args(0..=1) + .overrides_with(OPT_HEX_SUFFIXES) + .help("use hex suffixes instead of alphabetic"), + ) + .arg( + Arg::new(OPT_HEX_SUFFIXES_SHORT) + .short('x') + .action(clap::ArgAction::SetTrue) + .value_parser(|_: &str| -> Result { + Ok("0".to_string()) // force value to "0" + }) + .overrides_with(OPT_HEX_SUFFIXES_SHORT) .help("use hex suffixes instead of alphabetic"), ) + .arg( + Arg::new(OPT_SUFFIX_LENGTH) + .short('a') + .long(OPT_SUFFIX_LENGTH) + .value_name("N") + .default_value(OPT_DEFAULT_SUFFIX_LENGTH) + .help("use suffixes of fixed length N. 0 implies dynamic length."), + ) .arg( Arg::new(OPT_VERBOSE) .long(OPT_VERBOSE) @@ -411,15 +435,48 @@ impl Strategy { /// Parse the suffix type from the command-line arguments. fn suffix_type_from(matches: &ArgMatches) -> Result<(SuffixType, usize), SettingsError> { - if matches.value_source(OPT_NUMERIC_SUFFIXES) == Some(ValueSource::CommandLine) { - let suffix_start = matches.get_one::(OPT_NUMERIC_SUFFIXES); + // Collect provided suffixes (if any) + // Any combination is allowed + let mut suffixes = vec![]; + for suffix_id in [ + OPT_NUMERIC_SUFFIXES, + OPT_NUMERIC_SUFFIXES_SHORT, + OPT_HEX_SUFFIXES, + OPT_HEX_SUFFIXES_SHORT, + ] + .into_iter() + { + if matches.value_source(suffix_id) == Some(ValueSource::CommandLine) { + suffixes.push((matches.index_of(suffix_id), suffix_id)); + } + } + // Compare suffixes by the order specified in command line + // last one (highest index) is effective, all others are ignored + let mut effective_suffix_index = 0; + let mut effective_suffix_id = ""; + for suffix in suffixes.into_iter() { + let (suffix_index, suffix_id) = suffix; + if let Some(i) = suffix_index { + if i >= effective_suffix_index { + effective_suffix_index = i; + effective_suffix_id = suffix_id; + }; + } + } + + if effective_suffix_id == OPT_NUMERIC_SUFFIXES + || effective_suffix_id == OPT_NUMERIC_SUFFIXES_SHORT + { + let suffix_start = matches.get_one::(effective_suffix_id); let suffix_start = suffix_start.ok_or(SettingsError::SuffixNotParsable(String::new()))?; let suffix_start = suffix_start - .parse() + .parse::() .map_err(|_| SettingsError::SuffixNotParsable(suffix_start.to_string()))?; Ok((SuffixType::Decimal, suffix_start)) - } else if matches.value_source(OPT_HEX_SUFFIXES) == Some(ValueSource::CommandLine) { - let suffix_start = matches.get_one::(OPT_HEX_SUFFIXES); + } else if effective_suffix_id == OPT_HEX_SUFFIXES + || effective_suffix_id == OPT_HEX_SUFFIXES_SHORT + { + let suffix_start = matches.get_one::(effective_suffix_id); let suffix_start = suffix_start.ok_or(SettingsError::SuffixNotParsable(String::new()))?; let suffix_start = usize::from_str_radix(suffix_start, 16) .map_err(|_| SettingsError::SuffixNotParsable(suffix_start.to_string()))?; diff --git a/tests/by-util/test_split.rs b/tests/by-util/test_split.rs index de1bb9cdfbd..7708fd5acfe 100644 --- a/tests/by-util/test_split.rs +++ b/tests/by-util/test_split.rs @@ -2,7 +2,7 @@ // * // * For the full copyright and license information, please view the LICENSE // * file that was distributed with this source code. -// spell-checker:ignore xzaaa sixhundredfiftyonebytes ninetyonebytes threebytes asciilowercase fghij klmno pqrst uvwxyz fivelines twohundredfortyonebytes onehundredlines nbbbb +// spell-checker:ignore xzaaa sixhundredfiftyonebytes ninetyonebytes threebytes asciilowercase fghij klmno pqrst uvwxyz fivelines twohundredfortyonebytes onehundredlines nbbbb dxen use crate::common::util::{AtPath, TestScenario}; use rand::{thread_rng, Rng, SeedableRng}; @@ -715,7 +715,7 @@ fn test_multiple_of_input_chunk() { #[test] fn test_numeric_suffix() { let (at, mut ucmd) = at_and_ucmd!(); - ucmd.args(&["-n", "4", "--numeric-suffixes", "9", "threebytes.txt"]) + ucmd.args(&["-n", "4", "--numeric-suffixes=9", "threebytes.txt"]) .succeeds() .no_stdout() .no_stderr(); @@ -728,7 +728,7 @@ fn test_numeric_suffix() { #[test] fn test_hex_suffix() { let (at, mut ucmd) = at_and_ucmd!(); - ucmd.args(&["-n", "4", "--hex-suffixes", "9", "threebytes.txt"]) + ucmd.args(&["-n", "4", "--hex-suffixes=9", "threebytes.txt"]) .succeeds() .no_stdout() .no_stderr(); @@ -738,6 +738,158 @@ fn test_hex_suffix() { assert_eq!(at.read("x0c"), ""); } +#[test] +fn test_numeric_suffix_no_equal() { + new_ucmd!() + .args(&["-n", "4", "--numeric-suffixes", "9", "threebytes.txt"]) + .fails() + .stderr_contains("split: cannot open '9' for reading: No such file or directory"); +} + +#[test] +fn test_hex_suffix_no_equal() { + new_ucmd!() + .args(&["-n", "4", "--hex-suffixes", "9", "threebytes.txt"]) + .fails() + .stderr_contains("split: cannot open '9' for reading: No such file or directory"); +} + +/// Test for short numeric suffix not having any value +#[test] +fn test_short_numeric_suffix_no_value() { + let (at, mut ucmd) = at_and_ucmd!(); + ucmd.args(&["-n", "4", "-d", "threebytes.txt"]) + .succeeds() + .no_stdout() + .no_stderr(); + assert_eq!(at.read("x00"), "a"); + assert_eq!(at.read("x01"), "b"); + assert_eq!(at.read("x02"), "c"); + assert_eq!(at.read("x03"), ""); +} + +/// Test for long numeric suffix not having any value +#[test] +fn test_numeric_suffix_no_value() { + let (at, mut ucmd) = at_and_ucmd!(); + ucmd.args(&["-n", "4", "--numeric-suffixes", "threebytes.txt"]) + .succeeds() + .no_stdout() + .no_stderr(); + assert_eq!(at.read("x00"), "a"); + assert_eq!(at.read("x01"), "b"); + assert_eq!(at.read("x02"), "c"); + assert_eq!(at.read("x03"), ""); +} + +/// Test for short hex suffix not having any value +#[test] +fn test_short_hex_suffix_no_value() { + let (at, mut ucmd) = at_and_ucmd!(); + ucmd.args(&["-n", "4", "-x", "threebytes.txt"]) + .succeeds() + .no_stdout() + .no_stderr(); + assert_eq!(at.read("x00"), "a"); + assert_eq!(at.read("x01"), "b"); + assert_eq!(at.read("x02"), "c"); + assert_eq!(at.read("x03"), ""); +} + +/// Test for long hex suffix not having any value +#[test] +fn test_hex_suffix_no_value() { + let (at, mut ucmd) = at_and_ucmd!(); + ucmd.args(&["-n", "4", "--hex-suffixes", "threebytes.txt"]) + .succeeds() + .no_stdout() + .no_stderr(); + assert_eq!(at.read("x00"), "a"); + assert_eq!(at.read("x01"), "b"); + assert_eq!(at.read("x02"), "c"); + assert_eq!(at.read("x03"), ""); +} + +/// Test for short numeric suffix having value provided after space - should fail +#[test] +fn test_short_numeric_suffix_with_value_spaced() { + new_ucmd!() + .args(&["-n", "4", "-d", "9", "threebytes.txt"]) + .fails() + .stderr_contains("split: cannot open '9' for reading: No such file or directory"); +} + +/// Test for short numeric suffix having value provided after space - should fail +#[test] +fn test_short_hex_suffix_with_value_spaced() { + new_ucmd!() + .args(&["-n", "4", "-x", "9", "threebytes.txt"]) + .fails() + .stderr_contains("split: cannot open '9' for reading: No such file or directory"); +} + +/// Test for some combined short options +#[test] +fn test_short_combination() { + let (at, mut ucmd) = at_and_ucmd!(); + ucmd.args(&["-dxen", "4", "threebytes.txt"]) + .succeeds() + .no_stdout() + .no_stderr(); + assert_eq!(at.read("x00"), "a"); + assert_eq!(at.read("x01"), "b"); + assert_eq!(at.read("x02"), "c"); + assert_eq!(at.file_exists("x03"), false); +} + +/// Test for the last effective suffix, ignoring all others - numeric long last +/// Any combination of short and long (as well as duplicates) should be allowed +#[test] +fn test_effective_suffix_numeric_last() { + let (at, mut ucmd) = at_and_ucmd!(); + ucmd.args(&[ + "-n", + "4", + "--numeric-suffixes=7", + "--hex-suffixes=4", + "-d", + "-x", + "--numeric-suffixes=9", + "threebytes.txt", + ]) + .succeeds() + .no_stdout() + .no_stderr(); + assert_eq!(at.read("x09"), "a"); + assert_eq!(at.read("x10"), "b"); + assert_eq!(at.read("x11"), "c"); + assert_eq!(at.read("x12"), ""); +} + +/// Test for the last effective suffix, ignoring all others - hex long last +/// Any combination of short and long (as well as duplicates) should be allowed +#[test] +fn test_effective_suffix_hex_last() { + let (at, mut ucmd) = at_and_ucmd!(); + ucmd.args(&[ + "-n", + "4", + "--hex-suffixes=7", + "--numeric-suffixes=4", + "-x", + "-d", + "--hex-suffixes=9", + "threebytes.txt", + ]) + .succeeds() + .no_stdout() + .no_stderr(); + assert_eq!(at.read("x09"), "a"); + assert_eq!(at.read("x0a"), "b"); + assert_eq!(at.read("x0b"), "c"); + assert_eq!(at.read("x0c"), ""); +} + #[test] fn test_round_robin() { let (at, mut ucmd) = at_and_ucmd!(); From fc3bf941d3aa4eabde55c27f9c762c13a93712f7 Mon Sep 17 00:00:00 2001 From: zhitkoff Date: Thu, 24 Aug 2023 17:22:13 -0400 Subject: [PATCH 2/8] refactoring with overrides_with_all() in args definitions --- src/uu/split/src/split.rs | 82 +++++++++++++++++---------------------- 1 file changed, 36 insertions(+), 46 deletions(-) diff --git a/src/uu/split/src/split.rs b/src/uu/split/src/split.rs index 24fb5ca2235..fda2d91ebd1 100644 --- a/src/uu/split/src/split.rs +++ b/src/uu/split/src/split.rs @@ -13,8 +13,7 @@ mod platform; use crate::filenames::FilenameIterator; use crate::filenames::SuffixType; -use clap::ArgAction; -use clap::{crate_version, parser::ValueSource, Arg, ArgMatches, Command}; +use clap::{crate_version, parser::ValueSource, Arg, ArgAction, ArgMatches, Command}; use std::env; use std::fmt; use std::fs::{metadata, File}; @@ -131,7 +130,7 @@ pub fn uu_app() -> Command { .require_equals(true) .default_missing_value("0") .num_args(0..=1) - .overrides_with(OPT_NUMERIC_SUFFIXES) + .overrides_with_all([OPT_NUMERIC_SUFFIXES,OPT_NUMERIC_SUFFIXES_SHORT,OPT_HEX_SUFFIXES,OPT_HEX_SUFFIXES_SHORT]) .help("use numeric suffixes instead of alphabetic"), ) .arg( @@ -141,7 +140,7 @@ pub fn uu_app() -> Command { .value_parser(|_: &str| -> Result { Ok("0".to_string()) // force value to "0" }) - .overrides_with(OPT_NUMERIC_SUFFIXES_SHORT) + .overrides_with_all([OPT_NUMERIC_SUFFIXES,OPT_NUMERIC_SUFFIXES_SHORT,OPT_HEX_SUFFIXES,OPT_HEX_SUFFIXES_SHORT]) .help("use numeric suffixes instead of alphabetic"), ) .arg( @@ -150,7 +149,7 @@ pub fn uu_app() -> Command { .default_missing_value("0") .require_equals(true) .num_args(0..=1) - .overrides_with(OPT_HEX_SUFFIXES) + .overrides_with_all([OPT_NUMERIC_SUFFIXES,OPT_NUMERIC_SUFFIXES_SHORT,OPT_HEX_SUFFIXES,OPT_HEX_SUFFIXES_SHORT]) .help("use hex suffixes instead of alphabetic"), ) .arg( @@ -160,7 +159,7 @@ pub fn uu_app() -> Command { .value_parser(|_: &str| -> Result { Ok("0".to_string()) // force value to "0" }) - .overrides_with(OPT_HEX_SUFFIXES_SHORT) + .overrides_with_all([OPT_NUMERIC_SUFFIXES,OPT_NUMERIC_SUFFIXES_SHORT,OPT_HEX_SUFFIXES,OPT_HEX_SUFFIXES_SHORT]) .help("use hex suffixes instead of alphabetic"), ) .arg( @@ -435,55 +434,46 @@ impl Strategy { /// Parse the suffix type from the command-line arguments. fn suffix_type_from(matches: &ArgMatches) -> Result<(SuffixType, usize), SettingsError> { - // Collect provided suffixes (if any) - // Any combination is allowed - let mut suffixes = vec![]; - for suffix_id in [ - OPT_NUMERIC_SUFFIXES, - OPT_NUMERIC_SUFFIXES_SHORT, - OPT_HEX_SUFFIXES, - OPT_HEX_SUFFIXES_SHORT, - ] - .into_iter() - { - if matches.value_source(suffix_id) == Some(ValueSource::CommandLine) { - suffixes.push((matches.index_of(suffix_id), suffix_id)); - } - } - // Compare suffixes by the order specified in command line - // last one (highest index) is effective, all others are ignored - let mut effective_suffix_index = 0; - let mut effective_suffix_id = ""; - for suffix in suffixes.into_iter() { - let (suffix_index, suffix_id) = suffix; - if let Some(i) = suffix_index { - if i >= effective_suffix_index { - effective_suffix_index = i; - effective_suffix_id = suffix_id; - }; - } - } - - if effective_suffix_id == OPT_NUMERIC_SUFFIXES - || effective_suffix_id == OPT_NUMERIC_SUFFIXES_SHORT - { - let suffix_start = matches.get_one::(effective_suffix_id); + fn parse_num_suffix( + matches: &ArgMatches, + suffix_id: &str, + ) -> Result<(SuffixType, usize), SettingsError> { + let suffix_start = matches.get_one::(suffix_id); let suffix_start = suffix_start.ok_or(SettingsError::SuffixNotParsable(String::new()))?; let suffix_start = suffix_start .parse::() .map_err(|_| SettingsError::SuffixNotParsable(suffix_start.to_string()))?; Ok((SuffixType::Decimal, suffix_start)) - } else if effective_suffix_id == OPT_HEX_SUFFIXES - || effective_suffix_id == OPT_HEX_SUFFIXES_SHORT - { - let suffix_start = matches.get_one::(effective_suffix_id); + } + fn parse_hex_suffix( + matches: &ArgMatches, + suffix_id: &str, + ) -> Result<(SuffixType, usize), SettingsError> { + let suffix_start = matches.get_one::(suffix_id); let suffix_start = suffix_start.ok_or(SettingsError::SuffixNotParsable(String::new()))?; let suffix_start = usize::from_str_radix(suffix_start, 16) .map_err(|_| SettingsError::SuffixNotParsable(suffix_start.to_string()))?; Ok((SuffixType::Hexadecimal, suffix_start)) - } else { - // no numeric/hex suffix - Ok((SuffixType::Alphabetic, 0)) + } + // Check if the user is specifying one or more than one suffix + // Any combination of suffixes is allowed + // Since all suffixes are setup with 'overrides_with_all()' against themselves and each other, + // last one wins, all others are ignored and will not match Some(ValueSource::CommandLine) + // + // Note: right now, this exact behavior cannot be handled by + // `ArgGroup` since `ArgGroup` considers a default value `Arg` + // as "defined". + match ( + matches.value_source(OPT_NUMERIC_SUFFIXES) == Some(ValueSource::CommandLine), + matches.value_source(OPT_NUMERIC_SUFFIXES_SHORT) == Some(ValueSource::CommandLine), + matches.value_source(OPT_HEX_SUFFIXES) == Some(ValueSource::CommandLine), + matches.value_source(OPT_HEX_SUFFIXES_SHORT) == Some(ValueSource::CommandLine), + ) { + (true, false, false, false) => parse_num_suffix(matches, OPT_NUMERIC_SUFFIXES), + (false, true, false, false) => parse_num_suffix(matches, OPT_NUMERIC_SUFFIXES_SHORT), + (false, false, true, false) => parse_hex_suffix(matches, OPT_HEX_SUFFIXES), + (false, false, false, true) => parse_hex_suffix(matches, OPT_HEX_SUFFIXES_SHORT), + _ => Ok((SuffixType::Alphabetic, 0)), // no numeric/hex suffix, using default alphabetic } } From 16c8ad8f50640da8df9ba6ec15657170ee8502e5 Mon Sep 17 00:00:00 2001 From: zhitkoff Date: Thu, 24 Aug 2023 17:29:35 -0400 Subject: [PATCH 3/8] fixed comments --- tests/by-util/test_split.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/by-util/test_split.rs b/tests/by-util/test_split.rs index 7708fd5acfe..1e363a1984f 100644 --- a/tests/by-util/test_split.rs +++ b/tests/by-util/test_split.rs @@ -1,7 +1,7 @@ -// * This file is part of the uutils coreutils package. -// * -// * For the full copyright and license information, please view the LICENSE -// * file that was distributed with this source code. +// This file is part of the uutils coreutils package. +// +// For the full copyright and license information, please view the LICENSE +// file that was distributed with this source code. // spell-checker:ignore xzaaa sixhundredfiftyonebytes ninetyonebytes threebytes asciilowercase fghij klmno pqrst uvwxyz fivelines twohundredfortyonebytes onehundredlines nbbbb dxen use crate::common::util::{AtPath, TestScenario}; From 81062e08af364b47d7d6b302dad6477256d09c6c Mon Sep 17 00:00:00 2001 From: zhitkoff Date: Thu, 24 Aug 2023 17:38:15 -0400 Subject: [PATCH 4/8] updated help on suffixes to match GNU --- src/uu/split/src/split.rs | 30 +++++++++++++++--------------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/src/uu/split/src/split.rs b/src/uu/split/src/split.rs index fda2d91ebd1..8230e462e06 100644 --- a/src/uu/split/src/split.rs +++ b/src/uu/split/src/split.rs @@ -124,15 +124,6 @@ pub fn uu_app() -> Command { .help("do not generate empty output files with '-n'") .action(ArgAction::SetTrue), ) - .arg( - Arg::new(OPT_NUMERIC_SUFFIXES) - .long(OPT_NUMERIC_SUFFIXES) - .require_equals(true) - .default_missing_value("0") - .num_args(0..=1) - .overrides_with_all([OPT_NUMERIC_SUFFIXES,OPT_NUMERIC_SUFFIXES_SHORT,OPT_HEX_SUFFIXES,OPT_HEX_SUFFIXES_SHORT]) - .help("use numeric suffixes instead of alphabetic"), - ) .arg( Arg::new(OPT_NUMERIC_SUFFIXES_SHORT) .short('d') @@ -141,16 +132,16 @@ pub fn uu_app() -> Command { Ok("0".to_string()) // force value to "0" }) .overrides_with_all([OPT_NUMERIC_SUFFIXES,OPT_NUMERIC_SUFFIXES_SHORT,OPT_HEX_SUFFIXES,OPT_HEX_SUFFIXES_SHORT]) - .help("use numeric suffixes instead of alphabetic"), + .help("use numeric suffixes starting at 0, not alphabetic"), ) .arg( - Arg::new(OPT_HEX_SUFFIXES) - .long(OPT_HEX_SUFFIXES) - .default_missing_value("0") + Arg::new(OPT_NUMERIC_SUFFIXES) + .long(OPT_NUMERIC_SUFFIXES) .require_equals(true) + .default_missing_value("0") .num_args(0..=1) .overrides_with_all([OPT_NUMERIC_SUFFIXES,OPT_NUMERIC_SUFFIXES_SHORT,OPT_HEX_SUFFIXES,OPT_HEX_SUFFIXES_SHORT]) - .help("use hex suffixes instead of alphabetic"), + .help("same as -d, but allow setting the start value"), ) .arg( Arg::new(OPT_HEX_SUFFIXES_SHORT) @@ -160,7 +151,16 @@ pub fn uu_app() -> Command { Ok("0".to_string()) // force value to "0" }) .overrides_with_all([OPT_NUMERIC_SUFFIXES,OPT_NUMERIC_SUFFIXES_SHORT,OPT_HEX_SUFFIXES,OPT_HEX_SUFFIXES_SHORT]) - .help("use hex suffixes instead of alphabetic"), + .help("use hex suffixes starting at 0, not alphabetic"), + ) + .arg( + Arg::new(OPT_HEX_SUFFIXES) + .long(OPT_HEX_SUFFIXES) + .default_missing_value("0") + .require_equals(true) + .num_args(0..=1) + .overrides_with_all([OPT_NUMERIC_SUFFIXES,OPT_NUMERIC_SUFFIXES_SHORT,OPT_HEX_SUFFIXES,OPT_HEX_SUFFIXES_SHORT]) + .help("same as -x, but allow setting the start value"), ) .arg( Arg::new(OPT_SUFFIX_LENGTH) From d658f26d987795d2e5b48c4dc7f9b0b09d9d5421 Mon Sep 17 00:00:00 2001 From: zhitkoff Date: Thu, 24 Aug 2023 17:43:28 -0400 Subject: [PATCH 5/8] comments --- tests/by-util/test_split.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/by-util/test_split.rs b/tests/by-util/test_split.rs index 1e363a1984f..7708fd5acfe 100644 --- a/tests/by-util/test_split.rs +++ b/tests/by-util/test_split.rs @@ -1,7 +1,7 @@ -// This file is part of the uutils coreutils package. -// -// For the full copyright and license information, please view the LICENSE -// file that was distributed with this source code. +// * This file is part of the uutils coreutils package. +// * +// * For the full copyright and license information, please view the LICENSE +// * file that was distributed with this source code. // spell-checker:ignore xzaaa sixhundredfiftyonebytes ninetyonebytes threebytes asciilowercase fghij klmno pqrst uvwxyz fivelines twohundredfortyonebytes onehundredlines nbbbb dxen use crate::common::util::{AtPath, TestScenario}; From fb3b0bea6e9c2d2df9c2472908bf248938ac8a9f Mon Sep 17 00:00:00 2001 From: zhitkoff Date: Thu, 24 Aug 2023 17:57:07 -0400 Subject: [PATCH 6/8] refactor to remove value_parser() --- src/uu/split/src/split.rs | 50 +++++++++++++++------------------------ 1 file changed, 19 insertions(+), 31 deletions(-) diff --git a/src/uu/split/src/split.rs b/src/uu/split/src/split.rs index 8230e462e06..aa48dc73c49 100644 --- a/src/uu/split/src/split.rs +++ b/src/uu/split/src/split.rs @@ -128,9 +128,6 @@ pub fn uu_app() -> Command { Arg::new(OPT_NUMERIC_SUFFIXES_SHORT) .short('d') .action(clap::ArgAction::SetTrue) - .value_parser(|_: &str| -> Result { - Ok("0".to_string()) // force value to "0" - }) .overrides_with_all([OPT_NUMERIC_SUFFIXES,OPT_NUMERIC_SUFFIXES_SHORT,OPT_HEX_SUFFIXES,OPT_HEX_SUFFIXES_SHORT]) .help("use numeric suffixes starting at 0, not alphabetic"), ) @@ -147,9 +144,6 @@ pub fn uu_app() -> Command { Arg::new(OPT_HEX_SUFFIXES_SHORT) .short('x') .action(clap::ArgAction::SetTrue) - .value_parser(|_: &str| -> Result { - Ok("0".to_string()) // force value to "0" - }) .overrides_with_all([OPT_NUMERIC_SUFFIXES,OPT_NUMERIC_SUFFIXES_SHORT,OPT_HEX_SUFFIXES,OPT_HEX_SUFFIXES_SHORT]) .help("use hex suffixes starting at 0, not alphabetic"), ) @@ -434,27 +428,6 @@ impl Strategy { /// Parse the suffix type from the command-line arguments. fn suffix_type_from(matches: &ArgMatches) -> Result<(SuffixType, usize), SettingsError> { - fn parse_num_suffix( - matches: &ArgMatches, - suffix_id: &str, - ) -> Result<(SuffixType, usize), SettingsError> { - let suffix_start = matches.get_one::(suffix_id); - let suffix_start = suffix_start.ok_or(SettingsError::SuffixNotParsable(String::new()))?; - let suffix_start = suffix_start - .parse::() - .map_err(|_| SettingsError::SuffixNotParsable(suffix_start.to_string()))?; - Ok((SuffixType::Decimal, suffix_start)) - } - fn parse_hex_suffix( - matches: &ArgMatches, - suffix_id: &str, - ) -> Result<(SuffixType, usize), SettingsError> { - let suffix_start = matches.get_one::(suffix_id); - let suffix_start = suffix_start.ok_or(SettingsError::SuffixNotParsable(String::new()))?; - let suffix_start = usize::from_str_radix(suffix_start, 16) - .map_err(|_| SettingsError::SuffixNotParsable(suffix_start.to_string()))?; - Ok((SuffixType::Hexadecimal, suffix_start)) - } // Check if the user is specifying one or more than one suffix // Any combination of suffixes is allowed // Since all suffixes are setup with 'overrides_with_all()' against themselves and each other, @@ -469,10 +442,25 @@ fn suffix_type_from(matches: &ArgMatches) -> Result<(SuffixType, usize), Setting matches.value_source(OPT_HEX_SUFFIXES) == Some(ValueSource::CommandLine), matches.value_source(OPT_HEX_SUFFIXES_SHORT) == Some(ValueSource::CommandLine), ) { - (true, false, false, false) => parse_num_suffix(matches, OPT_NUMERIC_SUFFIXES), - (false, true, false, false) => parse_num_suffix(matches, OPT_NUMERIC_SUFFIXES_SHORT), - (false, false, true, false) => parse_hex_suffix(matches, OPT_HEX_SUFFIXES), - (false, false, false, true) => parse_hex_suffix(matches, OPT_HEX_SUFFIXES_SHORT), + (true, false, false, false) => { + let suffix_start = matches.get_one::(OPT_NUMERIC_SUFFIXES); + let suffix_start = + suffix_start.ok_or(SettingsError::SuffixNotParsable(String::new()))?; + let suffix_start = suffix_start + .parse::() + .map_err(|_| SettingsError::SuffixNotParsable(suffix_start.to_string()))?; + Ok((SuffixType::Decimal, suffix_start)) + } + (false, true, false, false) => Ok((SuffixType::Decimal, 0)), // short numeric suffix '-d', default start 0 + (false, false, true, false) => { + let suffix_start = matches.get_one::(OPT_HEX_SUFFIXES); + let suffix_start = + suffix_start.ok_or(SettingsError::SuffixNotParsable(String::new()))?; + let suffix_start = usize::from_str_radix(suffix_start, 16) + .map_err(|_| SettingsError::SuffixNotParsable(suffix_start.to_string()))?; + Ok((SuffixType::Hexadecimal, suffix_start)) + } + (false, false, false, true) => Ok((SuffixType::Hexadecimal, 0)), // short hex suffix '-x', default start 0 _ => Ok((SuffixType::Alphabetic, 0)), // no numeric/hex suffix, using default alphabetic } } From 6005908609cee7a1721400206b9e5eacf6a8195a Mon Sep 17 00:00:00 2001 From: zhitkoff Date: Sat, 26 Aug 2023 12:26:44 -0400 Subject: [PATCH 7/8] split: refactor suffix processing + updated tests --- src/uu/split/src/split.rs | 58 ++++++++++++++++++------------ tests/by-util/test_split.rs | 72 ++++++++++++++++++++++++++----------- 2 files changed, 87 insertions(+), 43 deletions(-) diff --git a/src/uu/split/src/split.rs b/src/uu/split/src/split.rs index 5a9b56b5381..453b8efbabf 100644 --- a/src/uu/split/src/split.rs +++ b/src/uu/split/src/split.rs @@ -126,7 +126,12 @@ pub fn uu_app() -> Command { Arg::new(OPT_NUMERIC_SUFFIXES_SHORT) .short('d') .action(clap::ArgAction::SetTrue) - .overrides_with_all([OPT_NUMERIC_SUFFIXES,OPT_NUMERIC_SUFFIXES_SHORT,OPT_HEX_SUFFIXES,OPT_HEX_SUFFIXES_SHORT]) + .overrides_with_all([ + OPT_NUMERIC_SUFFIXES, + OPT_NUMERIC_SUFFIXES_SHORT, + OPT_HEX_SUFFIXES, + OPT_HEX_SUFFIXES_SHORT + ]) .help("use numeric suffixes starting at 0, not alphabetic"), ) .arg( @@ -135,14 +140,24 @@ pub fn uu_app() -> Command { .require_equals(true) .default_missing_value("0") .num_args(0..=1) - .overrides_with_all([OPT_NUMERIC_SUFFIXES,OPT_NUMERIC_SUFFIXES_SHORT,OPT_HEX_SUFFIXES,OPT_HEX_SUFFIXES_SHORT]) + .overrides_with_all([ + OPT_NUMERIC_SUFFIXES, + OPT_NUMERIC_SUFFIXES_SHORT, + OPT_HEX_SUFFIXES, + OPT_HEX_SUFFIXES_SHORT + ]) .help("same as -d, but allow setting the start value"), ) .arg( Arg::new(OPT_HEX_SUFFIXES_SHORT) .short('x') .action(clap::ArgAction::SetTrue) - .overrides_with_all([OPT_NUMERIC_SUFFIXES,OPT_NUMERIC_SUFFIXES_SHORT,OPT_HEX_SUFFIXES,OPT_HEX_SUFFIXES_SHORT]) + .overrides_with_all([ + OPT_NUMERIC_SUFFIXES, + OPT_NUMERIC_SUFFIXES_SHORT, + OPT_HEX_SUFFIXES, + OPT_HEX_SUFFIXES_SHORT + ]) .help("use hex suffixes starting at 0, not alphabetic"), ) .arg( @@ -151,7 +166,12 @@ pub fn uu_app() -> Command { .default_missing_value("0") .require_equals(true) .num_args(0..=1) - .overrides_with_all([OPT_NUMERIC_SUFFIXES,OPT_NUMERIC_SUFFIXES_SHORT,OPT_HEX_SUFFIXES,OPT_HEX_SUFFIXES_SHORT]) + .overrides_with_all([ + OPT_NUMERIC_SUFFIXES, + OPT_NUMERIC_SUFFIXES_SHORT, + OPT_HEX_SUFFIXES, + OPT_HEX_SUFFIXES_SHORT + ]) .help("same as -x, but allow setting the start value"), ) .arg( @@ -429,36 +449,28 @@ fn suffix_type_from(matches: &ArgMatches) -> Result<(SuffixType, usize), Setting // Check if the user is specifying one or more than one suffix // Any combination of suffixes is allowed // Since all suffixes are setup with 'overrides_with_all()' against themselves and each other, - // last one wins, all others are ignored and will not match Some(ValueSource::CommandLine) - // - // Note: right now, this exact behavior cannot be handled by - // `ArgGroup` since `ArgGroup` considers a default value `Arg` - // as "defined". + // last one wins, all others are ignored match ( - matches.value_source(OPT_NUMERIC_SUFFIXES) == Some(ValueSource::CommandLine), - matches.value_source(OPT_NUMERIC_SUFFIXES_SHORT) == Some(ValueSource::CommandLine), - matches.value_source(OPT_HEX_SUFFIXES) == Some(ValueSource::CommandLine), - matches.value_source(OPT_HEX_SUFFIXES_SHORT) == Some(ValueSource::CommandLine), + matches.get_one::(OPT_NUMERIC_SUFFIXES), + matches.get_one::(OPT_HEX_SUFFIXES), + matches.get_flag(OPT_NUMERIC_SUFFIXES_SHORT), + matches.get_flag(OPT_HEX_SUFFIXES_SHORT), ) { - (true, false, false, false) => { - let suffix_start = matches.get_one::(OPT_NUMERIC_SUFFIXES); - let suffix_start = - suffix_start.ok_or(SettingsError::SuffixNotParsable(String::new()))?; + (Some(v), _, _, _) => { + let suffix_start = v; let suffix_start = suffix_start .parse::() .map_err(|_| SettingsError::SuffixNotParsable(suffix_start.to_string()))?; Ok((SuffixType::Decimal, suffix_start)) } - (false, true, false, false) => Ok((SuffixType::Decimal, 0)), // short numeric suffix '-d', default start 0 - (false, false, true, false) => { - let suffix_start = matches.get_one::(OPT_HEX_SUFFIXES); - let suffix_start = - suffix_start.ok_or(SettingsError::SuffixNotParsable(String::new()))?; + (_, Some(v), _, _) => { + let suffix_start = v; let suffix_start = usize::from_str_radix(suffix_start, 16) .map_err(|_| SettingsError::SuffixNotParsable(suffix_start.to_string()))?; Ok((SuffixType::Hexadecimal, suffix_start)) } - (false, false, false, true) => Ok((SuffixType::Hexadecimal, 0)), // short hex suffix '-x', default start 0 + (_, _, true, _) => Ok((SuffixType::Decimal, 0)), // short numeric suffix '-d', default start 0 + (_, _, _, true) => Ok((SuffixType::Hexadecimal, 0)), // short hex suffix '-x', default start 0 _ => Ok((SuffixType::Alphabetic, 0)), // no numeric/hex suffix, using default alphabetic } } diff --git a/tests/by-util/test_split.rs b/tests/by-util/test_split.rs index 1e363a1984f..f3317d4c7a4 100644 --- a/tests/by-util/test_split.rs +++ b/tests/by-util/test_split.rs @@ -758,56 +758,88 @@ fn test_hex_suffix_no_equal() { #[test] fn test_short_numeric_suffix_no_value() { let (at, mut ucmd) = at_and_ucmd!(); - ucmd.args(&["-n", "4", "-d", "threebytes.txt"]) + ucmd.args(&["-l", "9", "-d", "onehundredlines.txt"]) .succeeds() .no_stdout() .no_stderr(); - assert_eq!(at.read("x00"), "a"); - assert_eq!(at.read("x01"), "b"); - assert_eq!(at.read("x02"), "c"); - assert_eq!(at.read("x03"), ""); + assert_eq!(at.read("x00"), "00\n01\n02\n03\n04\n05\n06\n07\n08\n"); + assert_eq!(at.read("x01"), "09\n10\n11\n12\n13\n14\n15\n16\n17\n"); + assert_eq!(at.read("x02"), "18\n19\n20\n21\n22\n23\n24\n25\n26\n"); + assert_eq!(at.read("x03"), "27\n28\n29\n30\n31\n32\n33\n34\n35\n"); + assert_eq!(at.read("x04"), "36\n37\n38\n39\n40\n41\n42\n43\n44\n"); + assert_eq!(at.read("x05"), "45\n46\n47\n48\n49\n50\n51\n52\n53\n"); + assert_eq!(at.read("x06"), "54\n55\n56\n57\n58\n59\n60\n61\n62\n"); + assert_eq!(at.read("x07"), "63\n64\n65\n66\n67\n68\n69\n70\n71\n"); + assert_eq!(at.read("x08"), "72\n73\n74\n75\n76\n77\n78\n79\n80\n"); + assert_eq!(at.read("x09"), "81\n82\n83\n84\n85\n86\n87\n88\n89\n"); + assert_eq!(at.read("x10"), "90\n91\n92\n93\n94\n95\n96\n97\n98\n"); + assert_eq!(at.read("x11"), "99\n"); } /// Test for long numeric suffix not having any value #[test] fn test_numeric_suffix_no_value() { let (at, mut ucmd) = at_and_ucmd!(); - ucmd.args(&["-n", "4", "--numeric-suffixes", "threebytes.txt"]) + ucmd.args(&["-l", "9", "--numeric-suffixes", "onehundredlines.txt"]) .succeeds() .no_stdout() .no_stderr(); - assert_eq!(at.read("x00"), "a"); - assert_eq!(at.read("x01"), "b"); - assert_eq!(at.read("x02"), "c"); - assert_eq!(at.read("x03"), ""); + assert_eq!(at.read("x00"), "00\n01\n02\n03\n04\n05\n06\n07\n08\n"); + assert_eq!(at.read("x01"), "09\n10\n11\n12\n13\n14\n15\n16\n17\n"); + assert_eq!(at.read("x02"), "18\n19\n20\n21\n22\n23\n24\n25\n26\n"); + assert_eq!(at.read("x03"), "27\n28\n29\n30\n31\n32\n33\n34\n35\n"); + assert_eq!(at.read("x04"), "36\n37\n38\n39\n40\n41\n42\n43\n44\n"); + assert_eq!(at.read("x05"), "45\n46\n47\n48\n49\n50\n51\n52\n53\n"); + assert_eq!(at.read("x06"), "54\n55\n56\n57\n58\n59\n60\n61\n62\n"); + assert_eq!(at.read("x07"), "63\n64\n65\n66\n67\n68\n69\n70\n71\n"); + assert_eq!(at.read("x08"), "72\n73\n74\n75\n76\n77\n78\n79\n80\n"); + assert_eq!(at.read("x09"), "81\n82\n83\n84\n85\n86\n87\n88\n89\n"); + assert_eq!(at.read("x10"), "90\n91\n92\n93\n94\n95\n96\n97\n98\n"); + assert_eq!(at.read("x11"), "99\n"); } /// Test for short hex suffix not having any value #[test] fn test_short_hex_suffix_no_value() { let (at, mut ucmd) = at_and_ucmd!(); - ucmd.args(&["-n", "4", "-x", "threebytes.txt"]) + ucmd.args(&["-l", "9", "-x", "onehundredlines.txt"]) .succeeds() .no_stdout() .no_stderr(); - assert_eq!(at.read("x00"), "a"); - assert_eq!(at.read("x01"), "b"); - assert_eq!(at.read("x02"), "c"); - assert_eq!(at.read("x03"), ""); + assert_eq!(at.read("x00"), "00\n01\n02\n03\n04\n05\n06\n07\n08\n"); + assert_eq!(at.read("x01"), "09\n10\n11\n12\n13\n14\n15\n16\n17\n"); + assert_eq!(at.read("x02"), "18\n19\n20\n21\n22\n23\n24\n25\n26\n"); + assert_eq!(at.read("x03"), "27\n28\n29\n30\n31\n32\n33\n34\n35\n"); + assert_eq!(at.read("x04"), "36\n37\n38\n39\n40\n41\n42\n43\n44\n"); + assert_eq!(at.read("x05"), "45\n46\n47\n48\n49\n50\n51\n52\n53\n"); + assert_eq!(at.read("x06"), "54\n55\n56\n57\n58\n59\n60\n61\n62\n"); + assert_eq!(at.read("x07"), "63\n64\n65\n66\n67\n68\n69\n70\n71\n"); + assert_eq!(at.read("x08"), "72\n73\n74\n75\n76\n77\n78\n79\n80\n"); + assert_eq!(at.read("x09"), "81\n82\n83\n84\n85\n86\n87\n88\n89\n"); + assert_eq!(at.read("x0a"), "90\n91\n92\n93\n94\n95\n96\n97\n98\n"); + assert_eq!(at.read("x0b"), "99\n"); } /// Test for long hex suffix not having any value #[test] fn test_hex_suffix_no_value() { let (at, mut ucmd) = at_and_ucmd!(); - ucmd.args(&["-n", "4", "--hex-suffixes", "threebytes.txt"]) + ucmd.args(&["-l", "9", "--hex-suffixes", "onehundredlines.txt"]) .succeeds() .no_stdout() .no_stderr(); - assert_eq!(at.read("x00"), "a"); - assert_eq!(at.read("x01"), "b"); - assert_eq!(at.read("x02"), "c"); - assert_eq!(at.read("x03"), ""); + assert_eq!(at.read("x00"), "00\n01\n02\n03\n04\n05\n06\n07\n08\n"); + assert_eq!(at.read("x01"), "09\n10\n11\n12\n13\n14\n15\n16\n17\n"); + assert_eq!(at.read("x02"), "18\n19\n20\n21\n22\n23\n24\n25\n26\n"); + assert_eq!(at.read("x03"), "27\n28\n29\n30\n31\n32\n33\n34\n35\n"); + assert_eq!(at.read("x04"), "36\n37\n38\n39\n40\n41\n42\n43\n44\n"); + assert_eq!(at.read("x05"), "45\n46\n47\n48\n49\n50\n51\n52\n53\n"); + assert_eq!(at.read("x06"), "54\n55\n56\n57\n58\n59\n60\n61\n62\n"); + assert_eq!(at.read("x07"), "63\n64\n65\n66\n67\n68\n69\n70\n71\n"); + assert_eq!(at.read("x08"), "72\n73\n74\n75\n76\n77\n78\n79\n80\n"); + assert_eq!(at.read("x09"), "81\n82\n83\n84\n85\n86\n87\n88\n89\n"); + assert_eq!(at.read("x0a"), "90\n91\n92\n93\n94\n95\n96\n97\n98\n"); + assert_eq!(at.read("x0b"), "99\n"); } /// Test for short numeric suffix having value provided after space - should fail From 54dc1d88ef49749757c27d7127f5b7497144f485 Mon Sep 17 00:00:00 2001 From: zhitkoff Date: Sat, 26 Aug 2023 12:48:09 -0400 Subject: [PATCH 8/8] split: minor formatting --- src/uu/split/src/split.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/uu/split/src/split.rs b/src/uu/split/src/split.rs index 453b8efbabf..517031791bd 100644 --- a/src/uu/split/src/split.rs +++ b/src/uu/split/src/split.rs @@ -469,8 +469,8 @@ fn suffix_type_from(matches: &ArgMatches) -> Result<(SuffixType, usize), Setting .map_err(|_| SettingsError::SuffixNotParsable(suffix_start.to_string()))?; Ok((SuffixType::Hexadecimal, suffix_start)) } - (_, _, true, _) => Ok((SuffixType::Decimal, 0)), // short numeric suffix '-d', default start 0 - (_, _, _, true) => Ok((SuffixType::Hexadecimal, 0)), // short hex suffix '-x', default start 0 + (_, _, true, _) => Ok((SuffixType::Decimal, 0)), // short numeric suffix '-d', default start 0 + (_, _, _, true) => Ok((SuffixType::Hexadecimal, 0)), // short hex suffix '-x', default start 0 _ => Ok((SuffixType::Alphabetic, 0)), // no numeric/hex suffix, using default alphabetic } }