file_util: don't try to overwrite existing content-addressed file on Windows

lib/src/file_util.rs

@@ -90,21 +90,33 @@ pub fn normalize_path(path: &Path) -> PathBuf {
     }
 }
 
-// Like NamedTempFile::persist(), but also succeeds if the target already
-// exists.
+/// Like `NamedTempFile::persist()`, but doesn't try to overwrite the existing
+/// target on Windows.
 pub fn persist_content_addressed_temp_file<P: AsRef<Path>>(
     temp_file: NamedTempFile,
     new_path: P,
-) -> Result<File, PersistError> {
-    match temp_file.persist(&new_path) {
-        Ok(file) => Ok(file),
-        Err(PersistError { error, file }) => {
-            if let Ok(existing_file) = File::open(new_path) {
-                Ok(existing_file)
-            } else {
-                Err(PersistError { error, file })
+) -> io::Result<File> {
+    if cfg!(windows) {
+        // On Windows, overwriting file can fail if the file is opened without
+        // FILE_SHARE_DELETE for example. We don't need to take a risk if the
+        // file already exists.
+        match temp_file.persist_noclobber(&new_path) {
+            Ok(file) => Ok(file),
+            Err(PersistError { error, file: _ }) => {
+                if let Ok(existing_file) = File::open(new_path) {
+                    Ok(existing_file)
+                } else {
+                    Err(error)
+                }
             }
         }
+    } else {
+        // On Unix, rename() is atomic and should succeed even if the
+        // destination file exists. Checking if the target exists might involve
+        // non-atomic operation, so don't use persist_noclobber().
+        temp_file
+            .persist(new_path)
+            .map_err(|PersistError { error, file: _ }| error)
     }
 }
 

lib/src/local_working_copy.rs

@@ -494,7 +494,7 @@ pub enum TreeStateError {
     #[error("Persisting tree state to file {path}: {source}")]
     PersistTreeState {
         path: PathBuf,
-        source: tempfile::PersistError,
+        source: std::io::Error,
     },
     #[error("Filesystem monitor error: {0}")]
     Fsmonitor(Box<dyn Error + Send + Sync>),
@@ -646,9 +646,11 @@ impl TreeState {
         let target_path = self.state_path.join("tree_state");
         temp_file
             .persist(&target_path)
-            .map_err(|err| TreeStateError::PersistTreeState {
-                path: target_path.clone(),
-                source: err,
+            .map_err(|tempfile::PersistError { error, file: _ }| {
+                TreeStateError::PersistTreeState {
+                    path: target_path.clone(),
+                    source: error,
+                }
             })?;
         Ok(())
     }

lib/src/simple_op_store.rs

@@ -21,7 +21,7 @@ use std::io::{ErrorKind, Write};
 use std::path::{Path, PathBuf};
 
 use prost::Message;
-use tempfile::{NamedTempFile, PersistError};
+use tempfile::NamedTempFile;
 use thiserror::Error;
 
 use crate::backend::{CommitId, MillisSinceEpoch, ObjectId, Timestamp};
@@ -34,12 +34,6 @@ use crate::op_store::{
 };
 use crate::{git, op_store};
 
-impl From<PersistError> for OpStoreError {
-    fn from(err: PersistError) -> Self {
-        OpStoreError::Other(err.into())
-    }
-}
-
 #[derive(Debug, Error)]
 #[error("Failed to read {kind} with ID {id}: {err}")]
 struct DecodeError {
@@ -119,7 +113,8 @@ impl OpStore for SimpleOpStore {
 
         let id = ViewId::new(blake2b_hash(view).to_vec());
 
-        persist_content_addressed_temp_file(temp_file, self.view_path(&id))?;
+        persist_content_addressed_temp_file(temp_file, self.view_path(&id))
+            .map_err(|err| io_to_write_error(err, "view"))?;
         Ok(id)
     }
 
@@ -148,7 +143,8 @@ impl OpStore for SimpleOpStore {
 
         let id = OperationId::new(blake2b_hash(operation).to_vec());
 
-        persist_content_addressed_temp_file(temp_file, self.operation_path(&id))?;
+        persist_content_addressed_temp_file(temp_file, self.operation_path(&id))
+            .map_err(|err| io_to_write_error(err, "operation"))?;
         Ok(id)
     }
 }

lib/src/stacked_table.rs

@@ -359,10 +359,7 @@ impl TableSegment for MutableTable {
 
 #[derive(Debug, Error)]
 #[error(transparent)]
-pub enum TableStoreError {
-    IoError(#[from] io::Error),
-    PersistError(#[from] tempfile::PersistError),
-}
+pub struct TableStoreError(#[from] pub io::Error);
 
 pub type TableStoreResult<T> = Result<T, TableStoreError>;