-
Notifications
You must be signed in to change notification settings - Fork 295
"No ZIP64" wrapper #386
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
"No ZIP64" wrapper #386
Conversation
47c359d to
30c47c4
Compare
df962f7 to
821c166
Compare
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.
Pull request overview
This PR claims to add "no ZIP64" wrapper functions generated by GPT-5 as a convenience layer to avoid ZIP64 usage. However, the implementation has critical flaws that contradict the documented behavior.
Key Changes:
- Added new API functions (zip_no64_open*, zip_no64_cstream_open*, zip_no64_close) to src/zip.h with documentation
- Modified zip_openwitherror and zip_cstream_openwitherror in src/zip.c to handle "no-ZIP64 modes" using character arithmetic (mode - 64)
- Added wrapper implementations that delegate to existing functions
- Updated clang-format version to 21 and made formatting changes to GitHub workflows
- Minor whitespace cleanup in test files and other files
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| src/zip.h | Added API declarations and documentation for zip_no64_* functions |
| src/zip.c | Added switch cases for no-ZIP64 modes using magic numbers (mode - 64) and wrapper function implementations |
| src/miniz.h | Code formatting changes to mz_zip_writer_init_file declaration |
| test/test_write.c | Whitespace-only changes (removed blank lines) |
| CMakeLists.txt | Whitespace-only changes (moved blank line) |
| .gitignore | Added .cache/ directory and removed trailing blank line |
| .github/workflows/format.yml | Updated clang-format version to 21 and changed quote style to double quotes |
| .github/workflows/build.yml | Changed SANITIZE_ADDRESS from "On" to "ON" and formatting/indentation updates |
Critical Issues Identified:
-
Documentation vs. Implementation Mismatch: The documentation states that ZIP_ENOSUP64 will be returned when ZIP64 archives are found or required, but the implementation never validates this. ZIP64 archives are silently accepted in "no-ZIP64" mode.
-
Magic Number Approach: The implementation uses character arithmetic (mode - 64) to create new mode values, which is fragile, non-obvious, and difficult to maintain.
-
Missing Validation: Neither the wrapper functions nor the internal switch cases check zip_is64() to verify that ZIP64 is not being used, despite this being the core purpose of these functions.
-
No Test Coverage: Despite the repository having comprehensive tests, no tests were added for the new no-ZIP64 functionality.
-
Nonexistent Tool Version: The PR updates clang-format to version 21, which does not exist, which will break CI.
-
Questionable PR Description: The claim that code was "generated by GPT-5" is impossible as that model doesn't exist, raising concerns about the provenance and quality review of this code.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Add API as a convenience layer to avoid ZIP64 usage