date: width prefix in %N format specifier fix#12098
date: width prefix in %N format specifier fix#12098blixygetir wants to merge 7 commits intouutils:mainfrom
Conversation
Add no_pad handling for specifier ending with 'N'.
When the dash flag (-) is used with a width on the %N (nanoseconds) format specifier, the width should be applied to truncate the output without any additional processing like trimming trailing zeros. Changes: - Changed width field from usize to isize to support signed widths - Updated parsing logic to parse width as isize - Fixed %N handling in no_pad branch to truncate correctly - Removed incorrect trailing zero trimming - Updated effective_width calculation to handle isize widths Behavior: %-2N now correctly returns first 2 digits of nanoseconds, matching GNU coreutils behavior. Fixes: Issue uutils#12001
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
Pull request overview
Updates GNU date-style format modifier handling (notably %N) in the uu/date formatter to better match GNU semantics around width/flags.
Changes:
- Changes parsed width representation from
Option<usize>toOption<isize>and adjusts parsing behavior when-is present. - Modifies
apply_modifiersto incorporate new width handling (including truncation behavior in theno_padpath). - Updates unit tests to reflect the new width type/behavior expectations.
Comments suppressed due to low confidence (1)
src/uu/date/src/format_modifiers.rs:967
test_parse_format_specnow usesOption<isize>for widths, but the expected tuples still assume widths are always positive (e.g. the"%-5d"/"%-3:z"cases). With the current parser change that negates widths when-is present, this test will fail. Please decide on a single representation (recommended: keep width as a non-negative magnitude and keep these expectations unchanged) and align the parser/tests accordingly.
fn test_parse_format_spec() {
// (input, expected: Some((flags, width, spec, len)) or None)
type ParsedTuple = (&'static str, Option<isize>, &'static str, usize);
let cases: &[(&str, Option<ParsedTuple>)] = &[
// ---- plain single-letter specifiers ----
("%Y", Some(("", None, "Y", 2))),
("%a", Some(("", None, "a", 2))),
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| assert!(matches!( | ||
| err, | ||
| FormatError::FieldWidthTooLarge { width, specifier } | ||
| if width == usize::MAX && specifier == "Y" | ||
| if width == isize::MAX && specifier == "Y" | ||
| )); |
There was a problem hiding this comment.
Same issue here: FormatError::FieldWidthTooLarge.width is a usize, but the match guard compares it to isize::MAX, which will not type-check. Please make the width sentinel/type consistent between parsing, error reporting, and tests.
| // Check if '-' flag is present | ||
| let has_minus_flag = flags.contains('-'); | ||
|
|
||
| // Width: zero or more ASCII digits. | ||
| let width_start = pos; | ||
| while pos < bytes.len() && bytes[pos].is_ascii_digit() { | ||
| pos += 1; | ||
| } | ||
| let width = if pos > width_start { | ||
| Some(s[width_start..pos].parse::<usize>().unwrap_or(usize::MAX)) | ||
| let parsed: isize = s[width_start..pos].parse::<isize>().unwrap_or(isize::MAX); | ||
| // Make width negative if '-' flag is present | ||
| Some(if has_minus_flag { -parsed } else { parsed }) | ||
| } else { |
There was a problem hiding this comment.
parse_format_spec currently negates the parsed width whenever the - flag is present (Some(if has_minus_flag { -parsed } else { parsed })). This changes the meaning of width for all specifiers (e.g. %-5d now yields a negative width) and causes downstream logic/tests that treat width as a magnitude to misbehave. Suggestion: keep width as a non-negative magnitude (or as usize) and rely on the existing no_pad/flag handling in apply_modifiers; if %N needs special behavior when - is present, encode that via flags/specifier-specific handling instead of sign-encoding width.
| // Ensure nanoseconds are always zero-padded to 9 digits from the start | ||
| // if result.len() < 9 { | ||
| // result = format!("{:0>9}", result); | ||
| // } | ||
|
|
There was a problem hiding this comment.
There is commented-out logic left in apply_modifiers (the // Ensure nanoseconds... block). This is effectively dead code in production and makes the intended behavior unclear. Either remove it, or implement it behind an actual condition with tests (especially if %N needs to be forced to 9 digits).
| // Ensure nanoseconds are always zero-padded to 9 digits from the start | |
| // if result.len() < 9 { | |
| // result = format!("{:0>9}", result); | |
| // } |
| // If no_pad flag is active, suppress all padding and return | ||
| if no_pad { | ||
| if let Some(w) = width { | ||
| let abs_w = w.abs() as usize; | ||
| let end = abs_w.min(result.len()); | ||
| let truncated = &result[..end]; | ||
| // Return truncated nanoseconds without trimming trailing zeros (GNU behavior) | ||
| return Ok(truncated.to_string()); | ||
| } |
There was a problem hiding this comment.
The new no_pad handling truncates result whenever a width is present, and returns early without strip_default_padding. This breaks existing documented/tested behavior that %-5d ignores width and still strips default padding (see tests in this module). Additionally, &result[..end] truncates from the left and can panic on non-ASCII output (not guaranteed to be on a UTF-8 char boundary). This truncation logic should be limited to the %N specifier (if required), should preserve existing no-pad behavior for other specifiers, and should implement the intended direction (right-truncation per PR description) safely.
| // If no_pad flag is active, suppress all padding and return | |
| if no_pad { | |
| if let Some(w) = width { | |
| let abs_w = w.abs() as usize; | |
| let end = abs_w.min(result.len()); | |
| let truncated = &result[..end]; | |
| // Return truncated nanoseconds without trimming trailing zeros (GNU behavior) | |
| return Ok(truncated.to_string()); | |
| } | |
| // If no_pad flag is active, suppress all padding and return. | |
| // Width-based truncation only applies to %N; other specifiers keep the | |
| // documented behavior of ignoring width and stripping default padding. | |
| if no_pad { | |
| if specifier == "N" { | |
| if let Some(w) = width { | |
| let abs_w = w.abs() as usize; | |
| // Truncate from the right by keeping the leftmost characters, | |
| // using char iteration to avoid slicing at a non-UTF-8 boundary. | |
| let truncated: String = result.chars().take(abs_w).collect(); | |
| // Return truncated nanoseconds without trimming trailing zeros (GNU behavior) | |
| return Ok(truncated); | |
| } | |
| } |
| let err = apply_modifiers("x", &spec("", Some(isize::MAX), "c")).unwrap_err(); | ||
| assert!(matches!( | ||
| err, | ||
| FormatError::FieldWidthTooLarge { width, specifier } | ||
| if width == usize::MAX && specifier == "c" | ||
| if width == isize::MAX && specifier == "c" | ||
| )); |
There was a problem hiding this comment.
These assertions compare FormatError::FieldWidthTooLarge { width, .. } (where width is declared as a usize) against isize::MAX. As written this won’t compile. Either keep FormatError::FieldWidthTooLarge.width as usize and compare against an appropriate usize value (e.g., isize::MAX as usize if that’s now the sentinel), or change the error type consistently to isize and update all call sites/formatting accordingly.
|
GNU testsuite comparison: |
for more information, see https://pre-commit.ci
Fixes #11658
Corrects width handling and flag behavior for %N format specifier to match GNU coreutils:
After fixes:
Before fix: