Skip to content

Extend git-date tests and fix bug discovered in the process. #649

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

Merged
merged 2 commits into from
Dec 11, 2022

Conversation

willstott101
Copy link
Contributor

@willstott101 willstott101 commented Dec 10, 2022

It looks like your intuition about the baseline git-date test was correct @Byron

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).

I had a look at the baseline tests and updated them to round-trip the dates, parse -> check -> format -> check, to ensure they would have caught #646.

In the process I found a bug with the previously untested parsing of "raw" dates (unix secs with tz offset), causing the incorrect sign for the tz offset.

P.S. It seems a little odd to me to have a signed integer and a signed enum value for the tz offset - what problem does the duplication solve?


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.

@willstott101 willstott101 changed the title Bugfix/signed raw time Extend git-date tests and fix bug discovered in the process. Dec 10, 2022
@willstott101 willstott101 force-pushed the bugfix/signed-raw-time branch from 356f9a9 to f4ea59d Compare December 10, 2022 13:47
@Byron Byron merged commit f4ea59d into GitoxideLabs:main Dec 11, 2022
@Byron
Copy link
Member

Byron commented Dec 11, 2022

Thanks a lot for the fix, great catch! Baseline tests are such an important tool and I love them, especially when they help catch subtle issues :).

P.S. It seems a little odd to me to have a signed integer and a signed enum value for the tz offset - what problem does the duplication solve?

It's to allow round-tripping of parsed dates. These can be +0 or -0 for the timezone, and without keeping track of the sign we wouldn't be able to differentiate this case. This property is maintained throughout the codebase, except for trees which 'auto-fix' themselves in the light of a few somewhat unusual trees that old git versions wrote a decade ago :D, so they don't round-trip. That was were I chose to draw the line :D.

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