du: support arbitrary time formats #7687
Conversation
|
GNU testsuite comparison: |
There was a problem hiding this comment.
This is great, but since we are using chrono it might trigger a panic. This should probably still be merged, but it's at least something we should be aware of. Here is an example:
du --time --time-style=+%Y-%Q
There's no %Q specifier, so chrono panics, where GNU just prints a plain %Q. Isn't that lovely...
|
@tertsdiepraam I added a regex-based solution to escape invalid sequences, but it's unfortunately slow. It's linear time if the format string is valid, but potentially quadratic time if the format string is designed against this approach. I opened chronotope/chrono#1692 to potentially add GNU's functionality into chrono directly. Without that, I don't see a better solution than either panicking or using a regex. If you want me to revert Note: there is a third option in which we use the regex for validation, but error if it matches the input string. |
a82a795 to
df13534
Compare
|
GNU testsuite comparison: |
|
GNU testsuite comparison: |
|
Update: My PR to |
|
That's definitely a pretty clever solution, but that regex feels very complicated to me. We might ultimately just need to build our own formatting, like in #7255. Maybe a "safer" (but not more correct) temporary solution is to use As a sidenote, please do not open issues on behalf of uutils on crates without discussing with us. I'm worried that maintainers of crates like chrono and clap might get annoyed with our project if that happens too often. The maintainers of uutils often have more knowledge about the history of interaction with other maintainres. Great job on getting it merged though! We should definitely switch to that when we can. |
|
I agree that the regex is quite complicated. It wasn't a completely serious suggestion, but I got distracted by the possibility and then was pleased by the results. Is #7255 in a state to be worked on or are there blockers there that I don't know about? Is the provided list of differences from chrono strftime exhaustive? As for this PR, should I wait for changes from #7255 or switch to chrono's Sorry about the PR; I wasn't aware of the situation. Thanks for the feedback! |
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for arbitrary time formats to du by modifying the premature time-style check and enhancing its parsing logic.
- Updated the parse_time_style function in src/uu/du/src/du.rs to accept and process arbitrary time styles using a complex regex-based approach.
- Introduced new tests in tests/by-util/test_du.rs to verify valid time-style inputs and proper escaping.
- Added the regex dependency in src/uu/du/Cargo.toml to support the new parsing logic.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| tests/by-util/test_du.rs | Added tests for valid time-format inputs and escaping handling |
| src/uu/du/src/du.rs | Refactored parse_time_style to support arbitrary time formats with custom escaping logic |
| src/uu/du/Cargo.toml | Added regex dependency required for the new time format parsing |
| let mut start = 0; | ||
| while start < s.len() { | ||
| // FIXME: this should use let chains once they're stabilized | ||
| // See https://github.com/rust-lang/rust/issues/53667 |
There was a problem hiding this comment.
[nitpick] Consider adding a brief comment to explain the intent behind the complex regex and the replacement logic, which would help future maintainers understand the escaping mechanism.
| // See https://github.com/rust-lang/rust/issues/53667 | |
| // See https://github.com/rust-lang/rust/issues/53667 | |
| // The regex TIME_STYLE_REGEX is used to identify patterns in the time style string | |
| // that require escaping. Specifically, it looks for `%` characters that need to be | |
| // replaced with `%%` to ensure compatibility with the formatting function. |
Copilot uses AI. Check for mistakes.
closes #7665
Most of the logic was already here, but there was a premature check that raised an error.