merge: #3958

3958: ReplaceSubgraph interaction with replace_references r=jhelwig a=jhelwig

# Commits
## Log how many pre-existing edges there are, and how many removals/additions were requested when an ExclusiveEdgeMismatch conflict is detected

## Make Update::ReplaceSubgraph only import a single node, instead of the entire subgraph

The problem with importing the entire subgraph is that we also still generate `Update` entries for things below the root of the `Update::ReplaceSubgraph`, and that `find_in_self_or_create_using_onto` + `replace_references` ends up with the uinion of the "child" nodes from   both `self` and `onto` as the child nodes in the resulting graph.

What  we want is either only the child nodes from `onto` with no further `Update` entries for things below the root of the `Update::ReplaceSubgraph`, or for only the specific node referenced in `Update::ReplaceSubgraph` to be replaced, to maintain the pre-existing child nodes in `self`, and to let the `Update` entries for things below the root specified in `Update::ReplaceSubgraph` to handle the rest of the subgraph update/import.

## Do not modify the subgraph of the target of an Update::NewEdge when the edge target already exists

The scenario:

  * `detect_confilcts_and_updates` walks the `onto` graph as its basis for detecting things.
  * If we don't find any candidates for equivalency for a particular node in `onto` from the nodes in `to_rebase`, then we do nothing, and prune the walk of the `onto` graph at that point.
  * When processing a container in `find_container_membership_conflicts_and_updates` (processing a/the parent of the potentially new node).
  * There is an entry in `only_onto_edges` for the edge pointing to a node. (May or may not be to a new node)
      * We generate an `Update::NewEdge`
  * If the node exists already in `to_rebase` (not new)
    * When processing the DFS event for the node, we continue walking the `onto` graph through this node as long as we continue to find equivalent nodes in `to_rebase`, generating `Update`s as necessary.
  * If the node does _not_ already exist in `to_rebase` (it's new)
    * When processing the DFS event for the node, we prune the `onto` graph walk as we could not find an equivalent node, and do not generate any `Update` for the node that only exists in `onto`.

So, when `Update::NewEdge` happens in `perform_updates`, if an equivalent node is found, we want to use it as-is, since there should be other `Update` to process that will handle any updates necessary for the target (and any children). If no equivalent node is found, then we want to import the "new" bits from the subgraph starting at the target of the edge, since there won't be any `Update` to process that will handle bringing in those new things.


Co-authored-by: Jacob Helwig <[email protected]>

lib/dal/src/workspace_snapshot/graph.rs

@@ -740,6 +740,12 @@ impl WorkspaceSnapshotGraph {
                 if edge_kind_updates.additions.len()
                     > (edge_kind_updates.removals.len() + (1 - existing_outgoing_edges_of_kind))
                 {
+                    warn!(
+                        "ExclusiveEdgeMismatch: Found {} pre-existing edges. Requested {} removals, {} additions.",
+                        existing_outgoing_edges_of_kind,
+                        edge_kind_updates.removals.len(),
+                        edge_kind_updates.additions.len(),
+                    );
                     // The net count of outgoing edges of this kind is >1. Consider *ALL* of the
                     // additions to be in conflict.
                     for (
@@ -1605,6 +1611,36 @@ impl WorkspaceSnapshotGraph {
         algo::has_path_connecting(&self.graph, self.root_index, node, None)
     }
 
+    /// Import a single node from the `other` [`WorkspaceSnapshotGraph`] into `self`, if it does
+    /// not already exist in `self`.
+    pub fn find_in_self_or_import_node_from_other(
+        &mut self,
+        other: &WorkspaceSnapshotGraph,
+        other_node_index: NodeIndex,
+    ) -> WorkspaceSnapshotGraphResult<()> {
+        let node_weight_to_import = other.get_node_weight(other_node_index)?.clone();
+        let node_weight_id = node_weight_to_import.id();
+        let node_weight_lineage_id = node_weight_to_import.lineage_id();
+
+        if let Some(equivalent_node_index) =
+            self.find_equivalent_node(node_weight_id, node_weight_lineage_id)?
+        {
+            let equivalent_node_weight = self.get_node_weight(equivalent_node_index)?;
+            if !equivalent_node_weight
+                .vector_clock_write()
+                .is_newer_than(node_weight_to_import.vector_clock_write())
+            {
+                self.add_node(node_weight_to_import)?;
+
+                self.replace_references(equivalent_node_index)?;
+            }
+        } else {
+            self.add_node(node_weight_to_import)?;
+        };
+
+        Ok(())
+    }
+
     pub fn import_subgraph(
         &mut self,
         other: &WorkspaceSnapshotGraph,
@@ -2079,7 +2115,7 @@ impl WorkspaceSnapshotGraph {
                 } => {
                     let updated_source = self.get_latest_node_idx(source.index)?;
                     let destination =
-                        self.find_in_self_or_create_using_onto(destination.index, onto)?;
+                        self.reuse_from_self_or_import_subgraph_from_onto(destination.index, onto)?;
 
                     self.add_edge(updated_source, edge_weight.clone(), destination)?;
                 }
@@ -2103,7 +2139,14 @@ impl WorkspaceSnapshotGraph {
                 } => {
                     let updated_to_rebase =
                         self.get_latest_node_idx(to_rebase_subgraph_root.index)?;
-                    self.find_in_self_or_create_using_onto(onto_subgraph_root.index, onto)?;
+                    // We really only want to import the single node here, not the entire subgraph,
+                    // as we also generate Updates on how to handle bringing in the child
+                    // nodes. If we try to bring in the entire subgraph, we'll have a bad
+                    // interaction with `replace_references` where we'll end up with the set
+                    // union of the edges outgoing from both the "old" and "new" node. We only
+                    // want the edges from the "old" node as we'll have further Update events
+                    // that will handle bringing in the appropriate "new" edges.
+                    self.find_in_self_or_import_node_from_other(onto, onto_subgraph_root.index)?;
                     self.replace_references(updated_to_rebase)?;
                 }
                 Update::MergeCategoryNodes {
@@ -2175,8 +2218,8 @@ impl WorkspaceSnapshotGraph {
         Ok(self.get_node_index_by_id(other_id).ok())
     }
 
-    /// Find in self where self is the "to rebase" side or create using "onto".
-    fn find_in_self_or_create_using_onto(
+    /// Find in self (and use it as-is) where self is the "to rebase" side or import the entire subgraph from "onto".
+    fn reuse_from_self_or_import_subgraph_from_onto(
         &mut self,
         unchecked: NodeIndex,
         onto: &WorkspaceSnapshotGraph,
@@ -2196,19 +2239,7 @@ impl WorkspaceSnapshotGraph {
             };
 
             match equivalent_node {
-                Some(found_equivalent_node) => {
-                    let found_equivalent_node_weight =
-                        self.get_node_weight(found_equivalent_node)?;
-                    if found_equivalent_node_weight.merkle_tree_hash()
-                        != unchecked_node_weight.merkle_tree_hash()
-                    {
-                        self.import_subgraph(onto, unchecked)?;
-                        self.find_latest_idx_in_self_from_other_idx(onto, unchecked)?
-                            .ok_or(WorkspaceSnapshotGraphError::NodeWeightNotFound)?
-                    } else {
-                        found_equivalent_node
-                    }
-                }
+                Some(found_equivalent_node) => found_equivalent_node,
                 None => {
                     self.import_subgraph(onto, unchecked)?;
                     self.find_latest_idx_in_self_from_other_idx(onto, unchecked)?

lib/dal/src/workspace_snapshot/update.rs

@@ -21,6 +21,8 @@ pub enum Update {
         destination: NodeInformation,
         edge_kind: EdgeWeightKindDiscriminants,
     },
+    // This is not correctly named. We really only want to replace the single node, as we also
+    // generate Update entries to handle processing the rest of the subgraph.
     ReplaceSubgraph {
         onto: NodeInformation,
         // Check if already exists in "onto". Grab node weight from "to_rebase" and see if there is