fix(time-style): cryptic panic with invalid --time-syle format string #1282
Conversation
There was a problem hiding this comment.
The fix is to create a dummy StrftimeItems and iterate over it for errors to validate the format string beforehand.
I wonder what the performance characteristics of doing this is
Currently, snapshot tests are failling, and will have to be regenerated. just has a template in the repository for doing this
I have a few comment here, sorry for the volume of change requests but I do think none of them are nitpicky
src/options/view.rs
Outdated
| let non_recent = if non_recent.is_empty() { | ||
| panic!("{}", empty_non_recent_format_msg) | ||
| panic!("{}", invalid_non_recent_format_msg) |
There was a problem hiding this comment.
We'd prefer exiting with an appropriate exit code, like for instance as how is done here:
eprintln!("{e}");
trace!("exa.run: exit RUNTIME_ERROR");
exit(exits::RUNTIME_ERROR);There was a problem hiding this comment.
It seems exits isn't pub, so I'd have to change that. Would it be better to just return an error via OptionsError? OptionsError::Unsupported seems like a good candidate.
src/options/view.rs
Outdated
| let empty_non_recent_format_msg = "Custom timestamp format is empty, \ | ||
| please supply a chrono format string after the plus sign."; | ||
| let non_recent = lines.next().expect(empty_non_recent_format_msg); | ||
| let invalid_non_recent_format_msg = "Custom timestamp format is invalid, \ |
There was a problem hiding this comment.
This would be more appropriate as a const, I know it wasn't introduced in this PR but it would make a lot of sense to fix it while here
src/options/view.rs
Outdated
| let non_recent = lines.next().expect(empty_non_recent_format_msg); | ||
| let invalid_non_recent_format_msg = "Custom timestamp format is invalid, \ | ||
| please supply a valid chrono format string after the plus sign."; | ||
| let non_recent = lines.next().expect(invalid_non_recent_format_msg); |
There was a problem hiding this comment.
Here it would also be more appropriate to handle the error instead of just panicking through expect
src/options/view.rs
Outdated
| StrftimeItems::new(rec).for_each(|item| { | ||
| if matches!(item, Item::Error) { | ||
| panic!("{}", invalid_recent_format_msg) | ||
| } | ||
| }); |
There was a problem hiding this comment.
This should include a comment explaining the motivation behind the code here, as e.g. described in the PR
There was a problem hiding this comment.
Also, seeing as this is present above as well, it may be worth deduplicating this
src/options/view.rs
Outdated
| @@ -364,12 +371,18 @@ impl TimeFormat { | |||
| // - there is nothing after the first `\n` | |||
| // line 2 will be empty when: | |||
| // - there exist at least 2 `\n`, and no content between the 1st and 2nd `\n` | |||
| let empty_recent_format_msg = "Custom timestamp format for recent files is empty, \ | |||
| please supply a chrono format string at the second line."; | |||
| let invalid_recent_format_msg = | |||
There was a problem hiding this comment.
also candidate for becoming a const
Validate --time-style format string during option parsing so we can give a good error message rather than let rayon handle the panic and give no context. Fixes: eza-community#1240
Fixes: #1240
Validate --time-style format string during option parsing.
That format string is later given to chrono via
DateTime.format(fmt).to_string().ToStringis implemented forT: Displayin std but panics ifDisplay::fmtreturns an error, and the object returned byDateTime.format(StrftimeItems) lazily returns an error inDisplay::fmtwhen it finally sees something invalid in the format string. This is also done inside a rayon threadpool so the resulting panic has little context for users.The fix is to create a dummy
StrftimeItemsand iterate over it for errors to validate the format string beforehand.