mirrors/jj
1
0
Fork 1
mirror of https://github.com/martinvonz/jj.git synced 2024-12-27 06:27:43 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions

working_copy: start improving interface for mutations

Browse source 
This patch changes the interface for making changes to the working
copy by replacing `write_tree()` and `untrack()` by a single
`start_mutation()` method. The two functions now live on the returned
`LockedWorkingCopy` object instead. That is more flexible because the
caller can make multiple changes while the working copy is locked. It
also helps us reduce the risk of buggy callers that read the commit ID
before taking the lock, because we can now make it accessible only on
`LockedWorkingCopy`.
This commit is contained in:
Martin von Zweigbergk 2022-01-16 09:48:30 -08:00
parent 2916dc3423
commit 25d19e8a65
4 changed files with 62 additions and 59 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

35
lib/src/working_copy.rs
View file

@ -831,34 +831,21 @@ impl WorkingCopy {
Ok(stats) Ok(stats)
} }
pub fn write_tree(&mut self) -> LockedWorkingCopy { pub fn start_mutation(&mut self) -> LockedWorkingCopy {
let lock_path = self.state_path.join("working_copy.lock"); let lock_path = self.state_path.join("working_copy.lock");
let lock = FileLock::lock(lock_path); let lock = FileLock::lock(lock_path);
let current_proto = self.read_proto(); let current_proto = self.read_proto();
self.commit_id let old_commit_id = CommitId::new(current_proto.commit_id);
.replace(Some(CommitId::new(current_proto.commit_id))); self.commit_id.replace(Some(old_commit_id.clone()));
self.tree_state().as_mut().unwrap().write_tree();
LockedWorkingCopy { LockedWorkingCopy {
wc: self, wc: self,
lock, lock,
old_commit_id,
closed: false, closed: false,
} }
} }
pub fn untrack(&mut self, matcher: &dyn Matcher) -> Result<LockedWorkingCopy, UntrackError> {
let lock_path = self.state_path.join("working_copy.lock");
let lock = FileLock::lock(lock_path);
self.tree_state().as_mut().unwrap().untrack(matcher)?;
Ok(LockedWorkingCopy {
wc: self,
lock,
closed: false,
})
}
} }
// A working copy that's locked on disk. The tree state has already been // A working copy that's locked on disk. The tree state has already been
@ -867,18 +854,26 @@ pub struct LockedWorkingCopy<'a> {
wc: &'a mut WorkingCopy, wc: &'a mut WorkingCopy,
#[allow(dead_code)] #[allow(dead_code)]
lock: FileLock, lock: FileLock,
old_commit_id: CommitId,
closed: bool, closed: bool,
} }
impl LockedWorkingCopy<'_> { impl LockedWorkingCopy<'_> {
pub fn old_commit_id(&self) -> CommitId { /// The commit at the time the lock was taken
self.wc.current_commit_id() pub fn old_commit_id(&self) -> &CommitId {
&self.old_commit_id
} }
pub fn new_tree_id(&self) -> TreeId { pub fn write_tree(&mut self) -> TreeId {
self.wc.tree_state().as_mut().unwrap().write_tree();
self.wc.current_tree_id() self.wc.current_tree_id()
} }
pub fn untrack(&mut self, matcher: &dyn Matcher) -> Result<TreeId, UntrackError> {
self.wc.tree_state().as_mut().unwrap().untrack(matcher)?;
Ok(self.wc.current_tree_id())
}
pub fn finish(mut self, commit_id: CommitId) { pub fn finish(mut self, commit_id: CommitId) {
self.wc.commit_id.replace(Some(commit_id)); self.wc.commit_id.replace(Some(commit_id));
self.wc.save(); self.wc.save();

53
lib/tests/test_working_copy.rs
View file

@ -39,8 +39,8 @@ fn test_root(use_git: bool) {
let repo = &test_workspace.repo; let repo = &test_workspace.repo;
let wc = test_workspace.workspace.working_copy_mut(); let wc = test_workspace.workspace.working_copy_mut();
let locked_wc = wc.write_tree(); let mut locked_wc = wc.start_mutation();
let new_tree_id = locked_wc.new_tree_id(); let new_tree_id = locked_wc.write_tree();
locked_wc.discard(); locked_wc.discard();
let checkout_commit = repo.store().get_commit(repo.view().checkout()).unwrap(); let checkout_commit = repo.store().get_commit(repo.view().checkout()).unwrap();
assert_eq!(&new_tree_id, checkout_commit.tree().id()); assert_eq!(&new_tree_id, checkout_commit.tree().id());
@ -221,8 +221,8 @@ fn test_checkout_file_transitions(use_git: bool) {
wc.check_out(right_commit.clone()).unwrap(); wc.check_out(right_commit.clone()).unwrap();
// Check that the working copy is clean. // Check that the working copy is clean.
let locked_wc = wc.write_tree(); let mut locked_wc = wc.start_mutation();
let new_tree_id = locked_wc.new_tree_id(); let new_tree_id = locked_wc.write_tree();
locked_wc.discard(); locked_wc.discard();
assert_eq!(&new_tree_id, right_commit.tree().id()); assert_eq!(&new_tree_id, right_commit.tree().id());
@ -326,11 +326,12 @@ fn test_untrack() {
// Now we untrack the file called "unwanted" // Now we untrack the file called "unwanted"
let mut tx = repo.start_transaction("test"); let mut tx = repo.start_transaction("test");
let matcher = FilesMatcher::new(HashSet::from([unwanted_path.clone()])); let matcher = FilesMatcher::new(HashSet::from([unwanted_path.clone()]));
let unfinished_write = wc.untrack(&matcher).unwrap(); let mut locked_wc = wc.start_mutation();
let new_tree_id = locked_wc.untrack(&matcher).unwrap();
let new_commit = CommitBuilder::for_rewrite_from(&settings, repo.store(), &initial_commit) let new_commit = CommitBuilder::for_rewrite_from(&settings, repo.store(), &initial_commit)
.set_tree(unfinished_write.new_tree_id()) .set_tree(new_tree_id)
.write_to_repo(tx.mut_repo()); .write_to_repo(tx.mut_repo());
unfinished_write.finish(new_commit.id().clone()); locked_wc.finish(new_commit.id().clone());
tx.commit(); tx.commit();
// The file should still exist in the working copy. // The file should still exist in the working copy.
@ -346,10 +347,10 @@ fn test_untrack() {
// It should not get re-added if we write a new tree since we also added it // It should not get re-added if we write a new tree since we also added it
// to the .gitignore file. // to the .gitignore file.
let unfinished_write = wc.write_tree(); let mut locked_wc = wc.start_mutation();
let new_tree_id = unfinished_write.new_tree_id(); let new_tree_id = locked_wc.write_tree();
assert_eq!(new_tree_id, *new_commit.tree().id()); assert_eq!(new_tree_id, *new_commit.tree().id());
unfinished_write.discard(); locked_wc.discard();
} }
#[test_case(false ; "local backend")] #[test_case(false ; "local backend")]
@ -376,8 +377,8 @@ fn test_commit_racy_timestamps(use_git: bool) {
file.write_all(format!("contents {}", i).as_bytes()) file.write_all(format!("contents {}", i).as_bytes())
.unwrap(); .unwrap();
} }
let locked_wc = wc.write_tree(); let mut locked_wc = wc.start_mutation();
let new_tree_id = locked_wc.new_tree_id(); let new_tree_id = locked_wc.write_tree();
locked_wc.discard(); locked_wc.discard();
assert_ne!(new_tree_id, previous_tree_id); assert_ne!(new_tree_id, previous_tree_id);
previous_tree_id = new_tree_id; previous_tree_id = new_tree_id;
@ -410,8 +411,8 @@ fn test_gitignores(use_git: bool) {
testutils::write_working_copy_file(&workspace_root, &subdir_modified_path, "1"); testutils::write_working_copy_file(&workspace_root, &subdir_modified_path, "1");
let wc = test_workspace.workspace.working_copy_mut(); let wc = test_workspace.workspace.working_copy_mut();
let locked_wc = wc.write_tree(); let mut locked_wc = wc.start_mutation();
let new_tree_id1 = locked_wc.new_tree_id(); let new_tree_id1 = locked_wc.write_tree();
locked_wc.discard(); locked_wc.discard();
let tree1 = repo let tree1 = repo
.store() .store()
@ -440,8 +441,8 @@ fn test_gitignores(use_git: bool) {
testutils::write_working_copy_file(&workspace_root, &subdir_modified_path, "2"); testutils::write_working_copy_file(&workspace_root, &subdir_modified_path, "2");
testutils::write_working_copy_file(&workspace_root, &subdir_ignored_path, "2"); testutils::write_working_copy_file(&workspace_root, &subdir_ignored_path, "2");
let locked_wc = wc.write_tree(); let mut locked_wc = wc.start_mutation();
let new_tree_id2 = locked_wc.new_tree_id(); let new_tree_id2 = locked_wc.write_tree();
locked_wc.discard(); locked_wc.discard();
let tree2 = repo let tree2 = repo
.store() .store()
@ -505,8 +506,8 @@ fn test_gitignores_checkout_overwrites_ignored(use_git: bool) {
assert_eq!(buf, b"contents"); assert_eq!(buf, b"contents");
// Check that the file is in the tree created by committing the working copy // Check that the file is in the tree created by committing the working copy
let locked_wc = wc.write_tree(); let mut locked_wc = wc.start_mutation();
let new_tree_id = locked_wc.new_tree_id(); let new_tree_id = locked_wc.write_tree();
locked_wc.discard(); locked_wc.discard();
let new_tree = repo let new_tree = repo
.store() .store()
@ -556,8 +557,8 @@ fn test_gitignores_ignored_directory_already_tracked(use_git: bool) {
// Check that the file is still in the tree created by committing the working // Check that the file is still in the tree created by committing the working
// copy (that it didn't get removed because the directory is ignored) // copy (that it didn't get removed because the directory is ignored)
let locked_wc = wc.write_tree(); let mut locked_wc = wc.start_mutation();
let new_tree_id = locked_wc.new_tree_id(); let new_tree_id = locked_wc.write_tree();
locked_wc.discard(); locked_wc.discard();
let new_tree = repo let new_tree = repo
.store() .store()
@ -587,10 +588,10 @@ fn test_dotgit_ignored(use_git: bool) {
&RepoPath::from_internal_string(".git/file"), &RepoPath::from_internal_string(".git/file"),
"contents", "contents",
); );
let locked_working_copy = test_workspace.workspace.working_copy_mut().write_tree(); let mut locked_wc = test_workspace.workspace.working_copy_mut().start_mutation();
let new_tree_id = locked_working_copy.new_tree_id(); let new_tree_id = locked_wc.write_tree();
assert_eq!(new_tree_id, *repo.store().empty_tree_id()); assert_eq!(new_tree_id, *repo.store().empty_tree_id());
locked_working_copy.discard(); locked_wc.discard();
std::fs::remove_dir_all(&dotgit_path).unwrap(); std::fs::remove_dir_all(&dotgit_path).unwrap();
// Test with a .git file // Test with a .git file
@ -599,8 +600,8 @@ fn test_dotgit_ignored(use_git: bool) {
&RepoPath::from_internal_string(".git"), &RepoPath::from_internal_string(".git"),
"contents", "contents",
); );
let locked_working_copy = test_workspace.workspace.working_copy_mut().write_tree(); let mut locked_wc = test_workspace.workspace.working_copy_mut().start_mutation();
let new_tree_id = locked_working_copy.new_tree_id(); let new_tree_id = locked_wc.write_tree();
assert_eq!(new_tree_id, *repo.store().empty_tree_id()); assert_eq!(new_tree_id, *repo.store().empty_tree_id());
locked_working_copy.discard(); locked_wc.discard();
} }

4
lib/tests/test_working_copy_concurrent.rs
View file

@ -133,8 +133,8 @@ fn test_checkout_parallel(use_git: bool) {
// different tree than the one we just checked out, but since // different tree than the one we just checked out, but since
// write_tree() should take the same lock as check_out(), write_tree() // write_tree() should take the same lock as check_out(), write_tree()
// should never produce a different tree. // should never produce a different tree.
let locked_wc = workspace.working_copy_mut().write_tree(); let mut locked_wc = workspace.working_copy_mut().start_mutation();
let new_tree_id = locked_wc.new_tree_id(); let new_tree_id = locked_wc.write_tree();
locked_wc.discard(); locked_wc.discard();
assert!(tree_ids.contains(&new_tree_id)); assert!(tree_ids.contains(&new_tree_id));
}); });

29
src/commands.rs
View file

@ -293,7 +293,11 @@ impl WorkspaceCommandHelper {
} }
self.repo = tx.commit(); self.repo = tx.commit();
if let Some(new_wc_commit) = new_wc_commit { if let Some(new_wc_commit) = new_wc_commit {
let locked_working_copy = self.workspace.working_copy_mut().write_tree(); // TODO: We should probably do a regular checkout of new_wc_commit here even
// though it would usually have no effect. The issue is that if
// the update removed file ignored, we currently leave that as
// tracked, making it appear like it was added.
let locked_working_copy = self.workspace.working_copy_mut().start_mutation();
locked_working_copy.finish(new_wc_commit.id().clone()); locked_working_copy.finish(new_wc_commit.id().clone());
} }
} }
@ -456,9 +460,13 @@ impl WorkspaceCommandHelper {
fn maybe_commit_working_copy(&mut self, ui: &mut Ui) -> Result<(), CommandError> { fn maybe_commit_working_copy(&mut self, ui: &mut Ui) -> Result<(), CommandError> {
if self.may_update_working_copy { if self.may_update_working_copy {
let repo = self.repo.clone(); let repo = self.repo.clone();
let locked_wc = self.workspace.working_copy_mut().write_tree(); let mut locked_wc = self.workspace.working_copy_mut().start_mutation();
let old_commit_id = locked_wc.old_commit_id(); let new_tree_id = locked_wc.write_tree();
let old_commit = self.repo.store().get_commit(&old_commit_id).unwrap(); let old_commit = self
.repo
.store()
.get_commit(locked_wc.old_commit_id())
.unwrap();
// Check if the current checkout has changed on disk after we read it. It's fine // Check if the current checkout has changed on disk after we read it. It's fine
// if it has, but we'll need to reload the repo so the new commit is // if it has, but we'll need to reload the repo so the new commit is
// in the index and view. // in the index and view.
@ -468,7 +476,6 @@ impl WorkspaceCommandHelper {
// view when we reload. // view when we reload.
self.repo = repo.reload(); self.repo = repo.reload();
} }
let new_tree_id = locked_wc.new_tree_id();
if new_tree_id != *old_commit.tree().id() { if new_tree_id != *old_commit.tree().id() {
let mut tx = self.repo.start_transaction("commit working copy"); let mut tx = self.repo.start_transaction("commit working copy");
let mut_repo = tx.mut_repo(); let mut_repo = tx.mut_repo();
@ -1585,18 +1592,18 @@ fn cmd_untrack(
args.values_of("paths"), args.values_of("paths"),
)?; )?;
let mut tx = workspace_command.start_transaction("untrack paths"); let mut tx = workspace_command.start_transaction("untrack paths");
let unfinished_write = workspace_command let mut locked_working_copy = workspace_command.working_copy_mut().start_mutation();
.working_copy_mut() let new_tree_id = locked_working_copy
.untrack(matcher.as_ref()) .untrack(matcher.as_ref())
.map_err(|err| CommandError::InternalError(format!("Failed to untrack paths: {}", err)))?; .map_err(|err| CommandError::InternalError(format!("Failed to untrack paths: {}", err)))?;
let new_tree_id = unfinished_write.new_tree_id(); let old_commit = store
let old_commit_id = unfinished_write.old_commit_id(); .get_commit(locked_working_copy.old_commit_id())
let old_commit = store.get_commit(&old_commit_id).unwrap(); .unwrap();
let new_commit = CommitBuilder::for_rewrite_from(ui.settings(), &store, &old_commit) let new_commit = CommitBuilder::for_rewrite_from(ui.settings(), &store, &old_commit)
.set_tree(new_tree_id) .set_tree(new_tree_id)
.write_to_repo(tx.mut_repo()); .write_to_repo(tx.mut_repo());
tx.mut_repo().set_checkout(new_commit.id().clone()); tx.mut_repo().set_checkout(new_commit.id().clone());
unfinished_write.finish(new_commit.id().clone()); locked_working_copy.finish(new_commit.id().clone());
workspace_command.finish_transaction(ui, tx)?; workspace_command.finish_transaction(ui, tx)?;
// TODO: Is it better to have WorkingCopy::untrack() report if any matching // TODO: Is it better to have WorkingCopy::untrack() report if any matching