fix: do not write empty packs and indices (#1404)

Failure can occour on Windows as it's likely such a pack or index is already opened
during negotiation. Then, when an empty fetch via `--depth 1` is repeated,
a temporary file would be renamed onto one an mmapped pack or index, causing
a failure.

Now packs or indices aren't written anymore if they are empty.

Author: Byron

gix-pack/src/bundle/write/mod.rs

@@ -300,31 +300,37 @@ impl crate::Bundle {
                 )?;
                 drop(pack_entries_iter);
 
-                let data_path = directory.join(format!("pack-{}.pack", outcome.data_hash.to_hex()));
-                let index_path = data_path.with_extension("idx");
-                let keep_path = data_path.with_extension("keep");
+                if outcome.num_objects == 0 {
+                    WriteOutcome {
+                        outcome,
+                        data_path: None,
+                        index_path: None,
+                        keep_path: None,
+                    }
+                } else {
+                    let data_path = directory.join(format!("pack-{}.pack", outcome.data_hash.to_hex()));
+                    let index_path = data_path.with_extension("idx");
+                    let keep_path = data_path.with_extension("keep");
 
-                std::fs::write(&keep_path, b"")?;
-                Arc::try_unwrap(data_file)
-                    .expect("only one handle left after pack was consumed")
-                    .into_inner()
-                    .into_inner()
-                    .map_err(|err| Error::from(err.into_error()))?
-                    .persist(&data_path)?;
-                index_file
-                    .persist(&index_path)
-                    .map_err(|err| {
-                        progress.info(format!(
-                            "pack file at {} is retained despite failing to move the index file into place. You can use plumbing to make it usable.",
-                            data_path.display()
-                        ));
-                        err
-                    })?;
-                WriteOutcome {
-                    outcome,
-                    data_path: Some(data_path),
-                    index_path: Some(index_path),
-                    keep_path: Some(keep_path),
+                    std::fs::write(&keep_path, b"")?;
+                    Arc::try_unwrap(data_file)
+                        .expect("only one handle left after pack was consumed")
+                        .into_inner()
+                        .into_inner()
+                        .map_err(|err| Error::from(err.into_error()))?
+                        .persist(&data_path)?;
+                    index_file
+                        .persist(&index_path)
+                        .map_err(|err| {
+                            gix_features::trace::warn!("pack file at \"{}\" is retained despite failing to move the index file into place. You can use plumbing to make it usable.",data_path.display());
+                            err
+                        })?;
+                    WriteOutcome {
+                        outcome,
+                        data_path: Some(data_path),
+                        index_path: Some(index_path),
+                        keep_path: Some(keep_path),
+                    }
                 }
             }
             None => WriteOutcome {

gix-pack/tests/pack/data/output/count_and_entries.rs

@@ -322,13 +322,18 @@ fn traversals() -> crate::Result {
 
 #[test]
 fn empty_pack_is_allowed() {
-    write_and_verify(
-        db(DbKind::DeterministicGeneratedContent).unwrap(),
-        vec![],
-        hex_to_id("029d08823bd8a8eab510ad6ac75c823cfd3ed31e"),
-        None,
-    )
-    .unwrap();
+    assert_eq!(
+        write_and_verify(
+            db(DbKind::DeterministicGeneratedContent).unwrap(),
+            vec![],
+            hex_to_id("029d08823bd8a8eab510ad6ac75c823cfd3ed31e"),
+            None,
+        )
+        .unwrap_err()
+        .to_string(),
+        "pack data directory should be set",
+        "empty packs are not actually written as they would be useless"
+    );
 }
 
 fn write_and_verify(
@@ -392,7 +397,7 @@ fn write_and_verify(
             pack::bundle::write::Options::default(),
         )?
         .data_path
-        .expect("directory set"),
+        .ok_or("pack data directory should be set")?,
         object_hash,
     )?;
     // TODO: figure out why these hashes change, also depending on the machine, even though they are indeed stable.

gix-pack/tests/pack/index.rs

@@ -108,7 +108,7 @@ mod version {
         }
     }
 
-    #[cfg(feature = "internal-testing-gix-features-parallel")]
+    #[cfg(feature = "gix-features-parallel")]
     mod any {
         use std::{fs, io, sync::atomic::AtomicBool};
 
@@ -133,7 +133,7 @@ mod version {
                 index_path: &&str,
                 data_path: &&str,
             ) -> Result<(), Box<dyn std::error::Error>> {
-                let pack_iter = pack::data::input::BytesToEntriesIter::new_from_header(
+                let mut pack_iter = pack::data::input::BytesToEntriesIter::new_from_header(
                     io::BufReader::new(fs::File::open(fixture_path(data_path))?),
                     *mode,
                     *compressed,
@@ -148,12 +148,12 @@ mod version {
                     desired_kind,
                     || {
                         let file = std::fs::File::open(fixture_path(data_path))?;
-                        let map = unsafe { memmap2::MmapOptions::map_copy_read_only(&file)? };
+                        let map = unsafe { memmap2::MmapOptions::new().map_copy_read_only(&file)? };
                         Ok((slice_map, map))
                     },
-                    pack_iter,
+                    &mut pack_iter,
                     None,
-                    progress::Discard,
+                    &mut progress::Discard,
                     &mut actual,
                     &AtomicBool::new(false),
                     gix_hash::Kind::Sha1,
@@ -210,7 +210,7 @@ mod version {
                 assert_eq!(outcome.index_version, desired_kind);
                 assert_eq!(
                     outcome.index_hash,
-                    gix_hash::ObjectId::from(&expected[end_of_pack_hash..end_of_index_hash])
+                    gix_hash::ObjectId::try_from(&expected[end_of_pack_hash..end_of_index_hash])?
                 );
                 Ok(())
             }
@@ -227,7 +227,7 @@ mod version {
         #[test]
         fn lookup_missing() {
             let file = index::File::at(&fixture_path(INDEX_V2), gix_hash::Kind::Sha1).unwrap();
-            let prefix = gix_hash::Prefix::new(gix_hash::ObjectId::null(gix_hash::Kind::Sha1), 7).unwrap();
+            let prefix = gix_hash::Prefix::new(&gix_hash::Kind::Sha1.null(), 7).unwrap();
             assert!(file.lookup_prefix(prefix, None).is_none());
 
             let mut candidates = 1..1;