From e77971581b931ce52db19f4e0c2e4e0f0173258b Mon Sep 17 00:00:00 2001 From: arbelonson-source <269032023+arbelonson-source@users.noreply.github.com> Date: Mon, 31 Aug 2026 04:54:29 +0300 Subject: [PATCH] head, tail: keep a `+` in an invalid count's error, as GNU does `head`/`tail -c`/`-n` share a parser that always dropped the sign from the value an error names, matching GNU only for `-`: $ tail -c -5Ki f # GNU and ours agree tail: invalid number of bytes: '5Ki' $ tail -c +5Ki f # ours, before tail: invalid number of bytes: '5Ki' $ tail -c +5Ki f # GNU tail: invalid number of bytes: '+5Ki' GNU keeps a `+` but drops a `-`. The two sign parsers (`parse_signed_num_max`, and its unused-but-public sibling `parse_signed_num`) now report the original string when the sign was `+`, and the sign-stripped one otherwise, matching what was already documented but not implemented for `parse_signed_num` ("returns an error with the raw string"). `tail`'s own `-n` branch had a second, independent bug: it named the argument straight from `arg.quote()` rather than from the error the parser already built, so it never picked up the sign handling above at all, unlike its `-c` sibling next to it, which already went through the error. Both now go through the error. Found by a differential sweep across head/tail size suffixes on the most recently merged multiplier-suffix commit -- this bug was pre-existing, not introduced by it. --- src/uu/tail/src/args.rs | 2 +- .../lib/features/parser/parse_signed_num.rs | 52 ++++++++++++++++--- tests/by-util/test_head.rs | 22 ++++++++ tests/by-util/test_tail.rs | 22 ++++++++ 4 files changed, 90 insertions(+), 8 deletions(-) diff --git a/src/uu/tail/src/args.rs b/src/uu/tail/src/args.rs index 95724cfe0af..3638b81d9bc 100644 --- a/src/uu/tail/src/args.rs +++ b/src/uu/tail/src/args.rs @@ -102,7 +102,7 @@ impl FilterMode { } Err(e) => { let message = - translate!("tail-error-invalid-number-of-lines", "arg" => arg.quote()); + translate!("tail-error-invalid-number-of-lines", "arg" => e.to_string()); return Err(raise(message, arg, 'n', "lines", &e)); } } diff --git a/src/uucore/src/lib/features/parser/parse_signed_num.rs b/src/uucore/src/lib/features/parser/parse_signed_num.rs index 262f34cbaad..e103a950191 100644 --- a/src/uucore/src/lib/features/parser/parse_signed_num.rs +++ b/src/uucore/src/lib/features/parser/parse_signed_num.rs @@ -107,6 +107,16 @@ pub fn parse_signed_num_max(src: &str) -> Result { return Err(ParseSizeError::ParseFailure(src.to_string())); } + // GNU keeps a `+` in the value it reports back, but drops a `-`: `-5Ki` + // is reported as `5Ki`, while `+5Ki` is reported as `+5Ki`. Only `+` + // needs the unstripped string; a `-` (or no sign at all) keeps using + // `size_string` as before. + let reported = if sign == Some(SignPrefix::Plus) { + src + } else { + size_string + }; + // Remove leading zeros so size is interpreted as decimal, not octal let trimmed = size_string.trim_start_matches('0'); let had_leading_zeros = trimmed.len() != size_string.len(); @@ -120,10 +130,10 @@ pub fn parse_signed_num_max(src: &str) -> Result { // Otherwise "0K" would parse as 1KiB (bare suffix means 1). // A genuinely bare suffix with no digits at all (e.g. "kiB") // still parses as 1 of that unit. - parse_count(trimmed).map_err(|e| as_typed(e, size_string))?; + parse_count(trimmed).map_err(|e| as_typed(e, reported))?; 0 } else { - parse_count(trimmed).map_err(|e| as_typed(e, size_string))? + parse_count(trimmed).map_err(|e| as_typed(e, reported))? }; Ok(SignedNum { value, sign }) @@ -144,10 +154,18 @@ pub fn parse_signed_num(src: &str) -> Result { return Err(ParseSizeError::ParseFailure(src.to_string())); } + // GNU keeps a `+` in the value it reports back, but drops a `-`; see + // `parse_signed_num_max` for why. + let reported = if sign == Some(SignPrefix::Plus) { + src + } else { + size_string + }; + // Use parse_size_u64 but on failure, create our own error with the raw string // (without quotes) so callers can format it as needed let value = parse_size_u64(size_string) - .map_err(|_| ParseSizeError::ParseFailure(size_string.to_string()))?; + .map_err(|_| ParseSizeError::ParseFailure(reported.to_string()))?; Ok(SignedNum { value, sign }) } @@ -171,10 +189,13 @@ pub fn number_offset(src: &str) -> usize { /// /// Zeros are only removed so the number is read as decimal rather than octal, /// which is an implementation detail the message should not leak: GNU reports -/// `tail: invalid number of bytes: '007z'`, not `'7z'`. The sign is left off, -/// also matching GNU, which reports `-c-0fb` as `'0fb'`. -fn as_typed(error: ParseSizeError, size_string: &str) -> ParseSizeError { - let quoted = format!("{}", size_string.quote()); +/// `tail: invalid number of bytes: '007z'`, not `'7z'`. `reported` is the +/// caller's choice of what else to put back: GNU reports `-c-0fb` as `'0fb'` +/// (the sign left off) but `-c+0fb` as `'+0fb'` (the sign kept), so the +/// caller passes `src` or the sign-stripped string depending on which sign, +/// if any, was found. +fn as_typed(error: ParseSizeError, reported: &str) -> ParseSizeError { + let quoted = format!("{}", reported.quote()); match error { // These two carry the quoted operand and nothing else, so it can be // swapped for the one that was actually typed. @@ -292,6 +313,23 @@ mod tests { assert!(result.has_minus()); } + /// GNU keeps a `+` in the value an error reports, but drops a `-`: the + /// same value, `5Ki`, is invalid either way, but the message reads back + /// what the user typed only when they typed a `+`. + #[test] + fn test_error_keeps_plus_but_not_minus() { + let plus = parse_signed_num_max("+5Ki").unwrap_err().to_string(); + assert!(plus.contains("+5Ki"), "{plus}"); + + let minus = parse_signed_num_max("-5Ki").unwrap_err().to_string(); + assert!(!minus.contains('-'), "{minus}"); + assert!(minus.contains("5Ki"), "{minus}"); + + let unsigned = parse_signed_num_max("5Ki").unwrap_err().to_string(); + assert_eq!(plus.replace('+', ""), unsigned); + assert_eq!(minus, unsigned); + } + #[test] fn test_zero() { let result = parse_signed_num_max("0").unwrap(); diff --git a/tests/by-util/test_head.rs b/tests/by-util/test_head.rs index af03f02cf7a..d5d60bff6e1 100644 --- a/tests/by-util/test_head.rs +++ b/tests/by-util/test_head.rs @@ -400,6 +400,28 @@ fn test_head_invalid_num() { .stderr_is("head: invalid number of bytes: '³'\n"); } +/// A `-` sign is dropped from the value an error reports, but a `+` is kept, +/// as GNU does. +#[test] +fn test_invalid_num_keeps_a_plus_sign_but_not_a_minus() { + new_ucmd!() + .args(&["-c", "+5Ki", "emptyfile.txt"]) + .fails() + .stderr_is("head: invalid number of bytes: '+5Ki'\n"); + new_ucmd!() + .args(&["-n", "+5Ki", "emptyfile.txt"]) + .fails() + .stderr_is("head: invalid number of lines: '+5Ki'\n"); + new_ucmd!() + .args(&["-c", "-5Ki", "emptyfile.txt"]) + .fails() + .stderr_is("head: invalid number of bytes: '5Ki'\n"); + new_ucmd!() + .args(&["-n", "-5Ki", "emptyfile.txt"]) + .fails() + .stderr_is("head: invalid number of lines: '5Ki'\n"); +} + #[test] fn test_head_num_with_undocumented_sign_bytes() { // tail: '-' is not documented (8.32 man pages) diff --git a/tests/by-util/test_tail.rs b/tests/by-util/test_tail.rs index 2b30a3cef47..b1e982bdde5 100644 --- a/tests/by-util/test_tail.rs +++ b/tests/by-util/test_tail.rs @@ -5328,3 +5328,25 @@ fn test_invalid_count_keeps_its_leading_zeros() { .fails_with_code(1) .stderr_is("tail: invalid number of bytes: '0fb'\n"); } + +/// A `-` sign is dropped from the value an error reports, but a `+` is kept, +/// as GNU does; `-n` and `-c` must agree, since they share the same parser. +#[test] +fn test_invalid_count_keeps_a_plus_sign_but_not_a_minus() { + new_ucmd!() + .args(&["-c", "+5Ki", "/dev/null"]) + .fails_with_code(1) + .stderr_is("tail: invalid number of bytes: '+5Ki'\n"); + new_ucmd!() + .args(&["-n", "+5Ki", "/dev/null"]) + .fails_with_code(1) + .stderr_is("tail: invalid number of lines: '+5Ki'\n"); + new_ucmd!() + .args(&["-c", "-5Ki", "/dev/null"]) + .fails_with_code(1) + .stderr_is("tail: invalid number of bytes: '5Ki'\n"); + new_ucmd!() + .args(&["-n", "-5Ki", "/dev/null"]) + .fails_with_code(1) + .stderr_is("tail: invalid number of lines: '5Ki'\n"); +}