Skip to content

fix: always consider timestamps as UTC when loading from commits #646

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

Conversation

willstott101
Copy link
Contributor

@willstott101 willstott101 commented Dec 9, 2022

Hello, whilst working on #471 (very slowly every now and then) I realised gitoxide is incorrectly using replace_offset when we should be using to_offset.

Replace the offset. The date and time components remain unchanged.

Which sounds great, cause what git stores is a timestamp in UTC and an offset explaining in which timezone the commit was made. So we want to apply the offset without changing the time.. however the time crate doesn't store time as unix seconds! So replacing the offset without adjusting the stored day and time is not what we want.

let t = time::OffsetDateTime::from_unix_timestamp(123456789).unwrap();
let to = t.replace_offset(time::UtcOffset::from_whole_seconds(3000).unwrap());
assert_eq(t.unix_timestamp(), to.unix_timestamp()); // THIS FAILS

And just to verify for myself I wrote a small script which uses git to convert from the hardcoded timestamp in the tests of this repo, to various formats. https://gist.github.com/willstott101/ecbd08d359e7bbf8dbfa0ac26f25b367

This script outputs the following (which this patch makes the tests for formatting in this repo match)

human= Nov 30 1973        human-local= Nov 29 1973        iso= 1973-11-30 00:03:09 +0230    raw= 123456789

Ok for Byron review the PR on video?

  • I give my permission to record review and upload on YouTube publicly

If I think the review will be helpful for the community, then I might record and publish a video.

Copy link
Member

@Byron Byron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks so much for this contribution! Such a small change with massive impact, and good we catch this one now!

Seeing the git script to have a baseline, maybe this can be the inspiration for improving the existing baseline tests as well. (And I don't even know what that would be specifically).

@Byron
Copy link
Member

Byron commented Dec 9, 2022

(Merging, the CI failure is already fixed in main and unrelated)

@Byron Byron merged commit be0bbf5 into GitoxideLabs:main Dec 9, 2022
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.

2 participants