Repository navigation
gh-51067: Add remove() and repack() to ZipFile - #134627
gpshead merged 88 commits into
Conversation
Sorry, something went wrong.
|
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 skip news label instead. |
Sorry, something went wrong.
This comment was marked as off-topic.
There was a problem hiding this comment.
It probably would be better to raise an attributeError instead of a valueError here since you are trying to access an attribute a closed zipfile doesn’t have
Sorry, something went wrong.
This behavior simply resembles open() and write(), which raises a ValueError in various cases. Furthermore there has been a change from raising RuntimeError since Python 3.6:
|
Sorry, something went wrong.
|
Nicely inform @ubershmekel, @barneygale, @merwok, and @wimglenn about this PR. This should be more desirable and flexible than the previous PR, although cares must be taken as there might be a potential risk on the algorithm about reclaiming spaces. The previous PR is kept open in case some folks are interested in it. Will close when either one is accepted. |
Sorry, something went wrong.
- Separate individual validation tests. - Check underlying repacker not called in validation. - Use `unlink` to prevent FileNotFoundError. - Fix mode 'x' test.
- Set `_writing` to prevent `open('w').write()` during repacking.
- Move the protection logic to `ZipFile.repack()`.
Clarify that passing removed= reclaims a member's space regardless of how its local file entry was written, while the scan used when removed is omitted may leave entries in place (e.g. unsigned data descriptors under the default strict_descriptor=True). Adjust the remove() note so it no longer implies a bare repack() always removes the data, and add a short remove-then-repack(removed) example.
Without this check a stale local header whose compress_size points past the next referenced entry would make _validate_local_file_entry_sequence report more strippable bytes than the gap holds. The move loop would then over-advance entry_offset, drive a later header_offset negative, and fail in fp.seek() with the archive partially rewritten.
Avoids a tight loop if a future caller (or a truncated archive) ever asks to copy past EOF.
Replace **opts passthrough with the documented keyword-only signature so help()/inspect show the real parameters and the internal debug= hook is not reachable from public API.
zipfile is a stdlib module, not core/builtins.
The loop bound guarantees each slice is exactly dd_size bytes.
Consistent with the other _ZipRepacker scanners.
The previous spelling overrode __new__ to return a dict, which works but reads as a class that isn't one. Rename to comparable_zinfo() per PEP 8.
Single source of truth for the 1 MiB default used by both ZipFile.repack() and _ZipRepacker.
Covers the new bounds check in _validate_local_file_entry and the short-read guard in _copy_bytes.
gpshead
left a comment
There was a problem hiding this comment.
I applied a variety of minor edge case fixes for robustness and some code style cleanups.
Sorry, something went wrong.
I think the primary concerns were addressed. There's always room for documentation reworking. I realize I added a lot of text myself, maybe that should be rearranged a bit, but the important info is there that we expect people to be saving ZipInfo's from remove() calls and passing those to repack() as the well lit no-footguns path.
…134627) The docs included in the commit do the best job of describing this. Much discussion on the PR and issue. thank you to to core team folks jaraco, emmatyping, gpshead, and all others who added their constructive comments along the way. --------- Co-authored-by: blurb-it[bot] <43283697+blurb-it[bot]@users.noreply.github.com> Co-authored-by: Gregory P. Smith <greg@krypto.org>
This is a revised version of PR #103033, implementing two new methods in zipfile.ZipFile: remove() and repack(), as suggested in this comment.
Features
ZipFile.remove(zinfo_or_arcname)
ZipFile.repack(removed=None)
Rationales
Heuristics Used in repack()
Since repack() does not immediately clean up removed entries at the time a remove() is called, the header information of removed file entries may be missing, and thus it can be technically difficult to determine whether certain stale bytes are really previously removed files and safe to remove.
While local file entries begin with the magic signature PK\x03\x04, this alone is not a reliable indicator. For instance, a self-extracting ZIP file may contain executable code before the actual archive, which could coincidentally include such a signature, especially if it embeds ZIP-based content.
To safely reclaim space, repack() assumes that in a normal ZIP file, local file entries are stored consecutively:
Check the doc in the source code of _ZipRepacker.repack() (which is internally called by ZipFile.repack()) for more details.
Supported Modes
There has been opinions that a repacking should support mode 'w' and 'x' (e. g. #51067 (comment)).
This is NOT introduced since such modes do not truncate the file at the end of writing, and won't really shrink the file size after a removal has been made. Although we do can change the behavior for the existing API, some further care has to be made because mode 'w' and 'x' may be used on an unseekable file and will be broken by such change. OTOH, mode 'a' is not expected to work with an unseekable file since an initial seek is made immediately when it is opened.
📚 Documentation preview 📚: https://cpython-previews--134627.org.readthedocs.build/