Improve tip-pruning when extra-commits are set.

Author: Byron

crates/but-graph/src/init/mod.rs

@@ -253,8 +253,6 @@ impl Graph {
         } = options;
 
         let max_limit = Limit::new(limit);
-        // TODO: also traverse (outside)-branches that ought to be in the workspace. That way we have the desired ones
-        //       automatically and just have to find a way to prune the undesired ones.
         let ref_name = ref_name.into();
         if ref_name
             .as_ref()
@@ -454,7 +452,6 @@ impl Graph {
                 // have to adjust the existing queue item.
                 existing_segment
             } else {
-                // TODO: test in graph
                 let extra_target_sidx = graph.insert_root(branch_segment_from_name_and_meta(
                     None,
                     meta,

crates/but-graph/src/init/types.rs

@@ -27,7 +27,6 @@ impl Limit {
     /// Keep queueing without limit until `goal` is seen in a commit that has **it ahead of itself**.
     /// Then stop searching for that goal.
     /// `goals` are used to keep track of existing bitflags.
-    /// `origin` is used to know where the search for `goal` came from.
     ///
     /// ### Note
     ///
@@ -331,6 +330,9 @@ pub struct TopoWalk {
     direction: Direction,
     /// If `true`, don't return the first segment which is always the starting point.
     skip_tip: Option<()>,
+    /// If this is set during the iteration, we will store segment ids which didn't have any outgoing or
+    /// incoming connections, depending on the direction of traversal.
+    pub leafs: Option<Vec<SegmentIndex>>,
 }
 
 /// Lifecycle
@@ -351,6 +353,7 @@ impl TopoWalk {
             seen_empty_segments: Default::default(),
             direction,
             skip_tip: None,
+            leafs: None,
         }
     }
 }
@@ -392,7 +395,9 @@ impl TopoWalk {
         let (segment, first_commit_index) = self.next.pop_front()?;
         let available_range = self.select_range(graph, segment, first_commit_index)?;
 
+        let mut count = 0;
         for edge in graph.edges_directed(segment, self.direction) {
+            count += 1;
             match self.direction {
                 Direction::Outgoing => {
                     if edge
@@ -417,6 +422,9 @@ impl TopoWalk {
                 }
             }
         }
+        if let Some(leafs) = self.leafs.as_mut().filter(|_| count == 0) {
+            leafs.push(segment);
+        }
         Some((segment, available_range))
     }
 

crates/but-graph/src/init/walk.rs

@@ -679,13 +679,16 @@ pub fn propagate_flags_downward(
     flags_to_add: CommitFlags,
     dst_sidx: SegmentIndex,
     dst_commit: Option<CommitIndex>,
-) {
+    needs_leafs: bool,
+) -> Option<Vec<SegmentIndex>> {
     let mut topo = TopoWalk::start_from(dst_sidx, dst_commit, petgraph::Direction::Outgoing);
+    topo.leafs = needs_leafs.then(Vec::new);
     while let Some((segment, commit_range)) = topo.next(graph) {
         for commit in &mut graph[segment].commits[commit_range] {
             commit.flags |= flags_to_add;
         }
     }
+    topo.leafs.take().filter(|v| !v.is_empty())
 }
 
 /// Check `refs` for refs with remote tracking branches, and return a queue for them for traversal after creating a segment
@@ -814,12 +817,33 @@ pub fn possibly_split_occupied_segment(
     let new_flags = propagated_flags | top_flags | bottom_flags;
 
     // Only propagate if there is something new as propagation is slow
-    if new_flags != bottom_flags {
-        propagate_flags_downward(&mut graph.inner, new_flags, bottom_sidx, Some(bottom_cidx));
+    // Note that we currently assume that the flags are different also to get leafs, even though these
+    // depend on flags to propagate. This will be an issue, but seems to work well for all known cases.
+    let needs_leafs = !limit.goal_reached();
+    let leafs = if new_flags != bottom_flags
+        || (needs_leafs
+            && next
+                .iter()
+                .any(|(_, _, _, tip_limit)| !tip_limit.goal_flags().contains(limit.goal_flags())))
+    {
+        propagate_flags_downward(
+            &mut graph.inner,
+            new_flags,
+            bottom_sidx,
+            Some(bottom_cidx),
+            needs_leafs,
+        )
+    } else {
+        None
+    };
+
+    if let Some(leafs) = leafs {
+        propagate_goals_of_reachable_tips(next, leafs, limit.goal_flags());
     }
 
     // Find the tips that saw this commit, and adjust their limit it that would extend it.
     // The commit is the one we hit, but seen from the newly split segment which should never be empty.
+    // TODO: merge this into the new propagation based one.
     let bottom_commit_goals = Limit::new(None)
         .additional_goal(
             graph[bottom_sidx]
@@ -840,7 +864,21 @@ pub fn possibly_split_occupied_segment(
     Ok(())
 }
 
-/// Remove if there are only tips with integrated commits and delete empty segments of pruned tips,
+// Find all tips that are scheduled to be put into one of the `reachable_segments`
+// (reachable from the commit we just ran into)
+fn propagate_goals_of_reachable_tips(
+    next: &mut Queue,
+    reachable_segments: Vec<SegmentIndex>,
+    goal_flags: CommitFlags,
+) {
+    for (_, _, instruction, limit) in next.iter_mut() {
+        if reachable_segments.contains(&instruction.segment_idx()) {
+            *limit = limit.additional_goal(goal_flags);
+        }
+    }
+}
+
+/// Remove tips with only integrated commits and delete empty segments of pruned tips,
 /// as these are uninteresting.
 /// However, do so only if our entrypoint isn't integrated itself and is not in a workspace. The reason for this is that we
 /// always also traverse workspaces and their targets, even if the traversal starts outside a workspace.

crates/but-graph/tests/graph/init/with_workspace.rs

@@ -1777,39 +1777,32 @@ fn integrated_tips_stop_early_if_remote_is_not_configured() -> anyhow::Result<()
     │                   └── ·777b552 (⌂|🏘|✓|1)
     │                       └── ►:6[3]:anon:
     │                           └── ·ce4a760 (⌂|🏘|✓|1)
-    │                               ├── ►:7[5]:anon:
-    │                               │   └── ·01d0e1e (⌂|🏘|✓|1)
-    │                               │       └── ►:5[6]:main
-    │                               │           ├── ·4b3e5a8 (⌂|🏘|✓|1)
-    │                               │           ├── ·34d0715 (⌂|🏘|✓|1)
-    │                               │           └── ·eb5f731 (⌂|🏘|✓|1)
+    │                               ├── ►:7[4]:anon:
+    │                               │   └── ✂·01d0e1e (⌂|🏘|✓|1)
     │                               └── ►:8[4]:A-feat
-    │                                   ├── ·fea59b5 (⌂|🏘|✓|1)
-    │                                   └── ·4deea74 (⌂|🏘|✓|1)
-    │                                       └── →:7:
+    │                                   └── ✂·fea59b5 (⌂|🏘|✓|1)
     └── ►:2[0]:origin/main
         ├── 🟣d0df794 (✓)
         └── 🟣09c6e08 (✓)
             └── ►:4[1]:anon:
                 └── 🟣7b9f260 (✓)
-                    ├── →:5: (main)
+                    ├── ►:5[2]:main
+                    │   ├── 🟣4b3e5a8 (✓)
+                    │   ├── 🟣34d0715 (✓)
+                    │   └── 🟣eb5f731 (✓)
                     └── →:0: (A)
     ");
     // Because the branch is integrated, the surrounding workspace isn't shown.
     insta::assert_snapshot!(graph_workspace(&graph.to_workspace()?), @r"
     ⌂:0:A <> ✓!
     └── ≡:0:A
-        ├── :0:A
-        │   ├── ·79bbb29 (🏘️|✓)
-        │   ├── ·fc98174 (🏘️|✓)
-        │   ├── ·a381df5 (🏘️|✓)
-        │   ├── ·777b552 (🏘️|✓)
-        │   ├── ·ce4a760 (🏘️|✓)
-        │   └── ·01d0e1e (🏘️|✓)
-        └── :5:main
-            ├── ·4b3e5a8 (🏘️|✓)
-            ├── ·34d0715 (🏘️|✓)
-            └── ·eb5f731 (🏘️|✓)
+        └── :0:A
+            ├── ·79bbb29 (🏘️|✓)
+            ├── ·fc98174 (🏘️|✓)
+            ├── ·a381df5 (🏘️|✓)
+            ├── ·777b552 (🏘️|✓)
+            ├── ·ce4a760 (🏘️|✓)
+            └── ✂️·01d0e1e (🏘️|✓)
     ");
 
     // See what happens with an out-of-workspace HEAD and an arbitrary extra target.
@@ -2169,8 +2162,7 @@ fn workspace_obeys_limit_when_target_branch_is_missing() -> anyhow::Result<()> {
     │           └── ·03ad472 (⌂|🏘|1)
     │               └── ►:4[2]:A
     │                   ├── ·79bbb29 (⌂|🏘|✓|1)
-    │                   ├── ·fc98174 (⌂|🏘|✓|1)
-    │                   └── ✂·a381df5 (⌂|🏘|✓|1)
+    │                   └── ✂·fc98174 (⌂|🏘|✓|1)
     └── ►:1[0]:origin/main
         ├── 🟣d0df794 (✓)
         └── 🟣09c6e08 (✓)
@@ -2403,12 +2395,13 @@ fn partitions_with_long_and_short_connections_to_each_other() -> anyhow::Result<
     ");
 
     add_workspace(&mut meta);
-    let (id, ref_name) = id_at(&repo, "main");
+    let (main_id, main_ref_name) = id_at(&repo, "main");
     // Validate that we will perform long searches to connect connectable segments, without interfering
     // with other searches that may take even longer.
     // Also, without limit, we should be able to see all of 'main' without cut-off
-    let graph = Graph::from_commit_traversal(id, ref_name.clone(), &*meta, standard_options())?
-        .validated()?;
+    let graph =
+        Graph::from_commit_traversal(main_id, main_ref_name.clone(), &*meta, standard_options())?
+            .validated()?;
     insta::assert_snapshot!(graph_tree(&graph), @r"
     ├── 📕►►►:1[0]:gitbutler/workspace
     │   └── ·41ed0e4 (⌂|🏘)
@@ -2473,9 +2466,13 @@ fn partitions_with_long_and_short_connections_to_each_other() -> anyhow::Result<
     // When setting a limit when traversing 'main', it is respected.
     // We still want it to be found and connected though, and it's notable that the limit kicks in
     // once everything reconciled.
-    let graph =
-        Graph::from_commit_traversal(id, ref_name, &*meta, standard_options().with_limit_hint(1))?
-            .validated()?;
+    let graph = Graph::from_commit_traversal(
+        main_id,
+        main_ref_name,
+        &*meta,
+        standard_options().with_limit_hint(1),
+    )?
+    .validated()?;
     insta::assert_snapshot!(graph_tree(&graph), @r"
     ├── 📕►►►:1[0]:gitbutler/workspace
     │   └── ·41ed0e4 (⌂|🏘)
@@ -2488,12 +2485,7 @@ fn partitions_with_long_and_short_connections_to_each_other() -> anyhow::Result<
     │               │           ├── ·f49c977 (⌂|🏘|✓|1)
     │               │           ├── ·7b7ebb2 (⌂|🏘|✓|1)
     │               │           ├── ·dca4960 (⌂|🏘|✓|1)
-    │               │           ├── ·11c29b8 (⌂|🏘|✓|1)
-    │               │           ├── ·c32dd03 (⌂|🏘|✓|1)
-    │               │           ├── ·b625665 (⌂|🏘|✓|1)
-    │               │           ├── ·a821094 (⌂|🏘|✓|1)
-    │               │           ├── ·bce0c5e (⌂|🏘|✓|1)
-    │               │           └── ·3183e43 (⌂|🏘|✓|1)
+    │               │           └── ✂·11c29b8 (⌂|🏘|✓|1)
     │               └── ►:7[3]:long-main-to-workspace
     │                   ├── ·77f31a0 (⌂|🏘|✓)
     │                   ├── ·eb17e31 (⌂|🏘|✓)
@@ -2529,12 +2521,7 @@ fn partitions_with_long_and_short_connections_to_each_other() -> anyhow::Result<
             ├── ·f49c977 (🏘️|✓)
             ├── ·7b7ebb2 (🏘️|✓)
             ├── ·dca4960 (🏘️|✓)
-            ├── ·11c29b8 (🏘️|✓)
-            ├── ·c32dd03 (🏘️|✓)
-            ├── ·b625665 (🏘️|✓)
-            ├── ·a821094 (🏘️|✓)
-            ├── ·bce0c5e (🏘️|✓)
-            └── ·3183e43 (🏘️|✓)
+            └── ✂️·11c29b8 (🏘️|✓)
     ");
 
     // From the workspace, even without limit, we don't traverse all of 'main' as it's uninteresting.