Skip to content

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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

Qelxiros
Copy link
Contributor

@Qelxiros Qelxiros commented Apr 8, 2025

closes #7665

Most of the logic was already here, but there was a premature check that raised an error.

Copy link

github-actions bot commented Apr 8, 2025

GNU testsuite comparison:

Skip an intermittent issue tests/timeout/timeout (fails in this run but passes in the 'main' branch)
Skipping an intermittent issue tests/misc/stdbuf (passes in this run but fails in the 'main' branch)

Copy link
Member

@tertsdiepraam tertsdiepraam left a 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...

@Qelxiros
Copy link
Contributor Author

Qelxiros commented Apr 9, 2025

@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 df13534 in favor of performance, let me know.

Note: there is a third option in which we use the regex for validation, but error if it matches the input string.

@Qelxiros Qelxiros force-pushed the 7665-du-time-style branch from a82a795 to df13534 Compare April 9, 2025 02:20
Copy link

github-actions bot commented Apr 9, 2025

GNU testsuite comparison:

Skip an intermittent issue tests/timeout/timeout (fails in this run but passes in the 'main' branch)
Skipping an intermittent issue tests/misc/stdbuf (passes in this run but fails in the 'main' branch)

Copy link

GNU testsuite comparison:

Skip an intermittent issue tests/misc/stdbuf (fails in this run but passes in the 'main' branch)

@Qelxiros
Copy link
Contributor Author

Update: My PR to chrono has been merged, so we'll be able to use that implementation when they update to 0.4.41.

@tertsdiepraam
Copy link
Member

tertsdiepraam commented Apr 17, 2025

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 parse like chrono's maintainer suggested? Or implement our own iterator copied from that PR?

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.

@Qelxiros
Copy link
Contributor Author

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 StrftimeItems::parse and error gracefully?

Sorry about the PR; I wasn't aware of the situation. Thanks for the feedback!

@tertsdiepraam
Copy link
Member

This PR is fine too have next to #7255! #7255 is very big so might take a while to land. Looks like it's barely reviewed yet (and not at all by me so I don't really know what the state of it is).

Your changes can probably be merged soon, so let's just do that first.

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.

du: should support arbitrary format arg to --time-style
2 participants