Minor refactors

- lints
- test fixtures inline
- make test harder so it could run past the desired tip

Author: Byron

crates/but-graph/src/api.rs

@@ -76,14 +76,14 @@ impl Graph {
     /// Compute the first merge-base between two segments (may not be the lowest).
     ///
     /// **Warning**: This finds the first common ancestor encountered during traversal,
-    /// which may not be the correct merge-base when there are multiple paths between
-    /// segments (e.g., via merge commits). Use [`lowest_merge_base`](Self::lowest_merge_base)
-    /// for correct results in all cases.
+    /// which may not be the *desired* merge-base when there are multiple paths between
+    /// segments (e.g., via merge commits). Use [`lowest_merge_base`](Self::find_lowest_merge_base)
+    /// if in doubt.
     ///
     /// The segment representing the merge-base is expected to not be empty, as its first commit
     /// is usually what one is interested in.
     // TODO: should be multi, with extra segments as third parameter
-    pub fn first_merge_base(&self, a: SegmentIndex, b: SegmentIndex) -> Option<SegmentIndex> {
+    pub fn first_first_merge_base(&self, a: SegmentIndex, b: SegmentIndex) -> Option<SegmentIndex> {
         // TODO(perf): improve this by allowing to set bitflags on the segments themselves, to allow
         //       marking them accordingly, just like Git does.
         //       Right now we 'emulate' bitflags on pre-allocated data with two data sets, expensive
@@ -122,7 +122,7 @@ impl Graph {
 
     /// Compute the lowest merge-base between two segments.
     ///
-    /// Unlike [`first_merge_base`](Self::first_merge_base), this finds a common ancestor
+    /// Unlike [`first_merge_base`](Self::first_first_merge_base), this finds a common ancestor
     /// with "no path around it" - meaning ALL paths from both `a` and `b` must pass through
     /// the returned segment.
     ///
@@ -133,19 +133,24 @@ impl Graph {
     ///
     /// The segment representing the merge-base is expected to not be empty, as its first commit
     /// is usually what one is interested in.
-    pub fn lowest_merge_base(&self, a: SegmentIndex, b: SegmentIndex) -> Option<SegmentIndex> {
+    pub fn find_lowest_merge_base(&self, a: SegmentIndex, b: SegmentIndex) -> Option<SegmentIndex> {
         // Each walker tracks its origin and all nodes it has visited
         #[derive(Clone)]
         struct Walker {
+            // The segment the walker resides on currently.
             position: SegmentIndex,
+            // The color or flag used to mark segments.
             origin: u8, // COLOR_A or COLOR_B
+            // All segments that this walker has visited.
+            // TODO: could be better with bitflags.
             visited: BTreeSet<SegmentIndex>,
         }
 
         const COLOR_A: u8 = 1;
         const COLOR_B: u8 = 2;
 
         // Track which nodes have been visited by A-origin and B-origin walkers
+        // TODO: remove these duplicates - they are effectively
         let mut nodes_a: BTreeSet<SegmentIndex> = BTreeSet::new();
         let mut nodes_b: BTreeSet<SegmentIndex> = BTreeSet::new();
 
@@ -187,10 +192,11 @@ impl Graph {
 
                 for (&node, &generation) in &merged_nodes {
                     let in_all = walkers.iter().all(|w| w.visited.contains(&node));
-                    if in_all {
-                        if common_merged.is_none() || generation > common_merged.unwrap().1 {
-                            common_merged = Some((node, generation));
-                        }
+                    if in_all
+                        && common_merged
+                            .is_none_or(|(_sid, common_generation)| generation > common_generation)
+                    {
+                        common_merged = Some((node, generation));
                     }
                 }
 
@@ -200,6 +206,8 @@ impl Graph {
             }
 
             // Expand all walkers
+            // TODO: no, no, no, something is very strange here.
+            //       This should just be a stack of segments to visit.
             let mut next_walkers: Vec<Walker> = Vec::new();
 
             for walker in walkers {

crates/but-graph/src/projection/workspace.rs

@@ -562,7 +562,7 @@ impl Graph {
                 } else {
                     self.inner
                         .neighbors_directed(ws_tip_segment.id, Direction::Outgoing)
-                        .reduce(|a, b| self.first_merge_base(a, b).unwrap_or(a))
+                        .reduce(|a, b| self.first_first_merge_base(a, b).unwrap_or(a))
                         .and_then(|base| self[base].commits.first().map(|c| (c.id, base)))
                 }
             })
@@ -813,17 +813,15 @@ impl Graph {
             ComputeBaseTip::SingleBranch(tip) => (vec![tip], tip),
         };
         let mut count = 0;
-        let all_segments: Vec<_> = tips
+        let all_segments = tips
             .into_iter()
             .chain(target_ref.map(|t| t.segment_index))
             .chain(target_commit.map(|t| t.segment_index))
-            .chain(additional)
-            .collect();
+            .chain(additional);
 
         let base = all_segments
-            .into_iter()
             .inspect(|_| count += 1)
-            .reduce(|a, b| self.lowest_merge_base(a, b).unwrap_or(a))?;
+            .reduce(|a, b| self.find_lowest_merge_base(a, b).unwrap_or(a))?;
 
         if count < 2 || base == actual_tip {
             match tip {

crates/but-graph/tests/fixtures/scenarios.sh

@@ -179,29 +179,26 @@ git init four-diamond
 # A graph that demonstrates the merge-base bug: there's a "path around" the first
 # common ancestor found.
 #
-# This creates a diamond structure with a path around mid_a:
+# This creates a diamond structure with a path around mid_a via `mid_c`:
 #
-#       A                      B
-#       │                      │
-#       ▼                      ▼
-#     mid_a ◄───────────────── M (merge of mid_a and mid_c)
-#       │                     / \
-#       │                    ▼   ▼
-#       ▼                        mid_c
-#     base ◄─────────────────────┘
-#
-# The segment graph topology:
-# - seg_A -> seg_mid_a -> seg_base
-# - seg_B -> seg_mid_a (first parent of M) -> seg_base
-# - seg_B -> seg_mid_c (second parent of M) -> seg_base
-#
-# Key points:
-# - first_merge_base(A, B) returns mid_a (the first common ancestor encountered)
-# - But mid_a has a "path around" it: B can reach base via mid_c without going through mid_a
-# - lowest_merge_base(A, B) should return base (the dominator with no path around)
-
+#   A                   B
+#   │                   │
+# mid_a ─────────────── M (merge commit)
+#   │                 ╱   ╲
+#   │               ╱       ╲
+#   │             ╱           ╲
+#   │           ╱            mid_c
+#   │         ╱                │
+#   │       ╱                  │
+# main ◄───────────────────────┘#
+#   │
+#   M2
+#   │
+#   M1
 git init merge-base-path-around
 (cd merge-base-path-around
+  commit M1
+  commit M2
   # base commit - the true dominator
   commit base
 

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

@@ -1,5 +1,6 @@
 use but_graph::Graph;
 use but_testsupport::{graph_tree, graph_workspace, visualize_commit_graph_all};
+use gix::refs::Category;
 
 #[test]
 fn unborn() -> anyhow::Result<()> {
@@ -410,111 +411,100 @@ fn four_diamond() -> anyhow::Result<()> {
     Ok(())
 }
 
-/// This test demonstrates a bug in `first_merge_base`: it finds the first common
-/// ancestor encountered during traversal, but that's not necessarily the merge-base
-/// with "no paths around it."
+/// This test demonstrates the difference between `find_first_merge_base` and `find_lowest_merge_base`
+/// where the former finds the first common ancestor encountered during traversal,
+/// but that's not necessarily the merge-base with "no paths around it.
 ///
 /// The `merge-base-path-around` fixture creates this git commit graph:
 /// ```text
-///       A                      B
-///       │                      │
-///       ▼                      ▼
-///     mid_a ◄───────────────── M (merge of mid_a and mid_c)
-///       │                     / \
-///       │                    ▼   ▼
-///       ▼                        mid_c
-///     base ◄─────────────────────┘
+/// A                   B
+/// │                   │
+/// mid_a ─────────────── M (merge commit)
+/// │                 ╱   ╲
+/// │               ╱       ╲
+/// │             ╱           ╲
+/// │           ╱            mid_c
+/// │         ╱                │
+/// │       ╱                  │
+/// main ◄─────────────────────┘
 /// ```
 ///
-/// B is a merge commit with two parents: mid_a and mid_c.
-/// A sits on top of mid_a, which sits on base.
-/// mid_c is a sibling branch that also connects to base.
+/// `B` is a merge commit with two parents: `mid_a` and `mid_c`.
+/// A sits on top of `mid_a`, which sits on `main`.
+/// `mid_c` is a sibling branch that also connects to `main`.
 ///
-/// When finding merge-base of segments A and B:
-/// - Current (buggy) behavior: finds mid_a (first common ancestor encountered)
-/// - Correct behavior: finds base (the only point with no path around it)
+/// When finding merge-base of segments `A` and `B`:
+/// - undesired behavior: finds `mid_a` (first common ancestor encountered)
+/// - Desired behavior: finds `main` (the only point with no path around it)
 ///
-/// The issue is that mid_a has a "path around it": B can reach base via mid_c
-/// without going through mid_a. Therefore, mid_a is not the true merge-base
+/// The issue is that `mid_a` has a "path around it": `B` can reach `main` via `mid_c`
+/// without going through `mid_a`. Therefore, `mid_a` is not the true merge-base
 /// where ALL paths from both segments converge.
 #[test]
-fn first_merge_base_bug_path_around() -> anyhow::Result<()> {
+fn lowest_vs_first_merge_base() -> anyhow::Result<()> {
     let (repo, meta) = read_only_in_memory_scenario("merge-base-path-around")?;
-    insta::assert_snapshot!(visualize_commit_graph_all(&repo)?);
+    insta::assert_snapshot!(visualize_commit_graph_all(&repo)?, @"
+    * dceb229 (HEAD -> A) A
+    | * a9cdc20 (B) B merges mid_a and mid_c
+    |/| 
+    | * dcc619e (mid_c) mid_c
+    * | 9e44c07 (mid_a) mid_a
+    |/  
+    * ce1ecf3 (main) base
+    * bce0c5e M2
+    * 3183e43 M1
+    ");
 
     // Use B as an extra target so it's included in the graph
     let graph = Graph::from_head(
         &repo,
         &*meta,
         standard_options_with_extra_target(&repo, "B"),
     )?;
-    insta::assert_snapshot!(graph_tree(&graph));
+    insta::assert_snapshot!(graph_tree(&graph), @"
+
+    ├── 👉►:0[0]:A[🌳]
+    │   └── ·dceb229 (⌂|1)
+    │       └── ►:2[1]:mid_a
+    │           └── ·9e44c07 (⌂|✓|1)
+    │               └── ►:4[2]:main
+    │                   ├── ·ce1ecf3 (⌂|✓|1)
+    │                   └── ✂·bce0c5e (⌂|✓|1)
+    └── ►:1[0]:B
+        └── 🟣a9cdc20 (✓)
+            ├── →:2: (mid_a)
+            └── ►:3[1]:mid_c
+                └── 🟣dcc619e (✓)
+                    └── →:4: (main)
+    ");
 
     // Find segments by looking for their ref names in the segment list
     let find_segment = |name: &str| -> but_graph::SegmentIndex {
         graph
-            .segments()
-            .find(|&sidx| {
-                graph[sidx]
-                    .ref_name()
-                    .is_some_and(|rn| rn.as_bstr().to_string().ends_with(name))
-            })
+            .named_segment_by_ref_name(
+                Category::LocalBranch
+                    .to_full_name(name)
+                    .expect("statically known good ref names")
+                    .as_ref(),
+            )
             .unwrap_or_else(|| panic!("segment {name} not found"))
+            .id
     };
 
-    let seg_a = find_segment("/A");
-    let seg_b = find_segment("/B");
-    let seg_mid_a = find_segment("/mid_a");
-    let seg_main = find_segment("/main"); // base
-
-    // Verify graph connectivity using edges
-    // A should connect to mid_a
-    // B should connect to both mid_a and mid_c (it's a merge commit)
-    use petgraph::Direction;
-    let a_edges: Vec<_> = graph.edges_directed(seg_a, Direction::Outgoing).collect();
-    let b_edges: Vec<_> = graph.edges_directed(seg_b, Direction::Outgoing).collect();
-
-    assert_eq!(a_edges.len(), 1, "A should have 1 outgoing edge (to mid_a)");
-    assert_eq!(
-        b_edges.len(),
-        2,
-        "B should have 2 outgoing edges (to mid_a and mid_c) - this creates the 'path around'"
-    );
+    let seg_a = find_segment("A");
+    let seg_b = find_segment("B");
+    let seg_mid_a = find_segment("mid_a");
+    let seg_main = find_segment("main");
 
     // Now test first_merge_base
-    let merge_base = graph.first_merge_base(seg_a, seg_b);
-
-    // Common ancestors of A and B are: {mid_a, deep_a, base}
-    // - mid_a: reachable from A (directly) and B (first parent)
-    // - deep_a: reachable from A (via mid_a) and B (via mid_a)
-    // - base: reachable from A (via mid_a -> deep_a) and B (via mid_a OR mid_c)
-    //
-    // The BUG: first_merge_base returns mid_a because it's encountered first
-    // when walking from A. But mid_a has a "path around it" - B can reach base
-    // via mid_c without going through mid_a.
-    //
-    // The CORRECT merge-base is base, because ALL paths from both A and B
-    // must pass through base. There's no way around base.
-
-    // First verify we got one of the common ancestors
-    assert!(
-        merge_base == Some(seg_mid_a) || merge_base == Some(seg_main),
-        "merge_base should be one of the common ancestors (mid_a or main), got {:?}",
-        merge_base
-    );
-
-    // Document the buggy behavior of first_merge_base - this assertion shows it
-    // returns mid_a, which has a "path around it" via mid_c
+    let first_merge_base = graph.first_first_merge_base(seg_a, seg_b);
     assert_eq!(
-        merge_base,
+        first_merge_base,
         Some(seg_mid_a),
-        "first_merge_base returns mid_a, but there's a path around it. \
-         Use lowest_merge_base for correct results."
+        "the first merge-base is always one with a young generation",
     );
 
-    // Now test lowest_merge_base - this should return the correct answer
-    let lowest_merge_base = graph.lowest_merge_base(seg_a, seg_b);
-
+    let lowest_merge_base = graph.find_lowest_merge_base(seg_a, seg_b);
     assert_eq!(
         lowest_merge_base,
         Some(seg_main),

crates/but-graph/tests/graph/init/snapshots/graph__init__first_merge_base_bug_path_around-2.snap

@@ -423,7 +423,7 @@ pub mod merge {
         left: SegmentIndex,
         right: SegmentIndex,
     ) -> anyhow::Result<(gix::ObjectId, SegmentIndex)> {
-        let base_sidx = graph.first_merge_base(left, right).with_context(|| {
+        let base_sidx = graph.first_first_merge_base(left, right).with_context(|| {
             format!(
                 "Couldn't find merge-base between segments {l} and {r} - they are disjoint in the commit-graph",
                 l = left.index(),