-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
du: support arbitrary time formats #7687
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
base: main
Are you sure you want to change the base?
Conversation
GNU testsuite comparison:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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! |
closes #7665
Most of the logic was already here, but there was a premature check that raised an error.