diff --git a/lib/src/working_copy.rs b/lib/src/working_copy.rs index 0a93be9f0..1a614474c 100644 --- a/lib/src/working_copy.rs +++ b/lib/src/working_copy.rs @@ -816,8 +816,12 @@ impl WorkingCopy { self.write_proto(proto); } - pub fn check_out(&mut self, commit: Commit) -> Result { - assert!(commit.is_open()); + pub fn check_out( + &mut self, + old_commit_id: Option<&CommitId>, + new_commit: Commit, + ) -> Result { + assert!(new_commit.is_open()); let lock_path = self.state_path.join("working_copy.lock"); let _lock = FileLock::lock(lock_path); @@ -826,15 +830,12 @@ impl WorkingCopy { // access to that file can also serve as lock so only one process // at once can do a checkout. - // Check if the current checkout has changed on disk after we read it. It's safe - // to check out another commit regardless, but it's probably not what - // the caller wanted, so we let them know. - // - // We could safely add a version of this function without the check if we see a - // need for it. + // 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. let current_proto = self.read_proto(); - if let Some(commit_id_at_read_time) = self.commit_id.borrow().as_ref() { - if current_proto.commit_id != commit_id_at_read_time.as_bytes() { + if let Some(old_commit_id) = old_commit_id { + if current_proto.commit_id != old_commit_id.as_bytes() { return Err(CheckoutError::ConcurrentCheckout); } } @@ -843,9 +844,9 @@ impl WorkingCopy { .tree_state() .as_mut() .unwrap() - .check_out(commit.tree().id().clone())?; + .check_out(new_commit.tree().id().clone())?; - self.commit_id.replace(Some(commit.id().clone())); + self.commit_id.replace(Some(new_commit.id().clone())); self.save(); // TODO: Clear the "pending_checkout" file here. diff --git a/lib/tests/test_working_copy.rs b/lib/tests/test_working_copy.rs index a60cafea0..4d6050e9a 100644 --- a/lib/tests/test_working_copy.rs +++ b/lib/tests/test_working_copy.rs @@ -215,8 +215,8 @@ fn test_checkout_file_transitions(use_git: bool) { tx.commit(); let wc = test_workspace.workspace.working_copy_mut(); - wc.check_out(left_commit).unwrap(); - wc.check_out(right_commit.clone()).unwrap(); + wc.check_out(None, left_commit).unwrap(); + wc.check_out(None, right_commit.clone()).unwrap(); // Check that the working copy is clean. let mut locked_wc = wc.start_mutation(); @@ -324,7 +324,7 @@ fn test_reset() { test_workspace.repo = tx.commit(); let wc = test_workspace.workspace.working_copy_mut(); - wc.check_out(commit_with_file.clone()).unwrap(); + wc.check_out(None, commit_with_file.clone()).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()); @@ -511,7 +511,7 @@ fn test_gitignores_checkout_overwrites_ignored(use_git: bool) { // "contents". The exiting contents ("garbage") should be replaced in the // working copy. let wc = test_workspace.workspace.working_copy_mut(); - wc.check_out(commit).unwrap(); + wc.check_out(None, commit).unwrap(); // Check that the new contents are in the working copy let path = workspace_root.join("modified"); @@ -569,7 +569,7 @@ fn test_gitignores_ignored_directory_already_tracked(use_git: bool) { // Check out the commit with the file in ignored/ let wc = test_workspace.workspace.working_copy_mut(); - wc.check_out(commit).unwrap(); + wc.check_out(None, commit).unwrap(); // 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) diff --git a/lib/tests/test_working_copy_concurrent.rs b/lib/tests/test_working_copy_concurrent.rs index 6063c4936..4b071488b 100644 --- a/lib/tests/test_working_copy_concurrent.rs +++ b/lib/tests/test_working_copy_concurrent.rs @@ -47,19 +47,20 @@ fn test_concurrent_checkout(use_git: bool) { // Check out commit1 let wc1 = test_workspace1.workspace.working_copy_mut(); - wc1.check_out(commit1).unwrap(); + let commit_id1 = commit1.id().clone(); + wc1.check_out(None, commit1).unwrap(); // Check out commit2 from another process (simulated by another workspace // instance) let mut workspace2 = Workspace::load(&settings, workspace1_root.clone()).unwrap(); workspace2 .working_copy_mut() - .check_out(commit2.clone()) + .check_out(Some(&commit_id1), commit2.clone()) .unwrap(); // Checking out another commit (via the first repo instance) should now fail. assert_eq!( - wc1.check_out(commit3), + wc1.check_out(Some(&commit_id1), commit3), Err(CheckoutError::ConcurrentCheckout) ); @@ -109,7 +110,7 @@ fn test_checkout_parallel(use_git: bool) { test_workspace .workspace .working_copy_mut() - .check_out(commit) + .check_out(None, commit) .unwrap(); let mut threads = vec![]; @@ -125,7 +126,10 @@ fn test_checkout_parallel(use_git: bool) { .store() .get_commit(&commit_id) .unwrap(); - let stats = workspace.working_copy_mut().check_out(commit).unwrap(); + let stats = workspace + .working_copy_mut() + .check_out(None, commit) + .unwrap(); assert_eq!(stats.updated_files, 0); assert_eq!(stats.added_files, 1); assert_eq!(stats.removed_files, 1); diff --git a/src/commands.rs b/src/commands.rs index db605087c..b2d524079 100644 --- a/src/commands.rs +++ b/src/commands.rs @@ -549,9 +549,15 @@ impl WorkspaceCommandHelper { writeln!(ui, "Rebased {} descendant commits", num_rebased)?; } } + let old_commit_id = tx.base_repo().view().checkout().clone(); self.repo = tx.commit(); if self.may_update_working_copy { - let stats = update_working_copy(ui, &self.repo, self.workspace.working_copy_mut())?; + let stats = update_working_copy( + ui, + &self.repo, + self.workspace.working_copy_mut(), + &old_commit_id, + )?; if let Some(stats) = stats { if stats.added_files > 0 || stats.updated_files > 0 || stats.removed_files > 0 { writeln!( @@ -698,22 +704,24 @@ fn update_working_copy( ui: &mut Ui, repo: &Arc, wc: &mut WorkingCopy, + old_commit_id: &CommitId, ) -> Result, CommandError> { - let old_commit_id = wc.current_commit_id(); let new_commit_id = repo.view().checkout(); - if *new_commit_id == old_commit_id { + if 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(new_commit.clone()).map_err(|err| { - CommandError::InternalError(format!( - "Failed to check out commit {}: {}", - new_commit.id().hex(), - err - )) - })?; + let stats = wc + .check_out(Some(old_commit_id), new_commit.clone()) + .map_err(|err| { + CommandError::InternalError(format!( + "Failed to check out commit {}: {}", + new_commit.id().hex(), + err + )) + })?; ui.write("Working copy now at: ")?; ui.write_commit_summary(repo.as_repo_ref(), &new_commit)?; ui.write("\n")?;