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

working copy: pass commit instead of tree into check_out()

Browse source 
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.
This commit is contained in:
Martin von Zweigbergk 2023-10-16 10:21:54 -07:00 committed by Martin von Zweigbergk
parent 7c8a0a18f9
commit e1f00d9426
9 changed files with 88 additions and 50 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

3
cli/src/cli_util.rs
View file

@ -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 {}: {}",

3
cli/src/commands/mod.rs
View file

@ -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 {}: {}",

6
lib/src/local_working_copy.rs
View file

@ -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<CheckoutStats, CheckoutError> {
fn check_out(&mut self, commit: &Commit) -> Result<CheckoutStats, CheckoutError> {
// 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)
}

6
lib/src/working_copy.rs
View file

@ -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<MergedTreeId, SnapshotError>;
/// 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<CheckoutStats, CheckoutError>;
/// Check out the specified commit in the working copy.
fn check_out(&mut self, commit: &Commit) -> Result<CheckoutStats, CheckoutError>;
/// Update to another tree without touching the files in the working copy.
fn reset(&mut self, new_tree: &MergedTree) -> Result<(), ResetError>;

6
lib/src/workspace.rs
View file

@ -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<CheckoutStats, CheckoutError> {
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 {

52
lib/tests/test_local_working_copy.rs
View file

@ -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());

26
lib/tests/test_local_working_copy_concurrent.rs
View file

@ -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()

8
lib/tests/test_local_working_copy_sparse.rs
View file

@ -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/

28
lib/testutils/src/lib.rs
View file

@ -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<Box<dyn Backend>, BackendInitError> {
&move |store_path: &Path| -> Result<Box<dyn Backend>, 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<Store>, 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<Store>, tree_id: &MergedTreeId) -> String {
use std::fmt::Write;
let mut buf = String::new();