working copy: return Box<dyn LockedWorkingCopy> from `start_mutatio…

…n()`

cli/src/cli_util.rs

@@ -41,7 +41,6 @@ use jj_lib::git_backend::GitBackend;
 use jj_lib::gitignore::GitIgnoreFile;
 use jj_lib::hex_util::to_reverse_hex;
 use jj_lib::id_prefix::IdPrefixContext;
-use jj_lib::local_working_copy::LockedLocalWorkingCopy;
 use jj_lib::matchers::{EverythingMatcher, Matcher, PrefixMatcher, Visit};
 use jj_lib::merged_tree::{MergedTree, MergedTreeBuilder};
 use jj_lib::op_heads_store::{self, OpHeadResolutionError, OpHeadsStore};
@@ -859,7 +858,7 @@ impl WorkspaceCommandHelper {
 
     pub fn unchecked_start_working_copy_mutation(
         &self,
-    ) -> Result<(LockedLocalWorkingCopy, Commit), CommandError> {
+    ) -> Result<(Box<dyn LockedWorkingCopy>, Commit), CommandError> {
         self.check_working_copy_writable()?;
         let wc_commit = if let Some(wc_commit_id) = self.get_wc_commit_id() {
             self.repo().store().get_commit(wc_commit_id)?
@@ -874,7 +873,7 @@ impl WorkspaceCommandHelper {
 
     pub fn start_working_copy_mutation(
         &self,
-    ) -> Result<(LockedLocalWorkingCopy, Commit), CommandError> {
+    ) -> Result<(Box<dyn LockedWorkingCopy>, Commit), CommandError> {
         let (locked_working_copy, wc_commit) = self.unchecked_start_working_copy_mutation()?;
         if wc_commit.tree_id() != locked_working_copy.old_tree_id() {
             return Err(user_error("Concurrent working copy operation. Try again."));
@@ -1302,45 +1301,47 @@ Set which revision the branch points to with `jj branch set {branch_name} -r <RE
         // Compare working-copy tree and operation with repo's, and reload as needed.
         let mut locked_wc = self.workspace.working_copy().start_mutation()?;
         let old_op_id = locked_wc.old_operation_id().clone();
-        let (repo, wc_commit) = match check_stale_working_copy(&locked_wc, &wc_commit, &repo) {
-            Ok(None) => (repo, wc_commit),
-            Ok(Some(wc_operation)) => {
-                let repo = repo.reload_at(&wc_operation)?;
-                let wc_commit = if let Some(wc_commit) = get_wc_commit(&repo)? {
-                    wc_commit
-                } else {
-                    return Ok(()); // The workspace has been deleted (see above)
-                };
-                (repo, wc_commit)
-            }
-            Err(StaleWorkingCopyError::WorkingCopyStale) => {
-                return Err(user_error_with_hint(
-                    format!(
-                        "The working copy is stale (not updated since operation {}).",
-                        short_operation_hash(&old_op_id)
-                    ),
-                    "Run `jj workspace update-stale` to update it.
+        let (repo, wc_commit) =
+            match check_stale_working_copy(locked_wc.as_ref(), &wc_commit, &repo) {
+                Ok(None) => (repo, wc_commit),
+                Ok(Some(wc_operation)) => {
+                    let repo = repo.reload_at(&wc_operation)?;
+                    let wc_commit = if let Some(wc_commit) = get_wc_commit(&repo)? {
+                        wc_commit
+                    } else {
+                        return Ok(()); // The workspace has been deleted (see
+                                       // above)
+                    };
+                    (repo, wc_commit)
+                }
+                Err(StaleWorkingCopyError::WorkingCopyStale) => {
+                    return Err(user_error_with_hint(
+                        format!(
+                            "The working copy is stale (not updated since operation {}).",
+                            short_operation_hash(&old_op_id)
+                        ),
+                        "Run `jj workspace update-stale` to update it.
 See https://github.com/martinvonz/jj/blob/main/docs/working-copy.md#stale-working-copy \
-                     for more information.",
-                ));
-            }
-            Err(StaleWorkingCopyError::SiblingOperation) => {
-                return Err(CommandError::InternalError(format!(
-                    "The repo was loaded at operation {}, which seems to be a sibling of the \
-                     working copy's operation {}",
-                    short_operation_hash(repo.op_id()),
-                    short_operation_hash(&old_op_id)
-                )));
-            }
-            Err(StaleWorkingCopyError::UnrelatedOperation) => {
-                return Err(CommandError::InternalError(format!(
-                    "The repo was loaded at operation {}, which seems unrelated to the working \
-                     copy's operation {}",
-                    short_operation_hash(repo.op_id()),
-                    short_operation_hash(&old_op_id)
-                )));
-            }
-        };
+                         for more information.",
+                    ));
+                }
+                Err(StaleWorkingCopyError::SiblingOperation) => {
+                    return Err(CommandError::InternalError(format!(
+                        "The repo was loaded at operation {}, which seems to be a sibling of the \
+                         working copy's operation {}",
+                        short_operation_hash(repo.op_id()),
+                        short_operation_hash(&old_op_id)
+                    )));
+                }
+                Err(StaleWorkingCopyError::UnrelatedOperation) => {
+                    return Err(CommandError::InternalError(format!(
+                        "The repo was loaded at operation {}, which seems unrelated to the \
+                         working copy's operation {}",
+                        short_operation_hash(repo.op_id()),
+                        short_operation_hash(&old_op_id)
+                    )));
+                }
+            };
         self.user_repo = ReadonlyUserRepo::new(repo);
         let progress = crate::progress::snapshot_progress(ui);
         let new_tree_id = locked_wc.snapshot(SnapshotOptions {
@@ -1724,7 +1725,7 @@ pub enum StaleWorkingCopyError {
 
 #[instrument(skip_all)]
 pub fn check_stale_working_copy(
-    locked_wc: &LockedLocalWorkingCopy,
+    locked_wc: &dyn LockedWorkingCopy,
     wc_commit: &Commit,
     repo: &ReadonlyRepo,
 ) -> Result<Option<Operation>, StaleWorkingCopyError> {

cli/src/commands/debug.rs

@@ -20,7 +20,7 @@ use jj_lib::backend::ObjectId;
 use jj_lib::default_index_store::{DefaultIndexStore, ReadonlyIndexWrapper};
 use jj_lib::local_working_copy::LocalWorkingCopy;
 use jj_lib::revset;
-use jj_lib::working_copy::{LockedWorkingCopy, WorkingCopy};
+use jj_lib::working_copy::WorkingCopy;
 
 use crate::cli_util::{resolve_op_for_load, user_error, CommandError, CommandHelper};
 use crate::template_parser;

cli/src/commands/mod.rs

@@ -47,7 +47,7 @@ use jj_lib::revset_graph::{
 };
 use jj_lib::rewrite::{back_out_commit, merge_commit_trees, rebase_commit, DescendantRebaser};
 use jj_lib::settings::UserSettings;
-use jj_lib::working_copy::{LockedWorkingCopy, SnapshotOptions};
+use jj_lib::working_copy::SnapshotOptions;
 use jj_lib::workspace::Workspace;
 use jj_lib::{conflicts, file_util, revset};
 use maplit::{hashmap, hashset};
@@ -3903,7 +3903,7 @@ fn cmd_workspace_update_stale(
     let repo = workspace_command.repo().clone();
     let (mut locked_wc, desired_wc_commit) =
         workspace_command.unchecked_start_working_copy_mutation()?;
-    match check_stale_working_copy(&locked_wc, &desired_wc_commit, &repo) {
+    match check_stale_working_copy(locked_wc.as_ref(), &desired_wc_commit, &repo) {
         Ok(_) => {
             writeln!(
                 ui.stderr(),

lib/src/local_working_copy.rs

@@ -1312,7 +1312,7 @@ impl WorkingCopy for LocalWorkingCopy {
         Ok(self.tree_state()?.sparse_patterns())
     }
 
-    fn start_mutation(&self) -> Result<LockedLocalWorkingCopy, WorkingCopyStateError> {
+    fn start_mutation(&self) -> Result<Box<dyn LockedWorkingCopy>, WorkingCopyStateError> {
         let lock_path = self.state_path.join("working_copy.lock");
         let lock = FileLock::lock(lock_path);
 
@@ -1328,13 +1328,13 @@ impl WorkingCopy for LocalWorkingCopy {
         };
         let old_operation_id = wc.operation_id().clone();
         let old_tree_id = wc.tree_id()?.clone();
-        Ok(LockedLocalWorkingCopy {
+        Ok(Box::new(LockedLocalWorkingCopy {
             wc,
             lock,
             old_operation_id,
             old_tree_id,
             tree_state_dirty: false,
-        })
+        }))
     }
 }
 
@@ -1565,7 +1565,7 @@ impl LockedWorkingCopy for LockedLocalWorkingCopy {
 
     #[instrument(skip_all)]
     fn finish(
-        mut self,
+        mut self: Box<Self>,
         operation_id: OperationId,
     ) -> Result<Box<dyn WorkingCopy>, WorkingCopyStateError> {
         assert!(self.tree_state_dirty || &self.old_tree_id == self.wc.tree_id()?);

lib/src/working_copy.rs

@@ -25,7 +25,6 @@ use thiserror::Error;
 use crate::backend::{BackendError, MergedTreeId};
 use crate::fsmonitor::FsmonitorKind;
 use crate::gitignore::GitIgnoreFile;
-use crate::local_working_copy::LockedLocalWorkingCopy;
 use crate::merged_tree::MergedTree;
 use crate::op_store::{OperationId, WorkspaceId};
 use crate::repo_path::RepoPath;
@@ -60,8 +59,7 @@ pub trait WorkingCopy {
 
     /// Locks the working copy and returns an instance with methods for updating
     /// the working copy files and state.
-    // TODO: return a `Box<dyn LockedWorkingCopy>` instead
-    fn start_mutation(&self) -> Result<LockedLocalWorkingCopy, WorkingCopyStateError>;
+    fn start_mutation(&self) -> Result<Box<dyn LockedWorkingCopy>, WorkingCopyStateError>;
 }
 
 /// A working copy that's being modified.
@@ -105,7 +103,7 @@ pub trait LockedWorkingCopy {
     /// Finish the modifications to the working copy by writing the updated
     /// states to disk. Returns the new (unlocked) working copy.
     fn finish(
-        self,
+        self: Box<Self>,
         operation_id: OperationId,
     ) -> Result<Box<dyn WorkingCopy>, WorkingCopyStateError>;
 }

lib/src/workspace.rs

@@ -37,9 +37,7 @@ use crate::repo::{
 };
 use crate::settings::UserSettings;
 use crate::submodule_store::SubmoduleStore;
-use crate::working_copy::{
-    CheckoutError, CheckoutStats, LockedWorkingCopy, WorkingCopy, WorkingCopyStateError,
-};
+use crate::working_copy::{CheckoutError, CheckoutStats, WorkingCopy, WorkingCopyStateError};
 
 #[derive(Error, Debug)]
 pub enum WorkspaceInitError {

lib/tests/test_local_working_copy.rs

@@ -27,7 +27,7 @@ use std::sync::Arc;
 use itertools::Itertools;
 use jj_lib::backend::{MergedTreeId, ObjectId, TreeId, TreeValue};
 use jj_lib::fsmonitor::FsmonitorKind;
-use jj_lib::local_working_copy::{LocalWorkingCopy, LockedLocalWorkingCopy};
+use jj_lib::local_working_copy::LocalWorkingCopy;
 use jj_lib::merge::Merge;
 use jj_lib::merged_tree::{MergedTree, MergedTreeBuilder};
 use jj_lib::op_store::{OperationId, WorkspaceId};
@@ -925,7 +925,7 @@ fn test_fsmonitor() {
     testutils::write_working_copy_file(&workspace_root, &ignored_path, "ignored\n");
     testutils::write_working_copy_file(&workspace_root, &gitignore_path, "to/ignored\n");
 
-    let snapshot = |locked_wc: &mut LockedLocalWorkingCopy, paths: &[&RepoPath]| {
+    let snapshot = |locked_wc: &mut dyn LockedWorkingCopy, paths: &[&RepoPath]| {
         let fs_paths = paths
             .iter()
             .map(|p| p.to_fs_path(&workspace_root))
@@ -942,13 +942,13 @@ fn test_fsmonitor() {
 
     {
         let mut locked_wc = ws.working_copy().start_mutation().unwrap();
-        let tree_id = snapshot(&mut locked_wc, &[]);
+        let tree_id = snapshot(locked_wc.as_mut(), &[]);
         assert_eq!(tree_id, repo.store().empty_merged_tree_id());
     }
 
     {
         let mut locked_wc = ws.working_copy().start_mutation().unwrap();
-        let tree_id = snapshot(&mut locked_wc, &[&foo_path]);
+        let tree_id = snapshot(locked_wc.as_mut(), &[&foo_path]);
         insta::assert_snapshot!(testutils::dump_tree(repo.store(), &tree_id), @r###"
         tree d5e38c0a1b0ee5de47c5
           file "foo" (e99c2057c15160add351): "foo\n"
@@ -958,7 +958,7 @@ fn test_fsmonitor() {
     {
         let mut locked_wc = ws.working_copy().start_mutation().unwrap();
         let tree_id = snapshot(
-            &mut locked_wc,
+            locked_wc.as_mut(),
             &[&foo_path, &bar_path, &nested_path, &ignored_path],
         );
         insta::assert_snapshot!(testutils::dump_tree(repo.store(), &tree_id), @r###"
@@ -974,7 +974,7 @@ fn test_fsmonitor() {
         testutils::write_working_copy_file(&workspace_root, &foo_path, "updated foo\n");
         testutils::write_working_copy_file(&workspace_root, &bar_path, "updated bar\n");
         let mut locked_wc = ws.working_copy().start_mutation().unwrap();
-        let tree_id = snapshot(&mut locked_wc, &[&foo_path]);
+        let tree_id = snapshot(locked_wc.as_mut(), &[&foo_path]);
         insta::assert_snapshot!(testutils::dump_tree(repo.store(), &tree_id), @r###"
         tree e994a93c46f41dc91704
           file "bar" (94cc973e7e1aefb7eff6): "bar\n"
@@ -986,7 +986,7 @@ fn test_fsmonitor() {
     {
         std::fs::remove_file(foo_path.to_fs_path(&workspace_root)).unwrap();
         let mut locked_wc = ws.working_copy().start_mutation().unwrap();
-        let tree_id = snapshot(&mut locked_wc, &[&foo_path]);
+        let tree_id = snapshot(locked_wc.as_mut(), &[&foo_path]);
         insta::assert_snapshot!(testutils::dump_tree(repo.store(), &tree_id), @r###"
         tree 1df764981d4d74a4ecfa
           file "bar" (94cc973e7e1aefb7eff6): "bar\n"

lib/tests/test_local_working_copy_concurrent.rs

@@ -18,7 +18,7 @@ use std::thread;
 use assert_matches::assert_matches;
 use jj_lib::repo::Repo;
 use jj_lib::repo_path::RepoPath;
-use jj_lib::working_copy::{CheckoutError, LockedWorkingCopy, SnapshotOptions};
+use jj_lib::working_copy::{CheckoutError, SnapshotOptions};
 use jj_lib::workspace::Workspace;
 use testutils::{create_tree, write_working_copy_file, TestRepo, TestWorkspace};
 

lib/tests/test_local_working_copy_sparse.rs

@@ -17,7 +17,7 @@ use jj_lib::local_working_copy::LocalWorkingCopy;
 use jj_lib::matchers::EverythingMatcher;
 use jj_lib::repo::Repo;
 use jj_lib::repo_path::RepoPath;
-use jj_lib::working_copy::{CheckoutStats, LockedWorkingCopy, WorkingCopy};
+use jj_lib::working_copy::{CheckoutStats, WorkingCopy};
 use testutils::{create_tree, TestWorkspace};
 
 #[test]

lib/testutils/src/lib.rs

@@ -32,7 +32,7 @@ use jj_lib::store::Store;
 use jj_lib::transaction::Transaction;
 use jj_lib::tree::Tree;
 use jj_lib::tree_builder::TreeBuilder;
-use jj_lib::working_copy::{LockedWorkingCopy, SnapshotError, SnapshotOptions};
+use jj_lib::working_copy::{SnapshotError, SnapshotOptions};
 use jj_lib::workspace::Workspace;
 use tempfile::TempDir;