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

working_copy: pass in old commit ID to check_out()

Browse source 
`WorkingCopy::check_out()` currently fails if the commit recorded on
disk has changed since it was last read. It fails with a "concurrent
checkout" error. That usually works well in practice, but one can
imagine cases where it's not correct. For an example where the current
behavior is wrong, consider this sequence of events:

 1. Process A loads the repo and working copy.

 2. Process B loads the repo at operation A. It has not loaded the
    working copy yet.

 3. Process A writes an operation and updates the working copy.

 4. Process B loads the working copy and sees that it is checked out
    to the commit process B set it to. We don't currently have any
    checks that the working copy commit matches the view's checkout
    (though I plan to add that).

 5. Process B finishes its operation (which is now divergent with the
    operation written by process A). It updates the working copy to
    the checkout set in the repo view by process B. There's no data
    loss here, but the behavior is surprising because we would usually
    tell the user that we detected a concurrent update to the working
    copy.

We should instead check that the working copy's commit on disk matches
what the previous repo view said, i.e. the view at the start of the
operation we just committed. This patch does that by having the caller
pass in the expected old commit ID.
This commit is contained in:
Martin von Zweigbergk 2022-01-15 17:25:47 -08:00
parent 7103860f7e
commit 9e1869dcef
4 changed files with 45 additions and 32 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

25
lib/src/working_copy.rs
View file

@ -816,8 +816,12 @@ impl WorkingCopy {
self.write_proto(proto);
}
pub fn check_out(&mut self, commit: Commit) -> Result<CheckoutStats, CheckoutError> {
assert!(commit.is_open());
pub fn check_out(
&mut self,
old_commit_id: Option<&CommitId>,
new_commit: Commit,
) -> Result<CheckoutStats, CheckoutError> {
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.

10
lib/tests/test_working_copy.rs
View file

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

14
lib/tests/test_working_copy_concurrent.rs
View file

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

28
src/commands.rs
View file

@ -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<ReadonlyRepo>,
wc: &mut WorkingCopy,
old_commit_id: &CommitId,
) -> Result<Option<CheckoutStats>, 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")?;