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

Support append to an existing archive #215

Merged
merged 4 commits into from
Apr 19, 2021
Merged

Conversation

Contextualist
Copy link
Contributor

The implementation provides an alternative constructor, new_append, for ZipWriter, that takes a read-write buffer of existing archive and make it possible to add files to it.

new_append is mostly a copy of the ZipArchive constructor. It parses the central directory for the file list, and then set the cursor to the beginning of central directory in order to overwrite it. Users are expected to use the ZipWriter in the same way as if it is constructed from an empty buffer. When finish is called, the new central directory added will contains information for all prior files along with the appended files.

As the central directory parsing is borrowed from ZipArchive's internal implementation, I need to make ZipArchive ::get_directory_counts and central_header_to_zip_file public, which looks awkward to me. Is there a more graceful way? Also, suggestions on a better name for the constructor new_append are welcome.

@copyandexecute
Copy link

Great work! I would really looking forward to it accepting this SPECIFIC pullrequest since I need it for my work

@copyandexecute
Copy link

bump

@F0Xde
Copy link

F0Xde commented Jan 7, 2021

I would suggest just naming the function append, I don't think the additional new prefix is really required for a good understanding of what it does, but the current name is also totally fine imo.

@copyandexecute
Copy link

bump

src/read.rs Outdated
@@ -209,7 +209,7 @@ fn make_reader<'a>(
impl<R: Read + io::Seek> ZipArchive<R> {
/// Get the directory start offset and number of files. This is done in a
/// separate function to ease the control flow design.
fn get_directory_counts(
pub fn get_directory_counts(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pub(crate) will do the trick!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's nice. BTW, I saw in another PR that you mention about the current situation of having various edge-case API sprinkled around, and I really appreciate your works in #156 in your limited time. I'll see if I can help.

src/write.rs Show resolved Hide resolved
Copy link
Member

@Plecra Plecra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the look of this! I'll need to test a couple things soon, but there shouldn't be any problem :)

src/write.rs Outdated
}

for _ in 0..number_of_files {
let file = central_header_to_zip_file(&mut readwriter, archive_offset)?;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use an iterator here. Roughly

let files = (..number_of_files)
    .map(|_| central_header_to_zip_file(&mut readwriter, archiveoffset))
    .collect::<Result<Vec<_>, _>>()?;

@Contextualist
Copy link
Contributor Author

Thanks for your suggestions @Plecra! I learn something useful.

@Contextualist Contextualist requested a review from Plecra February 26, 2021 05:17
src/write.rs Outdated Show resolved Hide resolved
@Plecra Plecra force-pushed the append branch 2 times, most recently from 3def6bc to cfa3b11 Compare April 19, 2021 11:02
@Plecra
Copy link
Member

Plecra commented Apr 19, 2021

Thanks for the new API @Contextualist ! This looks great ~

@Plecra Plecra merged commit ce27261 into zip-rs:master Apr 19, 2021
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