From d62cc15ff0466d1660c47a9e9cfb1b8523395e10 Mon Sep 17 00:00:00 2001
From: Martin von Zweigbergk <martinvonz@google.com>
Date: Sat, 26 Mar 2022 10:16:23 -0700
Subject: [PATCH] repo: move merge with operation to transaction (#111)

It's the transaction's job to create a new operation, and that's where
the knowledge of parent operations is. By moving the logic for merging
in another operation there, we can make it contiuously update its set
of parent operations. That removes the risk of forgetting to add the
merged-in operation as a parent. It also makes it easier to reuse the
function from the CLI so we can inform the user about the process
(which is what I was investigating when I noticed that this cleanup
was possible).
---
 lib/src/repo.rs        | 19 +++----------------
 lib/src/transaction.rs | 23 +++++++++++++++++++----
 2 files changed, 22 insertions(+), 20 deletions(-)

diff --git a/lib/src/repo.rs b/lib/src/repo.rs
index bb8d8c416..d3cf7736f 100644
--- a/lib/src/repo.rs
+++ b/lib/src/repo.rs
@@ -26,7 +26,7 @@ use thiserror::Error;
 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};
+use crate::dag_walk::topo_order_reverse;
 use crate::index::{IndexRef, MutableIndex, ReadonlyIndex};
 use crate::index_store::IndexStore;
 use crate::op_heads_store::{LockedOpHeads, OpHeads, OpHeadsStore};
@@ -392,22 +392,9 @@ impl RepoLoader {
         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");
-        let merged_repo = tx.mut_repo();
-        let neighbors_fn = |op: &Operation| op.parents();
-        for (i, other_op_head) in op_heads.iter().enumerate().skip(1) {
-            let ancestor_op = closest_common_node(
-                op_heads[0..i].to_vec(),
-                vec![other_op_head.clone()],
-                &neighbors_fn,
-                &|op: &Operation| op.id().clone(),
-            )
-            .unwrap();
-            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);
+        for other_op_head in op_heads.into_iter().skip(1) {
+            tx.merge_operation(user_settings, other_op_head);
         }
-        tx.set_parent_ops(op_heads);
         tx.write()
     }
 
diff --git a/lib/src/transaction.rs b/lib/src/transaction.rs
index 1f1e92a1f..a59cd6b6f 100644
--- a/lib/src/transaction.rs
+++ b/lib/src/transaction.rs
@@ -16,11 +16,13 @@ use std::collections::HashMap;
 use std::sync::Arc;
 
 use crate::backend::Timestamp;
+use crate::dag_walk::closest_common_node;
 use crate::index::ReadonlyIndex;
 use crate::op_store;
 use crate::op_store::OperationMetadata;
 use crate::operation::Operation;
 use crate::repo::{MutableRepo, ReadonlyRepo, RepoLoader};
+use crate::settings::UserSettings;
 use crate::view::View;
 
 pub struct Transaction {
@@ -47,10 +49,6 @@ impl Transaction {
         self.repo.as_ref().unwrap().base_repo()
     }
 
-    pub fn set_parent_ops(&mut self, parent_ops: Vec<Operation>) {
-        self.parent_ops = parent_ops;
-    }
-
     pub fn set_tag(&mut self, key: String, value: String) {
         self.tags.insert(key, value);
     }
@@ -59,6 +57,23 @@ impl Transaction {
         self.repo.as_mut().unwrap()
     }
 
+    pub fn merge_operation(&mut self, user_settings: &UserSettings, other_op: Operation) {
+        let ancestor_op = closest_common_node(
+            self.parent_ops.clone(),
+            vec![other_op.clone()],
+            &|op: &Operation| op.parents(),
+            &|op: &Operation| op.id().clone(),
+        )
+        .unwrap();
+        let repo_loader = self.base_repo().loader();
+        let base_repo = repo_loader.load_at(&ancestor_op);
+        let other_repo = repo_loader.load_at(&other_op);
+        self.parent_ops.push(other_op);
+        let merged_repo = self.mut_repo();
+        merged_repo.merge(&base_repo, &other_repo);
+        merged_repo.rebase_descendants(user_settings);
+    }
+
     /// Writes the transaction to the operation store and publishes it.
     pub fn commit(self) -> Arc<ReadonlyRepo> {
         self.write().publish()