From 51022089e1ce3816b618059585942388b5c35fdd Mon Sep 17 00:00:00 2001 From: Martin von Zweigbergk Date: Mon, 16 Oct 2023 08:01:33 -0700 Subject: [PATCH 1/4] repo: define types for backend initializer functions `ReadonlyRepo::init()` takes callbacks for initializing each kind of backend. We called these things like `op_store_initializer`. I found that confusing because it is not a `OpStoreFactory` (which is for loading an existing backend). This patch tries to clarify that by renaming the arguments and adding types for each kind of callback function. --- cli/examples/custom-backend/main.rs | 2 +- lib/src/repo.rs | 42 +++++++++++-------- lib/src/workspace.rs | 64 ++++++++++++++--------------- lib/tests/test_git.rs | 32 +++++++-------- lib/testutils/src/lib.rs | 12 +++--- 5 files changed, 78 insertions(+), 74 deletions(-) diff --git a/cli/examples/custom-backend/main.rs b/cli/examples/custom-backend/main.rs index f834d0398c..b79a0f9413 100644 --- a/cli/examples/custom-backend/main.rs +++ b/cli/examples/custom-backend/main.rs @@ -54,7 +54,7 @@ fn run_custom_command( CustomCommands::InitJit => { let wc_path = command_helper.cwd(); // Initialize a workspace with the custom backend - Workspace::init_with_backend(command_helper.settings(), wc_path, |store_path| { + Workspace::init_with_backend(command_helper.settings(), wc_path, &|store_path| { Ok(Box::new(JitBackend::init(store_path)?)) })?; Ok(()) diff --git a/lib/src/repo.rs b/lib/src/repo.rs index a5d332db6b..c750af1e2b 100644 --- a/lib/src/repo.rs +++ b/lib/src/repo.rs @@ -119,39 +119,39 @@ pub enum RepoInitError { } impl ReadonlyRepo { - pub fn default_op_store_factory() -> impl FnOnce(&Path) -> Box { - |store_path| Box::new(SimpleOpStore::init(store_path)) + pub fn default_op_store_initializer() -> &'static OpStoreInitializer { + &|store_path| Box::new(SimpleOpStore::init(store_path)) } - pub fn default_op_heads_store_factory() -> impl FnOnce(&Path) -> Box { - |store_path| { + pub fn default_op_heads_store_initializer() -> &'static OpHeadsStoreInitializer { + &|store_path| { let store = SimpleOpHeadsStore::init(store_path); Box::new(store) } } - pub fn default_index_store_factory() -> impl FnOnce(&Path) -> Box { - |store_path| Box::new(DefaultIndexStore::init(store_path)) + pub fn default_index_store_initializer() -> &'static IndexStoreInitializer { + &|store_path| Box::new(DefaultIndexStore::init(store_path)) } - pub fn default_submodule_store_factory() -> impl FnOnce(&Path) -> Box { - |store_path| Box::new(DefaultSubmoduleStore::init(store_path)) + pub fn default_submodule_store_initializer() -> &'static SubmoduleStoreInitializer { + &|store_path| Box::new(DefaultSubmoduleStore::init(store_path)) } pub fn init( user_settings: &UserSettings, repo_path: &Path, - backend_factory: impl FnOnce(&Path) -> Result, BackendInitError>, - op_store_factory: impl FnOnce(&Path) -> Box, - op_heads_store_factory: impl FnOnce(&Path) -> Box, - index_store_factory: impl FnOnce(&Path) -> Box, - submodule_store_factory: impl FnOnce(&Path) -> Box, + backend_initializer: &BackendInitializer, + op_store_initializer: &OpStoreInitializer, + op_heads_store_initializer: &OpHeadsStoreInitializer, + index_store_initializer: &IndexStoreInitializer, + submodule_store_initializer: &SubmoduleStoreInitializer, ) -> Result, RepoInitError> { let repo_path = repo_path.canonicalize().context(repo_path)?; let store_path = repo_path.join("store"); fs::create_dir(&store_path).context(&store_path)?; - let backend = backend_factory(&store_path)?; + let backend = backend_initializer(&store_path)?; let backend_path = store_path.join("type"); fs::write(&backend_path, backend.name()).context(&backend_path)?; let store = Store::new(backend, user_settings.use_tree_conflict_format()); @@ -159,7 +159,7 @@ impl ReadonlyRepo { let op_store_path = repo_path.join("op_store"); fs::create_dir(&op_store_path).context(&op_store_path)?; - let op_store = op_store_factory(&op_store_path); + let op_store = op_store_initializer(&op_store_path); let op_store_type_path = op_store_path.join("type"); fs::write(&op_store_type_path, op_store.name()).context(&op_store_type_path)?; let op_store: Arc = Arc::from(op_store); @@ -182,7 +182,7 @@ impl ReadonlyRepo { }; let init_operation_id = op_store.write_operation(&init_operation).unwrap(); let init_operation = Operation::new(op_store.clone(), init_operation_id, init_operation); - let op_heads_store = op_heads_store_factory(&op_heads_path); + let op_heads_store = op_heads_store_initializer(&op_heads_path); op_heads_store.add_op_head(init_operation.id()); let op_heads_type_path = op_heads_path.join("type"); fs::write(&op_heads_type_path, op_heads_store.name()).context(&op_heads_type_path)?; @@ -190,14 +190,14 @@ impl ReadonlyRepo { let index_path = repo_path.join("index"); fs::create_dir(&index_path).context(&index_path)?; - let index_store = index_store_factory(&index_path); + let index_store = index_store_initializer(&index_path); let index_type_path = index_path.join("type"); fs::write(&index_type_path, index_store.name()).context(&index_type_path)?; let index_store = Arc::from(index_store); let submodule_store_path = repo_path.join("submodule_store"); fs::create_dir(&submodule_store_path).context(&submodule_store_path)?; - let submodule_store = submodule_store_factory(&submodule_store_path); + let submodule_store = submodule_store_initializer(&submodule_store_path); let submodule_store_type_path = submodule_store_path.join("type"); fs::write(&submodule_store_type_path, submodule_store.name()) .context(&submodule_store_type_path)?; @@ -341,6 +341,12 @@ impl Repo for ReadonlyRepo { } } +pub type BackendInitializer = dyn Fn(&Path) -> Result, BackendInitError>; +pub type OpStoreInitializer = dyn Fn(&Path) -> Box; +pub type OpHeadsStoreInitializer = dyn Fn(&Path) -> Box; +pub type IndexStoreInitializer = dyn Fn(&Path) -> Box; +pub type SubmoduleStoreInitializer = dyn Fn(&Path) -> Box; + type BackendFactory = Box Result, BackendLoadError>>; type OpStoreFactory = Box Box>; type OpHeadsStoreFactory = Box Box>; diff --git a/lib/src/workspace.rs b/lib/src/workspace.rs index f5f94134e2..40f7d768fa 100644 --- a/lib/src/workspace.rs +++ b/lib/src/workspace.rs @@ -25,18 +25,16 @@ use thiserror::Error; use crate::backend::{Backend, BackendInitError, MergedTreeId}; use crate::file_util::{self, IoResultExt as _, PathError}; use crate::git_backend::GitBackend; -use crate::index::IndexStore; use crate::local_backend::LocalBackend; use crate::local_working_copy::LocalWorkingCopy; use crate::merged_tree::MergedTree; -use crate::op_heads_store::OpHeadsStore; -use crate::op_store::{OpStore, OperationId, WorkspaceId}; +use crate::op_store::{OperationId, WorkspaceId}; use crate::repo::{ - CheckOutCommitError, ReadonlyRepo, Repo, RepoInitError, RepoLoader, StoreFactories, - StoreLoadError, + BackendInitializer, CheckOutCommitError, IndexStoreInitializer, OpHeadsStoreInitializer, + OpStoreInitializer, ReadonlyRepo, Repo, RepoInitError, RepoLoader, StoreFactories, + StoreLoadError, SubmoduleStoreInitializer, }; use crate::settings::UserSettings; -use crate::submodule_store::SubmoduleStore; use crate::working_copy::{ CheckoutError, CheckoutStats, LockedWorkingCopy, WorkingCopy, WorkingCopyStateError, }; @@ -141,9 +139,9 @@ impl Workspace { user_settings: &UserSettings, workspace_root: &Path, ) -> Result<(Self, Arc), WorkspaceInitError> { - Self::init_with_backend(user_settings, workspace_root, |store_path| { - Ok(Box::new(LocalBackend::init(store_path))) - }) + let backend_initializer: &'static BackendInitializer = + &|store_path| Ok(Box::new(LocalBackend::init(store_path))); + Self::init_with_backend(user_settings, workspace_root, backend_initializer) } /// Initializes a workspace with a new Git backend in .jj/git/ (bare Git @@ -152,9 +150,9 @@ impl Workspace { user_settings: &UserSettings, workspace_root: &Path, ) -> Result<(Self, Arc), WorkspaceInitError> { - Self::init_with_backend(user_settings, workspace_root, |store_path| { - Ok(Box::new(GitBackend::init_internal(store_path)?)) - }) + let backend_initializer: &'static BackendInitializer = + &|store_path| Ok(Box::new(GitBackend::init_internal(store_path)?)); + Self::init_with_backend(user_settings, workspace_root, backend_initializer) } /// Initializes a workspace with an existing Git backend at the specified @@ -164,7 +162,9 @@ impl Workspace { workspace_root: &Path, git_repo_path: &Path, ) -> Result<(Self, Arc), WorkspaceInitError> { - Self::init_with_backend(user_settings, workspace_root, |store_path| { + let workspace_root = workspace_root.to_owned(); + let git_repo_path = git_repo_path.to_owned(); + Self::init_with_backend(user_settings, &workspace_root.clone(), &move |store_path| { // If the git repo is inside the workspace, use a relative path to it so the // whole workspace can be moved without breaking. // TODO: Clean up path normalization. store_path is canonicalized by @@ -179,10 +179,8 @@ impl Workspace { } _ => git_repo_path.to_owned(), }; - Ok(Box::new(GitBackend::init_external( - store_path, - &store_relative_git_repo_path, - )?)) + let backend = GitBackend::init_external(store_path, &store_relative_git_repo_path)?; + Ok(Box::new(backend) as Box) }) } @@ -190,11 +188,11 @@ impl Workspace { pub fn init_with_factories( user_settings: &UserSettings, workspace_root: &Path, - backend_factory: impl FnOnce(&Path) -> Result, BackendInitError>, - op_store_factory: impl FnOnce(&Path) -> Box, - op_heads_store_factory: impl FnOnce(&Path) -> Box, - index_store_factory: impl FnOnce(&Path) -> Box, - submodule_store_factory: impl FnOnce(&Path) -> Box, + backend_initializer: &BackendInitializer, + op_store_initializer: &OpStoreInitializer, + op_heads_store_initializer: &OpHeadsStoreInitializer, + index_store_initializer: &IndexStoreInitializer, + submodule_store_initializer: &SubmoduleStoreInitializer, workspace_id: WorkspaceId, ) -> Result<(Self, Arc), WorkspaceInitError> { let jj_dir = create_jj_dir(workspace_root)?; @@ -204,11 +202,11 @@ impl Workspace { let repo = ReadonlyRepo::init( user_settings, &repo_dir, - backend_factory, - op_store_factory, - op_heads_store_factory, - index_store_factory, - submodule_store_factory, + backend_initializer, + op_store_initializer, + op_heads_store_initializer, + index_store_initializer, + submodule_store_initializer, ) .map_err(|repo_init_err| match repo_init_err { RepoInitError::Backend(err) => WorkspaceInitError::Backend(err), @@ -229,16 +227,16 @@ impl Workspace { pub fn init_with_backend( user_settings: &UserSettings, workspace_root: &Path, - backend_factory: impl FnOnce(&Path) -> Result, BackendInitError>, + backend_initializer: &BackendInitializer, ) -> Result<(Self, Arc), WorkspaceInitError> { Self::init_with_factories( user_settings, workspace_root, - backend_factory, - ReadonlyRepo::default_op_store_factory(), - ReadonlyRepo::default_op_heads_store_factory(), - ReadonlyRepo::default_index_store_factory(), - ReadonlyRepo::default_submodule_store_factory(), + backend_initializer, + ReadonlyRepo::default_op_store_initializer(), + ReadonlyRepo::default_op_heads_store_initializer(), + ReadonlyRepo::default_index_store_initializer(), + ReadonlyRepo::default_submodule_store_initializer(), WorkspaceId::default(), ) } diff --git a/lib/tests/test_git.rs b/lib/tests/test_git.rs index 29260fa677..4ae266ed9f 100644 --- a/lib/tests/test_git.rs +++ b/lib/tests/test_git.rs @@ -1032,16 +1032,16 @@ impl GitRepoData { let repo = ReadonlyRepo::init( &settings, &jj_repo_dir, - |store_path| { + &move |store_path| { Ok(Box::new(GitBackend::init_external( store_path, &git_repo_dir, )?)) }, - ReadonlyRepo::default_op_store_factory(), - ReadonlyRepo::default_op_heads_store_factory(), - ReadonlyRepo::default_index_store_factory(), - ReadonlyRepo::default_submodule_store_factory(), + ReadonlyRepo::default_op_store_initializer(), + ReadonlyRepo::default_op_heads_store_initializer(), + ReadonlyRepo::default_index_store_initializer(), + ReadonlyRepo::default_submodule_store_initializer(), ) .unwrap(); Self { @@ -1878,19 +1878,19 @@ fn test_init() { let git_repo = git2::Repository::init_bare(&git_repo_dir).unwrap(); let initial_git_commit = empty_git_commit(&git_repo, "refs/heads/main", &[]); std::fs::create_dir(&jj_repo_dir).unwrap(); - let repo = ReadonlyRepo::init( + let repo = &ReadonlyRepo::init( &settings, &jj_repo_dir, - |store_path| { + &move |store_path| { Ok(Box::new(GitBackend::init_external( store_path, &git_repo_dir, )?)) }, - ReadonlyRepo::default_op_store_factory(), - ReadonlyRepo::default_op_heads_store_factory(), - ReadonlyRepo::default_index_store_factory(), - ReadonlyRepo::default_submodule_store_factory(), + ReadonlyRepo::default_op_store_initializer(), + ReadonlyRepo::default_op_heads_store_initializer(), + ReadonlyRepo::default_index_store_initializer(), + ReadonlyRepo::default_submodule_store_initializer(), ) .unwrap(); // The refs were *not* imported -- it's the caller's responsibility to import @@ -2173,16 +2173,16 @@ fn set_up_push_repos(settings: &UserSettings, temp_dir: &TempDir) -> PushTestSet let jj_repo = ReadonlyRepo::init( settings, &jj_repo_dir, - |store_path| { + &move |store_path| { Ok(Box::new(GitBackend::init_external( store_path, &clone_repo_dir, )?)) }, - ReadonlyRepo::default_op_store_factory(), - ReadonlyRepo::default_op_heads_store_factory(), - ReadonlyRepo::default_index_store_factory(), - ReadonlyRepo::default_submodule_store_factory(), + ReadonlyRepo::default_op_store_initializer(), + ReadonlyRepo::default_op_heads_store_initializer(), + ReadonlyRepo::default_index_store_initializer(), + ReadonlyRepo::default_submodule_store_initializer(), ) .unwrap(); get_git_backend(&jj_repo) diff --git a/lib/testutils/src/lib.rs b/lib/testutils/src/lib.rs index 7606808a6a..d8d4569112 100644 --- a/lib/testutils/src/lib.rs +++ b/lib/testutils/src/lib.rs @@ -120,13 +120,13 @@ impl TestRepo { let repo = ReadonlyRepo::init( &settings, &repo_dir, - |store_path| -> Result, BackendInitError> { + &move |store_path| -> Result, BackendInitError> { backend.init_backend(store_path) }, - ReadonlyRepo::default_op_store_factory(), - ReadonlyRepo::default_op_heads_store_factory(), - ReadonlyRepo::default_index_store_factory(), - ReadonlyRepo::default_submodule_store_factory(), + ReadonlyRepo::default_op_store_initializer(), + ReadonlyRepo::default_op_heads_store_initializer(), + ReadonlyRepo::default_index_store_initializer(), + ReadonlyRepo::default_submodule_store_initializer(), ) .unwrap(); @@ -165,7 +165,7 @@ impl TestWorkspace { fs::create_dir(&workspace_root).unwrap(); let (workspace, repo) = - Workspace::init_with_backend(settings, &workspace_root, |store_path| { + Workspace::init_with_backend(settings, &workspace_root, &move |store_path| { backend.init_backend(store_path) }) .unwrap(); From eebf051b2ff7b82bd9456209b88932e3aff09ca5 Mon Sep 17 00:00:00 2001 From: Martin von Zweigbergk Date: Mon, 16 Oct 2023 10:21:54 -0700 Subject: [PATCH 2/4] working copy: pass commit instead of tree into `check_out()` Our internal working copy implementations at Google will need the commit so they can walk history backwards until they get to a "public" commit. They'll then use that to tell build tools and virtual file systems to present that as a base. I'm not sure if we'll need to update `reset()` too. It's currently only used by `jj untrack`, which doesn't change the commit's parent, so it wouldn't affect any history walks. --- cli/src/cli_util.rs | 3 +- cli/src/commands/mod.rs | 3 +- lib/src/local_working_copy.rs | 6 ++- lib/src/working_copy.rs | 6 +-- lib/src/workspace.rs | 6 +-- lib/tests/test_local_working_copy.rs | 52 ++++++++++++------- .../test_local_working_copy_concurrent.rs | 26 +++++----- lib/tests/test_local_working_copy_sparse.rs | 8 +-- lib/testutils/src/lib.rs | 28 +++++++++- 9 files changed, 88 insertions(+), 50 deletions(-) diff --git a/cli/src/cli_util.rs b/cli/src/cli_util.rs index ef82fdda6d..f3e1c21aae 100644 --- a/cli/src/cli_util.rs +++ b/cli/src/cli_util.rs @@ -2026,9 +2026,8 @@ pub fn update_working_copy( let stats = if Some(new_commit.tree_id()) != old_tree_id.as_ref() { // TODO: CheckoutError::ConcurrentCheckout should probably just result in a // warning for most commands (but be an error for the checkout command) - let new_tree = new_commit.tree()?; let stats = workspace - .check_out(repo.op_id().clone(), old_tree_id.as_ref(), &new_tree) + .check_out(repo.op_id().clone(), old_tree_id.as_ref(), new_commit) .map_err(|err| { CommandError::InternalError(format!( "Failed to check out commit {}: {}", diff --git a/cli/src/commands/mod.rs b/cli/src/commands/mod.rs index 8101f80830..a57498c22d 100644 --- a/cli/src/commands/mod.rs +++ b/cli/src/commands/mod.rs @@ -3911,10 +3911,9 @@ fn cmd_workspace_update_stale( if known_wc_commit.tree_id() != locked_ws.locked_wc().old_tree_id() { return Err(user_error("Concurrent working copy operation. Try again.")); } - let desired_tree = desired_wc_commit.tree()?; let stats = locked_ws .locked_wc() - .check_out(&desired_tree) + .check_out(&desired_wc_commit) .map_err(|err| { CommandError::InternalError(format!( "Failed to check out commit {}: {}", diff --git a/lib/src/local_working_copy.rs b/lib/src/local_working_copy.rs index fb8402f8df..a757b7c5ed 100644 --- a/lib/src/local_working_copy.rs +++ b/lib/src/local_working_copy.rs @@ -42,6 +42,7 @@ use tracing::{instrument, trace_span}; use crate::backend::{ BackendError, FileId, MergedTreeId, MillisSinceEpoch, ObjectId, SymlinkId, TreeId, TreeValue, }; +use crate::commit::Commit; use crate::conflicts; #[cfg(feature = "watchman")] use crate::fsmonitor::watchman; @@ -1514,9 +1515,10 @@ impl LockedWorkingCopy for LockedLocalWorkingCopy { Ok(tree_state.current_tree_id().clone()) } - fn check_out(&mut self, new_tree: &MergedTree) -> Result { + fn check_out(&mut self, commit: &Commit) -> Result { // TODO: Write a "pending_checkout" file with the new TreeId so we can // continue an interrupted update if we find such a file. + let new_tree = commit.tree()?; let stats = self .wc .tree_state_mut() @@ -1524,7 +1526,7 @@ impl LockedWorkingCopy for LockedLocalWorkingCopy { message: "Failed to load the working copy state".to_string(), err: err.into(), })? - .check_out(new_tree)?; + .check_out(&new_tree)?; self.tree_state_dirty = true; Ok(stats) } diff --git a/lib/src/working_copy.rs b/lib/src/working_copy.rs index 95111b5f2a..ae9cc637b1 100644 --- a/lib/src/working_copy.rs +++ b/lib/src/working_copy.rs @@ -23,6 +23,7 @@ use std::sync::Arc; use thiserror::Error; use crate::backend::{BackendError, MergedTreeId}; +use crate::commit::Commit; use crate::fsmonitor::FsmonitorKind; use crate::gitignore::GitIgnoreFile; use crate::merged_tree::MergedTree; @@ -79,9 +80,8 @@ pub trait LockedWorkingCopy { /// Snapshot the working copy and return the tree id. fn snapshot(&mut self, options: SnapshotOptions) -> Result; - /// Check out the specified tree in the working copy. - // TODO: Pass a Commit object here because some implementations need that. - fn check_out(&mut self, new_tree: &MergedTree) -> Result; + /// Check out the specified commit in the working copy. + fn check_out(&mut self, commit: &Commit) -> Result; /// Update to another tree without touching the files in the working copy. fn reset(&mut self, new_tree: &MergedTree) -> Result<(), ResetError>; diff --git a/lib/src/workspace.rs b/lib/src/workspace.rs index 40f7d768fa..6be8145252 100644 --- a/lib/src/workspace.rs +++ b/lib/src/workspace.rs @@ -23,11 +23,11 @@ use std::sync::Arc; use thiserror::Error; use crate::backend::{Backend, BackendInitError, MergedTreeId}; +use crate::commit::Commit; use crate::file_util::{self, IoResultExt as _, PathError}; use crate::git_backend::GitBackend; use crate::local_backend::LocalBackend; use crate::local_working_copy::LocalWorkingCopy; -use crate::merged_tree::MergedTree; use crate::op_store::{OperationId, WorkspaceId}; use crate::repo::{ BackendInitializer, CheckOutCommitError, IndexStoreInitializer, OpHeadsStoreInitializer, @@ -311,7 +311,7 @@ impl Workspace { &mut self, operation_id: OperationId, old_tree_id: Option<&MergedTreeId>, - new_tree: &MergedTree, + commit: &Commit, ) -> Result { let mut locked_ws = self.start_working_copy_mutation() @@ -328,7 +328,7 @@ impl Workspace { return Err(CheckoutError::ConcurrentCheckout); } } - let stats = locked_ws.locked_wc().check_out(new_tree)?; + let stats = locked_ws.locked_wc().check_out(commit)?; locked_ws .finish(operation_id) .map_err(|err| CheckoutError::Other { diff --git a/lib/tests/test_local_working_copy.rs b/lib/tests/test_local_working_copy.rs index 2247673cac..928b02f8a4 100644 --- a/lib/tests/test_local_working_copy.rs +++ b/lib/tests/test_local_working_copy.rs @@ -29,7 +29,7 @@ use jj_lib::backend::{MergedTreeId, ObjectId, TreeId, TreeValue}; use jj_lib::fsmonitor::FsmonitorKind; use jj_lib::local_working_copy::LocalWorkingCopy; use jj_lib::merge::Merge; -use jj_lib::merged_tree::{MergedTree, MergedTreeBuilder}; +use jj_lib::merged_tree::MergedTreeBuilder; use jj_lib::op_store::{OperationId, WorkspaceId}; use jj_lib::repo::{ReadonlyRepo, Repo}; use jj_lib::repo_path::{RepoPath, RepoPathComponent, RepoPathJoin}; @@ -37,7 +37,9 @@ use jj_lib::settings::UserSettings; use jj_lib::working_copy::{CheckoutStats, SnapshotError, SnapshotOptions}; use jj_lib::workspace::LockedWorkspace; use test_case::test_case; -use testutils::{create_tree, write_random_commit, TestRepoBackend, TestWorkspace}; +use testutils::{ + commit_with_tree, create_tree, write_random_commit, TestRepoBackend, TestWorkspace, +}; #[test] fn test_root() { @@ -187,13 +189,13 @@ fn test_checkout_file_transitions(backend: TestRepoBackend) { } let left_tree_id = left_tree_builder.write_tree(&store).unwrap(); let right_tree_id = right_tree_builder.write_tree(&store).unwrap(); - let left_tree = store.get_root_tree(&left_tree_id).unwrap(); - let right_tree = store.get_root_tree(&right_tree_id).unwrap(); + let left_commit = commit_with_tree(&store, left_tree_id); + let right_commit = commit_with_tree(&store, right_tree_id.clone()); let ws = &mut test_workspace.workspace; - ws.check_out(repo.op_id().clone(), None, &left_tree) + ws.check_out(repo.op_id().clone(), None, &left_commit) .unwrap(); - ws.check_out(repo.op_id().clone(), None, &right_tree) + ws.check_out(repo.op_id().clone(), None, &right_commit) .unwrap(); // Check that the working copy is clean. @@ -273,10 +275,12 @@ fn test_conflict_subdirectory() { let tree1 = create_tree(repo, &[(&path, "0")]); let tree2 = create_tree(repo, &[(&path, "1")]); let merged_tree = tree1.merge(&empty_tree, &tree2).unwrap(); + let commit1 = commit_with_tree(repo.store(), tree1.id()); + let merged_commit = commit_with_tree(repo.store(), merged_tree.id()); let repo = &test_workspace.repo; let ws = &mut test_workspace.workspace; - ws.check_out(repo.op_id().clone(), None, &tree1).unwrap(); - ws.check_out(repo.op_id().clone(), None, &merged_tree) + ws.check_out(repo.op_id().clone(), None, &commit1).unwrap(); + ws.check_out(repo.op_id().clone(), None, &merged_commit) .unwrap(); } @@ -290,8 +294,8 @@ fn test_tree_builder_file_directory_transition() { let workspace_root = ws.workspace_root().clone(); let mut check_out_tree = |tree_id: &TreeId| { let tree = repo.store().get_tree(&RepoPath::root(), tree_id).unwrap(); - ws.check_out(repo.op_id().clone(), None, &MergedTree::legacy(tree)) - .unwrap(); + let commit = commit_with_tree(repo.store(), MergedTreeId::Legacy(tree.id().clone())); + ws.check_out(repo.op_id().clone(), None, &commit).unwrap(); }; let parent_path = RepoPath::from_internal_string("foo/bar"); @@ -349,6 +353,7 @@ fn test_conflicting_changes_on_disk() { (&dir_file_path, "committed contents"), ], ); + let commit = commit_with_tree(repo.store(), tree.id()); std::fs::write( file_file_path.to_fs_path(&workspace_root), @@ -367,7 +372,7 @@ fn test_conflicting_changes_on_disk() { ) .unwrap(); - let stats = ws.check_out(repo.op_id().clone(), None, &tree).unwrap(); + let stats = ws.check_out(repo.op_id().clone(), None, &commit).unwrap(); assert_eq!( stats, CheckoutStats { @@ -410,8 +415,8 @@ fn test_reset() { ); let ws = &mut test_workspace.workspace; - ws.check_out(repo.op_id().clone(), None, &tree_with_file) - .unwrap(); + let commit = commit_with_tree(repo.store(), tree_with_file.id()); + ws.check_out(repo.op_id().clone(), None, &commit).unwrap(); // Test the setup: the file should exist on disk and in the tree state. assert!(ignored_path.to_fs_path(&workspace_root).is_file()); @@ -459,9 +464,11 @@ fn test_checkout_discard() { let store = repo.store(); let tree1 = create_tree(&repo, &[(&file1_path, "contents")]); let tree2 = create_tree(&repo, &[(&file2_path, "contents")]); + 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, &tree1).unwrap(); + ws.check_out(repo.op_id().clone(), None, &commit1).unwrap(); let wc: &LocalWorkingCopy = ws.working_copy().as_any().downcast_ref().unwrap(); let state_path = wc.state_path().to_path_buf(); @@ -472,7 +479,7 @@ fn test_checkout_discard() { // Start a checkout let mut locked_ws = ws.start_working_copy_mutation().unwrap(); - locked_ws.locked_wc().check_out(&tree2).unwrap(); + locked_ws.locked_wc().check_out(&commit2).unwrap(); // The change should be reflected in the working copy but not saved assert!(!file1_path.to_fs_path(&workspace_root).is_file()); assert!(file2_path.to_fs_path(&workspace_root).is_file()); @@ -661,8 +668,9 @@ fn test_gitignores_in_ignored_dir() { let ignored_path = RepoPath::from_internal_string("ignored/file"); let tree1 = create_tree(&test_workspace.repo, &[(&gitignore_path, "ignored\n")]); + let commit1 = commit_with_tree(test_workspace.repo.store(), tree1.id()); let ws = &mut test_workspace.workspace; - ws.check_out(op_id.clone(), None, &tree1).unwrap(); + ws.check_out(op_id.clone(), None, &commit1).unwrap(); testutils::write_working_copy_file(&workspace_root, &nested_gitignore_path, "!file\n"); testutils::write_working_copy_file(&workspace_root, &ignored_path, "contents"); @@ -713,12 +721,13 @@ fn test_gitignores_checkout_never_overwrites_ignored() { // Create a tree that adds the same file but with different contents let tree = create_tree(repo, &[(&modified_path, "contents")]); + let commit = commit_with_tree(repo.store(), tree.id()); // Now check out the tree that adds the file "modified" with contents // "contents". The exiting contents ("garbage") shouldn't be replaced in the // working copy. let ws = &mut test_workspace.workspace; - assert!(ws.check_out(repo.op_id().clone(), None, &tree).is_ok()); + assert!(ws.check_out(repo.op_id().clone(), None, &commit).is_ok()); // Check that the old contents are in the working copy let path = workspace_root.join("modified"); @@ -749,10 +758,11 @@ fn test_gitignores_ignored_directory_already_tracked() { (&deleted_path, "contents"), ], ); + let commit = commit_with_tree(repo.store(), tree.id()); // Check out the tree with the files in `ignored/` let ws = &mut test_workspace.workspace; - ws.check_out(repo.op_id().clone(), None, &tree).unwrap(); + ws.check_out(repo.op_id().clone(), None, &commit).unwrap(); // Make some changes inside the ignored directory and check that they are // detected when we snapshot. The files that are still there should not be @@ -843,8 +853,9 @@ fn test_gitsubmodule() { let tree_id = MergedTreeId::Legacy(tree_builder.write_tree()); let tree = store.get_root_tree(&tree_id).unwrap(); + let commit = commit_with_tree(repo.store(), tree.id()); let ws = &mut test_workspace.workspace; - ws.check_out(repo.op_id().clone(), None, &tree).unwrap(); + ws.check_out(repo.op_id().clone(), None, &commit).unwrap(); std::fs::create_dir(submodule_path.to_fs_path(&workspace_root)).unwrap(); @@ -880,10 +891,11 @@ fn test_existing_directory_symlink() { std::os::unix::fs::symlink("..", workspace_root.join("parent")).unwrap(); let file_path = RepoPath::from_internal_string("parent/escaped"); let tree = create_tree(repo, &[(&file_path, "contents")]); + let commit = commit_with_tree(repo.store(), tree.id()); // Checkout should fail because "parent" already exists and is a symlink. let ws = &mut test_workspace.workspace; - assert!(ws.check_out(repo.op_id().clone(), None, &tree).is_err()); + assert!(ws.check_out(repo.op_id().clone(), None, &commit).is_err()); // Therefore, "../escaped" shouldn't be created. assert!(!workspace_root.parent().unwrap().join("escaped").exists()); diff --git a/lib/tests/test_local_working_copy_concurrent.rs b/lib/tests/test_local_working_copy_concurrent.rs index 0cdce16cc5..a1d41d0a85 100644 --- a/lib/tests/test_local_working_copy_concurrent.rs +++ b/lib/tests/test_local_working_copy_concurrent.rs @@ -20,7 +20,7 @@ use jj_lib::repo::Repo; use jj_lib::repo_path::RepoPath; use jj_lib::working_copy::{CheckoutError, SnapshotOptions}; use jj_lib::workspace::Workspace; -use testutils::{create_tree, write_working_copy_file, TestRepo, TestWorkspace}; +use testutils::{commit_with_tree, create_tree, write_working_copy_file, TestRepo, TestWorkspace}; #[test] fn test_concurrent_checkout() { @@ -37,11 +37,14 @@ fn test_concurrent_checkout() { let tree1 = repo.store().get_root_tree(&tree_id1).unwrap(); let tree2 = repo.store().get_root_tree(&tree_id2).unwrap(); let tree3 = repo.store().get_root_tree(&tree_id3).unwrap(); + let commit1 = commit_with_tree(repo.store(), tree1.id()); + let commit2 = commit_with_tree(repo.store(), tree2.id()); + let commit3 = commit_with_tree(repo.store(), tree3.id()); // Check out tree1 let ws1 = &mut test_workspace1.workspace; // The operation ID is not correct, but that doesn't matter for this test - ws1.check_out(repo.op_id().clone(), None, &tree1).unwrap(); + ws1.check_out(repo.op_id().clone(), None, &commit1).unwrap(); // Check out tree2 from another process (simulated by another workspace // instance) @@ -51,12 +54,12 @@ fn test_concurrent_checkout() { &TestRepo::default_store_factories(), ) .unwrap(); - ws2.check_out(repo.op_id().clone(), Some(&tree_id1), &tree2) + ws2.check_out(repo.op_id().clone(), Some(&tree_id1), &commit2) .unwrap(); // Checking out another tree (via the first workspace instance) should now fail. assert_matches!( - ws1.check_out(repo.op_id().clone(), Some(&tree_id1), &tree3), + ws1.check_out(repo.op_id().clone(), Some(&tree_id1), &commit3), Err(CheckoutError::ConcurrentCheckout) ); @@ -93,16 +96,17 @@ fn test_checkout_parallel() { repo, &[(&RepoPath::from_internal_string("other file"), "contents")], ); + let commit = commit_with_tree(repo.store(), tree.id()); test_workspace .workspace - .check_out(repo.op_id().clone(), None, &tree) + .check_out(repo.op_id().clone(), None, &commit) .unwrap(); thread::scope(|s| { for tree_id in &tree_ids { let op_id = repo.op_id().clone(); let tree_ids = tree_ids.clone(); - let tree_id = tree_id.clone(); + let commit = commit_with_tree(repo.store(), tree_id.clone()); let settings = settings.clone(); let workspace_root = workspace_root.clone(); s.spawn(move || { @@ -112,13 +116,8 @@ fn test_checkout_parallel() { &TestRepo::default_store_factories(), ) .unwrap(); - let tree = workspace - .repo_loader() - .store() - .get_root_tree(&tree_id) - .unwrap(); // The operation ID is not correct, but that doesn't matter for this test - let stats = workspace.check_out(op_id, None, &tree).unwrap(); + let stats = workspace.check_out(op_id, None, &commit).unwrap(); assert_eq!(stats.updated_files, 0); assert_eq!(stats.added_files, 1); assert_eq!(stats.removed_files, 1); @@ -147,11 +146,12 @@ fn test_racy_checkout() { let path = RepoPath::from_internal_string("file"); let tree = create_tree(repo, &[(&path, "1")]); + let commit = commit_with_tree(repo.store(), tree.id()); let mut num_matches = 0; for _ in 0..100 { let ws = &mut test_workspace.workspace; - ws.check_out(op_id.clone(), None, &tree).unwrap(); + ws.check_out(op_id.clone(), None, &commit).unwrap(); assert_eq!( std::fs::read(path.to_fs_path(&workspace_root)).unwrap(), b"1".to_vec() diff --git a/lib/tests/test_local_working_copy_sparse.rs b/lib/tests/test_local_working_copy_sparse.rs index d7e4ead8f1..cb71aa69a2 100644 --- a/lib/tests/test_local_working_copy_sparse.rs +++ b/lib/tests/test_local_working_copy_sparse.rs @@ -18,7 +18,7 @@ use jj_lib::matchers::EverythingMatcher; use jj_lib::repo::Repo; use jj_lib::repo_path::RepoPath; use jj_lib::working_copy::{CheckoutStats, WorkingCopy}; -use testutils::{create_tree, TestWorkspace}; +use testutils::{commit_with_tree, create_tree, TestWorkspace}; #[test] fn test_sparse_checkout() { @@ -48,10 +48,11 @@ fn test_sparse_checkout() { (&dir2_file1_path, "contents"), ], ); + let commit = commit_with_tree(repo.store(), tree.id()); test_workspace .workspace - .check_out(repo.op_id().clone(), None, &tree) + .check_out(repo.op_id().clone(), None, &commit) .unwrap(); let ws = &mut test_workspace.workspace; @@ -161,9 +162,10 @@ fn test_sparse_commit() { ], ); + let commit = commit_with_tree(repo.store(), tree.id()); test_workspace .workspace - .check_out(repo.op_id().clone(), None, &tree) + .check_out(repo.op_id().clone(), None, &commit) .unwrap(); // Set sparse patterns to only dir1/ diff --git a/lib/testutils/src/lib.rs b/lib/testutils/src/lib.rs index d8d4569112..93c8993566 100644 --- a/lib/testutils/src/lib.rs +++ b/lib/testutils/src/lib.rs @@ -18,7 +18,10 @@ use std::path::{Path, PathBuf}; use std::sync::{Arc, Once}; use itertools::Itertools; -use jj_lib::backend::{Backend, BackendInitError, FileId, MergedTreeId, ObjectId, TreeValue}; +use jj_lib::backend::{ + self, Backend, BackendInitError, ChangeId, FileId, MergedTreeId, MillisSinceEpoch, ObjectId, + Signature, Timestamp, TreeValue, +}; use jj_lib::commit::Commit; use jj_lib::commit_builder::CommitBuilder; use jj_lib::git_backend::GitBackend; @@ -120,7 +123,7 @@ impl TestRepo { let repo = ReadonlyRepo::init( &settings, &repo_dir, - &move |store_path| -> Result, BackendInitError> { + &move |store_path: &Path| -> Result, BackendInitError> { backend.init_backend(store_path) }, ReadonlyRepo::default_op_store_initializer(), @@ -300,6 +303,27 @@ pub fn create_random_commit<'repo>( .set_description(format!("random commit {number}")) } +pub fn commit_with_tree(store: &Arc, tree_id: MergedTreeId) -> Commit { + let signature = Signature { + name: "Some One".to_string(), + email: "someone@example.com".to_string(), + timestamp: Timestamp { + timestamp: MillisSinceEpoch(0), + tz_offset: 0, + }, + }; + let commit = backend::Commit { + parents: vec![store.root_commit_id().clone()], + predecessors: vec![], + root_tree: tree_id, + change_id: ChangeId::from_hex("abcd"), + description: "description".to_string(), + author: signature.clone(), + committer: signature, + }; + store.write_commit(commit).unwrap() +} + pub fn dump_tree(store: &Arc, tree_id: &MergedTreeId) -> String { use std::fmt::Write; let mut buf = String::new(); From c83fe099034a012d3aab3f6c51dfb6431da894c0 Mon Sep 17 00:00:00 2001 From: Martin von Zweigbergk Date: Sat, 14 Oct 2023 05:52:50 -0700 Subject: [PATCH 3/4] 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); From b69326846c08175703e595e37691026f6960f13c Mon Sep 17 00:00:00 2001 From: Martin von Zweigbergk Date: Sun, 15 Oct 2023 16:01:47 -0700 Subject: [PATCH 4/4] workspace: make working-copy type customizable This add support for custom `jj` binaries to use custom working-copy backends. It works in the same way as with the other backends, i.e. we write a `.jj/working_copy/type` file when the working copy is initialized, and then we let that file control which implementation to use (see previous commit). I included an example of a (useless) working-copy implementation. I hope we can figure out a way to test the examples some day. --- cli/examples/custom-working-copy/main.rs | 244 +++++++++++++++++++++++ cli/src/cli_util.rs | 15 +- cli/src/commands/mod.rs | 4 +- lib/src/workspace.rs | 54 ++++- lib/tests/test_workspace.rs | 5 +- 5 files changed, 311 insertions(+), 11 deletions(-) create mode 100644 cli/examples/custom-working-copy/main.rs diff --git a/cli/examples/custom-working-copy/main.rs b/cli/examples/custom-working-copy/main.rs new file mode 100644 index 0000000000..0b32a918e7 --- /dev/null +++ b/cli/examples/custom-working-copy/main.rs @@ -0,0 +1,244 @@ +// Copyright 2023 The Jujutsu Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// https://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +use std::any::Any; +use std::path::{Path, PathBuf}; +use std::sync::Arc; + +use itertools::Itertools; +use jj_cli::cli_util::{CliRunner, CommandError, CommandHelper}; +use jj_cli::ui::Ui; +use jj_lib::backend::{Backend, MergedTreeId}; +use jj_lib::commit::Commit; +use jj_lib::git_backend::GitBackend; +use jj_lib::local_working_copy::LocalWorkingCopy; +use jj_lib::merged_tree::MergedTree; +use jj_lib::op_store::{OperationId, WorkspaceId}; +use jj_lib::repo::ReadonlyRepo; +use jj_lib::repo_path::RepoPath; +use jj_lib::store::Store; +use jj_lib::working_copy::{ + CheckoutError, CheckoutStats, LockedWorkingCopy, ResetError, SnapshotError, SnapshotOptions, + WorkingCopy, WorkingCopyStateError, +}; +use jj_lib::workspace::{default_working_copy_factories, WorkingCopyInitializer, Workspace}; + +#[derive(clap::Parser, Clone, Debug)] +enum CustomCommands { + /// Initialize a workspace using the "conflicts" working copy + InitConflicts, +} + +fn run_custom_command( + _ui: &mut Ui, + command_helper: &CommandHelper, + command: CustomCommands, +) -> Result<(), CommandError> { + match command { + CustomCommands::InitConflicts => { + let wc_path = command_helper.cwd(); + let backend_initializer = |store_path: &Path| { + let backend: Box = Box::new(GitBackend::init_internal(store_path)?); + Ok(backend) + }; + Workspace::init_with_factories( + command_helper.settings(), + wc_path, + &backend_initializer, + &ReadonlyRepo::default_op_store_initializer(), + &ReadonlyRepo::default_op_heads_store_initializer(), + &ReadonlyRepo::default_index_store_initializer(), + &ReadonlyRepo::default_submodule_store_initializer(), + &ConflictsWorkingCopy::initializer(), + WorkspaceId::default(), + )?; + Ok(()) + } + } +} + +fn main() -> std::process::ExitCode { + let mut working_copy_factories = default_working_copy_factories(); + working_copy_factories.insert( + ConflictsWorkingCopy::name().to_owned(), + Box::new(|store, working_copy_path, state_path| { + Box::new(ConflictsWorkingCopy::load( + store.clone(), + working_copy_path.to_owned(), + state_path.to_owned(), + )) + }), + ); + CliRunner::init() + .set_working_copy_factories(working_copy_factories) + .add_subcommand(run_custom_command) + .run() +} + +/// A working copy that adds a .conflicts file with a list of unresolved +/// conflicts. +/// +/// Most functions below just delegate to the inner working-copy backend. The +/// only interesting functions are `snapshot()` and `check_out()`. The former +/// adds `.conflicts` to the .gitignores. The latter writes the `.conflicts` +/// file to the working copy. +struct ConflictsWorkingCopy { + inner: Box, +} + +impl ConflictsWorkingCopy { + fn name() -> &'static str { + "conflicts" + } + + fn init( + store: Arc, + working_copy_path: PathBuf, + state_path: PathBuf, + workspace_id: WorkspaceId, + operation_id: OperationId, + ) -> Result { + let inner = LocalWorkingCopy::init( + store, + working_copy_path, + state_path, + operation_id, + workspace_id, + )?; + Ok(ConflictsWorkingCopy { + inner: Box::new(inner), + }) + } + + fn initializer() -> Box { + Box::new( + |store, working_copy_path, state_path, workspace_id, operation_id| { + let wc = Self::init( + store, + working_copy_path, + state_path, + workspace_id, + operation_id, + )?; + Ok(Box::new(wc)) + }, + ) + } + + fn load(store: Arc, working_copy_path: PathBuf, state_path: PathBuf) -> Self { + let inner = LocalWorkingCopy::load(store, working_copy_path, state_path); + ConflictsWorkingCopy { + inner: Box::new(inner), + } + } +} + +impl WorkingCopy for ConflictsWorkingCopy { + fn as_any(&self) -> &dyn Any { + self + } + + fn name(&self) -> &str { + Self::name() + } + + fn path(&self) -> &Path { + self.inner.path() + } + + fn workspace_id(&self) -> &WorkspaceId { + self.inner.workspace_id() + } + + fn operation_id(&self) -> &OperationId { + self.inner.operation_id() + } + + fn tree_id(&self) -> Result<&MergedTreeId, WorkingCopyStateError> { + self.inner.tree_id() + } + + fn sparse_patterns(&self) -> Result<&[RepoPath], WorkingCopyStateError> { + self.inner.sparse_patterns() + } + + fn start_mutation(&self) -> Result, WorkingCopyStateError> { + let inner = self.inner.start_mutation()?; + Ok(Box::new(LockedConflictsWorkingCopy { + wc_path: self.inner.path().to_owned(), + inner, + })) + } +} + +struct LockedConflictsWorkingCopy { + wc_path: PathBuf, + inner: Box, +} + +impl LockedWorkingCopy for LockedConflictsWorkingCopy { + fn as_any(&self) -> &dyn Any { + self + } + + fn as_any_mut(&mut self) -> &mut dyn Any { + self + } + + fn old_operation_id(&self) -> &OperationId { + self.inner.old_operation_id() + } + + fn old_tree_id(&self) -> &MergedTreeId { + self.inner.old_tree_id() + } + + fn snapshot(&mut self, mut options: SnapshotOptions) -> Result { + options.base_ignores = options.base_ignores.chain("", "/.conflicts".as_bytes()); + self.inner.snapshot(options) + } + + fn check_out(&mut self, commit: &Commit) -> Result { + let conflicts = commit + .tree()? + .conflicts() + .map(|(path, _value)| format!("{}\n", path.to_internal_file_string())) + .join(""); + std::fs::write(self.wc_path.join(".conflicts"), conflicts).unwrap(); + self.inner.check_out(commit) + } + + fn reset(&mut self, new_tree: &MergedTree) -> Result<(), ResetError> { + self.inner.reset(new_tree) + } + + fn sparse_patterns(&self) -> Result<&[RepoPath], WorkingCopyStateError> { + self.inner.sparse_patterns() + } + + fn set_sparse_patterns( + &mut self, + new_sparse_patterns: Vec, + ) -> Result { + self.inner.set_sparse_patterns(new_sparse_patterns) + } + + fn finish( + self: Box, + operation_id: OperationId, + ) -> Result, WorkingCopyStateError> { + let inner = self.inner.finish(operation_id)?; + Ok(Box::new(ConflictsWorkingCopy { inner })) + } +} diff --git a/cli/src/cli_util.rs b/cli/src/cli_util.rs index 60917483f6..3a9e7433ba 100644 --- a/cli/src/cli_util.rs +++ b/cli/src/cli_util.rs @@ -2685,6 +2685,7 @@ pub struct CliRunner { app: Command, extra_configs: Option, store_factories: Option, + working_copy_factories: Option>, dispatch_fn: CliDispatchFn, process_global_args_fns: Vec, } @@ -2704,6 +2705,7 @@ impl CliRunner { app: crate::commands::default_app(), extra_configs: None, store_factories: None, + working_copy_factories: None, dispatch_fn: Box::new(crate::commands::run_command), process_global_args_fns: vec![], } @@ -2727,6 +2729,15 @@ impl CliRunner { self } + /// Replaces working copy factories to be used. + pub fn set_working_copy_factories( + mut self, + working_copy_factories: HashMap, + ) -> Self { + self.working_copy_factories = Some(working_copy_factories); + self + } + /// Registers new subcommands in addition to the default ones. pub fn add_subcommand(mut self, custom_dispatch_fn: F) -> Self where @@ -2797,7 +2808,9 @@ 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 working_copy_factories = self + .working_copy_factories + .unwrap_or_else(|| default_working_copy_factories()); let command_helper = CommandHelper::new( self.app, cwd, diff --git a/cli/src/commands/mod.rs b/cli/src/commands/mod.rs index a57498c22d..d703175da5 100644 --- a/cli/src/commands/mod.rs +++ b/cli/src/commands/mod.rs @@ -48,7 +48,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::SnapshotOptions; -use jj_lib::workspace::Workspace; +use jj_lib::workspace::{default_working_copy_initializer, Workspace}; use jj_lib::{conflicts, file_util, revset}; use maplit::{hashmap, hashset}; use tracing::instrument; @@ -3753,10 +3753,12 @@ fn cmd_workspace_add( "Workspace named '{name}' already exists" ))); } + // TODO: How do we create a workspace with a non-default working copy? let (new_workspace, repo) = Workspace::init_workspace_with_existing_repo( command.settings(), &destination_path, repo, + default_working_copy_initializer(), workspace_id, )?; writeln!( diff --git a/lib/src/workspace.rs b/lib/src/workspace.rs index ef91b47ef1..b699af9587 100644 --- a/lib/src/workspace.rs +++ b/lib/src/workspace.rs @@ -97,6 +97,7 @@ fn init_working_copy( repo: &Arc, workspace_root: &Path, jj_dir: &Path, + working_copy_initializer: &WorkingCopyInitializer, workspace_id: WorkspaceId, ) -> Result<(Box, Arc), WorkspaceInitError> { let working_copy_state_path = jj_dir.join("working_copy"); @@ -113,14 +114,16 @@ fn init_working_copy( )?; let repo = tx.commit(); - let working_copy = LocalWorkingCopy::init( + let working_copy = working_copy_initializer( repo.store().clone(), workspace_root.to_path_buf(), - working_copy_state_path, - repo.op_id().clone(), + working_copy_state_path.clone(), workspace_id, + repo.op_id().clone(), )?; - Ok((Box::new(working_copy), repo)) + let working_copy_type_path = working_copy_state_path.join("type"); + fs::write(&working_copy_type_path, working_copy.name()).context(&working_copy_type_path)?; + Ok((working_copy, repo)) } impl Workspace { @@ -195,6 +198,7 @@ impl Workspace { op_heads_store_initializer: &OpHeadsStoreInitializer, index_store_initializer: &IndexStoreInitializer, submodule_store_initializer: &SubmoduleStoreInitializer, + working_copy_initializer: &WorkingCopyInitializer, workspace_id: WorkspaceId, ) -> Result<(Self, Arc), WorkspaceInitError> { let jj_dir = create_jj_dir(workspace_root)?; @@ -214,8 +218,14 @@ impl Workspace { RepoInitError::Backend(err) => WorkspaceInitError::Backend(err), RepoInitError::Path(err) => WorkspaceInitError::Path(err), })?; - let (working_copy, repo) = - init_working_copy(user_settings, &repo, workspace_root, &jj_dir, workspace_id)?; + let (working_copy, repo) = init_working_copy( + user_settings, + &repo, + workspace_root, + &jj_dir, + working_copy_initializer, + workspace_id, + )?; let repo_loader = repo.loader(); let workspace = Workspace::new(workspace_root, working_copy, repo_loader)?; Ok((workspace, repo)) @@ -239,6 +249,7 @@ impl Workspace { ReadonlyRepo::default_op_heads_store_initializer(), ReadonlyRepo::default_index_store_initializer(), ReadonlyRepo::default_submodule_store_initializer(), + default_working_copy_initializer(), WorkspaceId::default(), ) } @@ -247,6 +258,7 @@ impl Workspace { user_settings: &UserSettings, workspace_root: &Path, repo: &Arc, + working_copy_initializer: &WorkingCopyInitializer, workspace_id: WorkspaceId, ) -> Result<(Self, Arc), WorkspaceInitError> { let jj_dir = create_jj_dir(workspace_root)?; @@ -263,8 +275,14 @@ impl Workspace { ) .context(&repo_file_path)?; - let (working_copy, repo) = - init_working_copy(user_settings, repo, workspace_root, &jj_dir, workspace_id)?; + let (working_copy, repo) = init_working_copy( + user_settings, + repo, + workspace_root, + &jj_dir, + working_copy_initializer, + workspace_id, + )?; let workspace = Workspace::new(workspace_root, working_copy, repo.loader())?; Ok((workspace, repo)) } @@ -441,6 +459,19 @@ impl WorkspaceLoader { } } +pub fn default_working_copy_initializer() -> &'static WorkingCopyInitializer { + &|store: Arc, working_copy_path, state_path, workspace_id, operation_id| { + let wc = LocalWorkingCopy::init( + store, + working_copy_path, + state_path, + operation_id, + workspace_id, + )?; + Ok(Box::new(wc)) + } +} + pub fn default_working_copy_factories() -> HashMap { let mut factories: HashMap = HashMap::new(); factories.insert( @@ -456,4 +487,11 @@ pub fn default_working_copy_factories() -> HashMap { factories } +pub type WorkingCopyInitializer = dyn Fn( + Arc, + PathBuf, + PathBuf, + WorkspaceId, + OperationId, +) -> Result, WorkingCopyStateError>; pub type WorkingCopyFactory = Box, &Path, &Path) -> Box>; diff --git a/lib/tests/test_workspace.rs b/lib/tests/test_workspace.rs index db6f5944c4..cb5d8767f4 100644 --- a/lib/tests/test_workspace.rs +++ b/lib/tests/test_workspace.rs @@ -15,7 +15,9 @@ use assert_matches::assert_matches; use jj_lib::op_store::WorkspaceId; use jj_lib::repo::Repo; -use jj_lib::workspace::{default_working_copy_factories, Workspace, WorkspaceLoadError}; +use jj_lib::workspace::{ + default_working_copy_factories, default_working_copy_initializer, Workspace, WorkspaceLoadError, +}; use testutils::{TestRepo, TestWorkspace}; #[test] @@ -49,6 +51,7 @@ fn test_init_additional_workspace() { &settings, &ws2_root, &test_workspace.repo, + default_working_copy_initializer(), ws2_id.clone(), ) .unwrap();