SQUASH ME

Byron · Byron · commit fbce44f6ebd6 · 2025-08-01T17:04:58.000+02:00
diff --git a/crates/but-graph/src/init/post.rs b/crates/but-graph/src/init/post.rs
@@ -473,7 +473,12 @@ impl Graph {
                 .iter()
                 .flat_map(|s| s.commits_by_segment.iter().map(|(sidx, _)| *sidx))
         }) {
-            let s = &self.inner[sidx];
+            // The workspace might be stale by now as we delete empty segments.
+            // Thus be careful, and ignore non-existing ones - after all our workspace
+            // is temporary, nothing to worry about.
+            let Some(s) = self.inner.node_weight(sidx) else {
+                continue;
+            };
             if s.ref_name.is_some() || s.sibling_segment_id.is_some() {
                 continue;
             }
@@ -702,6 +707,9 @@ fn find_all_desired_stack_refs_in_commit<'a>(
     })
 }
 
+/// **Warning**: this can make workspace stacks stale, i.e. let them refer to non-existing segments.
+///              all accesses from hereon must be done with care. On the other hand, we can ignore
+///              that as our workspace is just temporary.
 fn delete_anon_if_empty_and_reconnect(graph: &mut Graph, sidx: SegmentIndex) {
     let segment = &graph[sidx];
     let may_delete = segment.commits.is_empty() && segment.ref_name.is_none();
@@ -717,6 +725,11 @@ fn delete_anon_if_empty_and_reconnect(graph: &mut Graph, sidx: SegmentIndex) {
     if outgoing.next().is_some() {
         return;
     }
+
+    tracing::debug!(
+        ?sidx,
+        "Deleting seemingly isolated and now completely unused segment"
+    );
     // Reconnect
     let new_target = first_outgoing.target();
     let incoming: Vec<_> = graph
diff --git a/crates/but-graph/src/projection/workspace.rs b/crates/but-graph/src/projection/workspace.rs
@@ -10,7 +10,7 @@ use crate::{
     projection::{Stack, StackCommit, StackCommitFlags, StackSegment},
 };
 use anyhow::Context;
-use bstr::ByteSlice;
+use bstr::{BStr, ByteSlice};
 use but_core::ref_metadata;
 use but_core::ref_metadata::StackId;
 use gix::reference::Category;
@@ -165,20 +165,6 @@ impl Workspace<'_> {
     }
 }
 
-/// Utilities
-impl Workspace<'_> {
-    /// Reconcile the metadata in `ws` to look like this instance.
-    ///
-    /// This is useful to assure that a possibly [simulated](Graph::redo_traversal_with_overlay()) workspace
-    /// *(that is soon to be reality)*
-    /// is going to be stored as-is in metadata.
-    pub fn update_metadata(&self, ws: &mut ref_metadata::Workspace) {
-        self.stacks[0].id
-        ws.stacks
-        todo!()
-    }
-}
-
 /// A classifier for the workspace.
 #[derive(Debug, Clone)]
 pub enum WorkspaceKind {
@@ -995,6 +981,12 @@ impl<'graph> Workspace<'graph> {
     pub fn ref_name(&self) -> Option<&'graph gix::refs::FullNameRef> {
         self.graph[self.id].ref_name.as_ref().map(|rn| rn.as_ref())
     }
+
+    /// Like [`Self::ref_name()`], but return a generic `<anonymous>` name for unnamed workspaces.
+    pub fn ref_name_display(&self) -> &BStr {
+        self.ref_name()
+            .map_or("<anonymous>".into(), |rn| rn.as_bstr())
+    }
 }
 
 /// Debugging
diff --git a/crates/but-workspace/src/branch/create_reference.rs b/crates/but-workspace/src/branch/create_reference.rs
@@ -103,11 +103,13 @@ impl<'a> ReferenceAnchor<'a> {
 
 pub(super) mod function {
     #![allow(clippy::indexing_slicing)]
+
     use crate::branch::{ReferenceAnchor, ReferencePosition};
     use anyhow::{Context, bail};
     use but_core::ref_metadata::{StackId, WorkspaceStack, WorkspaceStackBranch};
     use but_core::{RefMetadata, ref_metadata};
     use gix::refs::transaction::PreviousValue;
+    use std::borrow::Cow;
     use std::ops::DerefMut;
 
     /// Create a new reference named `ref_name` to point at a commit relative to `anchor`.
@@ -132,7 +134,17 @@ pub(super) mod function {
         let anchor = anchor.into();
 
         let ws_base = workspace.lower_bound;
-        let (check_if_commit_in_workspace, ref_target, insert): (_, _, Option<Instruction<'_>>) = {
+        // Note that we will never create metadata for a workspace!
+        let mut existing_ws_meta = workspace
+            .ref_name()
+            .and_then(|ws_ref| meta.workspace_opt(ws_ref).transpose())
+            .transpose()?;
+
+        let (check_if_id_in_workspace, ref_target_id, instruction): (
+            _,
+            _,
+            Option<Instruction<'_>>,
+        ) = {
             match anchor {
                 None => {
                     // The new ref exists already in the workspace, do nothing.
@@ -145,40 +157,47 @@ pub(super) mod function {
                     let base = ws_base.with_context(|| {
                         format!(
                             "workspace at {} is missing a base",
-                            workspace.ref_name().map_or_else(
-                                || "<anonymous>".to_string(),
-                                |rn| rn.as_bstr().to_string()
-                            )
+                            workspace.ref_name_display()
                         )
                     })?;
                     (
                         // do not validate, as the base is expectedly outside of workspace
                         false,
                         base,
-                        Some(None),
+                        Some(Instruction::Independent),
                     )
                 }
                 Some(ReferenceAnchor::AtCommit {
                     commit_id,
                     position,
                 }) => {
                     let mut validate_id = true;
-                    let id = position.resolve_commit(
-                        workspace
-                            .lookup_commit(
-                                workspace.try_find_owner_indexes_by_commit_id(commit_id)?,
-                            )
-                            .into(),
-                        ws_base,
-                    )?;
-                    if Some(id) == ws_base {
+                    let indexes = workspace.try_find_owner_indexes_by_commit_id(commit_id)?;
+                    let ref_target_id = position
+                        .resolve_commit(workspace.lookup_commit(indexes).into(), ws_base)?;
+                    let id_out_of_workspace = Some(ref_target_id) == ws_base;
+                    if id_out_of_workspace {
                         validate_id = false
                     }
-                    (validate_id, id, None)
+
+                    let instruction = existing_ws_meta
+                        .as_ref()
+                        .filter(|_| !id_out_of_workspace)
+                        .map(|_| {
+                            instruction_by_named_anchor_for_commit(
+                                workspace,
+                                commit_id,
+                                ref_target_id,
+                                position,
+                            )
+                        })
+                        .transpose()?;
+
+                    (validate_id, ref_target_id, instruction)
                 }
                 Some(ReferenceAnchor::AtSegment { ref_name, position }) => {
                     let mut validate_id = true;
-                    let id = if workspace.has_metadata() {
+                    let ref_target_id = if workspace.has_metadata() {
                         let (stack_idx, seg_idx) =
                             workspace.try_find_segment_owner_indexes_by_refname(ref_name)?;
                         let segment = &workspace.stacks[stack_idx].segments[seg_idx];
@@ -207,29 +226,28 @@ pub(super) mod function {
                             "BUG: empty segments aren't possible without workspace metadata",
                         )?.into(), ws_base)?
                     };
-                    (validate_id, id, Some(Some((ref_name, position))))
+                    (
+                        validate_id,
+                        ref_target_id,
+                        Some(Instruction::Dependent {
+                            ref_name: Cow::Borrowed(ref_name),
+                            position,
+                        }),
+                    )
                 }
             }
         };
 
-        let mut existing_ws_meta = workspace
-            .ref_name()
-            .and_then(|ws_ref| meta.workspace_opt(ws_ref).transpose())
+        let updated_ws_meta = existing_ws_meta
+            .take()
+            .zip(instruction)
+            .map(|(mut existing, instruction)| {
+                update_workspace_metadata(&mut existing, ref_name, instruction).map(|()| existing)
+            })
             .transpose()?;
-        let updated_ws_meta = if let Some(instruction) = insert {
-            if let Some(mut existing) = existing_ws_meta.take() {
-                update_workspace_metadata(&mut existing, ref_name, instruction)?;
-                Some(existing)
-            } else {
-                None
-            }
-        } else {
-            None
-        };
-
         // Assure this commit is in the workspace as well.
-        if check_if_commit_in_workspace {
-            workspace.try_find_owner_indexes_by_commit_id(ref_target)?;
+        if check_if_id_in_workspace {
+            workspace.try_find_owner_indexes_by_commit_id(ref_target_id)?;
         }
 
         let graph_with_new_ref = {
@@ -243,7 +261,7 @@ pub(super) mod function {
                 but_graph::init::Overlay::default()
                     .with_references_if_new(Some(gix::refs::Reference {
                         name: ref_name.into(),
-                        target: gix::refs::Target::Object(ref_target),
+                        target: gix::refs::Target::Object(ref_target_id),
                         peeled: None,
                     }))
                     .with_branch_metadata_override(Some((
@@ -265,24 +283,23 @@ pub(super) mod function {
         if !has_new_ref_as_standalone_segment {
             // TODO: this should probably be easier to understand for the UI, with error codes maybe?
             bail!(
-                "Reference '{}' cannot be created as segment at {ref_target}",
+                "Reference '{}' cannot be created as segment at {ref_target_id}",
                 ref_name.shorten()
             )
         }
 
         // Actually apply the changes
         repo.reference(
             ref_name,
-            ref_target,
-            PreviousValue::ExistingMustMatch(gix::refs::Target::Object(ref_target)),
+            ref_target_id,
+            PreviousValue::ExistingMustMatch(gix::refs::Target::Object(ref_target_id)),
             "Dependent branch by GitButler",
         )?;
         // Important to first update the workspace so we have the correct stack setup.
         if let Some(ws_meta) = updated_ws_meta {
-            // TODO: overwrite stored information with reality in new graph.
             meta.set_workspace(&ws_meta)?;
-        } else if let Some(mut existing) = existing_ws_meta {
-            updated_workspace.update_metadata(&mut *existing);
+        } else if let Some(existing) = existing_ws_meta {
+            // TODO: overwrite stored information with reality in new graph.
             meta.set_workspace(&existing)?;
         }
 
@@ -315,23 +332,25 @@ pub(super) mod function {
         new_ref: &gix::refs::FullNameRef,
         instruction: Instruction<'_>,
     ) -> anyhow::Result<()> {
+        if ws_meta.find_branch(new_ref).is_some() {
+            return Ok(());
+        }
         match instruction {
             // create new
-            None => {
-                if ws_meta.find_branch(new_ref).is_none() {
-                    ws_meta.stacks.push(WorkspaceStack {
-                        id: StackId::generate(),
-                        branches: vec![WorkspaceStackBranch {
-                            ref_name: new_ref.to_owned(),
-                            archived: false,
-                        }],
-                    })
-                }
-            }
+            Instruction::Independent => ws_meta.stacks.push(WorkspaceStack {
+                id: StackId::generate(),
+                branches: vec![WorkspaceStackBranch {
+                    ref_name: new_ref.to_owned(),
+                    archived: false,
+                }],
+            }),
             // insert dependent branch at anchor
-            Some((anchor_ref, position)) => {
+            Instruction::Dependent {
+                ref_name: anchor_ref,
+                position,
+            } => {
                 let (stack_idx, branch_idx) = ws_meta
-                    .find_owner_indexes_by_name(anchor_ref)
+                    .find_owner_indexes_by_name(anchor_ref.as_ref())
                     .with_context(|| {
                         format!(
                             "Couldn't find anchor '{}' in workspace metadata",
@@ -354,5 +373,82 @@ pub(super) mod function {
         Ok(())
     }
 
-    type Instruction<'a> = Option<(&'a gix::refs::FullNameRef, ReferencePosition)>;
+    /// Create the instruction that would be needed to insert the new ref-name into workspace data
+    /// so that it represents the `position` of `anchor_id` and `target_id` (being (`anchor_id` offset by `position`).
+    /// `position` indicates where, in relation to `anchor_id`, the ref name should be inserted.
+    /// The first name that is also in `ws_meta` will be used.
+    fn instruction_by_named_anchor_for_commit(
+        ws: &but_graph::projection::Workspace<'_>,
+        anchor_id: gix::ObjectId,
+        target_id: gix::ObjectId,
+        position: ReferencePosition,
+    ) -> anyhow::Result<Instruction<'static>> {
+        use ReferencePosition::*;
+        let (anchor_stack_idx, anchor_seg_idx, _anchor_commit_idx) = ws
+            .find_owner_indexes_by_commit_id(anchor_id)
+            .with_context(|| {
+                format!(
+                    "No segment in workspace at '{}' that holds {anchor_id}",
+                    ws.ref_name_display()
+                )
+            })?;
+        let (target_stack_idx, target_seg_idx, _target_commit_idx) = ws
+            .find_owner_indexes_by_commit_id(target_id)
+            .with_context(|| {
+                format!(
+                    "No segment in workspace at '{}' that holds {target_id}",
+                    ws.ref_name_display()
+                )
+            })?;
+        if anchor_stack_idx != target_stack_idx {
+            bail!(
+                "BUG: Anchor id {anchor_id} and {target_id} ended up in different stacks - how can that be?"
+            );
+        }
+
+        // TODO: this is missing a lot of tests for the empty-segment complexity, but also the
+        let stack = &ws.stacks[anchor_stack_idx];
+        if anchor_seg_idx == target_seg_idx {
+            // Find first non-empty segment in this stack upward and downward.
+            (0..anchor_seg_idx + 1).rev()
+                .find_map(|seg_idx| {
+                    stack.segments[seg_idx]
+                        .ref_name
+                        .as_ref()
+                        .map(|rn| (rn.as_ref(), Below))
+                })
+                .or_else(|| {
+                    (anchor_seg_idx + 1..stack.segments.len()).find_map(|seg_idx| {
+                        stack.segments[seg_idx]
+                            .ref_name
+                            .as_ref()
+                            .map(|rn| (rn.as_ref(), Above))
+                    })
+                })
+                .map(|(anchor_ref, position)| Instruction::Dependent {
+                    ref_name: Cow::Owned(anchor_ref.to_owned()),
+                    position,
+                })
+        } else {
+            if anchor_seg_idx + 1 != target_seg_idx {
+                bail!(
+                    "BUG: The anchor commit {anchor_id} must be above {target_id}, but the segments where {anchor_seg_idx} and {target_seg_idx}"
+                );
+            }
+            todo!("sort out start position and positioning")
+        }.with_context(|| {
+            format!(
+                "Didn't find a named segment in workspace at '{}' to indicate where to insert {anchor_id} {position:?}",
+                ws.ref_name_display()
+            )
+        })
+    }
+
+    enum Instruction<'a> {
+        Independent,
+        Dependent {
+            ref_name: Cow<'a, gix::refs::FullNameRef>,
+            position: ReferencePosition,
+        },
+    }
 }
diff --git a/crates/but-workspace/tests/workspace/branch.rs b/crates/but-workspace/tests/workspace/branch.rs
