Skip to content
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

Add initial support for deflate64 zip files #84

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

iffy
Copy link
Contributor

@iffy iffy commented Aug 6, 2024

This is a shot in the dark attempt at adding Deflate64 support for #11 . I've made this change by trial and error with the given sample Deflate64 zipfile. I haven't read any spec about Deflate64 and I don't know if the magic numbers I put in internal.nim are correct. But it unzips the file now.

If someone with more knowledge of how the zip file format works could chime in, that would be very welcome.

@guzba
Copy link
Owner

guzba commented Aug 6, 2024

Thanks for the PR however I do have concerns.

This PR changes internal.nim values for maxDistanceCodes and a couple other important arrays which concerns me. These changes are invalid for Deflate which is by far the primary and most important use of Zippy. Deflate64 is hyper niche by comparison.

I am not going to merge something that makes Zippy violate the Deflate spec so we'll need to find a way to signal "this is Deflate vs this is Deflate64" and have separate values for that.

@iffy
Copy link
Contributor Author

iffy commented Aug 6, 2024

Absolutely understandable. If I get some more time on this, I'll try to work out a change that doesn't modify existing Deflate stuff.

@guzba
Copy link
Owner

guzba commented Aug 6, 2024

One thought I had was adding dfDeflate64 as a new format parameter for compress/uncompress. I think (but will need to confirm) that a compressed blob out of Deflate is valid Deflate64 input so that'll be easy. The harder part will be doing Deflate64 uncompress correctly without it blowing up the complexity of an already very touchy code path.

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