Merge pull request #10459 from Byron/fix

follow-up fix for segment naming

Author: Byron

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

@@ -410,11 +410,22 @@ impl Graph {
                     local_tip_info
                 {
                     let local_sidx = graph.insert_root(branch_segment_from_name_and_meta_sibling(
-                        Some((local_ref_name, None)),
+                        None,
                         Some(target_segment),
                         meta,
-                        None,
+                        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,
@@ -425,7 +436,7 @@ impl Graph {
                             .without_allowance(),
                     ));
                     next.add_goal_to(tip, goal);
-                    (Some(local_sidx), goal)
+                    (has_sibling_link.then_some(local_sidx), goal)
                 } else {
                     (None, CommitFlags::empty())
                 };
@@ -492,13 +503,18 @@ impl Graph {
                 else {
                     continue;
                 };
-                // Avoid duplication before we create a new branch segment, these should not inetefere,
+                // Avoid duplication before we create a new branch segment, these should not interfere,
                 // just integrate.
                 if next.iter().any(|t| t.0 == segment_tip) {
                     continue;
                 };
-                let segment =
-                    branch_segment_from_name_and_meta(Some((segment.ref_name, None)), meta, None)?;
+                // We always want these segments named, we know they are supposed to be in the workspace,
+                // but don't do so forcefully (follow the rules).
+                let segment = branch_segment_from_name_and_meta(
+                    None,
+                    meta,
+                    Some((&ctx.refs_by_id, segment_tip.detach())),
+                )?;
                 let segment = graph.insert_root(segment);
                 _ = next.push_back_exhausted((
                     segment_tip.detach(),

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

@@ -317,13 +317,6 @@ impl Graph {
             .iter()
             .filter_map(|s| {
                 s.base_segment_id().and_then(|sidx| {
-                    let base_is_directly_connected_to_workspace = self
-                        .inner
-                        .neighbors_directed(sidx, Direction::Incoming)
-                        .any(|above_sidx| above_sidx == ws_sidx);
-                    if base_is_directly_connected_to_workspace {
-                        return None;
-                    }
                     let base_segment = &self[sidx];
                     // These are naturally in the workspace.
                     base_segment
@@ -410,61 +403,6 @@ impl Graph {
             return Ok(());
         };
 
-        // If the target branch made it into a stack segment which can also be named differently by preferably a ref with metadata,
-        // then do that instead. Let's prevent it from showing up as its own node as this leads to undesirable workspaces.
-        if let Some((target_segment_id, local_tracking_segment_of_target)) =
-            ws_target.as_ref().and_then(|t| {
-                self[t.segment_index]
-                    .sibling_segment_id
-                    .map(|sidx| (t.segment_index, sidx))
-            })
-            && let Some(segment_id) = ws_stacks.iter().find_map(|s| {
-                s.segments.iter().find_map(|s| {
-                    if s.id != local_tracking_segment_of_target {
-                        return None;
-                    }
-                    let first_commit = s.commits.first()?;
-                    (!first_commit.refs.is_empty()).then_some(s.id)
-                })
-            })
-        {
-            let s = &mut self[segment_id];
-            let c = s
-                .commits
-                .first_mut()
-                .expect("segment was chosen because it has at least one commit");
-            let mut candidates = c.refs.clone();
-            candidates.sort_by(|a, b| {
-                meta.branch_opt(a.as_ref())
-                    .ok()
-                    .map(|md| md.is_some())
-                    .cmp(&meta.branch_opt(b.as_ref()).ok().map(|md| md.is_some()))
-                    .then_with(|| a.cmp(b))
-            });
-            let candidate = candidates
-                .pop()
-                .expect("at least one ref or we wouldn't be here");
-            let current_name = s.ref_name.take();
-            let index_to_replace = c
-                .refs
-                .iter()
-                .position(|rn| rn == &candidate)
-                .expect("candidate is from a clone of 'refs', so must be contained");
-            // effectively swap the names
-            s.metadata = meta
-                .branch_opt(candidate.as_ref())?
-                .map(SegmentMetadata::Branch);
-            s.ref_name = Some(candidate);
-            if let Some(rn) = current_name {
-                c.refs[index_to_replace] = rn;
-            }
-
-            // Make sure we dissolve the sibling relationship for correctness, the node is now not related
-            // to the target branch anymore.
-            s.sibling_segment_id = None;
-            self[target_segment_id].sibling_segment_id = None;
-        }
-
         // Setup independent stacks, first by looking at potential bases.
         let candidates = self.candidates_for_independent_branches_in_workspace(
             ws_sidx,
@@ -479,10 +417,14 @@ impl Graph {
             let base_segment_name = base_segment.ref_name.clone();
             let matching_refs_per_stack: Vec<_> = find_all_desired_stack_refs_in_commit(
                 &ws_data,
-                base_segment_name
-                    .as_ref()
-                    .into_iter()
-                    .chain(base_segment.commits[0].refs.iter()),
+                base_segment_name.as_ref().into_iter().chain(
+                    base_segment
+                        .commits
+                        .first()
+                        .map(|c| c.refs.iter())
+                        .into_iter()
+                        .flatten(),
+                ),
                 Some((&self.inner, ws_sidx, &ws_stacks, &candidates)),
             )
             .collect();
@@ -568,10 +510,17 @@ impl Graph {
                     Some((last_created_segment.unwrap_or(s.id), base_sidx, c))
                 })
             {
-                let Some(refs_for_dependent_branches) =
-                    find_all_desired_stack_refs_in_commit(&ws_data, commit.refs.iter(), None)
-                        .next()
-                else {
+                let Some(refs_for_dependent_branches) = find_all_desired_stack_refs_in_commit(
+                    &ws_data,
+                    self[base_sidx]
+                        .ref_name
+                        .as_ref()
+                        .filter(|_| !commit.refs.is_empty())
+                        .into_iter()
+                        .chain(commit.refs.iter()),
+                    None,
+                )
+                .next() else {
                     continue;
                 };
 
@@ -593,12 +542,34 @@ impl Graph {
                 )?;
 
                 // As we didn't allow the previous function to deal with the commit, we do it.
-                self[base_sidx]
-                    .commits
+                let s = &mut self[base_sidx];
+                s.commits
                     .first_mut()
                     .expect("we know there is one already")
                     .refs
                     .retain(|rn| !refs_for_dependent_branches.contains(rn));
+                s.ref_name
+                    .take_if(|rn| refs_for_dependent_branches.contains(rn));
+                if s.ref_name.is_none() {
+                    s.metadata = None;
+                    if let Some(sibling) = s.sibling_segment_id.take() {
+                        self[sibling].sibling_segment_id = None;
+                    }
+                }
+
+                let s = &mut self[base_sidx];
+                if let Some(refs) = s
+                    .commits
+                    .first_mut()
+                    .filter(|c| !c.refs.is_empty())
+                    .map(|c| &mut c.refs)
+                    && let Some((name, md)) =
+                        disambiguate_refs_by_branch_metadata(refs.iter(), meta)
+                {
+                    refs.retain(|rn| rn != &name);
+                    s.ref_name = Some(name);
+                    s.metadata = md;
+                }
                 reconnect_edges(
                     self,
                     edges_from_segment_above,
@@ -790,6 +761,8 @@ impl Graph {
             s.sibling_segment_id = Some(remote_sidx);
             let rn = s.ref_name.as_ref().expect("just set it");
             s.commits.first_mut().unwrap().refs.retain(|crn| crn != rn);
+            // Assure the remote is also paired up!
+            self[remote_sidx].sibling_segment_id = Some(s.id);
         }
 
         // NOTE: setting this directly at iteration time isn't great as the post-processing then

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

@@ -386,6 +386,8 @@ pub fn queue_parents(
 /// As convenience, if `ref_name` is `Some` and the metadata is not set, it will look it up for you.
 /// If `ref_name` is `None`, and `refs_by_id_lookup` is `Some`, it will try to look up unambiguous
 /// references on that object.
+/// Note that `ref_name` should only be set if you are sure that it is unambiguous, and otherwise won't interfere with
+/// the post-processing or the workspace projection later.
 pub fn branch_segment_from_name_and_meta<T: RefMetadata>(
     ref_name: Option<(gix::refs::FullName, Option<SegmentMetadata>)>,
     meta: &OverlayMetadata<'_, T>,

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

@@ -105,9 +105,12 @@ impl Stack {
 
 impl std::fmt::Debug for Stack {
     fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
-        f.debug_struct(&format!("Stack({})", self.debug_string()))
-            .field("segments", &self.segments)
-            .finish()
+        let mut s = f.debug_struct(&format!("Stack({})", self.debug_string()));
+        s.field("segments", &self.segments);
+        if let Some(stack_id) = self.id {
+            s.field("id", &stack_id);
+        }
+        s.finish()
     }
 }
 
@@ -198,6 +201,20 @@ impl StackSegment {
     pub fn tip(&self) -> Option<gix::ObjectId> {
         self.commits.first().map(|c| c.id)
     }
+
+    /// Returns an iterator over all reachable reference names, that is the name of the segment if present
+    /// and all ref-names pointing to/stored in local commits.
+    pub fn ref_names(&self) -> impl Iterator<Item = &gix::refs::FullNameRef> {
+        self.ref_name
+            .as_ref()
+            .map(|r| r.as_ref())
+            .into_iter()
+            .chain(
+                self.commits
+                    .iter()
+                    .flat_map(|c| c.refs.iter().map(|r| r.as_ref())),
+            )
+    }
 }
 
 impl std::fmt::Debug for StackSegment {

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

@@ -517,8 +517,9 @@ impl Graph {
         }
 
         if ws.has_managed_ref() {
-            let (_lowest_base, lowest_base_sidx) =
+            let (lowest_base, lowest_base_sidx) =
                 ws_lower_bound.map_or((None, None), |(base, sidx)| (Some(base), Some(sidx)));
+            // dbg!(lowest_base, lowest_base_sidx);
             for stack_top_sidx in self
                 .inner
                 .neighbors_directed(ws_tip_segment.id, Direction::Outgoing)
@@ -570,9 +571,24 @@ impl Graph {
                         },
                         |s| Some(s.id) == ws.lower_bound_segment_id && s.metadata.is_none(),
                     )?
-                    .map(|segments| {
+                    .and_then(|segments| {
                         let stack_id = find_matching_stack_id(ws.metadata.as_ref(), &segments);
-                        Stack::from_base_and_segments(&self.inner, segments, stack_id)
+                        // If we find no stack ID, then the segment is not included in the workspace metadata,
+                        // indicating it's ignored. Just to be even more certain, if it starts with a commit
+                        // that is the workspace base, then we definitely don't want to show it - it's unapplied.
+                        if stack_id.is_none()
+                            && segments
+                                .first()
+                                .is_some_and(|s| s.commits.first().map(|c| c.id) == lowest_base)
+                        {
+                            None
+                        } else {
+                            Some(Stack::from_base_and_segments(
+                                &self.inner,
+                                segments,
+                                stack_id,
+                            ))
+                        }
                     }),
                 );
             }
@@ -702,17 +718,17 @@ fn find_matching_stack_id(
     metadata
         .stacks
         .iter()
-        .map(|s| {
+        .filter_map(|s| {
             let num_matching_refs = s
                 .branches
                 .iter()
                 .filter(|b| {
                     segments
                         .iter()
-                        .any(|s| s.ref_name.as_ref().is_some_and(|rn| rn == &b.ref_name))
+                        .any(|s| s.ref_names().any(|rn| rn == b.ref_name.as_ref()))
                 })
                 .count();
-            (s.id, num_matching_refs)
+            (num_matching_refs != 0).then_some((s.id, num_matching_refs))
         })
         .sorted_by(|(_, lhs), (_, rhs)| lhs.cmp(rhs).reverse())
         .next()

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

@@ -1109,5 +1109,13 @@ EOF
     git checkout -b A
     create_workspace_commit_once A
   )
+
+  git init unapplied-branch-on-base
+  (cd unapplied-branch-on-base
+    commit init
+    git branch unapplied
+    setup_target_to_match_main
+    create_workspace_commit_once main
+  )
 )