ok/jj
1
0
Fork 0
forked from mirrors/jj
Code Pull requests Activity

working copy: move LocalWorkingCopy::check_out() to Workspace

Browse source 
`LocalWorkingCopy::check_out()` can be expressed using the planned
`WorkingCopy` trait, so it doesn't need to be in the trait itself
`WorkingCopy`. I wasn't sure if I should make it a free function in
`working_copy`, but I ended up moving it onto `Workspace`.
This commit is contained in:
Martin von Zweigbergk 2023-10-13 14:46:26 -07:00 committed by Martin von Zweigbergk
parent 9fb5a4bcef
commit 35f9c12cb5
6 changed files with 90 additions and 86 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

8
cli/src/cli_util.rs
View file

@ -1392,7 +1392,7 @@ See https://github.com/martinvonz/jj/blob/main/docs/working-copy.md#stale-workin
assert!(self.may_update_working_copy);
let stats = update_working_copy(
&self.user_repo.repo,
self.workspace.working_copy_mut(),
&mut self.workspace,
maybe_old_commit,
new_commit,
)?;
@ -2015,7 +2015,7 @@ pub fn resolve_multiple_nonempty_revsets_default_single(
pub fn update_working_copy(
repo: &Arc<ReadonlyRepo>,
wc: &mut LocalWorkingCopy,
workspace: &mut Workspace,
old_commit: Option<&Commit>,
new_commit: &Commit,
) -> Result<Option<CheckoutStats>, CommandError> {
@ -2024,7 +2024,7 @@ pub fn update_working_copy(
// 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 = wc
let stats = workspace
.check_out(repo.op_id().clone(), old_tree_id.as_ref(), &new_tree)
.map_err(|err| {
CommandError::InternalError(format!(
@ -2036,7 +2036,7 @@ pub fn update_working_copy(
Some(stats)
} else {
// Record new operation id which represents the latest working-copy state
let locked_wc = wc.start_mutation()?;
let locked_wc = workspace.working_copy_mut().start_mutation()?;
locked_wc.finish(repo.op_id().clone())?;
None
};

29
lib/src/local_working_copy.rs
View file

@ -1553,35 +1553,6 @@ impl LocalWorkingCopy {
})
}
pub fn check_out(
&mut self,
operation_id: OperationId,
old_tree_id: Option<&MergedTreeId>,
new_tree: &MergedTree,
) -> Result<CheckoutStats, CheckoutError> {
let mut locked_wc = self.start_mutation().map_err(|err| CheckoutError::Other {
message: "Failed to start editing the working copy state".to_string(),
err: err.into(),
})?;
// Check if the current working-copy commit has changed on disk compared to what
// the caller expected. It's safe to check out another commit
// regardless, but it's probably not what the caller wanted, so we let
// them know.
if let Some(old_tree_id) = old_tree_id {
if *old_tree_id != locked_wc.old_tree_id {
return Err(CheckoutError::ConcurrentCheckout);
}
}
let stats = locked_wc.check_out(new_tree)?;
locked_wc
.finish(operation_id)
.map_err(|err| CheckoutError::Other {
message: "Failed to save the working copy state".to_string(),
err: err.into(),
})?;
Ok(stats)
}
#[cfg(feature = "watchman")]
pub fn query_watchman(
&self,

41
lib/src/workspace.rs
View file

@ -22,14 +22,17 @@ use std::sync::Arc;
use thiserror::Error;
use crate::backend::{Backend, BackendInitError};
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, WorkingCopyStateError};
use crate::local_working_copy::{
CheckoutError, CheckoutStats, LocalWorkingCopy, WorkingCopyStateError,
};
use crate::merged_tree::MergedTree;
use crate::op_heads_store::OpHeadsStore;
use crate::op_store::{OpStore, WorkspaceId};
use crate::op_store::{OpStore, OperationId, WorkspaceId};
use crate::repo::{
CheckOutCommitError, ReadonlyRepo, Repo, RepoInitError, RepoLoader, StoreFactories,
StoreLoadError,
@ -299,6 +302,38 @@ impl Workspace {
pub fn working_copy_mut(&mut self) -> &mut LocalWorkingCopy {
&mut self.working_copy
}
pub fn check_out(
&mut self,
operation_id: OperationId,
old_tree_id: Option<&MergedTreeId>,
new_tree: &MergedTree,
) -> Result<CheckoutStats, CheckoutError> {
let mut locked_wc =
self.working_copy
.start_mutation()
.map_err(|err| CheckoutError::Other {
message: "Failed to start editing the working copy state".to_string(),
err: err.into(),
})?;
// Check if the current working-copy commit has changed on disk compared to what
// the caller expected. It's safe to check out another commit
// regardless, but it's probably not what the caller wanted, so we let
// them know.
if let Some(old_tree_id) = old_tree_id {
if old_tree_id != locked_wc.old_tree_id() {
return Err(CheckoutError::ConcurrentCheckout);
}
}
let stats = locked_wc.check_out(new_tree)?;
locked_wc
.finish(operation_id)
.map_err(|err| CheckoutError::Other {
message: "Failed to save the working copy state".to_string(),
err: err.into(),
})?;
Ok(stats)
}
}
#[derive(Clone, Debug)]

59
lib/tests/test_local_working_copy.rs
View file

@ -190,10 +190,10 @@ fn test_checkout_file_transitions(backend: TestRepoBackend) {
let left_tree = store.get_root_tree(&left_tree_id).unwrap();
let right_tree = store.get_root_tree(&right_tree_id).unwrap();
let wc = test_workspace.workspace.working_copy_mut();
wc.check_out(repo.op_id().clone(), None, &left_tree)
let ws = &mut test_workspace.workspace;
ws.check_out(repo.op_id().clone(), None, &left_tree)
.unwrap();
wc.check_out(repo.op_id().clone(), None, &right_tree)
ws.check_out(repo.op_id().clone(), None, &right_tree)
.unwrap();
// Check that the working copy is clean.
@ -274,9 +274,9 @@ fn test_conflict_subdirectory() {
let tree2 = create_tree(repo, &[(&path, "1")]);
let merged_tree = tree1.merge(&empty_tree, &tree2).unwrap();
let repo = &test_workspace.repo;
let wc = test_workspace.workspace.working_copy_mut();
wc.check_out(repo.op_id().clone(), None, &tree1).unwrap();
wc.check_out(repo.op_id().clone(), None, &merged_tree)
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)
.unwrap();
}
@ -286,12 +286,11 @@ fn test_tree_builder_file_directory_transition() {
let test_workspace = TestWorkspace::init(&settings);
let repo = &test_workspace.repo;
let store = repo.store();
let mut workspace = test_workspace.workspace;
let workspace_root = workspace.workspace_root().clone();
let mut ws = test_workspace.workspace;
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();
let wc = workspace.working_copy_mut();
wc.check_out(repo.op_id().clone(), None, &MergedTree::legacy(tree))
ws.check_out(repo.op_id().clone(), None, &MergedTree::legacy(tree))
.unwrap();
};
@ -330,8 +329,8 @@ fn test_conflicting_changes_on_disk() {
let settings = testutils::user_settings();
let test_workspace = TestWorkspace::init(&settings);
let repo = &test_workspace.repo;
let mut workspace = test_workspace.workspace;
let workspace_root = workspace.workspace_root().clone();
let mut ws = test_workspace.workspace;
let workspace_root = ws.workspace_root().clone();
// file on disk conflicts with file in target commit
let file_file_path = RepoPath::from_internal_string("file-file");
@ -368,8 +367,7 @@ fn test_conflicting_changes_on_disk() {
)
.unwrap();
let wc = workspace.working_copy_mut();
let stats = wc.check_out(repo.op_id().clone(), None, &tree).unwrap();
let stats = ws.check_out(repo.op_id().clone(), None, &tree).unwrap();
assert_eq!(
stats,
CheckoutStats {
@ -411,9 +409,10 @@ fn test_reset() {
&[(&gitignore_path, "ignored\n"), (&ignored_path, "code")],
);
let wc = test_workspace.workspace.working_copy_mut();
wc.check_out(repo.op_id().clone(), None, &tree_with_file)
let ws = &mut test_workspace.workspace;
ws.check_out(repo.op_id().clone(), None, &tree_with_file)
.unwrap();
let wc = ws.working_copy_mut();
// Test the setup: the file should exist on disk and in the tree state.
assert!(ignored_path.to_fs_path(&workspace_root).is_file());
@ -432,7 +431,8 @@ fn test_reset() {
// Now test the opposite direction: resetting to a commit where the file is
// tracked. The file should become tracked (even though it's ignored).
let wc = test_workspace.workspace.working_copy_mut();
let ws = &mut test_workspace.workspace;
let wc = ws.working_copy_mut();
let mut locked_wc = wc.start_mutation().unwrap();
locked_wc.reset(&tree_with_file).unwrap();
locked_wc.finish(op_id.clone()).unwrap();
@ -459,9 +459,10 @@ fn test_checkout_discard() {
let tree1 = create_tree(&repo, &[(&file1_path, "contents")]);
let tree2 = create_tree(&repo, &[(&file2_path, "contents")]);
let wc = test_workspace.workspace.working_copy_mut();
let ws = &mut test_workspace.workspace;
ws.check_out(repo.op_id().clone(), None, &tree1).unwrap();
let wc = ws.working_copy_mut();
let state_path = wc.state_path().to_path_buf();
wc.check_out(repo.op_id().clone(), None, &tree1).unwrap();
// Test the setup: the file should exist on disk and in the tree state.
assert!(file1_path.to_fs_path(&workspace_root).is_file());
@ -651,8 +652,8 @@ 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 wc = test_workspace.workspace.working_copy_mut();
wc.check_out(op_id.clone(), None, &tree1).unwrap();
let ws = &mut test_workspace.workspace;
ws.check_out(op_id.clone(), None, &tree1).unwrap();
testutils::write_working_copy_file(&workspace_root, &nested_gitignore_path, "!file\n");
testutils::write_working_copy_file(&workspace_root, &ignored_path, "contents");
@ -705,8 +706,8 @@ fn test_gitignores_checkout_never_overwrites_ignored() {
// 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 wc = test_workspace.workspace.working_copy_mut();
assert!(wc.check_out(repo.op_id().clone(), None, &tree).is_ok());
let ws = &mut test_workspace.workspace;
assert!(ws.check_out(repo.op_id().clone(), None, &tree).is_ok());
// Check that the old contents are in the working copy
let path = workspace_root.join("modified");
@ -739,8 +740,8 @@ fn test_gitignores_ignored_directory_already_tracked() {
);
// Check out the tree with the files in `ignored/`
let wc = test_workspace.workspace.working_copy_mut();
wc.check_out(repo.op_id().clone(), None, &tree).unwrap();
let ws = &mut test_workspace.workspace;
ws.check_out(repo.op_id().clone(), None, &tree).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
@ -831,8 +832,8 @@ fn test_gitsubmodule() {
let tree_id = MergedTreeId::Legacy(tree_builder.write_tree());
let tree = store.get_root_tree(&tree_id).unwrap();
let wc = test_workspace.workspace.working_copy_mut();
wc.check_out(repo.op_id().clone(), None, &tree).unwrap();
let ws = &mut test_workspace.workspace;
ws.check_out(repo.op_id().clone(), None, &tree).unwrap();
std::fs::create_dir(submodule_path.to_fs_path(&workspace_root)).unwrap();
@ -870,8 +871,8 @@ fn test_existing_directory_symlink() {
let tree = create_tree(repo, &[(&file_path, "contents")]);
// Checkout should fail because "parent" already exists and is a symlink.
let wc = test_workspace.workspace.working_copy_mut();
assert!(wc.check_out(repo.op_id().clone(), None, &tree).is_err());
let ws = &mut test_workspace.workspace;
assert!(ws.check_out(repo.op_id().clone(), None, &tree).is_err());
// Therefore, "../escaped" shouldn't be created.
assert!(!workspace_root.parent().unwrap().join("escaped").exists());

29
lib/tests/test_local_working_copy_concurrent.rs
View file

@ -39,40 +39,35 @@ fn test_concurrent_checkout() {
let tree3 = repo.store().get_root_tree(&tree_id3).unwrap();
// Check out tree1
let wc1 = test_workspace1.workspace.working_copy_mut();
let ws1 = &mut test_workspace1.workspace;
// The operation ID is not correct, but that doesn't matter for this test
wc1.check_out(repo.op_id().clone(), None, &tree1).unwrap();
ws1.check_out(repo.op_id().clone(), None, &tree1).unwrap();
// Check out tree2 from another process (simulated by another workspace
// instance)
let mut workspace2 = Workspace::load(
let mut ws2 = Workspace::load(
&settings,
&workspace1_root,
&TestRepo::default_store_factories(),
)
.unwrap();
workspace2
.working_copy_mut()
.check_out(repo.op_id().clone(), Some(&tree_id1), &tree2)
ws2.check_out(repo.op_id().clone(), Some(&tree_id1), &tree2)
.unwrap();
// Checking out another tree (via the first workspace instance) should now fail.
assert_matches!(
wc1.check_out(repo.op_id().clone(), Some(&tree_id1), &tree3),
ws1.check_out(repo.op_id().clone(), Some(&tree_id1), &tree3),
Err(CheckoutError::ConcurrentCheckout)
);
// Check that the tree2 is still checked out on disk.
let workspace3 = Workspace::load(
let ws3 = Workspace::load(
&settings,
&workspace1_root,
&TestRepo::default_store_factories(),
)
.unwrap();
assert_eq!(
*workspace3.working_copy().current_tree_id().unwrap(),
tree_id2
);
assert_eq!(*ws3.working_copy().current_tree_id().unwrap(), tree_id2);
}
#[test]
@ -100,7 +95,6 @@ fn test_checkout_parallel() {
);
test_workspace
.workspace
.working_copy_mut()
.check_out(repo.op_id().clone(), None, &tree)
.unwrap();
@ -124,10 +118,7 @@ fn test_checkout_parallel() {
.get_root_tree(&tree_id)
.unwrap();
// The operation ID is not correct, but that doesn't matter for this test
let stats = workspace
.working_copy_mut()
.check_out(op_id, None, &tree)
.unwrap();
let stats = workspace.check_out(op_id, None, &tree).unwrap();
assert_eq!(stats.updated_files, 0);
assert_eq!(stats.added_files, 1);
assert_eq!(stats.removed_files, 1);
@ -158,8 +149,8 @@ fn test_racy_checkout() {
let mut num_matches = 0;
for _ in 0..100 {
let wc = test_workspace.workspace.working_copy_mut();
wc.check_out(op_id.clone(), None, &tree).unwrap();
let ws = &mut test_workspace.workspace;
ws.check_out(op_id.clone(), None, &tree).unwrap();
assert_eq!(
std::fs::read(path.to_fs_path(&workspace_root)).unwrap(),
b"1".to_vec()

10
lib/tests/test_local_working_copy_sparse.rs
View file

@ -49,8 +49,11 @@ fn test_sparse_checkout() {
],
);
test_workspace
.workspace
.check_out(repo.op_id().clone(), None, &tree)
.unwrap();
let wc = test_workspace.workspace.working_copy_mut();
wc.check_out(repo.op_id().clone(), None, &tree).unwrap();
// Set sparse patterns to only dir1/
let mut locked_wc = wc.start_mutation().unwrap();
@ -152,8 +155,11 @@ fn test_sparse_commit() {
],
);
test_workspace
.workspace
.check_out(repo.op_id().clone(), None, &tree)
.unwrap();
let wc = test_workspace.workspace.working_copy_mut();
wc.check_out(repo.op_id().clone(), None, &tree).unwrap();
// Set sparse patterns to only dir1/
let mut locked_wc = wc.start_mutation().unwrap();