Add graph subcommand to invoke graph visualization in the real world.

Also make some tweaks for better performance logging and better abort condition handling.
Now integrated workspace tips will be limited as well, if a limit was set.

Author: Byron

crates/but-graph/src/api.rs

@@ -177,6 +177,15 @@ impl Graph {
         self.inner.edge_count()
     }
 
+    /// Return the number of commits in all segments.
+    pub fn num_commits(&self) -> usize {
+        self.inner
+            .raw_nodes()
+            .iter()
+            .map(|n| n.weight.commits.len())
+            .sum::<usize>()
+    }
+
     /// Return an iterator over all indices of segments in the graph.
     pub fn segments(&self) -> impl Iterator<Item = SegmentIndex> {
         self.inner.node_indices()
@@ -257,7 +266,7 @@ impl Graph {
 
     /// Validate the graph for consistency and fail loudly when an issue was found, after printing the dot graph.
     /// Mostly useful for debugging to stop early when a connection wasn't created correctly.
-    #[cfg(target_os = "macos")]
+    #[cfg(unix)]
     pub fn validated_or_open_as_svg(self) -> anyhow::Result<Self> {
         for edge in self.inner.edge_references() {
             let res = check_edge(&self.inner, edge);
@@ -283,7 +292,8 @@ impl Graph {
     }
 
     /// Open an SVG dot visualization in the browser or panic if the `dot` or `open` tool can't be found.
-    #[cfg(target_os = "macos")]
+    #[cfg(unix)]
+    #[tracing::instrument(skip(self))]
     pub fn open_as_svg(&self) {
         use std::io::Write;
         use std::process::Stdio;

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

@@ -1,14 +1,14 @@
 use crate::{CommitFlags, Edge};
 use crate::{CommitIndex, Graph, Segment, SegmentIndex, SegmentMetadata};
-use anyhow::{Context, bail};
+use anyhow::bail;
 use but_core::RefMetadata;
-use gix::ObjectId;
 use gix::hashtable::hash_map::Entry;
 use gix::prelude::{ObjectIdExt, ReferenceExt};
 use gix::refs::Category;
 use petgraph::graph::EdgeReference;
 use petgraph::prelude::EdgeRef;
 use std::collections::VecDeque;
+use tracing::instrument;
 
 mod utils;
 use utils::*;
@@ -46,6 +46,7 @@ pub struct Options {
     /// * HEAD - uses the limit
     /// * workspaces with target branch - no limit, but auto-stop if workspace is exhausted as everything is integrated.
     ///     - The target branch: no limit
+    ///     - Integrated workspace branches: use the limit
     /// * workspace without target branch - uses the limit
     /// * remotes tracking branches - use the limit
     pub max_commits_outside_of_workspace: Option<usize>,
@@ -146,6 +147,7 @@ impl Graph {
     /// * The traversal is cut short when there is only tips which are integrated, even though named segments that are
     ///   supposed to be in the workspace will be fully traversed (implying they will stop at the first anon segment
     ///   as will happen at merge commits).
+    #[instrument(skip(meta, ref_name), err(Debug))]
     pub fn from_commit_traversal(
         tip: gix::Id<'_>,
         ref_name: impl Into<Option<gix::refs::FullName>>,
@@ -279,7 +281,7 @@ impl Graph {
                     Instruction::CollectCommit {
                         into: target_segment,
                     },
-                    /* unlimited traversal for 'negative' commits */
+                    /* unlimited traversal for integrated commits */
                     None,
                 ));
             }
@@ -307,67 +309,15 @@ impl Graph {
             propagated_flags |= src_flags;
             let segment_idx_for_id = match instruction {
                 Instruction::CollectCommit { into: src_sidx } => match seen.entry(id) {
-                    Entry::Occupied(mut existing_sidx) => {
-                        let dst_sidx = *existing_sidx.get();
-                        let (top_sidx, mut bottom_sidx) =
-                            // If a normal branch walks into a workspace branch, put the workspace branch on top.
-                            if graph[dst_sidx].workspace_metadata().is_some() &&
-                                graph[src_sidx].ref_name.as_ref()
-                                    .is_some_and(|rn| rn.category().is_some_and(|c| matches!(c, Category::LocalBranch))) {
-                                // `dst` is basically swapping with `src`, so must swap commits and connections.
-                                swap_commits_and_connections(&mut graph.inner, dst_sidx, src_sidx);
-                                swap_queued_segments(&mut next, dst_sidx, src_sidx);
-
-                                // Assure the first commit doesn't name the new owner segment.
-                                {
-                                    let s = &mut graph[src_sidx];
-                                    if let Some(c) = s.commits.first_mut() {
-                                        c.refs.retain(|rn| Some(rn) != s.ref_name.as_ref())
-                                    }
-                                    // Update the commit-ownership of the connecting commit, but also
-                                    // of all other commits in the segment.
-                                    existing_sidx.insert(src_sidx);
-                                    for commit_id in s.commits.iter().skip(1).map(|c| c.id) {
-                                        seen.entry(commit_id).insert(src_sidx);
-                                    }
-                                }
-                                (dst_sidx, src_sidx)
-                            } else {
-                                // `src` naturally runs into destination, so nothing needs to be done
-                                // except for connecting both. Commit ownership doesn't change.
-                                (src_sidx, dst_sidx)
-                            };
-                        let top_cidx = graph[top_sidx].last_commit_index();
-                        let mut bottom_cidx =
-                            graph[bottom_sidx].commit_index_of(id).with_context(|| {
-                                format!(
-                                    "BUG: Didn't find commit {id} in segment {bottom_sidx}",
-                                    bottom_sidx = dst_sidx.index(),
-                                )
-                            })?;
-
-                        if bottom_cidx != 0 {
-                            let new_bottom_sidx = split_commit_into_segment(
-                                &mut graph,
-                                &mut next,
-                                &mut seen,
-                                bottom_sidx,
-                                bottom_cidx,
-                            )?;
-                            bottom_sidx = new_bottom_sidx;
-                            bottom_cidx = 0;
-                        }
-                        graph.connect_segments(top_sidx, top_cidx, bottom_sidx, bottom_cidx);
-                        let top_flags = top_cidx
-                            .map(|cidx| graph[top_sidx].commits[cidx].flags)
-                            .unwrap_or_default();
-                        let bottom_flags = graph[bottom_sidx].commits[bottom_cidx].flags;
-                        propagate_flags_downward(
-                            &mut graph.inner,
-                            propagated_flags | top_flags | bottom_flags,
-                            bottom_sidx,
-                            Some(bottom_cidx),
-                        );
+                    Entry::Occupied(_) => {
+                        possibly_split_occupied_segment(
+                            &mut graph,
+                            &mut seen,
+                            &mut next,
+                            id,
+                            propagated_flags,
+                            src_sidx,
+                        )?;
                         continue;
                     }
                     Entry::Vacant(e) => {
@@ -387,23 +337,15 @@ impl Graph {
                     parent_above,
                     at_commit,
                 } => match seen.entry(id) {
-                    Entry::Occupied(existing_sidx) => {
-                        let bottom_sidx = *existing_sidx.get();
-                        let bottom = &graph[bottom_sidx];
-                        let bottom_cidx = bottom.commit_index_of(id).context(
-                            "BUG: bottom segment must contain ID, `seen` seems out of date",
+                    Entry::Occupied(_) => {
+                        possibly_split_occupied_segment(
+                            &mut graph,
+                            &mut seen,
+                            &mut next,
+                            id,
+                            propagated_flags,
+                            parent_above,
                         )?;
-                        if bottom_cidx != 0 {
-                            todo!("split bottom segment at `at_commit`");
-                        }
-                        let bottom_flags = bottom.commits[bottom_cidx].flags;
-                        graph.connect_segments(parent_above, at_commit, bottom_sidx, bottom_cidx);
-                        propagate_flags_downward(
-                            &mut graph.inner,
-                            propagated_flags | bottom_flags,
-                            bottom_sidx,
-                            Some(bottom_cidx),
-                        );
                         continue;
                     }
                     Entry::Vacant(e) => {
@@ -463,7 +405,7 @@ impl Graph {
                 limit,
             )?;
 
-            prune_integrated_tips(&mut graph.inner, &mut next, &desired_refs);
+            prune_integrated_tips(&mut graph.inner, &mut next, &desired_refs, max_limit);
         }
 
         graph.post_processed(
@@ -511,7 +453,7 @@ impl Instruction {
     }
 }
 
-type QueueItem = (ObjectId, CommitFlags, Instruction, Option<usize>);
+type QueueItem = (gix::ObjectId, CommitFlags, Instruction, Option<usize>);
 
 #[derive(Debug)]
 pub(crate) struct EdgeOwned {

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

@@ -7,6 +7,7 @@ use crate::{
 use anyhow::{Context, bail};
 use bstr::BString;
 use but_core::{RefMetadata, ref_metadata};
+use gix::hashtable::hash_map::Entry;
 use gix::prelude::ObjectIdExt;
 use gix::reference::Category;
 use gix::refs::FullName;
@@ -268,8 +269,20 @@ pub fn queue_parents(
             parent_above: current_sidx,
             at_commit: current_cidx,
         };
+        // It's important to try to split the limit evenly so we don't create too
+        // much extra gas here. We do, however, make sure that we see each segment of a parent
+        // with one commit so we know exactly where it stops.
+        // The problem with this is that we never get back the split limit when segments re-unite,
+        // so effectively we loose gas here.
+        let limit_per_parent = limit.map(|l| {
+            if l == 0 {
+                0
+            } else {
+                (l / parent_ids.len()).max(1)
+            }
+        });
         for pid in parent_ids {
-            next.push_back((*pid, flags, instruction, limit))
+            next.push_back((*pid, flags, instruction, limit_per_parent))
         }
     } else if !parent_ids.is_empty() {
         next.push_back((
@@ -629,18 +642,21 @@ pub fn try_queue_remote_tracking_branches(
 /// - keep tips that are adding segments that are or contain a workspace ref
 /// - prune the rest
 ///    - delete empty segments of pruned tips.
+///
+/// `limit` is applied to integrated workspace refs if they are not yet limited.
 pub fn prune_integrated_tips(
     graph: &mut PetGraph,
     next: &mut VecDeque<QueueItem>,
     workspace_refs: &BTreeSet<gix::refs::FullName>,
+    limit: Option<usize>,
 ) {
     let all_integated = next
         .iter()
         .all(|tip| tip.1.contains(CommitFlags::Integrated));
     if !all_integated {
         return;
     }
-    next.retain(|(_id, _flags, instruction, _limit)| {
+    next.retain_mut(|(_id, _flags, instruction, tip_limit)| {
         let sidx = instruction.segment_idx();
         let s = &graph[sidx];
         let any_segment_ref_is_contained_in_workspace = s
@@ -649,9 +665,81 @@ pub fn prune_integrated_tips(
             .into_iter()
             .chain(s.commits.iter().flat_map(|c| c.refs.iter()))
             .any(|segment_rn| workspace_refs.contains(segment_rn));
+        // For integrated workspace tips, use a limit to prevent runaway-worst-cases.
+        if any_segment_ref_is_contained_in_workspace && tip_limit.is_none() {
+            *tip_limit = limit;
+        }
         if !any_segment_ref_is_contained_in_workspace && s.commits.is_empty() {
             graph.remove_node(sidx);
         }
         any_segment_ref_is_contained_in_workspace
     });
 }
+
+pub fn possibly_split_occupied_segment(
+    graph: &mut Graph,
+    seen: &mut gix::revwalk::graph::IdMap<SegmentIndex>,
+    next: &mut VecDeque<QueueItem>,
+    id: gix::ObjectId,
+    propagated_flags: CommitFlags,
+    src_sidx: SegmentIndex,
+) -> anyhow::Result<()> {
+    let Entry::Occupied(mut existing_sidx) = seen.entry(id) else {
+        bail!("BUG: Can only work with occupied entries")
+    };
+    let dst_sidx = *existing_sidx.get();
+    let (top_sidx, mut bottom_sidx) =
+        // If a normal branch walks into a workspace branch, put the workspace branch on top.
+        if graph[dst_sidx].workspace_metadata().is_some() &&
+            graph[src_sidx].ref_name.as_ref()
+                .is_some_and(|rn| rn.category().is_some_and(|c| matches!(c, Category::LocalBranch))) {
+            // `dst` is basically swapping with `src`, so must swap commits and connections.
+            swap_commits_and_connections(&mut graph.inner, dst_sidx, src_sidx);
+            swap_queued_segments(next, dst_sidx, src_sidx);
+
+            // Assure the first commit doesn't name the new owner segment.
+            {
+                let s = &mut graph[src_sidx];
+                if let Some(c) = s.commits.first_mut() {
+                    c.refs.retain(|rn| Some(rn) != s.ref_name.as_ref())
+                }
+                // Update the commit-ownership of the connecting commit, but also
+                // of all other commits in the segment.
+                existing_sidx.insert(src_sidx);
+                for commit_id in s.commits.iter().skip(1).map(|c| c.id) {
+                    seen.entry(commit_id).insert(src_sidx);
+                }
+            }
+            (dst_sidx, src_sidx)
+        } else {
+            // `src` naturally runs into destination, so nothing needs to be done
+            // except for connecting both. Commit ownership doesn't change.
+            (src_sidx, dst_sidx)
+        };
+    let top_cidx = graph[top_sidx].last_commit_index();
+    let mut bottom_cidx = graph[bottom_sidx].commit_index_of(id).with_context(|| {
+        format!(
+            "BUG: Didn't find commit {id} in segment {bottom_sidx}",
+            bottom_sidx = dst_sidx.index(),
+        )
+    })?;
+
+    if bottom_cidx != 0 {
+        let new_bottom_sidx =
+            split_commit_into_segment(graph, next, seen, bottom_sidx, bottom_cidx)?;
+        bottom_sidx = new_bottom_sidx;
+        bottom_cidx = 0;
+    }
+    graph.connect_segments(top_sidx, top_cidx, bottom_sidx, bottom_cidx);
+    let top_flags = top_cidx
+        .map(|cidx| graph[top_sidx].commits[cidx].flags)
+        .unwrap_or_default();
+    let bottom_flags = graph[bottom_sidx].commits[bottom_cidx].flags;
+    propagate_flags_downward(
+        &mut graph.inner,
+        propagated_flags | top_flags | bottom_flags,
+        bottom_sidx,
+        Some(bottom_cidx),
+    );
+    Ok(())
+}

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

@@ -289,12 +289,20 @@ mkdir ws
   git init deduced-remote-ahead
   (cd deduced-remote-ahead
     commit init
+    git checkout -b A
     commit shared
     git checkout -b soon-remote;
+      git checkout -b tmp
+        commit feat-on-remote
+      git checkout soon-remote
+      git merge --no-ff -m "merge" tmp && git branch -d tmp
       commit only-remote-01;
       commit only-remote-02;
-    git checkout main && create_workspace_commit_once main
-    setup_remote_tracking soon-remote main "move"
+    git checkout A
+      commit A1
+      commit A2
+    create_workspace_commit_once A
+    setup_remote_tracking soon-remote A "move"
 
 cat <<EOF >>.git/config
 [remote "origin"]

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

@@ -202,6 +202,7 @@ fn four_diamond() -> anyhow::Result<()> {
         8,
         "just as many as are displayed in the tree"
     );
+    assert_eq!(graph.num_commits(), 8, "one commit per node");
     assert_eq!(
         graph.num_edges(),
         10,