coreutils icon indicating copy to clipboard operation
coreutils copied to clipboard

date: Catch format string that is not supported by chrono

Open jaggededgedjustice opened this issue 2 years ago • 3 comments

Attempting to use %#z with chrono gets a panic: https://github.com/chronotope/chrono/blob/378efb157d674c01761f538d4450705c2b1766a4/src/format/mod.rs#L683 This catches the format string and returns an error instead.

jaggededgedjustice avatar Dec 26 '22 15:12 jaggededgedjustice

Interesting! I have two questions.

  1. Why isn't this caught by the strftimeitems change? Does that function still panic in this scenario and should we report that upstream?

  2. What is the expected behaviour for %#z? Can we do something better than exiting with an error?

tertsdiepraam avatar Dec 26 '22 15:12 tertsdiepraam

1. Why isn't this caught by the strftimeitems change? Does that function still panic in this scenario and should we report that upstream?

This isn't caught by the change I made in #4240 because that catches issues where a string isn't successfully parsed, here it is successfully parsed but one of the format items is explicitly forbidden. The root cause is that chrono added %#z as a permissive time zone for parsing so there was a single specifier that would handle the different ways a time zone offset could be specified (with or without minutes) , but specifically forbade it's use when printing (see https://github.com/chronotope/chrono/pull/242)

2. What is the expected behaviour for `%#z`? Can we do something better than exiting with an error?

The expected behaviour according to GNU date is for %#z to be equivalent to %z. For GNU date # is a flag that means 'flip case if possible', e.g.

# date +%Z
GMT

# date +%#Z
gmt

However, currently chrono doesn't support the # flag so it should be considered an invalid formatting item, and using # with any other character but z will indeed result in an invalid format error. So I think I should make the error message the same as the other invalid format string message.

jaggededgedjustice avatar Dec 26 '22 18:12 jaggededgedjustice

Fails with:


--- TRY 3 STDERR:        coreutils::tests test_date::test_unsupported_format ---
thread 'test_date::test_unsupported_format' panicked at 'assertion failed: result.stderr_str().starts_with(\"date: do not use \'%#z\'\")', tests/by-util/test_date.rs:341:5
stack backtrace:
   0: rust_begin_unwind
             at /rustc/a55dd71d5fb0ec5a6a3a9e8c27b2127ba491ce52/library/std/src/panicking.rs:584:5
   1: core::panicking::panic_fmt
             at /rustc/a55dd71d5fb0ec5a6a3a9e8c27b2127ba491ce52/library/core/src/panicking.rs:142:14
   2: core::panicking::panic
             at /rustc/a55dd71d5fb0ec5a6a3a9e8c27b2127ba491ce52/library/core/src/panicking.rs:48:5
   3: tests::test_date::test_unsupported_format
             at ./tests/by-util/test_date.rs:341:5
   4: tests::test_date::test_unsupported_format::{{closure}}
             at ./tests/by-util/test_date.rs:338:1
   5: core::ops::function::FnOnce::call_once
             at /rustc/a55dd71d5fb0ec5a6a3a9e8c27b2127ba491ce52/library/core/src/ops/function.rs:248:5
   6: core::ops::function::FnOnce::call_once
             at /rustc/a55dd71d5fb0ec5a6a3a9e8c27b2127ba491ce52/library/core/src/ops/function.rs:248:5
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.

error: test run failed

sylvestre avatar Apr 28 '23 11:04 sylvestre

GNU testsuite comparison:

GNU test failed: tests/tail-2/inotify-dir-recreate. tests/tail-2/inotify-dir-recreate is passing on 'main'. Maybe you have to rebase?

github-actions[bot] avatar Apr 30 '23 13:04 github-actions[bot]

Can this be merged now? I don't think those test failures are related to this change

jaggededgedjustice avatar May 27 '23 13:05 jaggededgedjustice

yeah, sorry, i added a comment.

sylvestre avatar May 27 '23 13:05 sylvestre