Merge pull request zip-rs#98 from zip-rs/finish_owned

refactor: Make `ZipWriter::finish()` consume the `ZipWriter`

benches/merge_archive.rs

@@ -59,7 +59,7 @@ fn merge_archive_stored(bench: &mut Bencher) {
     bench.iter(|| {
         let buf = Cursor::new(Vec::new());
         let zip = ZipWriter::new(buf);
-        let mut zip = perform_merge(src.clone(), zip).unwrap();
+        let zip = perform_merge(src.clone(), zip).unwrap();
         let buf = zip.finish().unwrap().into_inner();
         assert_eq!(buf.len(), len);
     });
@@ -75,7 +75,7 @@ fn merge_archive_compressed(bench: &mut Bencher) {
     bench.iter(|| {
         let buf = Cursor::new(Vec::new());
         let zip = ZipWriter::new(buf);
-        let mut zip = perform_merge(src.clone(), zip).unwrap();
+        let zip = perform_merge(src.clone(), zip).unwrap();
         let buf = zip.finish().unwrap().into_inner();
         assert_eq!(buf.len(), len);
     });
@@ -90,7 +90,7 @@ fn merge_archive_raw_copy_file_stored(bench: &mut Bencher) {
     bench.iter(|| {
         let buf = Cursor::new(Vec::new());
         let zip = ZipWriter::new(buf);
-        let mut zip = perform_raw_copy_file(src.clone(), zip).unwrap();
+        let zip = perform_raw_copy_file(src.clone(), zip).unwrap();
         let buf = zip.finish().unwrap().into_inner();
         assert_eq!(buf.len(), len);
     });
@@ -106,7 +106,7 @@ fn merge_archive_raw_copy_file_compressed(bench: &mut Bencher) {
     bench.iter(|| {
         let buf = Cursor::new(Vec::new());
         let zip = ZipWriter::new(buf);
-        let mut zip = perform_raw_copy_file(src.clone(), zip).unwrap();
+        let zip = perform_raw_copy_file(src.clone(), zip).unwrap();
         let buf = zip.finish().unwrap().into_inner();
         assert_eq!(buf.len(), len);
     });

fuzz/Cargo.toml

@@ -11,6 +11,7 @@ cargo-fuzz = true
 [dependencies]
 libfuzzer-sys = "0.4"
 arbitrary = { version = "1.3.0", features = ["derive"] }
+replace_with = "0.1.7"
 
 [dependencies.zip]
 path = ".."

fuzz/fuzz_targets/fuzz_write.rs

@@ -2,7 +2,7 @@
 
 use arbitrary::Arbitrary;
 use libfuzzer_sys::fuzz_target;
-use std::cell::RefCell;
+use replace_with::replace_with_or_abort;
 use std::io::{Cursor, Read, Seek, Write};
 use std::path::PathBuf;
 
@@ -37,17 +37,15 @@ pub struct FuzzTestCase {
 }
 
 fn do_operation<T>(
-    writer: &mut RefCell<zip::ZipWriter<T>>,
+    writer: &mut zip::ZipWriter<T>,
     operation: &FileOperation,
     abort: bool,
     flush_on_finish_file: bool,
 ) -> Result<(), Box<dyn std::error::Error>>
 where
     T: Read + Write + Seek,
 {
-    writer
-        .borrow_mut()
-        .set_flush_on_finish_file(flush_on_finish_file);
+    writer.set_flush_on_finish_file(flush_on_finish_file);
     let path = &operation.path;
     match &operation.basic {
         BasicFileOperation::WriteNormalFile {
@@ -60,44 +58,44 @@ where
             if uncompressed_size >= u32::MAX as usize {
                 options = options.large_file(true);
             }
-            writer.borrow_mut().start_file_from_path(path, options)?;
+            writer.start_file_from_path(path, options)?;
             for chunk in contents {
-                writer.borrow_mut().write_all(chunk.as_slice())?;
+                writer.write_all(chunk.as_slice())?;
             }
         }
         BasicFileOperation::WriteDirectory(options) => {
-            writer.borrow_mut().add_directory_from_path(path, options.to_owned())?;
+            writer.add_directory_from_path(path, options.to_owned())?;
         }
         BasicFileOperation::WriteSymlinkWithTarget { target, options } => {
-            writer
-                .borrow_mut()
-                .add_symlink_from_path(&path, target, options.to_owned())?;
+            writer.add_symlink_from_path(&path, target, options.to_owned())?;
         }
         BasicFileOperation::ShallowCopy(base) => {
             do_operation(writer, &base, false, flush_on_finish_file)?;
-            writer.borrow_mut().shallow_copy_file_from_path(&base.path, &path)?;
+            writer.shallow_copy_file_from_path(&base.path, &path)?;
         }
         BasicFileOperation::DeepCopy(base) => {
             do_operation(writer, &base, false, flush_on_finish_file)?;
-            writer.borrow_mut().deep_copy_file_from_path(&base.path, &path)?;
+            writer.deep_copy_file_from_path(&base.path, &path)?;
         }
     }
     if abort {
-        writer.borrow_mut().abort_file().unwrap();
+        writer.abort_file().unwrap();
     }
     if operation.reopen {
-        let old_comment = writer.borrow().get_raw_comment().to_owned();
-        let new_writer =
-            zip::ZipWriter::new_append(writer.borrow_mut().finish().unwrap()).unwrap();
-        assert_eq!(&old_comment, new_writer.get_raw_comment());
-        *writer = new_writer.into();
+        let old_comment = writer.get_raw_comment().to_owned();
+        replace_with_or_abort(writer, |old_writer: zip::ZipWriter<T>| {
+            let new_writer =
+                zip::ZipWriter::new_append(old_writer.finish().unwrap()).unwrap();
+            assert_eq!(&old_comment, new_writer.get_raw_comment());
+            new_writer
+        });
     }
     Ok(())
 }
 
 fuzz_target!(|test_case: FuzzTestCase| {
-    let mut writer = RefCell::new(zip::ZipWriter::new(Cursor::new(Vec::new())));
-    writer.borrow_mut().set_raw_comment(test_case.comment);
+    let mut writer = zip::ZipWriter::new(Cursor::new(Vec::new()));
+    writer.set_raw_comment(test_case.comment);
     for (operation, abort) in test_case.operations {
         let _ = do_operation(
             &mut writer,
@@ -106,5 +104,5 @@ fuzz_target!(|test_case: FuzzTestCase| {
             test_case.flush_on_finish_file,
         );
     }
-    let _ = zip::ZipArchive::new(writer.borrow_mut().finish().unwrap());
+    let _ = zip::ZipArchive::new(writer.finish().unwrap());
 });

src/write.rs

@@ -1188,7 +1188,7 @@ impl<W: Write + Seek> ZipWriter<W> {
     ///
     /// This will return the writer, but one should normally not append any data to the end of the file.
     /// Note that the zipfile will also be finished on drop.
-    pub fn finish(&mut self) -> ZipResult<W> {
+    pub fn finish(mut self) -> ZipResult<W> {
         let _central_start = self.finalize()?;
         let inner = mem::replace(&mut self.inner, Closed);
         Ok(inner.unwrap())
@@ -1440,14 +1440,13 @@ impl<W: Write + Seek> GenericZipWriter<W> {
                         feature = "deflate-zlib-ng",
                     ))]
                     {
-                        return Ok(Box::new(move |bare| {
+                        Ok(Box::new(move |bare| {
                             GenericZipWriter::Deflater(DeflateEncoder::new(
                                 bare,
                                 Compression::new(level),
                             ))
-                        }));
+                        }))
                     }
-                    unreachable!()
                 }
                 #[cfg(feature = "deflate64")]
                 CompressionMethod::Deflate64 => Err(ZipError::UnsupportedArchive(

tests/zip_crypto.rs

@@ -49,7 +49,6 @@ fn encrypting_file() {
         .unwrap();
     archive.write_all(b"test").unwrap();
     archive.finish().unwrap();
-    drop(archive);
     let mut archive = zip::ZipArchive::new(Cursor::new(&mut buf)).unwrap();
     let mut file = archive.by_index_decrypt(0, b"password").unwrap();
     let mut buf = Vec::new();