From 8540536ea21efeacc028f236cbef8a8db1424cdf Mon Sep 17 00:00:00 2001
From: Yuya Nishihara <yuya@tcha.org>
Date: Tue, 22 Oct 2024 19:20:54 +0900
Subject: [PATCH] local_working_copy: detect error of file removal earlier

This should be safer than relying on file open error. It's scary to continue
processing if the file was a symlink.

I'll add a few more sanity checks to remove_old_file(), so it's extracted as a
function.
---
 lib/src/local_working_copy.rs        | 14 +++++++++++++-
 lib/tests/test_local_working_copy.rs | 27 +++++++++++++++++++++++++++
 2 files changed, 40 insertions(+), 1 deletion(-)

diff --git a/lib/src/local_working_copy.rs b/lib/src/local_working_copy.rs
index f062885c5..b1aca8c5c 100644
--- a/lib/src/local_working_copy.rs
+++ b/lib/src/local_working_copy.rs
@@ -466,6 +466,18 @@ fn create_parent_dirs(
     Ok(false)
 }
 
+/// Removes existing file named `disk_path` if any.
+fn remove_old_file(disk_path: &Path) -> Result<(), CheckoutError> {
+    match fs::remove_file(disk_path) {
+        Ok(()) => Ok(()),
+        Err(err) if err.kind() == io::ErrorKind::NotFound => Ok(()),
+        Err(err) => Err(CheckoutError::Other {
+            message: format!("Failed to remove file {}", disk_path.display()),
+            err: err.into(),
+        }),
+    }
+}
+
 fn mtime_from_metadata(metadata: &Metadata) -> MillisSinceEpoch {
     let time = metadata
         .modified()
@@ -1415,7 +1427,7 @@ impl TreeState {
             let disk_path = path.to_fs_path(&self.working_copy_path)?;
 
             if present_before {
-                fs::remove_file(&disk_path).ok();
+                remove_old_file(&disk_path)?;
             } else if disk_path.exists() {
                 changed_file_states.push((path, FileState::placeholder()));
                 stats.skipped_files += 1;
diff --git a/lib/tests/test_local_working_copy.rs b/lib/tests/test_local_working_copy.rs
index d8619362e..25a31b397 100644
--- a/lib/tests/test_local_working_copy.rs
+++ b/lib/tests/test_local_working_copy.rs
@@ -1184,6 +1184,33 @@ fn test_gitsubmodule() {
     );
 }
 
+#[test]
+fn test_check_out_existing_file_cannot_be_removed() {
+    let settings = testutils::user_settings();
+    let mut test_workspace = TestWorkspace::init(&settings);
+    let repo = &test_workspace.repo;
+    let workspace_root = test_workspace.workspace.workspace_root().to_owned();
+
+    let file_path = RepoPath::from_internal_string("file");
+    let tree1 = create_tree(repo, &[(file_path, "0")]);
+    let tree2 = create_tree(repo, &[(file_path, "1")]);
+    let commit1 = commit_with_tree(repo.store(), tree1.id());
+    let commit2 = commit_with_tree(repo.store(), tree2.id());
+
+    let ws = &mut test_workspace.workspace;
+    ws.check_out(repo.op_id().clone(), None, &commit1).unwrap();
+
+    // Trigger IO error by replacing file with directory.
+    std::fs::remove_file(file_path.to_fs_path_unchecked(&workspace_root)).unwrap();
+    std::fs::create_dir(file_path.to_fs_path_unchecked(&workspace_root)).unwrap();
+
+    let result = ws.check_out(repo.op_id().clone(), None, &commit2);
+    assert_matches!(
+        result,
+        Err(CheckoutError::Other { message, .. }) if message.contains("Failed to remove")
+    );
+}
+
 #[test]
 fn test_existing_directory_symlink() {
     let settings = testutils::user_settings();