-
-
Notifications
You must be signed in to change notification settings - Fork 30.7k
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
gh-127847: Fix position in the special-cased zipfile seek #127856
Conversation
dimaryaz
commented
Dec 12, 2024
•
edited by bedevere-app
bot
Loading
edited by bedevere-app
bot
- Issue: Data corruption when reading from two files in parallel in an uncompressed .zip #127847
Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool. If this change has little impact on Python users, wait for a maintainer to apply the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a great first contribution, thanks :)
I have a few nitpicks, mostly related to some high-level triage tidbits.
Misc/NEWS.d/next/Library/2024-12-12-07-27-51.gh-issue-127847.ksfNKM.rst
Outdated
Show resolved
Hide resolved
b52d390
to
57af21a
Compare
Nice tests and the bug report was great. The fix I suggested was incomplete (I forgot
cpython/Lib/zipfile/__init__.py Lines 816 to 835 in 8bbd379
|
Ah, got it. I used an if/else, just to avoid doing two seeks unnecessarily - do you think that's better? (Though an extra seek worked fine, too, so I can change it to that.) |
I like the single seek you've done. Nice work! (The CI tool failed for macos-13 but that looks unrelated to these changes. The failing test was: |
Co-authored-by: Peter Bierma <[email protected]>
613945d
to
4cfd007
Compare
Just rebased, and everything passed now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks for the fix :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks for the clean contribution and others for the thorough review.
…onGH-127856) --------- (cherry picked from commit 7ed6c5c) Co-authored-by: Dima Ryazanov <[email protected]> Co-authored-by: blurb-it[bot] <43283697+blurb-it[bot]@users.noreply.github.com> Co-authored-by: Peter Bierma <[email protected]> Co-authored-by: Jason R. Coombs <[email protected]>
…onGH-127856) --------- (cherry picked from commit 7ed6c5c) Co-authored-by: Dima Ryazanov <[email protected]> Co-authored-by: blurb-it[bot] <43283697+blurb-it[bot]@users.noreply.github.com> Co-authored-by: Peter Bierma <[email protected]> Co-authored-by: Jason R. Coombs <[email protected]>
GH-128225 is a backport of this pull request to the 3.13 branch. |
GH-128226 is a backport of this pull request to the 3.12 branch. |
…127856) (#128226) gh-127847: Fix position in the special-cased zipfile seek (GH-127856) --------- (cherry picked from commit 7ed6c5c) Co-authored-by: Dima Ryazanov <[email protected]> Co-authored-by: blurb-it[bot] <43283697+blurb-it[bot]@users.noreply.github.com> Co-authored-by: Peter Bierma <[email protected]> Co-authored-by: Jason R. Coombs <[email protected]>
…127856) (#128225) gh-127847: Fix position in the special-cased zipfile seek (GH-127856) --------- (cherry picked from commit 7ed6c5c) Co-authored-by: Dima Ryazanov <[email protected]> Co-authored-by: blurb-it[bot] <43283697+blurb-it[bot]@users.noreply.github.com> Co-authored-by: Peter Bierma <[email protected]> Co-authored-by: Jason R. Coombs <[email protected]>
Congrats on your first contribution @dimaryaz |
Thanks everyone! |