From e098c0193566e72006a893a6f863eec7c7d0ebc6 Mon Sep 17 00:00:00 2001 From: Martin von Zweigbergk Date: Sat, 12 Feb 2022 16:21:44 -0800 Subject: [PATCH] working_copy: replace commit ID by tree ID for checking for changes What matters for the working copy is the tree ID. We should be able to remove the commit ID. This patch gets us close. --- lib/src/working_copy.rs | 21 +++++----- lib/tests/test_working_copy_concurrent.rs | 6 +-- src/commands.rs | 49 ++++++++++++----------- 3 files changed, 41 insertions(+), 35 deletions(-) diff --git a/lib/src/working_copy.rs b/lib/src/working_copy.rs index df522546e..f8f703418 100644 --- a/lib/src/working_copy.rs +++ b/lib/src/working_copy.rs @@ -864,14 +864,17 @@ impl WorkingCopy { // Re-read from disk after taking the lock self.load_proto(); + // TODO: It's expensive to reload the whole tree. We should first check if it + // has changed. + self.tree_state.replace(None); let old_operation_id = self.operation_id(); - let old_commit_id = self.current_commit_id(); + let old_tree_id = self.current_tree_id(); LockedWorkingCopy { wc: self, lock, old_operation_id, - old_commit_id, + old_tree_id, closed: false, } } @@ -879,15 +882,15 @@ impl WorkingCopy { pub fn check_out( &mut self, operation_id: OperationId, - old_commit_id: Option<&CommitId>, + old_tree_id: Option<&TreeId>, new_commit: Commit, ) -> Result { let mut locked_wc = self.start_mutation(); // Check if the current checkout 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_commit_id) = old_commit_id { - if *old_commit_id != locked_wc.old_commit_id { + if let Some(old_tree_id) = old_tree_id { + if *old_tree_id != locked_wc.old_tree_id { locked_wc.discard(); return Err(CheckoutError::ConcurrentCheckout); } @@ -906,7 +909,7 @@ pub struct LockedWorkingCopy<'a> { #[allow(dead_code)] lock: FileLock, old_operation_id: OperationId, - old_commit_id: CommitId, + old_tree_id: TreeId, closed: bool, } @@ -916,9 +919,9 @@ impl LockedWorkingCopy<'_> { &self.old_operation_id } - /// The commit at the time the lock was taken - pub fn old_commit_id(&self) -> &CommitId { - &self.old_commit_id + /// The tree at the time the lock was taken + pub fn old_tree_id(&self) -> &TreeId { + &self.old_tree_id } pub fn write_tree(&mut self) -> TreeId { diff --git a/lib/tests/test_working_copy_concurrent.rs b/lib/tests/test_working_copy_concurrent.rs index ec7d01b9f..7b2d2fd3b 100644 --- a/lib/tests/test_working_copy_concurrent.rs +++ b/lib/tests/test_working_copy_concurrent.rs @@ -47,7 +47,7 @@ fn test_concurrent_checkout(use_git: bool) { // Check out commit1 let wc1 = test_workspace1.workspace.working_copy_mut(); - let commit_id1 = commit1.id().clone(); + let tree_id1 = commit1.tree_id().clone(); // The operation ID is not correct, but that doesn't matter for this test wc1.check_out(repo1.op_id().clone(), None, commit1).unwrap(); @@ -56,12 +56,12 @@ fn test_concurrent_checkout(use_git: bool) { let mut workspace2 = Workspace::load(&settings, workspace1_root.clone()).unwrap(); workspace2 .working_copy_mut() - .check_out(repo1.op_id().clone(), Some(&commit_id1), commit2.clone()) + .check_out(repo1.op_id().clone(), Some(&tree_id1), commit2.clone()) .unwrap(); // Checking out another commit (via the first repo instance) should now fail. assert_eq!( - wc1.check_out(repo1.op_id().clone(), Some(&commit_id1), commit3), + wc1.check_out(repo1.op_id().clone(), Some(&tree_id1), commit3), Err(CheckoutError::ConcurrentCheckout) ); diff --git a/src/commands.rs b/src/commands.rs index 81c505664..f1353e9aa 100644 --- a/src/commands.rs +++ b/src/commands.rs @@ -33,7 +33,7 @@ use clap::{crate_version, App, Arg, ArgMatches}; use criterion::Criterion; use git2::{Oid, Repository}; use itertools::Itertools; -use jujutsu_lib::backend::{BackendError, CommitId, Timestamp, TreeValue}; +use jujutsu_lib::backend::{BackendError, CommitId, Timestamp, TreeId, TreeValue}; use jujutsu_lib::commit::Commit; use jujutsu_lib::commit_builder::CommitBuilder; use jujutsu_lib::dag_walk::topo_order_reverse; @@ -501,8 +501,8 @@ impl WorkspaceCommandHelper { // in the index and view, and so we don't cause unnecessary // divergence. let checkout_commit = repo.store().get_commit(&checkout_id).unwrap(); - let wc_commit_id = locked_wc.old_commit_id().clone(); - if *checkout_commit.id() != wc_commit_id { + let wc_tree_id = locked_wc.old_tree_id().clone(); + if *checkout_commit.tree_id() != wc_tree_id { let wc_operation_data = self .repo .op_store() @@ -619,6 +619,7 @@ impl WorkspaceCommandHelper { fn finish_transaction(&mut self, ui: &mut Ui, mut tx: Transaction) -> Result<(), CommandError> { let mut_repo = tx.mut_repo(); + let store = mut_repo.store().clone(); if !mut_repo.has_changes() { writeln!(ui, "Nothing changed.")?; return Ok(()); @@ -629,11 +630,11 @@ impl WorkspaceCommandHelper { writeln!(ui, "Rebased {} descendant commits", num_rebased)?; } } - let maybe_old_commit_id = tx + let maybe_old_tree_id = tx .base_repo() .view() .get_checkout(&self.workspace_id()) - .cloned(); + .map(|commit_id| store.get_commit(commit_id).unwrap().tree_id().clone()); self.repo = tx.commit(); if self.may_update_working_copy { let stats = update_working_copy( @@ -641,7 +642,7 @@ impl WorkspaceCommandHelper { &self.repo, &self.workspace_id(), self.workspace.working_copy_mut(), - maybe_old_commit_id.as_ref(), + maybe_old_tree_id.as_ref(), )?; if let Some(stats) = stats { if stats.added_files > 0 || stats.updated_files > 0 || stats.removed_files > 0 { @@ -783,7 +784,7 @@ fn update_working_copy( repo: &Arc, workspace_id: &WorkspaceId, wc: &mut WorkingCopy, - old_commit_id: Option<&CommitId>, + old_tree_id: Option<&TreeId>, ) -> Result, CommandError> { let new_commit_id = match repo.view().get_checkout(workspace_id) { Some(new_commit_id) => new_commit_id, @@ -792,25 +793,27 @@ fn update_working_copy( return Ok(None); } }; - if Some(new_commit_id) == old_commit_id { - return Ok(None); - } - // TODO: CheckoutError::ConcurrentCheckout should probably just result in a - // warning for most commands (but be an error for the checkout command) let new_commit = repo.store().get_commit(new_commit_id).unwrap(); - let stats = wc - .check_out(repo.op_id().clone(), old_commit_id, new_commit.clone()) - .map_err(|err| { - CommandError::InternalError(format!( - "Failed to check out commit {}: {}", - new_commit.id().hex(), - err - )) - })?; + let stats = if Some(new_commit.tree_id()) != old_tree_id { + // TODO: CheckoutError::ConcurrentCheckout should probably just result in a + // warning for most commands (but be an error for the checkout command) + let stats = wc + .check_out(repo.op_id().clone(), old_tree_id, new_commit.clone()) + .map_err(|err| { + CommandError::InternalError(format!( + "Failed to check out commit {}: {}", + new_commit.id().hex(), + err + )) + })?; + Some(stats) + } else { + None + }; ui.write("Working copy now at: ")?; ui.write_commit_summary(repo.as_repo_ref(), workspace_id, &new_commit)?; ui.write("\n")?; - Ok(Some(stats)) + Ok(stats) } fn get_app<'help>() -> App<'help> { @@ -1765,7 +1768,7 @@ fn cmd_untrack( let mut tx = workspace_command.start_transaction("untrack paths"); let mut locked_working_copy = workspace_command.working_copy_mut().start_mutation(); - if current_checkout.id() != locked_working_copy.old_commit_id() { + if current_checkout.tree_id() != locked_working_copy.old_tree_id() { return Err(CommandError::UserError( "Concurrent working copy operation. Try again.".to_string(), ));