zip-rs / zip-old

Zip implementation in Rust
MIT License
732 stars 204 forks source link

Add support for extra field #186

Closed inorick closed 1 year ago

inorick commented 4 years ago

Support for the extra field of ZIP file headers is currently marked as future work.

My idea for an implementation is to add *_with_extra_data versions of the start_file and start_file_from_path methods that accept a slice with extra data to fill the extra field. The data would then be part of the ZipFileData struct.

I started preliminary work on it here: https://github.com/inorick/zip-rs/pull/1/commits/4393671828d2ab6ff95150d17f5109b2c05cc237

Please let me know what you think of this approach.

Plecra commented 4 years ago

Thanks for starting on the design! This seems reasonable with the crate's current API, and I'd be happy to merge a simple implementation so long as it had a solid set of tests.

I'd only add that the user should be able to pass owned data to to *_with_extra_data as they do for the name. It's likely the buffers they create will be unique to each file.

Edit: To be clear, I've been working on the same issue from a very different angle, so I'll be leaving most of this version to you 😀

Plecra commented 4 years ago

I'd like to welcome anyone looking at this issue to make a PR implementing the extra_field API for 0.5. Though it would be very useful, I'm not planning on adding more features to this version of the API myself

n3vu0r commented 4 years ago

I'm interested in aligned files which seems to be usually achieved by appending zeros to the local header as part of the extra field. Instead of passing the extra data directly, I would suggest to wrap it in a closure which itself provides a preliminary data_start():

pub fn start_file_with_extra_data<S, V, F>(&mut self, name: S, mut options: FileOptions, extra_data: F) -> ZipResult<()>
    where
        S: Into<String>,
        V: Into<Vec<u8>>,
        F: FnOnce(u64) -> V;

This would allow to calculate the pad length which I think is not reliable otherwise:

pub fn pad_to_align(data_start: u64, align: u16) -> Vec<u8> {                                           
    let align = align as u64;                                                                       
    vec![0; ((align - data_start % align) % align) as usize]                                        
}

zip.start_file_with_extra_data("file.ext", fopts, |data_start| pad_to_align(data_start, 64)]);

And it doesn't bloat the original use case:

zip.start_file_with_extra_data("file.ext", fopts, |_| extra_data);

What do you think? If nobody is working on a PR, I could give it a try soon.

n3vu0r commented 4 years ago

Alternative design:

Let start_file() return the preliminary data_start() instead of () (not sure if this is a breaking change, otherwise add a separate data_start() method for ZipWriter as well). And then the extra data or normal file data is simply written via io::Write and optionally at some point the data which has already been written can be marked as extra data by calling end_extra_data() which would just update the extra data length in the local file header.

Update: I think this alternate doesn't work, io::Writer usually compresses the data which it shouldn't for the extra data. But it could be done by adding start_file_with_extra_data() -> ZipResult<u64> and compression disabled until end_extra_data() is called.

inorick commented 3 years ago

I created an updated PR (#208) with the suggested changes (and clippy fixes). Please let me know what you think.

I just now realized that in the meantime a second PR with a separate implementation was created but I did not have a look at it yet.

Plecra commented 1 year ago

Done! Thanks for PR #200