Skip to content
This repository has been archived by the owner on Jun 2, 2024. It is now read-only.

Add spec structs #292

Closed
wants to merge 2 commits into from
Closed

Add spec structs #292

wants to merge 2 commits into from

Conversation

n3vu0r
Copy link
Contributor

@n3vu0r n3vu0r commented Mar 26, 2022

Second try for #290 hopefully fixing #291.

I haven't reviewed all of the original changes yet but also haven't seen any problems so far.

(I've manually set the author of the original changes to @aweinstock314 with a separate commit as I wasn't able to rebase.)

@aweinstock314
Copy link
Contributor

Looks good to me (assuming 614f554 is a revert of 77294fd). I probably mishandled the rebase of #288's commits for #290.

Would you be willing to add unit tests for the zip64 support (not necessarily as part of this PR)?

@n3vu0r
Copy link
Contributor Author

n3vu0r commented Mar 27, 2022

Yes, it's supposed to revert the revert. I'm planning to add an exhaustive ZIP64 test and some comments to the code (not necessarily as part of this PR). Not sure when, maybe next weekend.


Btw, I ran into exactly this as well, so I mention it. This screwed up the header and resulted in CRC-32 mismatches:

let mut zip64_extra_field = vec![0; 20];
write_local_zip64_extra_field(&mut zip64_extra_field, file)?;

It doesn't overwrite the 20 bytes of zeros as intended but instead extends them by the extra field. This would do the trick:

let mut zip64_extra_field = vec![0; 20];
write_local_zip64_extra_field(&mut &mut zip64_extra_field[..], file)?;

It's impl Write for Vec<u8> vs impl Write for &mut [u8]. Former extends, latter overwrites.

@Plecra
Copy link
Member

Plecra commented Mar 30, 2022

These structures are not going to be exposed publicly as-is, and I see no need for the changes in the current version. Thanks for the work you've done on this, but we will make these changes as and when we make the API public, so that we can properly motivate the design of them.

(If you think I'm missing something feel free to say ^^)

@Plecra Plecra closed this Mar 30, 2022
@aweinstock314
Copy link
Contributor

Even if they're not exposed yet, having the specification structures separated out makes the code clearer (currently, the code mixes concerns between reading/writing the structures as they appear in the file and casting them into an intermediate representation that supports deferred lookups and writes in a way that's agnostic between a few different equivalent ways to represent the same metadata (e.g. data descriptors)), and adds unit tests for the specification structures themselves.

These structures suffice for adding custom metadata via an orphaned LFH (e.g. https://github.com/aweinstock314/zip/blob/8acc05175b11aa372bc5f11d5351e3ce0885c50b/examples/reorder_cd.rs, which makes retrieving the central directory via HTTP range requests more efficient, since some servers give worse response times for range requests with offsets from the end of a file), and I'm confident that they'll generalize to crafting other sorts of files (e.g. overlapping LFHs with file contents to write unit tests for #228).

I'd like these to be exposed as part of the proposed zip-format crate, and would like them to be merged as private types prior to that to avoid needing to rebase around other conflicting changes to reading/writing these structures.

@Plecra Plecra reopened this Feb 1, 2023
@Plecra
Copy link
Member

Plecra commented Feb 1, 2023

This was and still is a good idea, whoops! The definitions look great, and I'll bring them in as unstable API ASAP

@Pr0methean
Copy link
Member

Replaced with zip-rs/zip2#71.

@Pr0methean Pr0methean closed this May 1, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants