From de4395ab7bae7a7acee598308794e36e2789f691 Mon Sep 17 00:00:00 2001 From: Martin von Zweigbergk Date: Wed, 16 Mar 2022 20:58:04 -0700 Subject: [PATCH] repo: when merging in removed head, rebase descendants (#111) --- lib/src/repo.rs | 71 ++++++++++++++--- lib/tests/test_bad_locking.rs | 15 +++- lib/tests/test_commit_concurrent.rs | 2 +- lib/tests/test_load_repo.rs | 2 +- lib/tests/test_operations.rs | 8 +- lib/tests/test_view.rs | 119 ++++++++++++++++++++++++++-- src/commands.rs | 3 +- 7 files changed, 193 insertions(+), 27 deletions(-) diff --git a/lib/src/repo.rs b/lib/src/repo.rs index e49e8531266..4fcc766cdb8 100644 --- a/lib/src/repo.rs +++ b/lib/src/repo.rs @@ -20,9 +20,10 @@ use std::ops::Deref; use std::path::{Path, PathBuf}; use std::sync::{Arc, Mutex}; +use itertools::Itertools; use thiserror::Error; -use crate::backend::{BackendError, CommitId}; +use crate::backend::{BackendError, ChangeId, CommitId}; use crate::commit::Commit; use crate::commit_builder::CommitBuilder; use crate::dag_walk::{closest_common_node, topo_order_reverse}; @@ -194,7 +195,7 @@ impl ReadonlyRepo { pub fn load_at_head(user_settings: &UserSettings, repo_path: PathBuf) -> Arc { RepoLoader::init(user_settings, repo_path) .load_at_head() - .resolve() + .resolve(user_settings) } pub fn loader(&self) -> RepoLoader { @@ -278,8 +279,8 @@ impl ReadonlyRepo { Transaction::new(mut_repo, description) } - pub fn reload_at_head(&self) -> Arc { - self.loader().load_at_head().resolve() + pub fn reload_at_head(&self, user_settings: &UserSettings) -> Arc { + self.loader().load_at_head().resolve(user_settings) } pub fn reload_at(&self, operation: &Operation) -> Arc { @@ -293,10 +294,10 @@ pub enum RepoAtHead { } impl RepoAtHead { - pub fn resolve(self) -> Arc { + pub fn resolve(self, user_settings: &UserSettings) -> Arc { match self { RepoAtHead::Single(repo) => repo, - RepoAtHead::Unresolved(unresolved) => unresolved.resolve(), + RepoAtHead::Unresolved(unresolved) => unresolved.resolve(user_settings), } } } @@ -308,10 +309,10 @@ pub struct UnresolvedHeadRepo { } impl UnresolvedHeadRepo { - pub fn resolve(self) -> Arc { + pub fn resolve(self, user_settings: &UserSettings) -> Arc { let merged_repo = self .repo_loader - .merge_op_heads(self.op_heads) + .merge_op_heads(user_settings, self.op_heads) .leave_unpublished(); self.locked_op_heads.finish(merged_repo.operation()); merged_repo @@ -383,7 +384,11 @@ impl RepoLoader { } } - fn merge_op_heads(&self, mut op_heads: Vec) -> UnpublishedOperation { + fn merge_op_heads( + &self, + user_settings: &UserSettings, + mut op_heads: Vec, + ) -> UnpublishedOperation { op_heads.sort_by_key(|op| op.store_operation().metadata.end_time.timestamp.clone()); let base_repo = self.load_at(&op_heads[0]); let mut tx = base_repo.start_transaction("resolve concurrent operations"); @@ -400,6 +405,7 @@ impl RepoLoader { let base_repo = self.load_at(&ancestor_op); let other_repo = self.load_at(other_op_head); merged_repo.merge(&base_repo, &other_repo); + merged_repo.rebase_descendants(user_settings); } let op_parent_ids = op_heads.iter().map(|op| op.id().clone()).collect(); tx.set_parents(op_parent_ids); @@ -810,15 +816,56 @@ impl MutableRepo { self.view_mut().add_public_head(added_head); } + let base_heads = base.heads().iter().cloned().collect_vec(); + let own_heads = self.view().heads().iter().cloned().collect_vec(); + let other_heads = other.heads().iter().cloned().collect_vec(); + let mut new_commit_by_change: HashMap> = HashMap::new(); + for added in self.index.walk_revs(&own_heads, &base_heads) { + new_commit_by_change + .entry(added.change_id()) + .or_default() + .push(added.commit_id().clone()); + } + for added in self.index.walk_revs(&other_heads, &base_heads) { + new_commit_by_change + .entry(added.change_id()) + .or_default() + .push(added.commit_id().clone()); + } + let mut rewritten_commits = HashMap::new(); + let mut abandoned_ids = vec![]; + for removed in self.index.walk_revs(&base_heads, &own_heads) { + if let Some(new_commit_ids) = new_commit_by_change.get(&removed.change_id()) { + for new_commit_id in new_commit_ids { + rewritten_commits.insert(removed.commit_id(), new_commit_id.clone()); + } + } else { + abandoned_ids.push(removed.commit_id()); + } + } + for removed in self.index.walk_revs(&base_heads, &other_heads) { + if let Some(new_commit_ids) = new_commit_by_change.get(&removed.change_id()) { + for new_commit_id in new_commit_ids { + rewritten_commits.insert(removed.commit_id(), new_commit_id.clone()); + } + } else { + abandoned_ids.push(removed.commit_id()); + } + } + for removed_head in base.heads().difference(other.heads()) { self.view_mut().remove_head(removed_head); } for added_head in other.heads().difference(base.heads()) { self.view_mut().add_head(added_head); } - // TODO: Should it be considered a conflict if a commit-head is removed on one - // side while a child or successor is created on another side? Maybe a - // warning? + + for (old_id, new_id) in rewritten_commits { + self.record_rewritten_commit(old_id, new_id); + } + for abandoned_id in abandoned_ids { + self.record_abandoned_commit(abandoned_id); + } let mut maybe_changed_ref_names = HashSet::new(); diff --git a/lib/tests/test_bad_locking.rs b/lib/tests/test_bad_locking.rs index ab4de258f33..3dec71a0156 100644 --- a/lib/tests/test_bad_locking.rs +++ b/lib/tests/test_bad_locking.rs @@ -109,7 +109,10 @@ fn test_bad_locking_children(use_git: bool) { let machine1_root = TempDir::new().unwrap().into_path(); copy_directory(workspace_root, &machine1_root); let machine1_workspace = Workspace::load(&settings, machine1_root.clone()).unwrap(); - let machine1_repo = machine1_workspace.repo_loader().load_at_head().resolve(); + let machine1_repo = machine1_workspace + .repo_loader() + .load_at_head() + .resolve(&settings); let mut machine1_tx = machine1_repo.start_transaction("test"); let child1 = testutils::create_random_commit(&settings, &machine1_repo) .set_parents(vec![initial.id().clone()]) @@ -120,7 +123,10 @@ fn test_bad_locking_children(use_git: bool) { let machine2_root = TempDir::new().unwrap().into_path(); copy_directory(workspace_root, &machine2_root); let machine2_workspace = Workspace::load(&settings, machine2_root.clone()).unwrap(); - let machine2_repo = machine2_workspace.repo_loader().load_at_head().resolve(); + let machine2_repo = machine2_workspace + .repo_loader() + .load_at_head() + .resolve(&settings); let mut machine2_tx = machine2_repo.start_transaction("test"); let child2 = testutils::create_random_commit(&settings, &machine2_repo) .set_parents(vec![initial.id().clone()]) @@ -132,7 +138,10 @@ fn test_bad_locking_children(use_git: bool) { let merged_path = TempDir::new().unwrap().into_path(); merge_directories(&machine1_root, workspace_root, &machine2_root, &merged_path); let merged_workspace = Workspace::load(&settings, merged_path).unwrap(); - let merged_repo = merged_workspace.repo_loader().load_at_head().resolve(); + let merged_repo = merged_workspace + .repo_loader() + .load_at_head() + .resolve(&settings); assert!(merged_repo.view().heads().contains(child1.id())); assert!(merged_repo.view().heads().contains(child2.id())); let op_id = merged_repo.op_id().clone(); diff --git a/lib/tests/test_commit_concurrent.rs b/lib/tests/test_commit_concurrent.rs index 31521c33b6a..6dfa53e6b74 100644 --- a/lib/tests/test_commit_concurrent.rs +++ b/lib/tests/test_commit_concurrent.rs @@ -61,7 +61,7 @@ fn test_commit_parallel(use_git: bool) { for thread in threads { thread.join().ok().unwrap(); } - let repo = repo.reload_at_head(); + let repo = repo.reload_at_head(&settings); // One commit per thread plus the commit from the initial checkout on top of the // root commit assert_eq!(repo.view().heads().len(), num_threads + 1); diff --git a/lib/tests/test_load_repo.rs b/lib/tests/test_load_repo.rs index 787f24cd690..d75d679173a 100644 --- a/lib/tests/test_load_repo.rs +++ b/lib/tests/test_load_repo.rs @@ -34,7 +34,7 @@ fn test_load_at_operation(use_git: bool) { // If we load the repo at head, we should not see the commit since it was // removed let loader = RepoLoader::init(&settings, repo.repo_path().clone()); - let head_repo = loader.load_at_head().resolve(); + let head_repo = loader.load_at_head().resolve(&settings); assert!(!head_repo.view().heads().contains(commit.id())); // If we load the repo at the previous operation, we should see the commit since diff --git a/lib/tests/test_operations.rs b/lib/tests/test_operations.rs index 614dfb59eaf..1fd3e1c31f0 100644 --- a/lib/tests/test_operations.rs +++ b/lib/tests/test_operations.rs @@ -68,7 +68,7 @@ fn test_consecutive_operations(use_git: bool) { assert_ne!(op_id1, op_id0); assert_eq!(list_dir(&op_heads_dir), vec![op_id1.hex()]); - let repo = repo.reload_at_head(); + let repo = repo.reload_at_head(&settings); let mut tx2 = repo.start_transaction("transaction 2"); testutils::create_random_commit(&settings, &repo).write_to_repo(tx2.mut_repo()); let op_id2 = tx2.commit().operation().id().clone(); @@ -78,7 +78,7 @@ fn test_consecutive_operations(use_git: bool) { // Reloading the repo makes no difference (there are no conflicting operations // to resolve). - let _repo = repo.reload_at_head(); + let _repo = repo.reload_at_head(&settings); assert_eq!(list_dir(&op_heads_dir), vec![op_id2.hex()]); } @@ -115,7 +115,7 @@ fn test_concurrent_operations(use_git: bool) { assert_eq!(actual_heads_on_disk, expected_heads_on_disk); // Reloading the repo causes the operations to be merged - let repo = repo.reload_at_head(); + let repo = repo.reload_at_head(&settings); let merged_op_id = repo.op_id().clone(); assert_ne!(merged_op_id, op_id0); assert_ne!(merged_op_id, op_id1); @@ -175,6 +175,6 @@ fn test_isolation(use_git: bool) { tx2.commit(); assert_heads(repo.as_repo_ref(), vec![initial.id()]); // After reload, the base repo sees both rewrites. - let repo = repo.reload_at_head(); + let repo = repo.reload_at_head(&settings); assert_heads(repo.as_repo_ref(), vec![rewrite1.id(), rewrite2.id()]); } diff --git a/lib/tests/test_view.rs b/lib/tests/test_view.rs index 16f42f9672a..dc4e8cdd53d 100644 --- a/lib/tests/test_view.rs +++ b/lib/tests/test_view.rs @@ -12,6 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. +use jujutsu_lib::commit_builder::CommitBuilder; use jujutsu_lib::op_store::{BranchTarget, RefTarget, WorkspaceId}; use jujutsu_lib::testutils; use jujutsu_lib::testutils::CommitGraphBuilder; @@ -121,7 +122,7 @@ fn test_merge_views_heads() { tx2.mut_repo().add_public_head(&public_head_add_tx2); tx2.commit(); - let repo = repo.reload_at_head(); + let repo = repo.reload_at_head(&settings); let expected_heads = hashset! { head_unchanged.id().clone(), @@ -215,7 +216,7 @@ fn test_merge_views_checkout() { std::thread::sleep(std::time::Duration::from_millis(1)); tx2.commit(); - let repo = repo.reload_at_head(); + let repo = repo.reload_at_head(&settings); // We currently arbitrarily pick the first transaction's checkout (first by // transaction end time). @@ -302,7 +303,7 @@ fn test_merge_views_branches() { ); tx2.commit(); - let repo = repo.reload_at_head(); + let repo = repo.reload_at_head(&settings); let expected_main_branch = BranchTarget { local_target: Some(RefTarget::Conflict { removes: vec![main_branch_local_tx0.id().clone()], @@ -360,7 +361,7 @@ fn test_merge_views_tags() { .set_tag("v1.0".to_string(), RefTarget::Normal(v1_tx2.id().clone())); tx2.commit(); - let repo = repo.reload_at_head(); + let repo = repo.reload_at_head(&settings); let expected_v1 = RefTarget::Conflict { removes: vec![v1_tx0.id().clone()], adds: vec![v1_tx1.id().clone(), v1_tx2.id().clone()], @@ -422,7 +423,7 @@ fn test_merge_views_git_refs() { ); tx2.commit(); - let repo = repo.reload_at_head(); + let repo = repo.reload_at_head(&settings); let expected_main_branch = RefTarget::Conflict { removes: vec![main_branch_tx0.id().clone()], adds: vec![main_branch_tx1.id().clone(), main_branch_tx2.id().clone()], @@ -436,3 +437,111 @@ fn test_merge_views_git_refs() { } ); } + +#[test_case(false ; "rewrite first")] +#[test_case(true ; "add child first")] +fn test_merge_views_child_on_rewritten(child_first: bool) { + // Tests that a child added in one operation get rebased if a concurrent + // operation rewrote the parent. + let settings = testutils::user_settings(); + let test_repo = testutils::init_repo(&settings, false); + + // We start with just commit A. Operation 1 adds commit B on top. Operation 2 + // rewrites A as A2. + let mut tx = test_repo.repo.start_transaction("test"); + let commit_a = + testutils::create_random_commit(&settings, &test_repo.repo).write_to_repo(tx.mut_repo()); + let repo = tx.commit(); + + let mut tx1 = repo.start_transaction("test"); + let commit_b = testutils::create_random_commit(&settings, &repo) + .set_parents(vec![commit_a.id().clone()]) + .write_to_repo(tx1.mut_repo()); + + let mut tx2 = repo.start_transaction("test"); + let commit_a2 = CommitBuilder::for_rewrite_from(&settings, repo.store(), &commit_a) + .set_description("A2".to_string()) + .write_to_repo(tx2.mut_repo()); + tx2.mut_repo().rebase_descendants(&settings); + + // Now commit both transaction. The commit timestamp currently determines the + // merge order, so we check both orders. + let repo = if child_first { + let op_id1 = tx1.commit().op_id().clone(); + let op_id2 = tx2.commit().op_id().clone(); + let repo = repo.reload_at_head(&settings); + // Test the setup + assert_eq!(*repo.operation().parent_ids(), vec![op_id1, op_id2]); + repo + } else { + let op_id2 = tx2.commit().op_id().clone(); + let op_id1 = tx1.commit().op_id().clone(); + let repo = repo.reload_at_head(&settings); + // Test the setup + assert_eq!(*repo.operation().parent_ids(), vec![op_id2, op_id1]); + repo + }; + + // A new B2 commit (B rebased onto A2) should be the only head. + let heads = repo.view().heads(); + assert_eq!(heads.len(), 1); + let b2_id = heads.iter().next().unwrap(); + let commit_b2 = repo.store().get_commit(b2_id).unwrap(); + assert_eq!(commit_b2.change_id(), commit_b.change_id()); + assert_eq!(commit_b2.parent_ids(), vec![commit_a2.id().clone()]); +} + +#[test_case(false ; "abandon first")] +#[test_case(true ; "add child first")] +fn test_merge_views_child_on_abandoned(child_first: bool) { + // Tests that a child added in one operation get rebased if a concurrent + // operation abandoned their parent. + let settings = testutils::user_settings(); + let test_repo = testutils::init_repo(&settings, false); + + // We start with commit B on top of commit A. Operation 1 adds commit C on top. + // Operation 2 abandons B. + let mut tx = test_repo.repo.start_transaction("test"); + let commit_a = + testutils::create_random_commit(&settings, &test_repo.repo).write_to_repo(tx.mut_repo()); + let commit_b = testutils::create_random_commit(&settings, &test_repo.repo) + .set_parents(vec![commit_a.id().clone()]) + .write_to_repo(tx.mut_repo()); + let repo = tx.commit(); + + let mut tx1 = repo.start_transaction("test"); + let commit_c = testutils::create_random_commit(&settings, &repo) + .set_parents(vec![commit_b.id().clone()]) + .write_to_repo(tx1.mut_repo()); + + let mut tx2 = repo.start_transaction("test"); + tx2.mut_repo() + .record_abandoned_commit(commit_b.id().clone()); + tx2.mut_repo().rebase_descendants(&settings); + + // Now commit both transaction. The commit timestamp currently determines the + // merge order, so we check both orders. + let repo = if child_first { + let op_id1 = tx1.commit().op_id().clone(); + let op_id2 = tx2.commit().op_id().clone(); + let repo = repo.reload_at_head(&settings); + // Test the setup + assert_eq!(*repo.operation().parent_ids(), vec![op_id1, op_id2]); + repo + } else { + let op_id2 = tx2.commit().op_id().clone(); + let op_id1 = tx1.commit().op_id().clone(); + let repo = repo.reload_at_head(&settings); + // Test the setup + assert_eq!(*repo.operation().parent_ids(), vec![op_id2, op_id1]); + repo + }; + + // A new C2 commit (C rebased onto A) should be the only head. + let heads = repo.view().heads(); + assert_eq!(heads.len(), 1); + let c2_id = heads.iter().next().unwrap(); + let commit_c2 = repo.store().get_commit(c2_id).unwrap(); + assert_eq!(commit_c2.change_id(), commit_c.change_id()); + assert_eq!(commit_c2.parent_ids(), vec![commit_a.id().clone()]); +} diff --git a/src/commands.rs b/src/commands.rs index 2fc58fb8ee6..732e3c13e45 100644 --- a/src/commands.rs +++ b/src/commands.rs @@ -218,7 +218,8 @@ jj init --git-repo=."; ui, "Concurrent modification detected, resolving automatically.", )?; - unresolved.resolve() + // TODO: Tell the user how many commits were rebased. + unresolved.resolve(ui.settings()) } } } else {