working_copy: start improving interface for mutations

This patch changes the interface for making changes to the working
copy by replacing `write_tree()` and `untrack()` by a single
`start_mutation()` method. The two functions now live on the returned
`LockedWorkingCopy` object instead. That is more flexible because the
caller can make multiple changes while the working copy is locked. It
also helps us reduce the risk of buggy callers that read the commit ID
before taking the lock, because we can now make it accessible only on
`LockedWorkingCopy`.

lib/src/working_copy.rs

@@ -831,34 +831,21 @@ impl WorkingCopy {
         Ok(stats)
     }
 
-    pub fn write_tree(&mut self) -> LockedWorkingCopy {
+    pub fn start_mutation(&mut self) -> LockedWorkingCopy {
         let lock_path = self.state_path.join("working_copy.lock");
         let lock = FileLock::lock(lock_path);
 
         let current_proto = self.read_proto();
-        self.commit_id
-            .replace(Some(CommitId::new(current_proto.commit_id)));
-        self.tree_state().as_mut().unwrap().write_tree();
+        let old_commit_id = CommitId::new(current_proto.commit_id);
+        self.commit_id.replace(Some(old_commit_id.clone()));
 
         LockedWorkingCopy {
             wc: self,
             lock,
+            old_commit_id,
             closed: false,
         }
     }
-
-    pub fn untrack(&mut self, matcher: &dyn Matcher) -> Result<LockedWorkingCopy, UntrackError> {
-        let lock_path = self.state_path.join("working_copy.lock");
-        let lock = FileLock::lock(lock_path);
-
-        self.tree_state().as_mut().unwrap().untrack(matcher)?;
-
-        Ok(LockedWorkingCopy {
-            wc: self,
-            lock,
-            closed: false,
-        })
-    }
 }
 
 // A working copy that's locked on disk. The tree state has already been
@@ -867,18 +854,26 @@ pub struct LockedWorkingCopy<'a> {
     wc: &'a mut WorkingCopy,
     #[allow(dead_code)]
     lock: FileLock,
+    old_commit_id: CommitId,
     closed: bool,
 }
 
 impl LockedWorkingCopy<'_> {
-    pub fn old_commit_id(&self) -> CommitId {
-        self.wc.current_commit_id()
+    /// The commit at the time the lock was taken
+    pub fn old_commit_id(&self) -> &CommitId {
+        &self.old_commit_id
     }
 
-    pub fn new_tree_id(&self) -> TreeId {
+    pub fn write_tree(&mut self) -> TreeId {
+        self.wc.tree_state().as_mut().unwrap().write_tree();
         self.wc.current_tree_id()
     }
 
+    pub fn untrack(&mut self, matcher: &dyn Matcher) -> Result<TreeId, UntrackError> {
+        self.wc.tree_state().as_mut().unwrap().untrack(matcher)?;
+        Ok(self.wc.current_tree_id())
+    }
+
     pub fn finish(mut self, commit_id: CommitId) {
         self.wc.commit_id.replace(Some(commit_id));
         self.wc.save();

lib/tests/test_working_copy.rs

@@ -39,8 +39,8 @@ fn test_root(use_git: bool) {
     let repo = &test_workspace.repo;
 
     let wc = test_workspace.workspace.working_copy_mut();
-    let locked_wc = wc.write_tree();
-    let new_tree_id = locked_wc.new_tree_id();
+    let mut locked_wc = wc.start_mutation();
+    let new_tree_id = locked_wc.write_tree();
     locked_wc.discard();
     let checkout_commit = repo.store().get_commit(repo.view().checkout()).unwrap();
     assert_eq!(&new_tree_id, checkout_commit.tree().id());
@@ -221,8 +221,8 @@ fn test_checkout_file_transitions(use_git: bool) {
     wc.check_out(right_commit.clone()).unwrap();
 
     // Check that the working copy is clean.
-    let locked_wc = wc.write_tree();
-    let new_tree_id = locked_wc.new_tree_id();
+    let mut locked_wc = wc.start_mutation();
+    let new_tree_id = locked_wc.write_tree();
     locked_wc.discard();
     assert_eq!(&new_tree_id, right_commit.tree().id());
 
@@ -326,11 +326,12 @@ fn test_untrack() {
     // Now we untrack the file called "unwanted"
     let mut tx = repo.start_transaction("test");
     let matcher = FilesMatcher::new(HashSet::from([unwanted_path.clone()]));
-    let unfinished_write = wc.untrack(&matcher).unwrap();
+    let mut locked_wc = wc.start_mutation();
+    let new_tree_id = locked_wc.untrack(&matcher).unwrap();
     let new_commit = CommitBuilder::for_rewrite_from(&settings, repo.store(), &initial_commit)
-        .set_tree(unfinished_write.new_tree_id())
+        .set_tree(new_tree_id)
         .write_to_repo(tx.mut_repo());
-    unfinished_write.finish(new_commit.id().clone());
+    locked_wc.finish(new_commit.id().clone());
     tx.commit();
 
     // The file should still exist in the working copy.
@@ -346,10 +347,10 @@ fn test_untrack() {
 
     // It should not get re-added if we write a new tree since we also added it
     // to the .gitignore file.
-    let unfinished_write = wc.write_tree();
-    let new_tree_id = unfinished_write.new_tree_id();
+    let mut locked_wc = wc.start_mutation();
+    let new_tree_id = locked_wc.write_tree();
     assert_eq!(new_tree_id, *new_commit.tree().id());
-    unfinished_write.discard();
+    locked_wc.discard();
 }
 
 #[test_case(false ; "local backend")]
@@ -376,8 +377,8 @@ fn test_commit_racy_timestamps(use_git: bool) {
             file.write_all(format!("contents {}", i).as_bytes())
                 .unwrap();
         }
-        let locked_wc = wc.write_tree();
-        let new_tree_id = locked_wc.new_tree_id();
+        let mut locked_wc = wc.start_mutation();
+        let new_tree_id = locked_wc.write_tree();
         locked_wc.discard();
         assert_ne!(new_tree_id, previous_tree_id);
         previous_tree_id = new_tree_id;
@@ -410,8 +411,8 @@ fn test_gitignores(use_git: bool) {
     testutils::write_working_copy_file(&workspace_root, &subdir_modified_path, "1");
 
     let wc = test_workspace.workspace.working_copy_mut();
-    let locked_wc = wc.write_tree();
-    let new_tree_id1 = locked_wc.new_tree_id();
+    let mut locked_wc = wc.start_mutation();
+    let new_tree_id1 = locked_wc.write_tree();
     locked_wc.discard();
     let tree1 = repo
         .store()
@@ -440,8 +441,8 @@ fn test_gitignores(use_git: bool) {
     testutils::write_working_copy_file(&workspace_root, &subdir_modified_path, "2");
     testutils::write_working_copy_file(&workspace_root, &subdir_ignored_path, "2");
 
-    let locked_wc = wc.write_tree();
-    let new_tree_id2 = locked_wc.new_tree_id();
+    let mut locked_wc = wc.start_mutation();
+    let new_tree_id2 = locked_wc.write_tree();
     locked_wc.discard();
     let tree2 = repo
         .store()
@@ -505,8 +506,8 @@ fn test_gitignores_checkout_overwrites_ignored(use_git: bool) {
     assert_eq!(buf, b"contents");
 
     // Check that the file is in the tree created by committing the working copy
-    let locked_wc = wc.write_tree();
-    let new_tree_id = locked_wc.new_tree_id();
+    let mut locked_wc = wc.start_mutation();
+    let new_tree_id = locked_wc.write_tree();
     locked_wc.discard();
     let new_tree = repo
         .store()
@@ -556,8 +557,8 @@ fn test_gitignores_ignored_directory_already_tracked(use_git: bool) {
 
     // Check that the file is still in the tree created by committing the working
     // copy (that it didn't get removed because the directory is ignored)
-    let locked_wc = wc.write_tree();
-    let new_tree_id = locked_wc.new_tree_id();
+    let mut locked_wc = wc.start_mutation();
+    let new_tree_id = locked_wc.write_tree();
     locked_wc.discard();
     let new_tree = repo
         .store()
@@ -587,10 +588,10 @@ fn test_dotgit_ignored(use_git: bool) {
         &RepoPath::from_internal_string(".git/file"),
         "contents",
     );
-    let locked_working_copy = test_workspace.workspace.working_copy_mut().write_tree();
-    let new_tree_id = locked_working_copy.new_tree_id();
+    let mut locked_wc = test_workspace.workspace.working_copy_mut().start_mutation();
+    let new_tree_id = locked_wc.write_tree();
     assert_eq!(new_tree_id, *repo.store().empty_tree_id());
-    locked_working_copy.discard();
+    locked_wc.discard();
     std::fs::remove_dir_all(&dotgit_path).unwrap();
 
     // Test with a .git file
@@ -599,8 +600,8 @@ fn test_dotgit_ignored(use_git: bool) {
         &RepoPath::from_internal_string(".git"),
         "contents",
     );
-    let locked_working_copy = test_workspace.workspace.working_copy_mut().write_tree();
-    let new_tree_id = locked_working_copy.new_tree_id();
+    let mut locked_wc = test_workspace.workspace.working_copy_mut().start_mutation();
+    let new_tree_id = locked_wc.write_tree();
     assert_eq!(new_tree_id, *repo.store().empty_tree_id());
-    locked_working_copy.discard();
+    locked_wc.discard();
 }

lib/tests/test_working_copy_concurrent.rs

@@ -133,8 +133,8 @@ fn test_checkout_parallel(use_git: bool) {
             // different tree than the one we just checked out, but since
             // write_tree() should take the same lock as check_out(), write_tree()
             // should never produce a different tree.
-            let locked_wc = workspace.working_copy_mut().write_tree();
-            let new_tree_id = locked_wc.new_tree_id();
+            let mut locked_wc = workspace.working_copy_mut().start_mutation();
+            let new_tree_id = locked_wc.write_tree();
             locked_wc.discard();
             assert!(tree_ids.contains(&new_tree_id));
         });

src/commands.rs

@@ -293,7 +293,11 @@ impl WorkspaceCommandHelper {
             }
             self.repo = tx.commit();
             if let Some(new_wc_commit) = new_wc_commit {
-                let locked_working_copy = self.workspace.working_copy_mut().write_tree();
+                // TODO: We should probably do a regular checkout of new_wc_commit here even
+                // though it would usually have no effect. The issue is that if
+                // the update removed file ignored, we currently leave that as
+                // tracked, making it appear like it was added.
+                let locked_working_copy = self.workspace.working_copy_mut().start_mutation();
                 locked_working_copy.finish(new_wc_commit.id().clone());
             }
         }
@@ -456,9 +460,13 @@ impl WorkspaceCommandHelper {
     fn maybe_commit_working_copy(&mut self, ui: &mut Ui) -> Result<(), CommandError> {
         if self.may_update_working_copy {
             let repo = self.repo.clone();
-            let locked_wc = self.workspace.working_copy_mut().write_tree();
-            let old_commit_id = locked_wc.old_commit_id();
-            let old_commit = self.repo.store().get_commit(&old_commit_id).unwrap();
+            let mut locked_wc = self.workspace.working_copy_mut().start_mutation();
+            let new_tree_id = locked_wc.write_tree();
+            let old_commit = self
+                .repo
+                .store()
+                .get_commit(locked_wc.old_commit_id())
+                .unwrap();
             // Check if the current checkout has changed on disk after we read it. It's fine
             // if it has, but we'll need to reload the repo so the new commit is
             // in the index and view.
@@ -468,7 +476,6 @@ impl WorkspaceCommandHelper {
                 // view when we reload.
                 self.repo = repo.reload();
             }
-            let new_tree_id = locked_wc.new_tree_id();
             if new_tree_id != *old_commit.tree().id() {
                 let mut tx = self.repo.start_transaction("commit working copy");
                 let mut_repo = tx.mut_repo();
@@ -1585,18 +1592,18 @@ fn cmd_untrack(
         args.values_of("paths"),
     )?;
     let mut tx = workspace_command.start_transaction("untrack paths");
-    let unfinished_write = workspace_command
-        .working_copy_mut()
+    let mut locked_working_copy = workspace_command.working_copy_mut().start_mutation();
+    let new_tree_id = locked_working_copy
         .untrack(matcher.as_ref())
         .map_err(|err| CommandError::InternalError(format!("Failed to untrack paths: {}", err)))?;
-    let new_tree_id = unfinished_write.new_tree_id();
-    let old_commit_id = unfinished_write.old_commit_id();
-    let old_commit = store.get_commit(&old_commit_id).unwrap();
+    let old_commit = store
+        .get_commit(locked_working_copy.old_commit_id())
+        .unwrap();
     let new_commit = CommitBuilder::for_rewrite_from(ui.settings(), &store, &old_commit)
         .set_tree(new_tree_id)
         .write_to_repo(tx.mut_repo());
     tx.mut_repo().set_checkout(new_commit.id().clone());
-    unfinished_write.finish(new_commit.id().clone());
+    locked_working_copy.finish(new_commit.id().clone());
     workspace_command.finish_transaction(ui, tx)?;
 
     // TODO: Is it better to have WorkingCopy::untrack() report if any matching