Merge pull request #13086 from gitbutlerapp/mg-branch-11

Add pre-flight conflict detection for cross-stack commit and branch moves

Author: mtsgrd

crates/gitbutler-branch-actions/src/actions.rs

@@ -396,10 +396,6 @@ pub fn move_commit(
     ctx.verify(guard.write_permission())?;
     ensure_open_workspace_mode(ctx, guard.read_permission())
         .context("Moving a commit requires open workspace mode")?;
-    let _ = ctx.create_snapshot(
-        SnapshotDetails::new(OperationKind::MoveCommit),
-        guard.write_permission(),
-    );
     move_commits::move_commit(
         ctx,
         target_stack_id,
@@ -420,10 +416,6 @@ pub fn move_branch(
     ctx.verify(guard.write_permission())?;
     ensure_open_workspace_mode(ctx, guard.read_permission())
         .context("Moving a branch requires open workspace mode")?;
-    let _ = ctx.create_snapshot(
-        SnapshotDetails::new(OperationKind::MoveBranch),
-        guard.write_permission(),
-    );
     crate::move_branch::move_branch(
         ctx,
         target_stack_id,

crates/gitbutler-branch-actions/src/move_branch.rs

@@ -3,13 +3,18 @@ use but_core::ref_metadata::StackId;
 use but_ctx::{Context, access::RepoExclusive};
 use but_rebase::{Rebase, RebaseStep};
 use but_workspace::legacy::stack_ext::StackExt;
+use gitbutler_oplog::{
+    OplogExt,
+    entry::{OperationKind, SnapshotDetails},
+};
 use gitbutler_reference::{LocalRefname, Refname};
-use gitbutler_stack::{StackBranch, VirtualBranchesHandle};
+use gitbutler_stack::StackBranch;
 use gitbutler_workspace::branch_trees::{WorkspaceState, update_uncommitted_changes};
 use gix::refs::transaction::PreviousValue;
 use serde::Serialize;
 
-use crate::BranchManagerExt;
+use crate::{BranchManagerExt, VirtualBranchesExt as _, move_commits::bail_on_new_conflicts};
+use anyhow::bail;
 
 #[derive(Debug, Clone, Serialize)]
 #[serde(rename_all = "camelCase")]
@@ -22,55 +27,143 @@ pub struct MoveBranchResult {
 }
 
 pub(crate) fn move_branch(
-    ctx: &Context,
+    ctx: &mut Context,
     target_stack_id: StackId,
     target_branch_name: &str,
     source_stack_id: StackId,
     subject_branch_name: &str,
     perm: &mut RepoExclusive,
 ) -> Result<MoveBranchResult> {
+    if source_stack_id == target_stack_id {
+        bail!("Cannot move a branch within the same stack; use the reorder operation instead.");
+    }
     let old_workspace = WorkspaceState::create(ctx, perm.read_permission())?;
-    let repo = ctx.repo.get()?;
-    let mut vb_state = VirtualBranchesHandle::new(ctx.project_data_dir());
 
-    let source_stack = vb_state.get_stack_in_workspace(source_stack_id)?;
-    let source_merge_base = source_stack.merge_base(ctx)?;
+    let (source_merge_base, dest_merge_base, source_branch_pr_number) = {
+        let vb_state = ctx.virtual_branches();
+        let source_stack = vb_state.get_stack_in_workspace(source_stack_id)?;
+        let dest_stack = vb_state.get_stack_in_workspace(target_stack_id)?;
+        let pr_number = source_stack
+            .branches()
+            .into_iter()
+            .find(|b| b.name == subject_branch_name)
+            .context("Subject branch not found in source stack")?
+            .pr_number;
+        (
+            source_stack.merge_base(ctx)?,
+            dest_stack.merge_base(ctx)?,
+            pr_number,
+        )
+    };
+
+    // Cross-stack move: compute both rebases (ODB-only writes), check for
+    // conflicts, then snapshot, then apply state changes.
+    let (source_output, dest_output, source_will_be_deleted) = {
+        let repo = ctx.repo.get()?;
+        let vb_state = ctx.virtual_branches();
+        let source_stack = vb_state.get_stack_in_workspace(source_stack_id)?;
+        let dest_stack = vb_state.get_stack_in_workspace(target_stack_id)?;
+
+        let (subject_branch_steps, remaining_steps) =
+            extract_branch_steps(ctx, &repo, &source_stack, subject_branch_name)?;
+
+        let source_will_be_deleted = remaining_steps.is_empty();
+
+        // Source: rebase remaining commits without the moved branch (if any remain).
+        let source_output = if !source_will_be_deleted {
+            let mut src_rebase = Rebase::new(&repo, source_merge_base, None)?;
+            src_rebase.steps(remaining_steps)?;
+            src_rebase.rebase_noops(false);
+            Some(src_rebase.rebase(&*ctx.cache.get_cache()?)?)
+        } else {
+            None
+        };
+
+        // Dest: rebase dest stack with the moved branch injected.
+        let new_dest_steps = inject_branch_steps(
+            ctx,
+            &repo,
+            &dest_stack,
+            target_branch_name,
+            subject_branch_steps,
+        )?;
+        let mut dst_rebase = Rebase::new(&repo, dest_merge_base, None)?;
+        dst_rebase.steps(new_dest_steps)?;
+        dst_rebase.rebase_noops(false);
+        let dest_output = dst_rebase.rebase(&*ctx.cache.get_cache()?)?;
+
+        // Conflict check — bail before any state is written.
+        if let Some(ref src_out) = source_output {
+            bail_on_new_conflicts(
+                &repo,
+                src_out,
+                "This move would cause a conflict in the source stack: \
+                 other commits depend on the changes being moved.",
+            )?;
+        }
+        bail_on_new_conflicts(
+            &repo,
+            &dest_output,
+            "This move would cause a conflict in the destination stack: \
+             the branch does not apply cleanly at the target location.",
+        )?;
 
-    let source_branch = source_stack
-        .branches()
-        .into_iter()
-        .find(|b| b.name == subject_branch_name)
-        .context("Subject branch not found in source stack")?;
+        (source_output, dest_output, source_will_be_deleted)
+    };
 
-    let destination_stack = vb_state.get_stack_in_workspace(target_stack_id)?;
-    let destination_merge_base = destination_stack.merge_base(ctx)?;
+    // Snapshot after the conflict check, but before any state writes.
+    let _ = ctx.create_snapshot(SnapshotDetails::new(OperationKind::MoveBranch), perm);
 
-    let (subject_branch_steps, deleted_stacks) = extract_and_rebase_source_branch(
-        ctx,
-        source_stack_id,
-        subject_branch_name,
-        &repo,
-        &mut vb_state,
-        source_stack,
-        source_merge_base,
-    )?;
+    // Apply source changes.
+    let mut deleted_stacks = Vec::new();
+    {
+        let repo = ctx.repo.get()?;
+        let mut vb_state = ctx.virtual_branches();
+        if source_will_be_deleted {
+            vb_state.delete_branch_entry(&source_stack_id)?;
+            deleted_stacks.push(source_stack_id);
+        } else if let Some(src_out) = source_output {
+            let mut source_stack = vb_state.get_stack_in_workspace(source_stack_id)?;
+            let new_source_head = repo.find_commit(src_out.top_commit)?;
+            source_stack.remove_branch(ctx, subject_branch_name)?;
+            source_stack.set_stack_head(&mut vb_state, &repo, new_source_head.id().detach())?;
+            source_stack.set_heads_from_rebase_output(ctx, src_out.references)?;
+        }
+    }
 
-    // Inject the extracted branch steps into the destination stack and rebase the stack
-    inject_branch_steps_into_destination(
-        ctx,
-        target_branch_name,
-        subject_branch_name,
-        &repo,
-        &mut vb_state,
-        destination_stack,
-        destination_merge_base,
-        subject_branch_steps,
-        source_branch.pr_number,
-    )?;
+    // Apply dest changes.
+    {
+        let repo = ctx.repo.get()?;
+        let mut vb_state = ctx.virtual_branches();
+        let mut destination_stack = vb_state.get_stack_in_workspace(target_stack_id)?;
+        let new_destination_head = repo.find_commit(dest_output.top_commit)?;
+
+        // StackBranch::new validates that the supplied commit is within the current stack
+        // range. The rebased subject head isn't "in range" yet because the stack head hasn't
+        // been updated, so we seed the new branch with the anchor branch's current head as a
+        // placeholder. set_heads_from_rebase_output corrects it to the proper commit below.
+        let anchor_ref = dest_output
+            .references
+            .iter()
+            .find(|r| r.reference.to_string() == target_branch_name)
+            .context("target branch not found in dest rebase output")?;
+
+        let mut new_head =
+            StackBranch::new(anchor_ref.commit_id, subject_branch_name.to_string(), &repo)?;
+        new_head.pr_number = source_branch_pr_number;
+
+        destination_stack.add_series(ctx, new_head, Some(target_branch_name.to_string()))?;
+        destination_stack.set_stack_head(
+            &mut vb_state,
+            &repo,
+            new_destination_head.id().detach(),
+        )?;
+        destination_stack.set_heads_from_rebase_output(ctx, dest_output.references)?;
+    }
 
     let new_workspace = WorkspaceState::create(ctx, perm.read_permission())?;
     let _ = update_uncommitted_changes(ctx, old_workspace, new_workspace, perm);
-    crate::integration::update_workspace_commit_with_vb_state(&vb_state, ctx, false)
+    crate::integration::update_workspace_commit_with_vb_state(&ctx.virtual_branches(), ctx, false)
         .context("failed to update gitbutler workspace")?;
 
     Ok(MoveBranchResult {
@@ -88,17 +181,17 @@ pub(crate) fn tear_off_branch(
 ) -> Result<MoveBranchResult> {
     let old_workspace = WorkspaceState::create(ctx, perm.read_permission())?;
     let repo = ctx.repo.get()?;
-    let mut vb_state = VirtualBranchesHandle::new(ctx.project_data_dir());
 
-    let source_stack = vb_state.get_stack_in_workspace(source_stack_id)?;
+    let source_stack = ctx
+        .virtual_branches()
+        .get_stack_in_workspace(source_stack_id)?;
     let source_merge_base = source_stack.merge_base(ctx)?;
 
     let (subject_branch_steps, deleted_stacks) = extract_and_rebase_source_branch(
         ctx,
         source_stack_id,
         subject_branch_name,
         &repo,
-        &mut vb_state,
         source_stack,
         source_merge_base,
     )?;
@@ -126,7 +219,7 @@ pub(crate) fn tear_off_branch(
 
     let new_workspace = WorkspaceState::create(ctx, perm.read_permission())?;
     let _ = update_uncommitted_changes(ctx, old_workspace, new_workspace, perm);
-    crate::integration::update_workspace_commit_with_vb_state(&vb_state, ctx, false)
+    crate::integration::update_workspace_commit_with_vb_state(&ctx.virtual_branches(), ctx, false)
         .context("failed to update gitbutler workspace")?;
 
     let branch_manager = ctx.branch_manager();
@@ -144,63 +237,12 @@ pub(crate) fn tear_off_branch(
     })
 }
 
-#[expect(clippy::too_many_arguments)]
-/// Injects the extracted branch steps into the destination stack and rebases it.
-fn inject_branch_steps_into_destination(
-    ctx: &Context,
-    target_branch_name: &str,
-    subject_branch_name: &str,
-    repo: &gix::Repository,
-    vb_state: &mut VirtualBranchesHandle,
-    destination_stack: gitbutler_stack::Stack,
-    destination_merge_base: gix::ObjectId,
-    subject_branch_steps: Vec<RebaseStep>,
-    subject_branch_pr_number: Option<usize>,
-) -> Result<(), anyhow::Error> {
-    let new_destination_steps = inject_branch_steps(
-        ctx,
-        repo,
-        &destination_stack,
-        target_branch_name,
-        subject_branch_steps,
-    )?;
-
-    let mut destination_stack_rebase = Rebase::new(repo, destination_merge_base, None)?;
-    destination_stack_rebase.steps(new_destination_steps)?;
-    destination_stack_rebase.rebase_noops(false);
-    let destination_rebase_result = destination_stack_rebase.rebase(&*ctx.cache.get_cache()?)?;
-    let new_destination_head = repo.find_commit(destination_rebase_result.top_commit)?;
-    let mut destination_stack = destination_stack;
-
-    let target_branch_reference = destination_rebase_result
-        .clone()
-        .references
-        .into_iter()
-        .find(|r| r.reference.to_string() == target_branch_name)
-        .context("subject branch not found in rebase output")?;
-
-    let target_branch_head = target_branch_reference.commit_id;
-
-    let mut new_head = StackBranch::new(target_branch_head, subject_branch_name.to_string(), repo)?;
-
-    new_head.pr_number = subject_branch_pr_number;
-
-    destination_stack.add_series(ctx, new_head, Some(target_branch_name.to_string()))?;
-
-    destination_stack.set_stack_head(vb_state, repo, new_destination_head.id().detach())?;
-
-    destination_stack
-        .set_heads_from_rebase_output(ctx, destination_rebase_result.clone().references)?;
-    Ok(())
-}
-
 /// Extracts the steps corresponding to the branch to move, and rebases the source stack without those steps.
 fn extract_and_rebase_source_branch(
     ctx: &Context,
     source_stack_id: StackId,
     subject_branch_name: &str,
     repository: &gix::Repository,
-    vb_state: &mut VirtualBranchesHandle,
     source_stack: gitbutler_stack::Stack,
     source_merge_base: gix::ObjectId,
 ) -> Result<(Vec<RebaseStep>, Vec<StackId>), anyhow::Error> {
@@ -211,7 +253,8 @@ fn extract_and_rebase_source_branch(
 
     if new_source_steps.is_empty() {
         // If there are no other branches left in the source stack, delete the stack.
-        vb_state.delete_branch_entry(&source_stack_id)?;
+        ctx.virtual_branches()
+            .delete_branch_entry(&source_stack_id)?;
         deleted_stacks.push(source_stack_id);
     } else {
         // Rebase the source stack without the extracted branch steps
@@ -223,13 +266,24 @@ fn extract_and_rebase_source_branch(
 
         source_stack.remove_branch(ctx, subject_branch_name)?;
 
-        source_stack.set_stack_head(vb_state, repository, new_source_head.id().detach())?;
+        source_stack.set_stack_head(
+            &mut ctx.virtual_branches(),
+            repository,
+            new_source_head.id().detach(),
+        )?;
 
         source_stack.set_heads_from_rebase_output(ctx, source_rebase_result.clone().references)?;
     }
     Ok((subject_branch_steps, deleted_stacks))
 }
 
+/// Splits the source stack's rebase steps into two groups: those belonging to
+/// `subject_branch_name` and those that remain.
+///
+/// Steps are partitioned by scanning for a `Reference` marker whose name matches
+/// the subject branch (either as a Git ref or a virtual ref). All steps between
+/// consecutive Reference markers are considered part of that branch. Returns
+/// `(subject_steps, remaining_steps)`, both in execution order (oldest first).
 fn extract_branch_steps(
     ctx: &Context,
     repository: &gix::Repository,
@@ -287,7 +341,7 @@ fn inject_branch_steps(
     branch_steps: Vec<RebaseStep>,
 ) -> Result<Vec<RebaseStep>> {
     let destination_steps = destination_stack.as_rebase_steps_rev(ctx)?;
-    let mut branch_steps = branch_steps.clone();
+    let mut branch_steps = branch_steps;
     branch_steps.reverse();
 
     let mut new_destination_steps = Vec::new();