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

CHANGELOG.md

@@ -24,6 +24,15 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 * The [standard `$NO_COLOR` environment variable](https://no-color.org/) is now
   respected.
 
+* (#111) When undoing an earlier operation, any new commits on top of commits
+  from the undo operation will be rebased away. For example, let's say you
+  rebase commit A so it becomes a new commit A', and then you create commit B
+  on top of A'. If you now undo the rebase operation, commit B will be rebased
+  to be on top of A instead. The same logic is used if the repo was modified
+  by concurrent operations (so if one operation added B on top of A, and one
+  operation rebased A as A', then B would be automatically rebased on top of
+  A'). See #111 for more examples.
+
 ## [0.3.3] - 2022-03-16
 
 No changes, only trying to get the automated build to work.

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,55 @@ impl MutableRepo {
             self.view_mut().add_public_head(added_head);
         }
 
-        for removed_head in base.heads().difference(other.heads()) {
-            self.view_mut().remove_head(removed_head);
+        // TODO: This code is a bit repetitive...
+        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 (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);
+        }
+
+        // No need to remove heads removed by other because we already marked them abandoned or
+        // rewritten.
         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?
 
         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_rewrite.rs

@@ -706,7 +706,7 @@ fn test_rebase_descendants_divergent_rewrite(use_git: bool) {
 
     // Commit B was replaced by commit B2. Commit D was replaced by commits D2 and
     // D3. Commit F was replaced by commit F2. Commit C should be rebased onto
-    // B2. Commit E should not be rebased. Commit F should be rebased onto
+    // B2. Commit E should not be rebased. Commit G should be rebased onto
     // commit F2.
     //
     // G