feat(time_display): time should use timezone settings #1327

erwinvaneijk · 2025-01-10T12:45:37Z

When a file timestamp is displayed, it should use the timezone info from the time of the timestamp, not from when it is displayed. This occurs when a file timestamp is updated under daylight saving time, and displayed at a time when not under daylight saving time.

The bug is quite subtle, as it depends on the time of construction of the timestamp, not on the moment it is read.

This fixes issue #653 .

tessus · 2025-01-10T16:42:20Z

The pipeline is still not happy. I think another cargo fmt --all -- is in order. ;-)

erwinvaneijk · 2025-01-10T19:01:41Z

The pipeline is still not happy. I think another cargo fmt --all -- is in order. ;-)

Yes, go a bit annoyed because only FreeBSD tests failed (different rustc settings?), and after some to-and-from work, I've forgotten to run cargo fmt. Sigh. Hope I've got everything this time.

tessus · 2025-01-10T19:10:33Z

Yea, I hear ya. I've run into BSD related issues myself. And don't get me started on illumos pipelines. No project should ever use those. Argh!

erwinvaneijk · 2025-01-10T20:01:34Z

But, all tests pass! Yeah!

@cafkafk it's ready for review and/or merge.

src/output/render/times.rs

PThorpe92

LGTM 👍 , just left a minor comment

gierens

Nice catch! Generally LGTM! Thanks!

The only two things I think could be improved are:

Most of the commit messages are understandable in the context of this PR but maybe a bit too general for this to be still the case after rebase/merge when this is part of the wider history. So adding scopes and being a little more precise and adding context would help.
Some of the small fix commits might be better of either squashed together or into the commits that caused them.

tessus · 2025-01-14T10:31:24Z

I cannot speak for @erwinvaneijk but in my projects I don't care about the commit messages in a PR. What is important is that you don't do a merge commit, but rather a squash commit - and then use a convential commit message.

This means that the person who merges the PR can pick whatever commit message they want and there will be only one commit.

P.S.: Previously I asked people to rebase, squash, force push in PRs, but I no longer do that. It's easier to follow the development of the PR if this is not done. Especially when there are a lot of comments and change requests.

erwinvaneijk · 2025-01-14T12:10:47Z

Glad you like the PR @gierens, thank you for the review.

However, the changes you request to the commit messages cannot be done (or my git skills are seriously lacking in this department), therefore this PR will keep the status 'changes requested' which I cannot fulfil. Or did I miss requested changes to the code itself?

That goes for me too. The merge to the main should be well-formatted, and it's a choice whether or not to squash or just merge. Trying to create a squash merge in this PR that then can be merged is a pain, IMNSHO.

tessus · 2025-01-17T18:50:47Z

@erwinvaneijk I can squash and force-push if the devs insist on it. but I need access/permissions to your repo/branch.

@cafkafk can you please have a look? the 2 current reviews didn't make much sense.

The first review would have let eza fail silently and as a result shown wrong dates/times without notifying the user.
The second review was about commit messages in this PR. Don't get me wrong, but why does gh have a Squash and merge button?
I am also ok with helping the author to squash the commits.

erwinvaneijk · 2025-01-17T19:21:45Z

@erwinvaneijk I can squash and force-push if the devs insist on it. but I need access/permissions to your repo/branch.

I've added you to my repos, thanx for the effort you want to make.

tessus · 2025-01-17T20:23:49Z

Done. I've used feat because eza technically never claimed it would respect the timezone info.

gierens · 2025-01-17T23:10:34Z

@tessus sorry for the late reply, been a bit busy lately, otherwise I would've helped out with rebasing the commits.

the 2 current reviews didn't make much sense.

I try to elaborate a bit more, see below.

The second review was about commit messages in this PR. Don't get me wrong, but why does gh have a Squash and merge button? I am also ok with helping the author to squash the commits.

As the CONTRIBUTING.md states, we like small and conventional commits that make a minimal self-contained change to the code. So for example, when adding a trait in one file and implementing it for some struct in another two commits would be a good choice. There is of course always room for interpretation. But doing a Squash-Merge on a PR that alters several things at once goes directly against that concept. If you do that in your repos that is fine, we do it differently though, which is also why we only have the Rebase option configured on the Merge button.

gierens

By "squashing some of the smaller fix commits" I did not mean squashing everything. What I meant was, there originally were commits that fixed typos in previous commits of this PR, those could've been squashed together. And the commits for removal of unused lifetime names could've been squashed together as they are minor format fixes unrelated to the PR's actual intent.

If you want please rebase it again accordingly, otherwise I can do that tomorrow.

tessus · 2025-01-17T23:30:00Z

@gierens ah, ok. with CONTRIBUTING.md it makes more sense. in this case, this PR could have been squashed into 2 commits. The code change itself and the clippy fixes.

Anyway, I now have already squashed all commits into one. I don't have a backup.

P.S.: I didn't do a rebase. A rebase is not possible:

$ git rebase main
Current branch issue-653-2 is up to date.

gierens · 2025-01-17T23:33:08Z

@gierens ah, ok. with CONTRIBUTING.md it makes more sense.

Sorry for not explaining it very well before.

Anyway, I now have already squashed all commits into one. I don't have a backup.

No problem, I can quickly do that tomorrow but now I have to 😴

tessus · 2025-01-17T23:34:06Z

A rebase is not possible:

$ git rebase main
Current branch issue-653-2 is up to date.

tessus · 2025-01-17T23:35:13Z

A rebase would only work, if you have commits in main after now 2025-01-17 23:35:01 +0000

gierens · 2025-01-17T23:36:01Z

P.S.: I didn't do a rebase. A rebase is not possible:
$ git rebase main
Current branch issue-653-2 is up to date.

Sorry, I didn't mean rebase like that ... I meant picking it apart into multiple commits again, I usually use the term rebase as reference to interactive rebase git rebase -i ... which can be used for more intense history rewriting. So I use rebase as umbrella term for more intense history rewriting sometimes :D ... Not the clearest way to put it maybe, but it's getting late :D

But don't worry, I can quickly do that tomorrow.

tessus · 2025-01-17T23:39:26Z

There's no rebase -i possible anymore either. They are squashed. Or are you talking about using the reflog to do a hard reset?

tessus · 2025-01-17T23:57:57Z

Ok, I used the reflog to do a hard reset and created 2 commits.

tessus · 2025-01-18T19:15:16Z

@gierens is it ok now?

gierens

Sorry for the late reply, was again little overwhelmed by other things.

@gierens is it ok now?

Almost, I split off the addition of dependencies and tests into separate commits. Now it looks clean to me. Thx both of you for your effort and patience :)

LGTM!

tessus · 2025-01-19T23:31:15Z

To be honest those 2 additional commits make no sense.

A code change should not be taken apart, just to create commits. One commit should be about one code change (fix/feat/...).
e.g.: this PR was about fixing the TZ behavior. But since there was no bug because there was no TZ code, it's actually a feature.
The tests and adding crates to Cargo.toml are part of that change.

Additionally the code change wouldn't work without adding the crates as dependencies. Tests are usually also put in the same commit.

IMO this is not the idea behind conventional commits. Yes, you could have a second commit for the tests, but only if they were not in the first commit, because the author didn't write tests. But creating a separate commit just to use a build(deps) message is over the top.

I've been 30 years in IT and I have never seen a project that has such a weird commit strategy.

P.S.: I understand that one wants 2 commits, one for the change and another for the clippy stuff, but I would still merge it, if it were only 1 commit. So 2 commits are perfectlty valid. I would even be ok with 3 commits (tests in a separate commit), even though that's debatable. But these 4 commits are really, really weird.

erwinvaneijk · 2025-01-20T06:34:45Z

To be honest those 2 additional commits make no sense.

A code change should not be taken apart, just to create commits. One commit should be about one code change (fix/feat/...). e.g.: this PR was about fixing the TZ behavior. But since there was no bug because there was no TZ code, it's actually a feature. The tests and adding crates to Cargo.toml are part of that change.

Everyone: thank you for the help getting this bug fixed. It doesn’t display the right time (under conditions), which exa did do properly. I’ve not spent time figuring out where the bug was introduced, but there you go.

If someone with the appropriate rights can press the merge button, that would be excellent!

cafkafk

The code looks okay, but this seems to give some pretty harsh performance penalties...

hyperfine -w 500 -r 500  '{cmd} -l ../../dbc' --parameter-list cmd ./target/release/eza,eza,./target/release/eza,eza --time-unit microsecond -N
Benchmark 1: ./target/release/eza -l ../../dbc
  Time (mean ± σ):     9110.7 µs ± 2264.3 µs    [User: 5555.8 µs, System: 14314.8 µs]
  Range (min … max):   3641.7 µs … 14175.8 µs    500 runs
 
Benchmark 2: eza -l ../../dbc
  Time (mean ± σ):     6797.0 µs ± 2506.1 µs    [User: 3778.8 µs, System: 10336.2 µs]
  Range (min … max):   2668.1 µs … 25183.7 µs    500 runs
 
Benchmark 3: ./target/release/eza -l ../../dbc
  Time (mean ± σ):     9824.4 µs ± 1973.4 µs    [User: 6026.3 µs, System: 15514.3 µs]
  Range (min … max):   3340.9 µs … 16388.8 µs    500 runs
 
Benchmark 4: eza -l ../../dbc
  Time (mean ± σ):     6749.1 µs ± 2599.9 µs    [User: 3646.6 µs, System: 10192.5 µs]
  Range (min … max):   2584.6 µs … 24336.6 µs    500 runs
 
Summary
  eza -l ../../dbc ran
    1.01 ± 0.54 times faster than eza -l ../../dbc
    1.35 ± 0.62 times faster than ./target/release/eza -l ../../dbc
    1.46 ± 0.63 times faster than ./target/release/eza -l ../../dbc

I wonder if we can somehow mitigate this, it's pretty severe...

cafkafk · 2025-01-20T15:59:44Z

src/output/render/times.rs

-    fn render(self, style: Style, time_offset: FixedOffset, time_format: TimeFormat) -> TextCell {
-        let datestamp = if let Some(time) = self {
+    fn render(self, style: Style, time_format: TimeFormat) -> TextCell {
+        let datestamp = if let Ok(timezone_str) = iana_time_zone::get_timezone() {


It may be the case that running iana_time_zone::get_timezone() for each render all gives us a constant O(n) penalty here that could be a O(1) if we just put the timezone into memory if it will be displayed. Could be e.g. a static oncelock or something else, assuming eza will never run long enough for a timezone change happening while running being fatal (in fact, it may be preferable so that we always assume the tz from when eza was started vs. having two timezones in the very unlikely crossover case).

Yes, that is an issue. I'll see what we can do in this area. The assumption that the TZ will not change within the runtime of eza is justified.

The code is now checked in, could you repeat the benchmarks?

MartinFillon

lgtm

When a file timestamp is displayed, it should use the timezone info from the time of the timestamp, not from when it is displayed. This occurs when a file timestamp is updated under daylight saving time, and displayed at a time when not under daylight saving time. This bug is quite subtle, as it depends on the time of construction of the timestamp, not on the moment it is read.

- lifetimes that can be elided - not use return as last statement in function

Perform a timezone lookup only once at initialization, assuming that the timezone doesn't change during exection.

erwinvaneijk requested a review from cafkafk as a code owner January 10, 2025 12:45

erwinvaneijk force-pushed the issue-653-2 branch from 6bcd549 to efe1577 Compare January 10, 2025 12:58

erwinvaneijk requested review from MartinFillon, PThorpe92 and gierens as code owners January 10, 2025 13:28

PThorpe92 reviewed Jan 10, 2025

View reviewed changes

src/output/render/times.rs Outdated Show resolved Hide resolved

PThorpe92 previously approved these changes Jan 10, 2025

View reviewed changes

erwinvaneijk dismissed PThorpe92’s stale review via 4147572 January 10, 2025 22:13

gierens requested changes Jan 14, 2025

View reviewed changes

erwinvaneijk force-pushed the issue-653-2 branch from 70f333b to 27bfb6a Compare January 17, 2025 19:22

tessus force-pushed the issue-653-2 branch from 27bfb6a to e033293 Compare January 17, 2025 20:21

gierens requested changes Jan 17, 2025

View reviewed changes

tessus force-pushed the issue-653-2 branch from e033293 to dedcdb5 Compare January 17, 2025 23:57

tessus force-pushed the issue-653-2 branch from dedcdb5 to d668148 Compare January 17, 2025 23:59

tessus mentioned this pull request Jan 18, 2025

inconsistency in rust versions and pipelines #1335

Closed

gierens force-pushed the issue-653-2 branch from d668148 to ee7246e Compare January 19, 2025 23:06

gierens self-requested a review January 19, 2025 23:11

gierens previously approved these changes Jan 19, 2025

View reviewed changes

This was referenced Jan 20, 2025

fix: stdin behavior #1336

Open

build(deps): bump dirs from 5.0.1 to 6.0.0 #1330

Merged

fix: support additional yaml file extension #1328

Merged

feat(windows): report executable files #1318

Open

cafkafk requested changes Jan 20, 2025

View reviewed changes

cafkafk force-pushed the issue-653-2 branch from ee7246e to aa7b606 Compare January 20, 2025 15:55

cafkafk reviewed Jan 20, 2025

View reviewed changes

MartinFillon previously approved these changes Jan 21, 2025

View reviewed changes

erwinvaneijk dismissed stale reviews from MartinFillon and gierens via db4f85e January 21, 2025 16:32

erwinvaneijk added 5 commits January 21, 2025 17:38

build(deps): add chrono-tz and iana-time-zone dependencies

6a3b15a

test(output/time): add tests to check timezone output

2a7e7d5

fix(clippy): linter issues

adda6e5

- lifetimes that can be elided - not use return as last statement in function

perf: improve repeated timezone lookups

2e8ebbf

Perform a timezone lookup only once at initialization, assuming that the timezone doesn't change during exection.

erwinvaneijk force-pushed the issue-653-2 branch from db4f85e to 2e8ebbf Compare January 21, 2025 16:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(time_display): time should use timezone settings #1327

feat(time_display): time should use timezone settings #1327

erwinvaneijk commented Jan 10, 2025

tessus commented Jan 10, 2025

erwinvaneijk commented Jan 10, 2025

tessus commented Jan 10, 2025

erwinvaneijk commented Jan 10, 2025

PThorpe92 left a comment

gierens left a comment •

edited

Loading

tessus commented Jan 14, 2025 •

edited

Loading

erwinvaneijk commented Jan 14, 2025 •

edited

Loading

tessus commented Jan 17, 2025

erwinvaneijk commented Jan 17, 2025

tessus commented Jan 17, 2025

gierens commented Jan 17, 2025 •

edited

Loading

gierens left a comment

tessus commented Jan 17, 2025 •

edited

Loading

gierens commented Jan 17, 2025

tessus commented Jan 17, 2025

tessus commented Jan 17, 2025

gierens commented Jan 17, 2025 •

edited

Loading

tessus commented Jan 17, 2025

tessus commented Jan 17, 2025

tessus commented Jan 18, 2025

gierens left a comment •

edited

Loading

tessus commented Jan 19, 2025 •

edited

Loading

erwinvaneijk commented Jan 20, 2025

cafkafk left a comment

cafkafk Jan 20, 2025

erwinvaneijk Jan 20, 2025

erwinvaneijk Jan 21, 2025

MartinFillon left a comment

feat(time_display): time should use timezone settings #1327

Are you sure you want to change the base?

feat(time_display): time should use timezone settings #1327

Conversation

erwinvaneijk commented Jan 10, 2025

tessus commented Jan 10, 2025

erwinvaneijk commented Jan 10, 2025

tessus commented Jan 10, 2025

erwinvaneijk commented Jan 10, 2025

PThorpe92 left a comment

Choose a reason for hiding this comment

gierens left a comment • edited Loading

Choose a reason for hiding this comment

tessus commented Jan 14, 2025 • edited Loading

erwinvaneijk commented Jan 14, 2025 • edited Loading

tessus commented Jan 17, 2025

erwinvaneijk commented Jan 17, 2025

tessus commented Jan 17, 2025

gierens commented Jan 17, 2025 • edited Loading

gierens left a comment

Choose a reason for hiding this comment

tessus commented Jan 17, 2025 • edited Loading

gierens commented Jan 17, 2025

tessus commented Jan 17, 2025

tessus commented Jan 17, 2025

gierens commented Jan 17, 2025 • edited Loading

tessus commented Jan 17, 2025

tessus commented Jan 17, 2025

tessus commented Jan 18, 2025

gierens left a comment • edited Loading

Choose a reason for hiding this comment

tessus commented Jan 19, 2025 • edited Loading

erwinvaneijk commented Jan 20, 2025

cafkafk left a comment

Choose a reason for hiding this comment

cafkafk Jan 20, 2025

Choose a reason for hiding this comment

erwinvaneijk Jan 20, 2025

Choose a reason for hiding this comment

erwinvaneijk Jan 21, 2025

Choose a reason for hiding this comment

MartinFillon left a comment

Choose a reason for hiding this comment

gierens left a comment •

edited

Loading

tessus commented Jan 14, 2025 •

edited

Loading

erwinvaneijk commented Jan 14, 2025 •

edited

Loading

gierens commented Jan 17, 2025 •

edited

Loading

tessus commented Jan 17, 2025 •

edited

Loading

gierens commented Jan 17, 2025 •

edited

Loading

gierens left a comment •

edited

Loading

tessus commented Jan 19, 2025 •

edited

Loading