fix: assure duplicate packs and indices aren't overwriting existing o…

…nes (#1404).

That way, on Windows there is no chance for access-denied errors.
Maybe there are other advantages as well even for Unix.

Since packs are usually written rarely, in comparison to loose objects,
the extra file accesss seems acceptable.

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

@@ -310,26 +310,35 @@ impl crate::Bundle {
                 } 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");
+                    let keep_path = if data_path.is_file() {
+                        // avoid trying to overwrite existing files, we know they have the same content
+                        // and this is likely to fail on Windows as negotiation opened the pack.
+                        None
+                    } else {
+                        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| {
-                            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
-                        })?;
+                        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)?;
+                        Some(keep_path)
+                    };
+                    if !index_path.is_file() {
+                        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),
+                        keep_path,
                     }
                 }
             }

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

@@ -33,18 +33,20 @@ impl Default for Options {
 #[derive(PartialEq, Eq, Debug, Hash, Ord, PartialOrd, Clone)]
 #[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))]
 pub struct Outcome {
-    /// The successful result of the index write operation
+    /// The successful result of the index write operation.
     pub index: crate::index::write::Outcome,
-    /// The version of the pack
+    /// The version of the pack.
     pub pack_version: crate::data::Version,
-    /// The kind of hash stored within the pack and indices
+    /// The kind of hash stored within the pack and indices.
     pub object_hash: gix_hash::Kind,
 
-    /// The path to the pack index file
+    /// The path to the pack index file.
     pub index_path: Option<PathBuf>,
-    /// The path to the pack data file
+    /// The path to the pack data file.
     pub data_path: Option<PathBuf>,
     /// The path to the `.keep` file to prevent collection of the newly written pack until refs are pointing to it.
+    /// It might be `None` if the file at `data_path` already existed, indicating that we have received a pack that
+    /// was already present locally.
     ///
     /// The file is created right before moving the pack data and index data into place (i.e. `data_path` and `index_path`)
     /// and is expected to be removed by the caller when ready.