Make entrypoint handling clear instead of implying it with insert_root().

This already led to a possible bug when multiple workspaces are used,
and makes it hard to understand how this is really working.

Author: Byron

crates/but-graph/src/api.rs

@@ -11,17 +11,24 @@ use std::ops::{Deref, Index, IndexMut};
 /// Mutation
 impl Graph {
     /// Insert `segment` to the graph so that it's not connected to any other segment, and return its index.
+    ///
     /// Note that as a side effect, the [entrypoint](Self::lookup_entrypoint()) will also be set if it's not
     /// set yet.
-    pub fn insert_root(&mut self, segment: Segment) -> SegmentIndex {
-        let index = self.inner.add_node(segment);
-        self.inner[index].id = index;
+    pub fn insert_segment_set_entrypoint(&mut self, segment: Segment) -> SegmentIndex {
+        let index = self.insert_segment(segment);
         if self.entrypoint.is_none() {
             self.entrypoint = Some((index, None))
         }
         index
     }
 
+    /// Insert `segment` to the graph so that it's not connected to any other segment, and return its index.
+    pub fn insert_segment(&mut self, segment: Segment) -> SegmentIndex {
+        let index = self.inner.add_node(segment);
+        self.inner[index].id = index;
+        index
+    }
+
     /// Put `dst` on top of `src`, connecting it from the `src_commit` specifically,
     /// an index valid for [`Segment::commits_unique_from_tip`] in `src` to the commit at `dst_commit` in `dst`.
     ///

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

@@ -145,7 +145,7 @@ impl Graph {
                 // It's OK to default-initialise this here as overlays are only used when redoing
                 // the traversal.
                 let (_repo, meta, _entrypoint) = Overlay::default().into_parts(repo, meta);
-                graph.insert_root(branch_segment_from_name_and_meta(
+                graph.insert_segment_set_entrypoint(branch_segment_from_name_and_meta(
                     Some((ref_name, None)),
                     &meta,
                     None,
@@ -337,7 +337,7 @@ impl Graph {
             dangerously_skip_postprocessing_for_debugging,
         };
         if tip_is_not_workspace_commit {
-            let current = graph.insert_root(branch_segment_from_name_and_meta(
+            let current = graph.insert_segment_set_entrypoint(branch_segment_from_name_and_meta(
                 None,
                 meta,
                 Some((&ctx.refs_by_id, tip)),
@@ -388,7 +388,16 @@ impl Graph {
             // The limits for the target ref and the worktree ref are synced so they can always find each other,
             // while being able to stop when the entrypoint is included.
             ws_segment.metadata = Some(SegmentMetadata::Workspace(ws_meta));
-            let ws_segment = graph.insert_root(ws_segment);
+            let ws_segment = graph.insert_segment(ws_segment);
+            if graph.entrypoint.is_none()
+                && graph
+                    .entrypoint_ref
+                    .as_ref()
+                    .zip(ref_name.as_ref())
+                    .is_some_and(|(a, b)| a == b)
+            {
+                graph.entrypoint = Some((ws_segment, None));
+            }
             // As workspaces typically have integration branches which can help us to stop the traversal,
             // pick these up first.
             _ = next.push_front_exhausted((
@@ -403,45 +412,45 @@ impl Graph {
             ));
 
             if let Some((target_ref, target_ref_id, local_tip_info)) = target {
-                let target_segment = graph.insert_root(branch_segment_from_name_and_meta(
+                let target_segment = graph.insert_segment(branch_segment_from_name_and_meta(
                     Some((target_ref, None)),
                     meta,
                     None,
                 )?);
-                let (local_sidx, local_goal) = if let Some((local_ref_name, target_local_tip)) =
-                    local_tip_info
-                {
-                    let local_sidx = graph.insert_root(branch_segment_from_name_and_meta_sibling(
-                        None,
-                        Some(target_segment),
-                        meta,
-                        Some((&ctx.refs_by_id, target_local_tip)),
-                    )?);
-                    // We use auto-naming based on ambiguity - if the name ends up something else,
-                    // remove the nodes sibling link.
-                    let has_sibling_link = {
-                        let s = &mut graph[local_sidx];
-                        if s.ref_name.as_ref().is_none_or(|rn| rn != &local_ref_name) {
-                            s.sibling_segment_id = None;
-                            false
-                        } else {
-                            true
-                        }
+                let (local_sidx, local_goal) =
+                    if let Some((local_ref_name, target_local_tip)) = local_tip_info {
+                        let local_sidx =
+                            graph.insert_segment(branch_segment_from_name_and_meta_sibling(
+                                None,
+                                Some(target_segment),
+                                meta,
+                                Some((&ctx.refs_by_id, target_local_tip)),
+                            )?);
+                        // We use auto-naming based on ambiguity - if the name ends up something else,
+                        // remove the nodes sibling link.
+                        let has_sibling_link = {
+                            let s = &mut graph[local_sidx];
+                            if s.ref_name.as_ref().is_none_or(|rn| rn != &local_ref_name) {
+                                s.sibling_segment_id = None;
+                                false
+                            } else {
+                                true
+                            }
+                        };
+                        let goal = goals.flag_for(target_local_tip).unwrap_or_default();
+                        _ = next.push_front_exhausted((
+                            target_local_tip,
+                            CommitFlags::NotInRemote | goal,
+                            Instruction::CollectCommit { into: local_sidx },
+                            max_limit
+                                .with_indirect_goal(tip, &mut goals)
+                                .without_allowance(),
+                        ));
+                        next.add_goal_to(tip, goal);
+                        (has_sibling_link.then_some(local_sidx), goal)
+                    } else {
+                        (None, CommitFlags::empty())
                     };
-                    let goal = goals.flag_for(target_local_tip).unwrap_or_default();
-                    _ = next.push_front_exhausted((
-                        target_local_tip,
-                        CommitFlags::NotInRemote | goal,
-                        Instruction::CollectCommit { into: local_sidx },
-                        max_limit
-                            .with_indirect_goal(tip, &mut goals)
-                            .without_allowance(),
-                    ));
-                    next.add_goal_to(tip, goal);
-                    (has_sibling_link.then_some(local_sidx), goal)
-                } else {
-                    (None, CommitFlags::empty())
-                };
                 _ = next.push_front_exhausted((
                     target_ref_id,
                     CommitFlags::Integrated,
@@ -468,7 +477,7 @@ impl Graph {
                 // have to adjust the existing queue item.
                 existing_segment
             } else {
-                let extra_target_sidx = graph.insert_root(branch_segment_from_name_and_meta(
+                let extra_target_sidx = graph.insert_segment(branch_segment_from_name_and_meta(
                     None,
                     meta,
                     Some((&ctx.refs_by_id, extra_target)),
@@ -517,7 +526,7 @@ impl Graph {
                     meta,
                     Some((&ctx.refs_by_id, segment_tip.detach())),
                 )?;
-                let segment = graph.insert_root(segment);
+                let segment = graph.insert_segment(segment);
                 _ = next.push_back_exhausted((
                     segment_tip.detach(),
                     CommitFlags::NotInRemote,

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

@@ -6,6 +6,7 @@ use crate::projection::workspace;
 use crate::{Commit, CommitFlags, CommitIndex, Edge, Graph, SegmentIndex, SegmentMetadata};
 use anyhow::{Context as _, bail};
 use but_core::{RefMetadata, ref_metadata};
+use gix::ObjectId;
 use gix::prelude::ObjectIdExt;
 use gix::reference::Category;
 use petgraph::Direction;
@@ -52,13 +53,8 @@ impl Graph {
     ) -> anyhow::Result<Self> {
         self.hard_limit_hit = hard_limit;
 
-        // For the first id to be inserted into our entrypoint segment, set index.
-        if let Some((segment, ep_commit)) = self.entrypoint.as_mut() {
-            *ep_commit = self
-                .inner
-                .node_weight(*segment)
-                .and_then(|s| s.commit_index_of(tip));
-        }
+        // For the first id to be inserted into our entrypoint segment, set commit index.
+        self.update_entrypoint_commit_index(tip);
 
         if dangerously_skip_postprocessing_for_debugging {
             return Ok(self);
@@ -79,6 +75,8 @@ impl Graph {
 
         // Point entrypoint to the right spot after all the virtual branches were added.
         self.set_entrypoint_to_ref_name(meta)?;
+        // After the entrypoint changed, also update its commit-index again, for good measure.
+        self.update_entrypoint_commit_index(tip);
 
         // However, when it comes to using remotes to disambiguate, it's better to
         // *not* do that before workspaces are sorted as it might incorrectly place
@@ -98,6 +96,18 @@ impl Graph {
         Ok(self)
     }
 
+    /// Ensure the entrypoint commit-index is updated to match the actual tip commit.
+    /// This may get out of sync when we do our graph manipulations, and it's easier
+    /// to set it right in post than to let everything deal with this.
+    fn update_entrypoint_commit_index(&mut self, tip: ObjectId) {
+        if let Some((segment, ep_commit)) = self.entrypoint.as_mut() {
+            *ep_commit = self
+                .inner
+                .node_weight(*segment)
+                .and_then(|s| s.commit_index_of(tip));
+        }
+    }
+
     /// After everything, assure the entrypoint still points to a segment with the correct ref-name,
     /// if one was given when starting the traversal.
     /// If not, try to find a segment with the right ref-name.
@@ -161,7 +171,7 @@ impl Graph {
                 );
                 let entrypoint_segment =
                     branch_segment_from_name_and_meta(Some((desired_ref_name, None)), meta, None)?;
-                let entrypoint_sidx = self.insert_root(entrypoint_segment);
+                let entrypoint_sidx = self.insert_segment_set_entrypoint(entrypoint_segment);
                 self.connect_segments(
                     entrypoint_sidx,
                     None,

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

@@ -114,8 +114,9 @@ pub fn prioritize_initial_tips_and_assure_ws_commit_ownership<T: RefMetadata>(
                 .find(|t| t.0 == ws_tip)
                 .cloned()
                 .expect("each ws-tip has one entry on queue");
-            let new_anon_segment =
-                graph.insert_root(branch_segment_from_name_and_meta(None, meta, None)?);
+            let new_anon_segment = graph.insert_segment_set_entrypoint(
+                branch_segment_from_name_and_meta(None, meta, None)?,
+            );
             // This segment acts as stand-in - always process it even if the queue says it's done.
             _ = next.push_front_exhausted((
                 ws_tip,
@@ -728,11 +729,12 @@ pub fn try_queue_remote_tracking_branches<T: RefMetadata>(
         let Some(remote_tip) = try_refname_to_id(repo, remote_tracking_branch.as_ref())? else {
             continue;
         };
-        let remote_segment = graph.insert_root(branch_segment_from_name_and_meta(
-            Some((remote_tracking_branch.clone(), None)),
-            meta,
-            None,
-        )?);
+        let remote_segment =
+            graph.insert_segment_set_entrypoint(branch_segment_from_name_and_meta(
+                Some((remote_tracking_branch.clone(), None)),
+                meta,
+                None,
+            )?);
 
         let remote_limit = limit.with_indirect_goal(id, goals);
         // These flags are to be attached to `id` so it can propagate itself later.

crates/but-graph/src/lib.rs

@@ -230,8 +230,8 @@ pub struct Graph {
     inner: init::PetGraph,
     /// From where the graph was created. This is useful if one wants to focus on a subset of the graph.
     ///
-    /// The [`CommitIndex`] is empty if the entry point is an empty segment, one that is supposed to receive
-    /// commits later.
+    /// The [`CommitIndex`] is empty if the entry point is an empty segment, to indicate that the traversal
+    /// tip isn't contained in it.
     entrypoint: Option<(SegmentIndex, Option<CommitIndex>)>,
     /// The ref_name used when starting the graph traversal. It is set to help assure that the entrypoint stays
     /// on the correctly named segment, knowing that the post-process may alter segments quite substantially

crates/but-graph/tests/graph/vis.rs

@@ -28,7 +28,7 @@ fn post_graph_traversal() -> anyhow::Result<()> {
         ..Default::default()
     };
 
-    let local_target = graph.insert_root(local_target);
+    let local_target = graph.insert_segment_set_entrypoint(local_target);
     graph.connect_new_segment(
         local_target,
         None,
@@ -96,7 +96,7 @@ fn post_graph_traversal() -> anyhow::Result<()> {
 #[test]
 fn detached_head() {
     let mut graph = Graph::default();
-    graph.insert_root(Segment {
+    graph.insert_segment_set_entrypoint(Segment {
         commits: vec![commit(id("a"), None, CommitFlags::empty())],
         ..Default::default()
     });