Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

date: Catch format string that is not supported by chrono #4244

Merged
merged 7 commits into from
May 27, 2023

Conversation

jaggededgedjustice
Copy link
Contributor

@jaggededgedjustice jaggededgedjustice commented Dec 26, 2022

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

Fixes: #4538

@tertsdiepraam
Copy link
Member

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?

@@ -244,6 +244,9 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> {
Ok(date) => {
// GNU `date` uses `%N` for nano seconds, however crate::chrono uses `%f`
let format_string = &format_string.replace("%N", "%f");
if format_string.contains("%#z") {
return Err(USimpleError::new(1, "do not use '%#z' is is undefined"));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another typo here. I think we can also be a bit more descriptive than the chrono error.

@jaggededgedjustice
Copy link
Contributor Author

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 chronotope/chrono#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.

@uutils uutils deleted a comment from github-actions bot Apr 28, 2023
@sylvestre
Copy link
Contributor

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

@github-actions
Copy link

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?

@jaggededgedjustice
Copy link
Contributor Author

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

@sylvestre
Copy link
Contributor

yeah, sorry, i added a comment.

@sylvestre sylvestre merged commit 81a854a into uutils:main May 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

date +%#z panic on Chrono "Do not try to write %#z it is undefined"
3 participants