From 00c9a1ae118d64b3ed91acd0c46fc64f1ba81d58 Mon Sep 17 00:00:00 2001 From: Martin von Zweigbergk Date: Sat, 12 Feb 2022 23:28:59 -0800 Subject: [PATCH] working_copy: stop taking commit ID in `LockedWorkingCopy::finish()` We used to use the value to detect races, but we use the tree ID and the operation ID these days, so we don't need the commit ID. By changing this, we can avoid creating some commit IDs in tests, which is why I tackled this issue now. --- lib/src/working_copy.rs | 7 +++---- lib/tests/test_working_copy.rs | 22 +++++----------------- src/commands.rs | 13 ++++++------- 3 files changed, 14 insertions(+), 28 deletions(-) diff --git a/lib/src/working_copy.rs b/lib/src/working_copy.rs index 6d187ea66..0e07dbbe1 100644 --- a/lib/src/working_copy.rs +++ b/lib/src/working_copy.rs @@ -32,7 +32,7 @@ use tempfile::NamedTempFile; use thiserror::Error; use crate::backend::{ - BackendError, CommitId, ConflictId, FileId, MillisSinceEpoch, SymlinkId, TreeId, TreeValue, + BackendError, ConflictId, FileId, MillisSinceEpoch, SymlinkId, TreeId, TreeValue, }; use crate::commit::Commit; use crate::conflicts::{materialize_conflict, update_conflict_from_content}; @@ -874,9 +874,8 @@ impl WorkingCopy { return Err(CheckoutError::ConcurrentCheckout); } } - let new_commit_id = new_commit.id().clone(); let stats = locked_wc.check_out(new_commit.tree_id().clone())?; - locked_wc.finish(operation_id, new_commit_id); + locked_wc.finish(operation_id); Ok(stats) } } @@ -923,7 +922,7 @@ impl LockedWorkingCopy<'_> { self.wc.tree_state().as_mut().unwrap().reset(new_tree) } - pub fn finish(mut self, operation_id: OperationId, _commit_id: CommitId) { + pub fn finish(mut self, operation_id: OperationId) { self.wc.tree_state().as_mut().unwrap().save(); self.wc.operation_id.replace(Some(operation_id)); self.wc.save(); diff --git a/lib/tests/test_working_copy.rs b/lib/tests/test_working_copy.rs index cfd2b07a4..a213d0023 100644 --- a/lib/tests/test_working_copy.rs +++ b/lib/tests/test_working_copy.rs @@ -312,13 +312,6 @@ fn test_reset() { let mut tx = repo.start_transaction("test"); let store = repo.store(); let root_commit = store.root_commit_id(); - let commit_without_file = CommitBuilder::for_open_commit( - &settings, - store, - root_commit.clone(), - tree_without_file.id().clone(), - ) - .write_to_repo(tx.mut_repo()); let commit_with_file = CommitBuilder::for_open_commit( &settings, store, @@ -330,7 +323,7 @@ fn test_reset() { test_workspace.repo = repo.clone(); let wc = test_workspace.workspace.working_copy_mut(); - wc.check_out(repo.op_id().clone(), None, commit_with_file.clone()) + wc.check_out(repo.op_id().clone(), None, commit_with_file) .unwrap(); // Test the setup: the file should exist on disk and in the tree state. @@ -342,7 +335,7 @@ fn test_reset() { // commit the working copy (because it's ignored). let mut locked_wc = wc.start_mutation(); locked_wc.reset(&tree_without_file).unwrap(); - locked_wc.finish(repo.op_id().clone(), commit_without_file.id().clone()); + locked_wc.finish(repo.op_id().clone()); assert!(ignored_path.to_fs_path(&workspace_root).is_file()); assert!(!wc.file_states().contains_key(&ignored_path)); let mut locked_wc = wc.start_mutation(); @@ -355,7 +348,7 @@ fn test_reset() { // commit the working copy (because it's ignored). let mut locked_wc = wc.start_mutation(); locked_wc.reset(&tree_without_file).unwrap(); - locked_wc.finish(repo.op_id().clone(), commit_without_file.id().clone()); + locked_wc.finish(repo.op_id().clone()); assert!(ignored_path.to_fs_path(&workspace_root).is_file()); assert!(!wc.file_states().contains_key(&ignored_path)); let mut locked_wc = wc.start_mutation(); @@ -367,7 +360,7 @@ fn test_reset() { // tracked. The file should become tracked (even though it's ignored). let mut locked_wc = wc.start_mutation(); locked_wc.reset(&tree_with_file).unwrap(); - locked_wc.finish(repo.op_id().clone(), commit_with_file.id().clone()); + locked_wc.finish(repo.op_id().clone()); assert!(ignored_path.to_fs_path(&workspace_root).is_file()); assert!(wc.file_states().contains_key(&ignored_path)); let mut locked_wc = wc.start_mutation(); @@ -487,12 +480,7 @@ fn test_gitignores(use_git: bool) { let wc = test_workspace.workspace.working_copy_mut(); let mut locked_wc = wc.start_mutation(); let new_tree_id1 = locked_wc.write_tree(); - let mut tx = repo.start_transaction("test"); - let initial_commit = - CommitBuilder::for_new_commit(&settings, repo.store(), new_tree_id1.clone()) - .write_to_repo(tx.mut_repo()); - let repo = tx.commit(); - locked_wc.finish(repo.op_id().clone(), initial_commit.id().clone()); + locked_wc.finish(repo.op_id().clone()); let tree1 = repo .store() .get_tree(&RepoPath::root(), &new_tree_id1) diff --git a/src/commands.rs b/src/commands.rs index 04cc7de9d..68bf475c7 100644 --- a/src/commands.rs +++ b/src/commands.rs @@ -299,16 +299,15 @@ impl WorkspaceCommandHelper { .repo .store() .get_commit(new_git_head.as_ref().unwrap())?; - let new_wc_commit = - tx.mut_repo() - .check_out(workspace_id, &self.settings, &new_checkout); + tx.mut_repo() + .check_out(workspace_id, &self.settings, &new_checkout); // The working copy was presumably updated by the git command that updated HEAD, // so we just need to reset our working copy state to it without updating // working copy files. locked_working_copy.reset(&new_checkout.tree())?; tx.mut_repo().rebase_descendants(&self.settings); self.repo = tx.commit(); - locked_working_copy.finish(self.repo.op_id().clone(), new_wc_commit.id().clone()); + locked_working_copy.finish(self.repo.op_id().clone()); } else { self.repo = tx.commit(); } @@ -580,7 +579,7 @@ impl WorkspaceCommandHelper { } self.repo = tx.commit(); - locked_wc.finish(self.repo.op_id().clone(), commit.id().clone()); + locked_wc.finish(self.repo.op_id().clone()); } else { locked_wc.discard(); } @@ -1801,7 +1800,7 @@ fn cmd_untrack( locked_working_copy.reset(&new_tree)?; } } - let new_commit = CommitBuilder::for_rewrite_from(ui.settings(), &store, ¤t_checkout) + CommitBuilder::for_rewrite_from(ui.settings(), &store, ¤t_checkout) .set_tree(new_tree_id) .write_to_repo(tx.mut_repo()); let num_rebased = tx.mut_repo().rebase_descendants(ui.settings()); @@ -1809,7 +1808,7 @@ fn cmd_untrack( writeln!(ui, "Rebased {} descendant commits", num_rebased)?; } let repo = tx.commit(); - locked_working_copy.finish(repo.op_id().clone(), new_commit.id().clone()); + locked_working_copy.finish(repo.op_id().clone()); Ok(()) }