repo: when merging in removed head, rebase descendants (#111)

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<ReadonlyRepo> {
         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<ReadonlyRepo> {
-        self.loader().load_at_head().resolve()
+    pub fn reload_at_head(&self, user_settings: &UserSettings) -> Arc<ReadonlyRepo> {
+        self.loader().load_at_head().resolve(user_settings)
     }
 
     pub fn reload_at(&self, operation: &Operation) -> Arc<ReadonlyRepo> {
@@ -293,10 +294,10 @@ pub enum RepoAtHead {
 }
 
 impl RepoAtHead {
-    pub fn resolve(self) -> Arc<ReadonlyRepo> {
+    pub fn resolve(self, user_settings: &UserSettings) -> Arc<ReadonlyRepo> {
         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<ReadonlyRepo> {
+    pub fn resolve(self, user_settings: &UserSettings) -> Arc<ReadonlyRepo> {
         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<Operation>) -> UnpublishedOperation {
+    fn merge_op_heads(
+        &self,
+        user_settings: &UserSettings,
+        mut op_heads: Vec<Operation>,
+    ) -> 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<ChangeId, Vec<CommitId>> = 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();
 

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

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

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

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()]);
 }

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()]);
+}