◐ Shell
reader mode source ↗
Skip to content

bpo-29097: Forego fold detection on windows for low timestamp values#2385

Merged
abalkin merged 7 commits into
python:masterfrom
ammaraskar:fold_detection_windows
Jul 25, 2018
Merged

bpo-29097: Forego fold detection on windows for low timestamp values#2385
abalkin merged 7 commits into
python:masterfrom
ammaraskar:fold_detection_windows

Conversation

@ammaraskar

@ammaraskar ammaraskar commented Jun 24, 2017 •
edited by bedevere-bot
Loading

Copy link
Copy Markdown
Member

See the bug tracker for more details on why I think this is a clean way to resolve this issue https://bugs.python.org/issue29097

https://bugs.python.org/issue29097

@mention-bot

mention-bot commented Jun 24, 2017

Copy link
Copy Markdown

@ammaraskar, thanks for your PR! By analyzing the history of the files in this pull request, we identified @tim-one, @abalkin and @pitrou to be potential reviewers.

@jleclanche jleclanche mentioned this pull request Dec 27, 2017
Closed
@jleclanche jleclanche mentioned this pull request Feb 23, 2018
Closed
berkerpeksag
berkerpeksag requested changes May 16, 2018
View reviewed changes

@berkerpeksag berkerpeksag left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hide comment

We also need a NEWS entry for this.

@bedevere-bot

bedevere-bot commented May 16, 2018

Copy link
Copy Markdown

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

ammaraskar added 2 commits May 16, 2018 11:42
@ammaraskar
Forego fold detection for timestamps in the range [0, 86399] on Windows. …
8f163d3 
On Windows, passing a negative value to local results in an OSError because localtime_s on Windows does not support negative timestamps. Unfortunately this means that fold detection for timestamps between 0 and max_fold_seconds will result in this OSError since we subtract max_fold_seconds from the timestamp to detect a fold. However, since we know there haven't been any folds in the interval [0, max_fold_seconds) in any timezone, we can hackily just forego fold detection for this time range on Windows.
@ammaraskar
Add test to ensure there is no OSError for a timestamp of 0
488bc8b
@ammaraskar ammaraskar force-pushed the fold_detection_windows branch from 11d1ddf to f60dbb1 Compare May 16, 2018 18:46
@ammaraskar ammaraskar force-pushed the fold_detection_windows branch from f60dbb1 to 008778f Compare May 16, 2018 18:47
@ammaraskar

ammaraskar commented May 16, 2018

Copy link
Copy Markdown
Member Author

#2530 revealed that the pure python version of fromtimestamp lacks a similar check, going to add it in.

@ammaraskar ammaraskar force-pushed the fold_detection_windows branch 2 times, most recently from bae05d4 to 7c8847d Compare May 16, 2018 20:32
@ammaraskar ammaraskar force-pushed the fold_detection_windows branch from 7c8847d to 636ec5d Compare May 16, 2018 21:11
@ammaraskar

ammaraskar commented May 16, 2018

Copy link
Copy Markdown
Member Author

I have made the requested changes; please review again

@bedevere-bot

bedevere-bot commented May 16, 2018

Copy link
Copy Markdown

Thanks for making the requested changes!

@berkerpeksag: please review the changes made to this pull request.

@terryjreedy terryjreedy requested a review from abalkin June 28, 2018 17:26
@ammaraskar ammaraskar force-pushed the fold_detection_windows branch from 9be3a35 to 49a6937 Compare July 6, 2018 01:02
@ammaraskar ammaraskar force-pushed the fold_detection_windows branch from 49a6937 to 9202447 Compare July 6, 2018 01:16
14 hidden items Load more…
@jleclanche

jleclanche commented Jul 16, 2018

Copy link
Copy Markdown

This could land in 3.7.1 or 3.7.2, yeah?

@abalkin abalkin self-assigned this Jul 25, 2018
@abalkin abalkin merged commit 96d1e69 into python:master Jul 25, 2018
@miss-islington

miss-islington commented Jul 25, 2018

Copy link
Copy Markdown
Contributor

Thanks @ammaraskar for the PR, and @abalkin for merging it 🌮🎉.. I'm working now to backport this PR to: 3.7.
🐍🍒⛏🤖 I'm not a witch! I'm not a witch!

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jul 25, 2018
@ammaraskar @miss-islington
bpo-29097: Forego fold detection on windows for low timestamp values (p… …
2bfbe99 
…ythonGH-2385)

On Windows, passing a negative value to local results in an OSError because localtime_s on Windows does not support negative timestamps. Unfortunately this means that fold detection for timestamps between 0 and max_fold_seconds will result in this OSError since we subtract max_fold_seconds from the timestamp to detect a fold. However, since we know there haven't been any folds in the interval [0, max_fold_seconds) in any timezone, we can hackily just forego fold detection for this time range on Windows.
(cherry picked from commit 96d1e69)

Co-authored-by: Ammar Askar <ammar_askar@hotmail.com>
@bedevere-bot

bedevere-bot commented Jul 25, 2018

Copy link
Copy Markdown

GH-8466 is a backport of this pull request to the 3.7 branch.

@miss-islington

miss-islington commented Jul 25, 2018

Copy link
Copy Markdown
Contributor

Thanks @ammaraskar for the PR, and @abalkin for merging it 🌮🎉.. I'm working now to backport this PR to: 3.6.
🐍🍒⛏🤖

@miss-islington

miss-islington commented Jul 25, 2018

Copy link
Copy Markdown
Contributor

Sorry, @ammaraskar and @abalkin, I could not cleanly backport this to 3.6 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 96d1e69a12ed8ab80203277e1abdaf573457a964 3.6

abalkin pushed a commit that referenced this pull request Jul 25, 2018
@miss-islington @ammaraskar @abalkin
bpo-29097: Forego fold detection on windows for low timestamp values (G… …
9736493 
…H-2385) (GH-8466)

On Windows, passing a negative value to local results in an OSError because localtime_s on Windows does not support negative timestamps. Unfortunately this means that fold detection for timestamps between 0 and max_fold_seconds will result in this OSError since we subtract max_fold_seconds from the timestamp to detect a fold. However, since we know there haven't been any folds in the interval [0, max_fold_seconds) in any timezone, we can hackily just forego fold detection for this time range on Windows.
(cherry picked from commit 96d1e69)

Co-authored-by: Ammar Askar <ammar_askar@hotmail.com>
ammaraskar added a commit to ammaraskar/cpython that referenced this pull request Jul 27, 2018
@ammaraskar
[3.6] bpo-29097: Forego fold detection on windows for low timestamp v… …
bea48f9 
…alues (pythonGH-2385)

On Windows, passing a negative value to local results in an OSError because localtime_s on Windows does not support negative timestamps. Unfortunately this means that fold detection for timestamps between 0 and max_fold_seconds will result in this OSError since we subtract max_fold_seconds from the timestamp to detect a fold. However, since we know there haven't been any folds in the interval [0, max_fold_seconds) in any timezone, we can hackily just forego fold detection for this time range on Windows..
(cherry picked from commit 96d1e69)

Co-authored-by: Ammar Askar <ammar_askar@hotmail.com>
@bedevere-bot

bedevere-bot commented Jul 27, 2018

Copy link
Copy Markdown

GH-8498 is a backport of this pull request to the 3.6 branch.

abalkin pushed a commit that referenced this pull request Jul 27, 2018
@ammaraskar @abalkin
[3.6] bpo-29097: Forego fold detection on windows for low timestamp v… …
6ea8a3a 
…alues (GH-2385) (GH-8498)

On Windows, passing a negative value to local results in an OSError because localtime_s on Windows does not support negative timestamps. Unfortunately this means that fold detection for timestamps between 0 and max_fold_seconds will result in this OSError since we subtract max_fold_seconds from the timestamp to detect a fold. However, since we know there haven't been any folds in the interval [0, max_fold_seconds) in any timezone, we can hackily just forego fold detection for this time range on Windows..
(cherry picked from commit 96d1e69)

Co-authored-by: Ammar Askar <ammar_askar@hotmail.com>
@dirkf dirkf mentioned this pull request Apr 5, 2023
Closed
11 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@abalkin abalkin abalkin left review comments

@pganssle pganssle pganssle left review comments

@berkerpeksag berkerpeksag

Assignees

@abalkin abalkin

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.