From 0ed6a9f882e1909a0ae81bd2014add0657fa450d Mon Sep 17 00:00:00 2001 From: Joining7943 <111500881+Joining7943@users.noreply.github.com> Date: Mon, 6 Feb 2023 10:13:31 +0100 Subject: [PATCH] tail: Fix parsing of sleep interval. Use duration parser from fundu crate. Activate tests for parsing sleep interval --- Cargo.lock | 7 ++++++ Cargo.toml | 3 ++- src/uu/tail/Cargo.toml | 2 ++ src/uu/tail/src/args.rs | 27 +++++++++++++--------- tests/by-util/test_tail.rs | 46 +++++++++++++++++--------------------- 5 files changed, 48 insertions(+), 37 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 3df301eac..c9a6094a0 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -878,6 +878,12 @@ dependencies = [ "libc", ] +[[package]] +name = "fundu" +version = "0.3.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "925250bc259498d4008ee072bf16586083ab2c491aa4b06b3c4d0a6556cebd74" + [[package]] name = "futures" version = "0.3.25" @@ -3094,6 +3100,7 @@ version = "0.0.17" dependencies = [ "atty", "clap", + "fundu", "libc", "memchr", "nix", diff --git a/Cargo.toml b/Cargo.toml index 679f96d25..ebd8685ca 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,7 +1,7 @@ # coreutils (uutils) # * see the repository LICENSE, README, and CONTRIBUTING files for more information -# spell-checker:ignore (libs) libselinux gethostid procfs bigdecimal kqueue +# spell-checker:ignore (libs) libselinux gethostid procfs bigdecimal kqueue fundu [package] name = "coreutils" @@ -282,6 +282,7 @@ filetime = "0.2" fnv = "1.0.7" fs_extra = "1.1.0" fts-sys = "0.2" +fundu = "0.3.0" gcd = "2.2" glob = "0.3.0" half = "2.1" diff --git a/src/uu/tail/Cargo.toml b/src/uu/tail/Cargo.toml index d6dbf2fb7..12736f36a 100644 --- a/src/uu/tail/Cargo.toml +++ b/src/uu/tail/Cargo.toml @@ -1,3 +1,4 @@ +# spell-checker:ignore (libs) kqueue fundu [package] name = "uu_tail" version = "0.0.17" @@ -22,6 +23,7 @@ notify = { workspace=true } uucore = { workspace=true, features=["ringbuffer", "lines"] } same-file = { workspace=true } atty = { workspace=true } +fundu = { workspace=true } [target.'cfg(windows)'.dependencies] windows-sys = { workspace=true, features = ["Win32_System_Threading", "Win32_Foundation"] } diff --git a/src/uu/tail/src/args.rs b/src/uu/tail/src/args.rs index 5f7ea1028..bfc314185 100644 --- a/src/uu/tail/src/args.rs +++ b/src/uu/tail/src/args.rs @@ -3,13 +3,14 @@ // * For the full copyright and license information, please view the LICENSE // * file that was distributed with this source code. -// spell-checker:ignore (ToDO) kqueue Signum +// spell-checker:ignore (ToDO) kqueue Signum fundu use crate::paths::Input; use crate::{parse, platform, Quotable}; use atty::Stream; use clap::crate_version; use clap::{parser::ValueSource, Arg, ArgAction, ArgMatches, Command}; +use fundu::DurationParser; use same_file::Handle; use std::collections::VecDeque; use std::ffi::OsString; @@ -148,16 +149,20 @@ impl Settings { settings.retry = matches.get_flag(options::RETRY) || matches.get_flag(options::FOLLOW_RETRY); - if let Some(s) = matches.get_one::(options::SLEEP_INT) { - settings.sleep_sec = match s.parse::() { - Ok(s) => Duration::from_secs_f32(s), - Err(_) => { - return Err(UUsageError::new( - 1, - format!("invalid number of seconds: {}", s.quote()), - )) - } - } + if let Some(source) = matches.get_one::(options::SLEEP_INT) { + // Advantage of `fundu` over `Duration::(try_)from_secs_f64(source.parse().unwrap())`: + // * doesn't panic on errors like `Duration::from_secs_f64` would. + // * no precision loss, rounding errors or other floating point problems. + // * evaluates to `Duration::MAX` if the parsed number would have exceeded + // `DURATION::MAX` or `infinity` was given + // * not applied here but it supports customizable time units and provides better error + // messages + settings.sleep_sec = + DurationParser::without_time_units() + .parse(source) + .map_err(|_| { + UUsageError::new(1, format!("invalid number of seconds: '{source}'")) + })?; } settings.use_polling = matches.get_flag(options::USE_POLLING); diff --git a/tests/by-util/test_tail.rs b/tests/by-util/test_tail.rs index 5670e23c9..b2ec0e7bd 100644 --- a/tests/by-util/test_tail.rs +++ b/tests/by-util/test_tail.rs @@ -13,6 +13,7 @@ use crate::common::random::*; use crate::common::util::*; use pretty_assertions::assert_eq; use rand::distributions::Alphanumeric; +use rstest::rstest; use std::char::from_digit; use std::fs::File; use std::io::Write; @@ -4453,29 +4454,24 @@ fn test_follow_when_files_are_pointing_to_same_relative_file_and_file_stays_same .stdout_only(expected_stdout); } -#[test] -#[cfg(disable_until_fixed)] -fn test_args_sleep_interval_when_illegal_argument_then_usage_error() { - let scene = TestScenario::new(util_name!()); - for interval in [ - &format!("{}0", f64::MAX), - &format!("{}0.0", f64::MAX), - "1_000", - ".", - "' '", - "", - " ", - "0,0", - "one.zero", - ".zero", - "one.", - "0..0", - ] { - scene - .ucmd() - .args(&["--sleep-interval", interval]) - .run() - .usage_error(format!("invalid number of seconds: '{}'", interval)) - .code_is(1); - } +#[rstest] +#[case::exponent_exceed_float_max("1.0e2048")] +#[case::underscore_delimiter("1_000")] +#[case::only_point(".")] +#[case::space_in_primes("' '")] +#[case::space(" ")] +#[case::empty("")] +#[case::comma_separator("0,0")] +#[case::words_nominator_fract("one.zero")] +#[case::words_fract(".zero")] +#[case::words_nominator("one.")] +#[case::two_points("0..0")] +#[case::seconds_unit("1.0s")] +#[case::circumflex_exponent("1.0e^1000")] +fn test_args_sleep_interval_when_illegal_argument_then_usage_error(#[case] sleep_interval: &str) { + new_ucmd!() + .args(&["--sleep-interval", sleep_interval]) + .run() + .usage_error(format!("invalid number of seconds: '{sleep_interval}'")) + .code_is(1); }