From cc860f771c9bfd0b5c73f6c660912e43fcd787e0 Mon Sep 17 00:00:00 2001
From: Yuya Nishihara <yuya@tcha.org>
Date: Mon, 1 Aug 2022 14:56:08 +0900
Subject: [PATCH] working_copy: do not overwrite ignored file

Since the file should have been removed on Diff::Modified case, we can always
expect that write_file/conflict() creates new file.
---
 lib/src/working_copy.rs               | 11 +++--------
 lib/tests/test_working_copy.rs        | 26 +++++++-------------------
 lib/tests/test_working_copy_sparse.rs |  5 -----
 3 files changed, 10 insertions(+), 32 deletions(-)

diff --git a/lib/src/working_copy.rs b/lib/src/working_copy.rs
index 62fc6d273..53c561d44 100644
--- a/lib/src/working_copy.rs
+++ b/lib/src/working_copy.rs
@@ -176,6 +176,7 @@ fn sparse_patterns_from_proto(proto: &crate::protos::working_copy::TreeState) ->
 }
 
 fn create_parent_dirs(disk_path: &Path) -> Result<(), CheckoutError> {
+    // TODO: Check that we don't follow symlinks while creating parent dirs.
     fs::create_dir_all(disk_path.parent().unwrap()).map_err(|err| CheckoutError::IoError {
         message: format!(
             "Failed to create parent directories for {}",
@@ -643,12 +644,9 @@ impl TreeState {
         executable: bool,
     ) -> Result<FileState, CheckoutError> {
         create_parent_dirs(disk_path)?;
-        // TODO: Check that we're not overwriting an un-ignored file here (which might
-        // be created by a concurrent process).
         let mut file = OpenOptions::new()
             .write(true)
-            .create(true)
-            .truncate(true)
+            .create_new(true) // Don't overwrite un-ignored file. Don't follow symlink.
             .open(disk_path)
             .map_err(|err| CheckoutError::IoError {
                 message: format!("Failed to open file {} for writing", disk_path.display()),
@@ -710,12 +708,9 @@ impl TreeState {
     ) -> Result<FileState, CheckoutError> {
         create_parent_dirs(disk_path)?;
         let conflict = self.store.read_conflict(path, id)?;
-        // TODO: Check that we're not overwriting an un-ignored file here (which might
-        // be created by a concurrent process).
         let mut file = OpenOptions::new()
             .write(true)
-            .create(true)
-            .truncate(true)
+            .create_new(true) // Don't overwrite un-ignored file. Don't follow symlink.
             .open(disk_path)
             .map_err(|err| CheckoutError::IoError {
                 message: format!("Failed to open file {} for writing", disk_path.display()),
diff --git a/lib/tests/test_working_copy.rs b/lib/tests/test_working_copy.rs
index 315f839e7..ceb8b8444 100644
--- a/lib/tests/test_working_copy.rs
+++ b/lib/tests/test_working_copy.rs
@@ -554,9 +554,9 @@ fn test_gitignores(use_git: bool) {
 
 #[test_case(false ; "local backend")]
 #[test_case(true ; "git backend")]
-fn test_gitignores_checkout_overwrites_ignored(use_git: bool) {
-    // Tests that a .gitignore'd file gets overwritten if check out a commit where
-    // the file is tracked.
+fn test_gitignores_checkout_never_overwrites_ignored(use_git: bool) {
+    // Tests that a .gitignore'd file doesn't get overwritten if check out a commit
+    // where the file is tracked.
 
     let _home_dir = testutils::new_user_home();
     let settings = testutils::user_settings();
@@ -579,27 +579,15 @@ fn test_gitignores_checkout_overwrites_ignored(use_git: bool) {
     let tree = repo.store().get_tree(&RepoPath::root(), &tree_id).unwrap();
 
     // Now check out the tree that adds the file "modified" with contents
-    // "contents". The exiting contents ("garbage") should be replaced in the
+    // "contents". The exiting contents ("garbage") shouldn't be replaced in the
     // working copy.
     let wc = test_workspace.workspace.working_copy_mut();
-    wc.check_out(repo.op_id().clone(), None, &tree).unwrap();
+    assert!(wc.check_out(repo.op_id().clone(), None, &tree).is_err());
 
-    // Check that the new contents are in the working copy
+    // Check that the old contents are in the working copy
     let path = workspace_root.join("modified");
     assert!(path.is_file());
-    assert_eq!(std::fs::read(&path).unwrap(), b"contents");
-
-    // Check that the file is in the tree created by snapshotting the working copy
-    let mut locked_wc = wc.start_mutation();
-    let new_tree_id = locked_wc.snapshot(GitIgnoreFile::empty()).unwrap();
-    locked_wc.discard();
-    let new_tree = repo
-        .store()
-        .get_tree(&RepoPath::root(), &new_tree_id)
-        .unwrap();
-    assert!(new_tree
-        .entry(&RepoPathComponent::from("modified"))
-        .is_some());
+    assert_eq!(std::fs::read(&path).unwrap(), b"garbage");
 }
 
 #[test_case(false ; "local backend")]
diff --git a/lib/tests/test_working_copy_sparse.rs b/lib/tests/test_working_copy_sparse.rs
index defa81f19..8fe73a549 100644
--- a/lib/tests/test_working_copy_sparse.rs
+++ b/lib/tests/test_working_copy_sparse.rs
@@ -183,11 +183,6 @@ fn test_sparse_commit() {
     let sparse_patterns = vec![dir1_path, dir2_path];
     locked_wc.set_sparse_patterns(sparse_patterns).unwrap();
     locked_wc.finish(repo.op_id().clone());
-    // Write out a modified version of dir2/file1 again because it was overwritten
-    // when we added dir2/ to the sparse patterns.
-    // TODO: We shouldn't overwrite files when updating (there's already a TODO
-    // about that in `TreeState::write_file()`).
-    std::fs::write(dir2_file1_path.to_fs_path(&working_copy_path), "modified").unwrap();
 
     // Create a tree from the working copy. Only dir1/file1 and dir2/file1 should be
     // updated in the tree.