Cover more cases when applying branches to prevent failures.

Byron · Byron · commit c048ffa82bfe · 2025-10-06T19:40:57.000+02:00
Otherwise, we think we applied the branch, but it's not enough
due to ambiguity. Then the stack or segment wouldn't actually
show up unless we make the right choice of adding it as dependent
branch instead of stack.
diff --git a/crates/but-core/src/ref_metadata.rs b/crates/but-core/src/ref_metadata.rs
@@ -68,6 +68,28 @@ impl Workspace {
         }
         true
     }
+
+    /// Insert `branch` as new segment if it's not yet contained in the workspace,
+    /// and insert it above the given `anchor` segment name, which maybe the tip of a stack or any segment within one
+    /// Returns `true` if the ref was newly added, or `false` if it already existed, or `None` if `anchor` didn't exist.
+    pub fn insert_new_segment_above_anchor_if_not_present(
+        &mut self,
+        branch: &FullNameRef,
+        anchor: &FullNameRef,
+    ) -> Option<bool> {
+        if self.contains_ref(branch) {
+            return Some(false);
+        };
+        let (stack_idx, segment_idx) = self.find_owner_indexes_by_name(anchor)?;
+        self.stacks[stack_idx].branches.insert(
+            segment_idx,
+            WorkspaceStackBranch {
+                ref_name: branch.to_owned(),
+                archived: false,
+            },
+        );
+        Some(true)
+    }
 }
 
 /// Access
diff --git a/crates/but-core/tests/core/ref_metadata.rs b/crates/but-core/tests/core/ref_metadata.rs
@@ -20,6 +20,70 @@ mod workspace {
         assert_eq!(ws.stack_names().collect::<Vec<_>>(), [b_ref, a_ref, c_ref]);
     }
 
+    #[test]
+    fn insert_new_segment_above_anchor_if_not_present() {
+        let mut ws = Workspace::default();
+        assert_eq!(ws.stacks.len(), 0);
+
+        let a_ref = r("refs/heads/A");
+        let b_ref = r("refs/heads/B");
+        assert_eq!(
+            ws.insert_new_segment_above_anchor_if_not_present(b_ref, a_ref),
+            None,
+            "anchor doesn't exist"
+        );
+        assert!(ws.add_or_insert_new_stack_if_not_present(a_ref, None));
+        assert_eq!(
+            ws.insert_new_segment_above_anchor_if_not_present(b_ref, a_ref),
+            Some(true),
+            "anchor existed and it was added"
+        );
+        assert_eq!(
+            ws.insert_new_segment_above_anchor_if_not_present(b_ref, a_ref),
+            Some(false),
+            "anchor existed and it was NOT added as it already existed"
+        );
+
+        let c_ref = r("refs/heads/C");
+        assert_eq!(
+            ws.insert_new_segment_above_anchor_if_not_present(c_ref, a_ref),
+            Some(true)
+        );
+
+        insta::assert_snapshot!(but_testsupport::sanitize_uuids_and_timestamps(format!("{ws:#?}")), @r#"
+        Workspace {
+            ref_info: RefInfo { created_at: None, updated_at: None },
+            stacks: [
+                WorkspaceStack {
+                    id: 1,
+                    branches: [
+                        WorkspaceStackBranch {
+                            ref_name: FullName(
+                                "refs/heads/B",
+                            ),
+                            archived: false,
+                        },
+                        WorkspaceStackBranch {
+                            ref_name: FullName(
+                                "refs/heads/C",
+                            ),
+                            archived: false,
+                        },
+                        WorkspaceStackBranch {
+                            ref_name: FullName(
+                                "refs/heads/A",
+                            ),
+                            archived: false,
+                        },
+                    ],
+                },
+            ],
+            target_ref: None,
+            push_remote: None,
+        }
+        "#);
+    }
+
     fn r(name: &str) -> &gix::refs::FullNameRef {
         name.try_into().expect("statically known ref")
     }
diff --git a/crates/but-workspace/src/branch/apply.rs b/crates/but-workspace/src/branch/apply.rs
@@ -157,6 +157,7 @@ pub(crate) mod function {
                 repo,
                 checkout::Options {
                     uncommitted_changes,
+                    skip_head_update: false,
                 },
             )?;
             let graph = workspace.graph.redo_traversal_with_overlay(
@@ -262,7 +263,39 @@ pub(crate) mod function {
         {
             let ws_mut: &mut Workspace = &mut ws_md;
             for rn in &branches_to_apply {
-                ws_mut.add_or_insert_new_stack_if_not_present(rn, order);
+                // Here we have to check if the new ref would be able to become its own stack,
+                // or if it has to be a dependent branch. Stacks only work if the ref rests on a base
+                // outside the workspace, so if we find it in the workspace (in an ambiguous spot) it must be
+                // a dependent branch
+                if let Some(segment_to_insert_above) = workspace
+                    .stacks
+                    .iter()
+                    .flat_map(|stack| stack.segments.iter())
+                    .find_map(|segment| {
+                        segment.commits.iter().flat_map(|c| c.refs.iter()).find_map(
+                            |ambiguous_rn| {
+                                (ambiguous_rn.as_ref() == *rn)
+                                    .then_some(segment.ref_name.as_ref())
+                                    .flatten()
+                            },
+                        )
+                    })
+                {
+                    if ws_mut
+                        .insert_new_segment_above_anchor_if_not_present(
+                            rn,
+                            segment_to_insert_above.as_ref(),
+                        )
+                        .is_none()
+                    {
+                        // For now bail, until we know it's worth fixing this case automatically.
+                        bail!(
+                            "Missing reference {segment_to_insert_above} which should be known to workspace metadata to serve as insertion position for {rn}"
+                        );
+                    }
+                } else {
+                    ws_mut.add_or_insert_new_stack_if_not_present(rn, order);
+                }
             }
         }
         let ws_md_override = Some((workspace_ref_name_to_update.clone(), (*ws_md).clone()));
@@ -353,31 +386,55 @@ pub(crate) mod function {
             });
         }
 
-        // All work is done, persist and exit.
-        // Note that it could be that some stacks aren't merged in,
-        // while being present in the workspace metadata.
-        // This is OK for us. We also trust that the hero-branch was merged in, no matter what.
-        if let Some(storage) = in_memory_repo.objects.take_object_memory() {
-            storage.persist(repo)?;
-            drop(in_memory_repo);
-        }
         let prev_head_id = graph
             .entrypoint_commit()
             .context("BUG: how is it possible that there is no head commit?")?
             .id;
         let new_head_id = merge_result.workspace_commit_id;
         let graph = graph.redo_traversal_with_overlay(
-            repo,
+            &in_memory_repo,
             meta,
             overlay.with_entrypoint(new_head_id, Some(workspace_ref_name_to_update.clone())),
         )?;
+
+        let workspace = graph.to_workspace()?;
+        let unapplied_branches: Vec<_> = branches_to_apply
+            .iter()
+            .filter_map(|rn| {
+                if workspace.refname_is_segment(rn) {
+                    None
+                } else {
+                    Some(rn)
+                }
+            })
+            .collect();
+        if !unapplied_branches.is_empty() {
+            bail!(
+                "Unexpectedly failed to apply {branches} which is/are still not in the workspace",
+                branches = unapplied_branches
+                    .iter()
+                    .map(|rn| rn.shorten().to_string())
+                    .collect::<Vec<_>>()
+                    .join(", ")
+            )
+        }
+
+        // All work is done, persist and exit.
+        // Note that it could be that some stacks aren't merged in,
+        // while being present in the workspace metadata.
+        // This is OK for us. We also trust that the hero-branch was merged in, no matter what.
+        if let Some(storage) = in_memory_repo.objects.take_object_memory() {
+            storage.persist(repo)?;
+            drop(in_memory_repo);
+        }
         persist_metadata(meta, &branches_to_apply, &ws_md)?;
         crate::branch::safe_checkout(
             prev_head_id,
             new_head_id,
             repo,
             checkout::Options {
                 uncommitted_changes,
+                skip_head_update: true,
             },
         )?;
 
@@ -416,10 +473,9 @@ pub(crate) mod function {
         new_ref_target: gix::ObjectId,
         new_ref: Option<&gix::refs::FullNameRef>,
     ) -> anyhow::Result<()> {
-        // This also means we want HEAD to point to it.
-        let head_message = "GitButler switch to workspace during apply-branch".into();
         let edits = match new_ref {
             None => {
+                let head_message = "GitButler checkout workspace during apply-branch".into();
                 vec![RefEdit {
                     change: Change::Update {
                         log: LogChange {
@@ -435,6 +491,8 @@ pub(crate) mod function {
                 }]
             }
             Some(new_ref) => {
+                // This also means we want HEAD to point to it.
+                let head_message = "GitButler switch to workspace during apply-branch".into();
                 vec![
                     RefEdit {
                         change: Change::Update {
diff --git a/crates/but-workspace/src/branch/checkout/function.rs b/crates/but-workspace/src/branch/checkout/function.rs
@@ -50,6 +50,7 @@ pub fn safe_checkout(
     repo: &gix::Repository,
     Options {
         uncommitted_changes: conflicting_worktree_changes_opts,
+        skip_head_update,
     }: Options,
 ) -> anyhow::Result<Outcome> {
     let source_tree = current_head_id.attach(repo).object()?.peel_to_tree()?;
@@ -138,7 +139,7 @@ pub fn safe_checkout(
     }
 
     let mut head_update = None;
-    if new_object.kind.is_commit() {
+    if new_object.kind.is_commit() && !skip_head_update {
         let needs_update = repo
             .head()?
             .id()
diff --git a/crates/but-workspace/src/branch/checkout/mod.rs b/crates/but-workspace/src/branch/checkout/mod.rs
@@ -17,6 +17,10 @@ pub enum UncommitedWorktreeChanges {
 pub struct Options {
     /// How to deal with uncommitted changes.
     pub uncommitted_changes: UncommitedWorktreeChanges,
+    /// If `true`, do not change `HEAD` to the new commit.
+    ///
+    /// This is typically to be avoided, but may be used if you want to change the HEAD location yourself.
+    pub skip_head_update: bool,
 }
 
 /// The successful outcome of [super::safe_checkout()] operation.
diff --git a/crates/but-workspace/tests/fixtures/scenario/no-ws-ref-stack-and-dependent-branch.sh b/crates/but-workspace/tests/fixtures/scenario/no-ws-ref-stack-and-dependent-branch.sh
@@ -0,0 +1,21 @@
+#!/usr/bin/env bash
+
+set -eu -o pipefail
+
+source "${BASH_SOURCE[0]%/*}/shared.sh"
+
+### General Description
+
+# A target branch along with a stack and a depdendent branch at the same tip, without workspace commit.
+git init
+commit M
+setup_target_to_match_main
+git checkout -b A
+commit A1
+git branch D
+git branch E
+commit A2
+git branch B
+git branch C
+
+git checkout main
diff --git a/crates/but-workspace/tests/workspace/branch/apply_unapply_commit_uncommit.rs b/crates/but-workspace/tests/workspace/branch/apply_unapply_commit_uncommit.rs
@@ -521,6 +521,84 @@ fn detached_head_journey() -> anyhow::Result<()> {
     Ok(())
 }
 
+#[test]
+fn apply_two_ambiguous_stacks_with_target() -> anyhow::Result<()> {
+    let (_tmp, graph, repo, mut meta, _description) =
+        named_writable_scenario_with_description_and_graph(
+            "no-ws-ref-stack-and-dependent-branch",
+            |_meta| {},
+        )?;
+    insta::assert_snapshot!(visualize_commit_graph_all(&repo)?, @r"
+    * f084d61 (C, B, A) A2
+    * 7076dee (E, D) A1
+    * 85efbe4 (HEAD -> main, origin/main) M
+    ");
+
+    let ws = graph.to_workspace()?;
+    insta::assert_snapshot!(graph_workspace(&ws), @r"
+    ⌂:0:main <> ✓!
+    └── ≡:0:main
+        └── :0:main
+            └── ·85efbe4
+    ");
+
+    // Apply `A` first.
+    let out =
+        but_workspace::branch::apply(r("refs/heads/A"), &ws, &repo, &mut meta, default_options())?;
+    insta::assert_debug_snapshot!(out, @r"
+    Outcome {
+        workspace_changed: true,
+        workspace_ref_created: true,
+    }
+    ");
+    let graph = out.graph;
+    let ws = graph.to_workspace()?;
+    insta::assert_snapshot!(graph_workspace(&ws), @r"
+    📕🏘️:0:gitbutler/workspace <> ✓refs/remotes/origin/main on 85efbe4
+    └── ≡📙:3:A on 85efbe4
+        └── 📙:3:A
+            ├── ·f084d61 (🏘️) ►B, ►C
+            └── ·7076dee (🏘️) ►D, ►E
+    ");
+    insta::assert_snapshot!(visualize_commit_graph_all(&repo)?, @r"
+    * 6a706b7 (HEAD -> gitbutler/workspace) GitButler Workspace Commit
+    * f084d61 (C, B, A) A2
+    * 7076dee (E, D) A1
+    * 85efbe4 (origin/main, main) M
+    ");
+
+    // Apply `B` - the only sane way is to make it its own stack, but allow it to diverge.
+    let out =
+        but_workspace::branch::apply(r("refs/heads/B"), &ws, &repo, &mut meta, default_options())
+            .expect("apply actually works");
+    insta::assert_debug_snapshot!(out, @r"
+    Outcome {
+        workspace_changed: true,
+        workspace_ref_created: false,
+    }
+    ");
+
+    let graph = out.graph;
+    let ws = graph.to_workspace()?;
+    insta::assert_snapshot!(graph_workspace(&ws), @r"
+    📕🏘️:0:gitbutler/workspace <> ✓refs/remotes/origin/main on 85efbe4
+    └── ≡📙:4:B on 85efbe4
+        ├── 📙:4:B
+        └── 📙:5:A
+            ├── ·f084d61 (🏘️) ►C
+            └── ·7076dee (🏘️) ►D, ►E
+    ");
+    insta::assert_snapshot!(visualize_commit_graph_all(&repo)?, @r"
+    * badd1b4 (HEAD -> gitbutler/workspace) GitButler Workspace Commit
+    * f084d61 (C, B, A) A2
+    * 7076dee (E, D) A1
+    * 85efbe4 (origin/main, main) M
+    ");
+
+    // TODO: add all other dependent branches as well.
+    Ok(())
+}
+
 #[test]
 fn auto_checkout_of_enclosing_workspace_flat() -> anyhow::Result<()> {
     let (_tmp, graph, repo, mut meta, _description) =
diff --git a/crates/but-workspace/tests/workspace/branch/checkout.rs b/crates/but-workspace/tests/workspace/branch/checkout.rs
@@ -827,6 +827,7 @@ fn unrelated_additions_do_not_affect_worktree_changes() -> anyhow::Result<()> {
 fn overwrite_options() -> checkout::Options {
     checkout::Options {
         uncommitted_changes: UncommitedWorktreeChanges::KeepConflictingInSnapshotAndOverwrite,
+        skip_head_update: false,
     }
 }
 

Original file line number	Diff line number	Diff line change
`@@ -827,6 +827,7 @@ fn unrelated_additions_do_not_affect_worktree_changes() -> anyhow::Result<()> {`
`827`	`827`	`fn overwrite_options() -> checkout::Options {`
`828`	`828`	`checkout::Options {`
`829`	`829`	`uncommitted_changes: UncommitedWorktreeChanges::KeepConflictingInSnapshotAndOverwrite,`
	`830`	`+ skip_head_update: false,`
`830`	`831`	`}`
`831`	`832`	`}`
`832`	`833`