From c83fe099034a012d3aab3f6c51dfb6431da894c0 Mon Sep 17 00:00:00 2001 From: Martin von Zweigbergk Date: Sat, 14 Oct 2023 05:52:50 -0700 Subject: [PATCH] workspace: load working copy implementation dynamically This makes `Workspace::load()` look a new `.jj/working_copy/type` file in order to load the right working copy implementation, just like `Repo::load()` picks the right backends based on `.jj/store/type`, `.jj/op_store/type`, etc. We don't write the file yet, and we don't have a way of adding alternative working copy implementations, so it will always be `LocalWorkingCopy` for now. --- cli/src/cli_util.rs | 16 ++++- lib/src/local_working_copy.rs | 6 +- lib/src/repo.rs | 2 +- lib/src/workspace.rs | 60 +++++++++++++++---- lib/tests/test_bad_locking.rs | 29 ++++++--- .../test_local_working_copy_concurrent.rs | 5 +- lib/tests/test_workspace.rs | 11 +++- 7 files changed, 103 insertions(+), 26 deletions(-) diff --git a/cli/src/cli_util.rs b/cli/src/cli_util.rs index f3e1c21aae..60917483f6 100644 --- a/cli/src/cli_util.rs +++ b/cli/src/cli_util.rs @@ -12,7 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. -use std::collections::{HashSet, VecDeque}; +use std::collections::{HashMap, HashSet, VecDeque}; use std::env::{self, ArgsOs, VarError}; use std::ffi::{OsStr, OsString}; use std::fmt::Debug; @@ -64,7 +64,8 @@ use jj_lib::working_copy::{ WorkingCopyStateError, }; use jj_lib::workspace::{ - LockedWorkspace, Workspace, WorkspaceInitError, WorkspaceLoadError, WorkspaceLoader, + default_working_copy_factories, LockedWorkspace, WorkingCopyFactory, Workspace, + WorkspaceInitError, WorkspaceLoadError, WorkspaceLoader, }; use jj_lib::{dag_walk, file_util, git, revset}; use once_cell::unsync::OnceCell; @@ -501,6 +502,7 @@ pub struct CommandHelper { layered_configs: LayeredConfigs, maybe_workspace_loader: Result, store_factories: StoreFactories, + working_copy_factories: HashMap, } impl CommandHelper { @@ -515,6 +517,7 @@ impl CommandHelper { layered_configs: LayeredConfigs, maybe_workspace_loader: Result, store_factories: StoreFactories, + working_copy_factories: HashMap, ) -> Self { // `cwd` is canonicalized for consistency with `Workspace::workspace_root()` and // to easily compute relative paths between them. @@ -530,6 +533,7 @@ impl CommandHelper { layered_configs, maybe_workspace_loader, store_factories, + working_copy_factories, } } @@ -599,7 +603,11 @@ impl CommandHelper { pub fn load_workspace(&self) -> Result { let loader = self.workspace_loader()?; loader - .load(&self.settings, &self.store_factories) + .load( + &self.settings, + &self.store_factories, + &self.working_copy_factories, + ) .map_err(|err| map_workspace_load_error(err, &self.global_args)) } @@ -2789,6 +2797,7 @@ impl CliRunner { let config = layered_configs.merge(); ui.reset(&config)?; let settings = UserSettings::from_config(config); + let working_copy_factories = default_working_copy_factories(); let command_helper = CommandHelper::new( self.app, cwd, @@ -2799,6 +2808,7 @@ impl CliRunner { layered_configs, maybe_workspace_loader, self.store_factories.unwrap_or_default(), + working_copy_factories, ); (self.dispatch_fn)(ui, &command_helper) } diff --git a/lib/src/local_working_copy.rs b/lib/src/local_working_copy.rs index a757b7c5ed..da52f06499 100644 --- a/lib/src/local_working_copy.rs +++ b/lib/src/local_working_copy.rs @@ -1290,7 +1290,7 @@ impl WorkingCopy for LocalWorkingCopy { } fn name(&self) -> &str { - "local" + Self::name() } fn path(&self) -> &Path { @@ -1340,6 +1340,10 @@ impl WorkingCopy for LocalWorkingCopy { } impl LocalWorkingCopy { + pub fn name() -> &'static str { + "local" + } + /// Initializes a new working copy at `working_copy_path`. The working /// copy's state will be stored in the `state_path` directory. The working /// copy will have the empty tree checked out. diff --git a/lib/src/repo.rs b/lib/src/repo.rs index c750af1e2b..d3d6a5ad43 100644 --- a/lib/src/repo.rs +++ b/lib/src/repo.rs @@ -548,7 +548,7 @@ impl StoreFactories { } } -fn read_store_type_compat( +pub fn read_store_type_compat( store: &'static str, path: impl AsRef, default: impl FnOnce() -> &'static str, diff --git a/lib/src/workspace.rs b/lib/src/workspace.rs index 6be8145252..ef91b47ef1 100644 --- a/lib/src/workspace.rs +++ b/lib/src/workspace.rs @@ -14,6 +14,7 @@ #![allow(missing_docs)] +use std::collections::HashMap; use std::fs; use std::fs::File; use std::io::{self, Write}; @@ -30,11 +31,12 @@ use crate::local_backend::LocalBackend; use crate::local_working_copy::LocalWorkingCopy; use crate::op_store::{OperationId, WorkspaceId}; use crate::repo::{ - BackendInitializer, CheckOutCommitError, IndexStoreInitializer, OpHeadsStoreInitializer, - OpStoreInitializer, ReadonlyRepo, Repo, RepoInitError, RepoLoader, StoreFactories, - StoreLoadError, SubmoduleStoreInitializer, + read_store_type_compat, BackendInitializer, CheckOutCommitError, IndexStoreInitializer, + OpHeadsStoreInitializer, OpStoreInitializer, ReadonlyRepo, Repo, RepoInitError, RepoLoader, + StoreFactories, StoreLoadError, SubmoduleStoreInitializer, }; use crate::settings::UserSettings; +use crate::store::Store; use crate::working_copy::{ CheckoutError, CheckoutStats, LockedWorkingCopy, WorkingCopy, WorkingCopyStateError, }; @@ -271,9 +273,10 @@ impl Workspace { user_settings: &UserSettings, workspace_path: &Path, store_factories: &StoreFactories, + working_copy_factories: &HashMap, ) -> Result { let loader = WorkspaceLoader::init(workspace_path)?; - let workspace = loader.load(user_settings, store_factories)?; + let workspace = loader.load(user_settings, store_factories, working_copy_factories)?; Ok(workspace) } @@ -406,14 +409,51 @@ impl WorkspaceLoader { &self, user_settings: &UserSettings, store_factories: &StoreFactories, + working_copy_factories: &HashMap, ) -> Result { let repo_loader = RepoLoader::init(user_settings, &self.repo_dir, store_factories)?; - let working_copy = LocalWorkingCopy::load( - repo_loader.store().clone(), - self.workspace_root.clone(), - self.working_copy_state_path.clone(), - ); - let workspace = Workspace::new(&self.workspace_root, Box::new(working_copy), repo_loader)?; + let working_copy = self.load_working_copy(repo_loader.store(), working_copy_factories)?; + let workspace = Workspace::new(&self.workspace_root, working_copy, repo_loader)?; Ok(workspace) } + + fn load_working_copy( + &self, + store: &Arc, + working_copy_factories: &HashMap, + ) -> Result, StoreLoadError> { + // For compatibility with existing repos. TODO: Delete default in 0.17+ + let working_copy_type = read_store_type_compat( + "working copy", + self.working_copy_state_path.join("type"), + LocalWorkingCopy::name, + )?; + let working_copy_factory = + working_copy_factories + .get(&working_copy_type) + .ok_or_else(|| StoreLoadError::UnsupportedType { + store: "working copy", + store_type: working_copy_type.to_string(), + })?; + let working_copy = + working_copy_factory(store, &self.workspace_root, &self.working_copy_state_path); + Ok(working_copy) + } } + +pub fn default_working_copy_factories() -> HashMap { + let mut factories: HashMap = HashMap::new(); + factories.insert( + LocalWorkingCopy::name().to_owned(), + Box::new(|store, working_copy_path, store_path| { + Box::new(LocalWorkingCopy::load( + store.clone(), + working_copy_path.to_owned(), + store_path.to_owned(), + )) + }), + ); + factories +} + +pub type WorkingCopyFactory = Box, &Path, &Path) -> Box>; diff --git a/lib/tests/test_bad_locking.rs b/lib/tests/test_bad_locking.rs index e285fb19b1..cfa9a8faa7 100644 --- a/lib/tests/test_bad_locking.rs +++ b/lib/tests/test_bad_locking.rs @@ -16,7 +16,7 @@ use std::path::Path; use itertools::Itertools; use jj_lib::repo::{Repo, StoreFactories}; -use jj_lib::workspace::Workspace; +use jj_lib::workspace::{default_working_copy_factories, Workspace}; use test_case::test_case; use testutils::{create_random_commit, load_repo_at_head, TestRepoBackend, TestWorkspace}; @@ -115,8 +115,13 @@ fn test_bad_locking_children(backend: TestRepoBackend) { // Simulate a write of a commit that happens on one machine let machine1_root = test_workspace.root_dir().join("machine1"); copy_directory(workspace_root, &machine1_root); - let machine1_workspace = - Workspace::load(&settings, &machine1_root, &StoreFactories::default()).unwrap(); + let machine1_workspace = Workspace::load( + &settings, + &machine1_root, + &StoreFactories::default(), + &default_working_copy_factories(), + ) + .unwrap(); let machine1_repo = machine1_workspace .repo_loader() .load_at_head(&settings) @@ -131,8 +136,13 @@ fn test_bad_locking_children(backend: TestRepoBackend) { // Simulate a write of a commit that happens on another machine let machine2_root = test_workspace.root_dir().join("machine2"); copy_directory(workspace_root, &machine2_root); - let machine2_workspace = - Workspace::load(&settings, &machine2_root, &StoreFactories::default()).unwrap(); + let machine2_workspace = Workspace::load( + &settings, + &machine2_root, + &StoreFactories::default(), + &default_working_copy_factories(), + ) + .unwrap(); let machine2_repo = machine2_workspace .repo_loader() .load_at_head(&settings) @@ -148,8 +158,13 @@ fn test_bad_locking_children(backend: TestRepoBackend) { // both machines let merged_path = test_workspace.root_dir().join("merged"); merge_directories(&machine1_root, workspace_root, &machine2_root, &merged_path); - let merged_workspace = - Workspace::load(&settings, &merged_path, &StoreFactories::default()).unwrap(); + let merged_workspace = Workspace::load( + &settings, + &merged_path, + &StoreFactories::default(), + &default_working_copy_factories(), + ) + .unwrap(); let merged_repo = merged_workspace .repo_loader() .load_at_head(&settings) diff --git a/lib/tests/test_local_working_copy_concurrent.rs b/lib/tests/test_local_working_copy_concurrent.rs index a1d41d0a85..46e0afa639 100644 --- a/lib/tests/test_local_working_copy_concurrent.rs +++ b/lib/tests/test_local_working_copy_concurrent.rs @@ -19,7 +19,7 @@ use assert_matches::assert_matches; use jj_lib::repo::Repo; use jj_lib::repo_path::RepoPath; use jj_lib::working_copy::{CheckoutError, SnapshotOptions}; -use jj_lib::workspace::Workspace; +use jj_lib::workspace::{default_working_copy_factories, Workspace}; use testutils::{commit_with_tree, create_tree, write_working_copy_file, TestRepo, TestWorkspace}; #[test] @@ -52,6 +52,7 @@ fn test_concurrent_checkout() { &settings, &workspace1_root, &TestRepo::default_store_factories(), + &default_working_copy_factories(), ) .unwrap(); ws2.check_out(repo.op_id().clone(), Some(&tree_id1), &commit2) @@ -68,6 +69,7 @@ fn test_concurrent_checkout() { &settings, &workspace1_root, &TestRepo::default_store_factories(), + &default_working_copy_factories(), ) .unwrap(); assert_eq!(*ws3.working_copy().tree_id().unwrap(), tree_id2); @@ -114,6 +116,7 @@ fn test_checkout_parallel() { &settings, &workspace_root, &TestRepo::default_store_factories(), + &default_working_copy_factories(), ) .unwrap(); // The operation ID is not correct, but that doesn't matter for this test diff --git a/lib/tests/test_workspace.rs b/lib/tests/test_workspace.rs index a3fd002a4f..db6f5944c4 100644 --- a/lib/tests/test_workspace.rs +++ b/lib/tests/test_workspace.rs @@ -15,7 +15,7 @@ use assert_matches::assert_matches; use jj_lib::op_store::WorkspaceId; use jj_lib::repo::Repo; -use jj_lib::workspace::{Workspace, WorkspaceLoadError}; +use jj_lib::workspace::{default_working_copy_factories, Workspace, WorkspaceLoadError}; use testutils::{TestRepo, TestWorkspace}; #[test] @@ -28,6 +28,7 @@ fn test_load_bad_path() { &settings, &workspace_root, &TestRepo::default_store_factories(), + &default_working_copy_factories(), ); assert_matches!( result.err(), @@ -65,8 +66,12 @@ fn test_init_additional_workspace() { workspace.repo_path().canonicalize().unwrap() ); assert_eq!(*ws2.workspace_root(), ws2_root.canonicalize().unwrap()); - let same_workspace = - Workspace::load(&settings, &ws2_root, &TestRepo::default_store_factories()); + let same_workspace = Workspace::load( + &settings, + &ws2_root, + &TestRepo::default_store_factories(), + &default_working_copy_factories(), + ); assert!(same_workspace.is_ok()); let same_workspace = same_workspace.unwrap(); assert_eq!(same_workspace.workspace_id(), &ws2_id);