From 538cad69d386d5fd18c5c6e4ed899628af870528 Mon Sep 17 00:00:00 2001 From: Mattias Granlund Date: Sat, 28 Mar 2026 14:53:09 +0200 Subject: [PATCH] Add pre-flight conflict detection for cross-stack commit and branch moves MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When moving a commit or branch to a different stack, both rebases are computed against the ODB first — no refs, no snapshot, no worktree state is touched. Their outputs are then inspected for newly-conflicted commits before any state is written. This means each stack is rebased exactly once per operation. Source check: if removing the moved commit(s) would cause remaining commits in the source stack to conflict, the move is rejected — those commits depend on the changes being moved away. Destination check: if inserting the moved commit(s) onto the destination stack would cause a conflict, the move is rejected — the changes do not apply cleanly at the target location. Pre-existing conflicts in the source stack are exempted: if a commit was already conflicted before the move, it is not counted as a new conflict caused by the move. Moves that are now prevented: - Moving a commit whose removal would leave dependent commits in the source stack that can no longer rebase cleanly. - Moving a commit onto a destination stack that already modified the same content, causing a conflict at the insertion point. - Moving a branch whose removal would strand dependent branches in the source stack. - Moving a branch onto a destination that has already modified the same content. Moves that are not affected: - Intra-stack moves (source == destination); these cannot produce inter-stack conflicts and skip the check entirely. - Cross-stack moves where source and destination are truly independent (disjoint file changes). - Moves where the source stack has pre-existing conflicts unrelated to the commit being moved. --- .../gitbutler-branch-actions/src/actions.rs | 8 - .../src/move_branch.rs | 242 +++++++---- .../src/move_commits.rs | 215 ++++++++-- .../branch-actions/virtual_branches/mod.rs | 66 +++ .../virtual_branches/move_branch.rs | 405 ++++++++++++++++++ .../move_commit_to_vbranch.rs | 320 ++++++++++++++ 6 files changed, 1108 insertions(+), 148 deletions(-) create mode 100644 crates/gitbutler-branch-actions/tests/branch-actions/virtual_branches/move_branch.rs diff --git a/crates/gitbutler-branch-actions/src/actions.rs b/crates/gitbutler-branch-actions/src/actions.rs index ee4743f407f..9b2bd22b3c9 100644 --- a/crates/gitbutler-branch-actions/src/actions.rs +++ b/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, diff --git a/crates/gitbutler-branch-actions/src/move_branch.rs b/crates/gitbutler-branch-actions/src/move_branch.rs index beef3ebbfe3..08918e77327 100644 --- a/crates/gitbutler-branch-actions/src/move_branch.rs +++ b/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 { + 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,9 +181,10 @@ pub(crate) fn tear_off_branch( ) -> Result { 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( @@ -98,7 +192,6 @@ pub(crate) fn tear_off_branch( 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, - subject_branch_pr_number: Option, -) -> 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, Vec), 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, ) -> Result> { 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(); diff --git a/crates/gitbutler-branch-actions/src/move_commits.rs b/crates/gitbutler-branch-actions/src/move_commits.rs index daca88e697e..26a4afd0b21 100644 --- a/crates/gitbutler-branch-actions/src/move_commits.rs +++ b/crates/gitbutler-branch-actions/src/move_commits.rs @@ -2,7 +2,11 @@ use anyhow::{Context as _, Result, anyhow, bail}; use but_ctx::{Context, access::RepoExclusive}; use but_rebase::RebaseStep; use but_workspace::legacy::stack_ext::StackExt; -use gitbutler_stack::{StackId, VirtualBranchesHandle}; +use gitbutler_oplog::{ + OplogExt, + entry::{OperationKind, SnapshotDetails}, +}; +use gitbutler_stack::StackId; use gitbutler_workspace::branch_trees::{WorkspaceState, update_uncommitted_changes}; use serde::Serialize; @@ -12,16 +16,14 @@ use crate::VirtualBranchesExt; /// /// commit will end up at the top of the destination stack pub(crate) fn move_commit( - ctx: &Context, + ctx: &mut Context, target_stack_id: StackId, subject_commit_oid: gix::ObjectId, perm: &mut RepoExclusive, source_stack_id: StackId, ) -> Result> { let old_workspace = WorkspaceState::create(ctx, perm.read_permission())?; - let mut vb_state = ctx.virtual_branches(); - let repo = ctx.repo.get()?; - + let vb_state = ctx.virtual_branches(); let applied_stacks = vb_state .list_stacks_in_workspace() .context("failed to read virtual branches")?; @@ -30,7 +32,7 @@ pub(crate) fn move_commit( bail!("Destination branch not found"); } - let mut source_stack = vb_state + let source_stack = vb_state .try_stack(source_stack_id)? .ok_or(anyhow!("Source stack not found"))?; @@ -38,17 +40,89 @@ pub(crate) fn move_commit( .try_stack(target_stack_id)? .ok_or(anyhow!("Destination branch not found"))?; - repo.find_commit(subject_commit_oid) - .with_context(|| format!("commit {subject_commit_oid} to be moved could not be found"))?; - - take_commit_from_source_stack(ctx, &mut source_stack, subject_commit_oid)?; - - move_commit_to_destination_stack(&mut vb_state, ctx, destination_stack, subject_commit_oid)?; + if source_stack_id == target_stack_id { + // Intra-stack move: no cross-stack conflicts are possible, so just + // snapshot and apply sequentially. + let _ = ctx.create_snapshot(SnapshotDetails::new(OperationKind::MoveCommit), perm); + let mut source_stack = source_stack; + take_commit_from_source_stack(ctx, &mut source_stack, subject_commit_oid)?; + move_commit_to_destination_stack(ctx, destination_stack, subject_commit_oid)?; + } else { + // Cross-stack move: compute both rebases (ODB-only writes), check for + // conflicts, then snapshot, then apply state changes. + let source_merge_base = source_stack.merge_base(ctx)?; + let dest_merge_base = destination_stack.merge_base(ctx)?; + + let (source_output, dest_output) = { + let repo = ctx.repo.get()?; + repo.find_commit(subject_commit_oid).with_context(|| { + format!("commit {subject_commit_oid} to be moved could not be found") + })?; + + // Source: rebase remaining commits without the moved one. + let source_output = rebase_without_commit( + ctx, + &repo, + &source_stack, + subject_commit_oid, + source_merge_base, + )?; + + // Dest: rebase dest stack with the moved commit inserted at the top. + let dest_output = rebase_with_commit_at_top( + ctx, + &repo, + &destination_stack, + subject_commit_oid, + dest_merge_base, + )?; + + // Conflict check — bail before any state is written. + bail_on_new_conflicts( + &repo, + &source_output, + "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 commit does not apply cleanly at the target location.", + )?; + + (source_output, dest_output) + }; + + // Snapshot after the conflict check, but before any state writes. + let _ = ctx.create_snapshot(SnapshotDetails::new(OperationKind::MoveCommit), perm); + + // Apply source changes. + { + let repo = ctx.repo.get()?; + let mut vb_state = ctx.virtual_branches(); + let mut source_stack = vb_state + .try_stack(source_stack_id)? + .ok_or(anyhow!("Source stack not found"))?; + source_stack.set_heads_from_rebase_output(ctx, source_output.references)?; + source_stack.set_stack_head(&mut vb_state, &repo, source_output.top_commit)?; + } + + // Apply dest changes. + { + let repo = ctx.repo.get()?; + let mut vb_state = ctx.virtual_branches(); + let mut destination_stack = vb_state + .try_stack(target_stack_id)? + .ok_or(anyhow!("Destination branch not found"))?; + destination_stack.set_heads_from_rebase_output(ctx, dest_output.references)?; + destination_stack.set_stack_head(&mut vb_state, &repo, dest_output.top_commit)?; + } + } let new_workspace = WorkspaceState::create(ctx, perm.read_permission())?; - // Even if this fails, it's not actionable 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(None) @@ -65,6 +139,56 @@ pub enum MoveCommitIllegalAction { HasDependentUncommittedChanges, } +/// Rebase `stack` with `subject_commit_oid` removed, writing only to the ODB. +fn rebase_without_commit( + ctx: &Context, + repo: &gix::Repository, + stack: &gitbutler_stack::Stack, + subject_commit_oid: gix::ObjectId, + merge_base: gix::ObjectId, +) -> Result { + let steps: Vec = stack + .as_rebase_steps(ctx)? + .into_iter() + .filter(|s| match s { + RebaseStep::Pick { commit_id, .. } => *commit_id != subject_commit_oid, + _ => true, + }) + .collect(); + let mut rebase = but_rebase::Rebase::new(repo, Some(merge_base), None)?; + rebase.rebase_noops(false); + rebase.steps(steps)?; + rebase.rebase(&*ctx.cache.get_cache()?) +} + +/// Rebase `stack` with `subject_commit_oid` inserted at the top, writing only to the ODB. +/// +/// TODO: In the future we can make the API provide additional info for exactly +/// where to place the commit on the destination stack. +fn rebase_with_commit_at_top( + ctx: &Context, + repo: &gix::Repository, + stack: &gitbutler_stack::Stack, + subject_commit_oid: gix::ObjectId, + merge_base: gix::ObjectId, +) -> Result { + let mut steps = stack.as_rebase_steps(ctx)?; + if steps.is_empty() { + anyhow::bail!("destination stack has no branches to insert into"); + } + steps.insert( + steps.len() - 1, + RebaseStep::Pick { + commit_id: subject_commit_oid, + new_message: None, + }, + ); + let mut rebase = but_rebase::Rebase::new(repo, Some(merge_base), None)?; + rebase.rebase_noops(false); + rebase.steps(steps)?; + rebase.rebase(&*ctx.cache.get_cache()?) +} + /// Remove the commit from the source stack. /// /// Will fail if the commit is not in the source stack or if has dependent changes. @@ -75,22 +199,7 @@ fn take_commit_from_source_stack( ) -> Result, anyhow::Error> { let merge_base = source_stack.merge_base(ctx)?; let repo = ctx.repo.get()?; - let steps = source_stack - .as_rebase_steps(ctx)? - .into_iter() - .filter(|s| match s { - RebaseStep::Pick { - commit_id, - new_message: _, - } => commit_id != &subject_commit_id, - _ => true, - }) - .collect::>(); - let mut rebase = but_rebase::Rebase::new(&repo, Some(merge_base), None)?; - rebase.rebase_noops(false); - rebase.steps(steps)?; - let output = rebase.rebase(&*ctx.cache.get_cache()?)?; - + let output = rebase_without_commit(ctx, &repo, source_stack, subject_commit_id, merge_base)?; source_stack.set_heads_from_rebase_output(ctx, output.references)?; let mut vb_state = ctx.virtual_branches(); source_stack.set_stack_head(&mut vb_state, &repo, output.top_commit)?; @@ -99,28 +208,42 @@ fn take_commit_from_source_stack( /// Move the commit to the destination stack. fn move_commit_to_destination_stack( - vb_state: &mut VirtualBranchesHandle, ctx: &Context, mut destination_stack: gitbutler_stack::Stack, commit_id: gix::ObjectId, ) -> Result<(), anyhow::Error> { let repo = ctx.repo.get()?; let merge_base = destination_stack.merge_base(ctx)?; - let mut steps = destination_stack.as_rebase_steps(ctx)?; - // TODO: In the future we can make the API provide additional info for exactly where to place the commit on the destination stack - steps.insert( - steps.len() - 1, - RebaseStep::Pick { - commit_id, - new_message: None, - }, - ); - let mut rebase = but_rebase::Rebase::new(&repo, Some(merge_base), None)?; - rebase.rebase_noops(false); - rebase.steps(steps)?; - let output = rebase.rebase(&*ctx.cache.get_cache()?)?; - + let output = rebase_with_commit_at_top(ctx, &repo, &destination_stack, commit_id, merge_base)?; destination_stack.set_heads_from_rebase_output(ctx, output.references)?; - destination_stack.set_stack_head(vb_state, &repo, output.top_commit)?; + destination_stack.set_stack_head(&mut ctx.virtual_branches(), &repo, output.top_commit)?; + Ok(()) +} + +/// Check a rebase output for commits that became conflicted as a result of +/// the rebase (excluding commits that were already conflicted beforehand). +/// +/// This is used to validate rebase outputs computed during the move operations +/// before any state is written to disk. +pub(crate) fn bail_on_new_conflicts( + repo: &gix::Repository, + output: &but_rebase::RebaseOutput, + error_message: &str, +) -> Result<()> { + use gix::prelude::ObjectIdExt as _; + for (_, old, new) in &output.commit_mapping { + let was_conflicted = but_core::Commit::from_id(old.attach(repo)) + .with_context(|| format!("failed to read original commit {old}"))? + .is_conflicted(); + if was_conflicted { + continue; + } + if but_core::Commit::from_id(new.attach(repo)) + .with_context(|| format!("failed to read rebased commit {new}"))? + .is_conflicted() + { + bail!("{error_message}"); + } + } Ok(()) } diff --git a/crates/gitbutler-branch-actions/tests/branch-actions/virtual_branches/mod.rs b/crates/gitbutler-branch-actions/tests/branch-actions/virtual_branches/mod.rs index 43b1db63aee..599d2f727f1 100644 --- a/crates/gitbutler-branch-actions/tests/branch-actions/virtual_branches/mod.rs +++ b/crates/gitbutler-branch-actions/tests/branch-actions/virtual_branches/mod.rs @@ -73,6 +73,7 @@ mod create_virtual_branch_from_branch; mod init; mod list; mod list_details; +mod move_branch; mod move_commit_to_vbranch; mod oplog; mod save_and_unapply_virtual_branch; @@ -82,6 +83,71 @@ mod undo_commit; mod update_commit_message; mod workspace_migration; +/// Create a raw git commit parented to `parent` that sets `filename` to `content`, +/// bypassing GitButler's stack API. Used in tests to construct competitor commits +/// or to inject a known tree state without going through the worktree. +pub fn make_commit_on_file( + repo: &gix::Repository, + parent: gix::ObjectId, + filename: &str, + content: &[u8], +) -> anyhow::Result { + let blob_id = repo.write_blob(content)?.detach(); + let parent_commit = repo.find_commit(parent)?; + let mut editor = parent_commit.tree()?.edit()?; + editor.upsert(filename, gix::object::tree::EntryKind::Blob, blob_id)?; + let tree_id = editor.write()?.detach(); + Ok(repo + .write_object(gix::objs::Commit { + tree: tree_id, + parents: [parent].into(), + message: "raw test commit".into(), + ..parent_commit.decode()?.to_owned()? + })? + .detach()) +} + +/// Cherry-pick `competing_oid` (which shares an ancestor with `onto_oid` and modifies the +/// same content) onto `onto_oid` to produce a conflicted commit, then update the source +/// stack head to that conflicted commit. Returns the conflicted commit's OID. +/// +/// The conflicted commit is parented to `onto_oid`, so after this call the source stack +/// history is: merge_base → onto_oid → conflicted. +pub fn push_conflicted_commit_onto( + ctx: &but_ctx::Context, + stack_id: gitbutler_stack::StackId, + onto_oid: gix::ObjectId, + competing_oid: gix::ObjectId, +) -> anyhow::Result { + let (_guard, repo, ws, _db) = ctx.workspace_and_db()?; + let conflicted_oid = but_rebase::cherry_pick_one( + &repo, + onto_oid, + competing_oid, + but_rebase::cherry_pick::PickMode::Unconditionally, + but_rebase::cherry_pick::EmptyCommit::Keep, + )?; + assert!( + but_core::Commit::from_id(repo.find_commit(conflicted_oid)?.id())?.is_conflicted(), + "cherry_pick_one must have produced a conflicted commit for the test to be meaningful" + ); + let ref_name = ws + .stacks + .iter() + .find(|s| s.id == Some(stack_id)) + .ok_or_else(|| anyhow::anyhow!("stack not found in workspace"))? + .ref_name() + .ok_or_else(|| anyhow::anyhow!("stack has no ref name"))? + .to_owned(); + repo.reference( + ref_name.as_ref(), + conflicted_oid, + gix::refs::transaction::PreviousValue::Any, + "test: push conflicted commit", + )?; + Ok(conflicted_oid) +} + pub fn list_commit_files( ctx: &Context, commit_id: gix::ObjectId, diff --git a/crates/gitbutler-branch-actions/tests/branch-actions/virtual_branches/move_branch.rs b/crates/gitbutler-branch-actions/tests/branch-actions/virtual_branches/move_branch.rs new file mode 100644 index 00000000000..abb22e6ab58 --- /dev/null +++ b/crates/gitbutler-branch-actions/tests/branch-actions/virtual_branches/move_branch.rs @@ -0,0 +1,405 @@ +use bstr::ByteSlice; +use but_testsupport::legacy::stack_details; +use gitbutler_branch::BranchCreateRequest; + +use super::{Test, create_commit}; + +/// Move a branch that has no dependencies on commits remaining in the source +/// stack. The source stack is left empty and deleted; the destination gains +/// the branch. No conflicts should arise. +#[test] +fn move_branch_non_overlapping() { + let Test { repo, ctx, .. } = &mut Test::default(); + + std::fs::write(repo.path().join("base.txt"), "base\n").unwrap(); + repo.commit_all("M"); + repo.push(); + + let mut guard = ctx.exclusive_worktree_access(); + gitbutler_branch_actions::set_base_branch( + ctx, + &"refs/remotes/origin/master".parse().unwrap(), + guard.write_permission(), + ) + .unwrap(); + drop(guard); + + // Source stack: one branch, edits file-a. + std::fs::write(repo.path().join("file-a.txt"), "source content\n").unwrap(); + let mut guard = ctx.exclusive_worktree_access(); + let source_entry = gitbutler_branch_actions::create_virtual_branch( + ctx, + &BranchCreateRequest { + name: Some("source-stack".into()), + ..Default::default() + }, + guard.write_permission(), + ) + .unwrap(); + drop(guard); + let source_id = source_entry.id; + create_commit(ctx, source_id, "source: add file-a").unwrap(); + + let details = stack_details(ctx); + let (_, source_sd) = details.iter().find(|(id, _)| *id == source_id).unwrap(); + let source_branch_name = source_sd.branch_details[0].name.to_str_lossy().to_string(); + + // Destination stack: one branch, edits file-b. + std::fs::write(repo.path().join("file-b.txt"), "dest content\n").unwrap(); + let mut guard = ctx.exclusive_worktree_access(); + let dest_entry = gitbutler_branch_actions::create_virtual_branch( + ctx, + &BranchCreateRequest { + name: Some("dest-stack".into()), + ..Default::default() + }, + guard.write_permission(), + ) + .unwrap(); + drop(guard); + let dest_id = dest_entry.id; + create_commit(ctx, dest_id, "dest: add file-b").unwrap(); + + let details = stack_details(ctx); + let (_, dest_sd) = details.iter().find(|(id, _)| *id == dest_id).unwrap(); + let dest_branch_name = dest_sd.branch_details[0].name.to_str_lossy().to_string(); + + let result = gitbutler_branch_actions::move_branch( + ctx, + dest_id, + &dest_branch_name, + source_id, + &source_branch_name, + ); + assert!(result.is_ok(), "move_branch should succeed: {result:?}"); + + let details = stack_details(ctx); + + // Source stack was the only branch — it should be deleted. + let result = result.unwrap(); + assert!( + result.deleted_stacks.contains(&source_id), + "source stack should be deleted after its only branch was moved" + ); + + // Destination should now have 2 branches, no conflicts. + let (_, dest_sd) = details + .iter() + .find(|(id, _)| *id == dest_id) + .expect("dest stack should exist"); + assert_eq!( + dest_sd.branch_details.len(), + 2, + "dest should have 2 branches" + ); + for bd in &dest_sd.branch_details { + assert!( + bd.commits.iter().all(|c| !c.has_conflicts), + "no conflicts expected" + ); + } +} + +/// Moving a branch whose commits are depended upon by remaining branches +/// in the source stack must be rejected: the remaining branches would conflict +/// when rebased without the moved branch. +#[test] +fn move_branch_with_dependency_rejected() { + let Test { repo, ctx, .. } = &mut Test::default(); + + std::fs::write(repo.path().join("base.txt"), "base\n").unwrap(); + repo.commit_all("M"); + repo.push(); + + let mut guard = ctx.exclusive_worktree_access(); + gitbutler_branch_actions::set_base_branch( + ctx, + &"refs/remotes/origin/master".parse().unwrap(), + guard.write_permission(), + ) + .unwrap(); + drop(guard); + + // Source stack: bottom branch creates shared.txt; top branch modifies it. + std::fs::write(repo.path().join("shared.txt"), "alpha\nbravo\ncharlie\n").unwrap(); + let mut guard = ctx.exclusive_worktree_access(); + let source_entry = gitbutler_branch_actions::create_virtual_branch( + ctx, + &BranchCreateRequest { + name: Some("source-stack".into()), + ..Default::default() + }, + guard.write_permission(), + ) + .unwrap(); + drop(guard); + let source_id = source_entry.id; + create_commit(ctx, source_id, "bottom: create shared.txt").unwrap(); + + let details = stack_details(ctx); + let (_, source_sd) = details.iter().find(|(id, _)| *id == source_id).unwrap(); + let bottom_branch_name = source_sd.branch_details[0].name.to_str_lossy().to_string(); + + gitbutler_branch_actions::stack::create_branch( + ctx, + source_id, + gitbutler_branch_actions::stack::CreateSeriesRequest { + name: "source-top".to_string(), + target_patch: None, + preceding_head: None, + }, + ) + .unwrap(); + + std::fs::write( + repo.path().join("shared.txt"), + "alpha\nbravo_modified\ncharlie\n", + ) + .unwrap(); + let mut guard = ctx.exclusive_worktree_access(); + but_workspace::legacy::commit_engine::create_commit_simple( + ctx, + source_id, + None, + vec![but_core::DiffSpec { + previous_path: None, + path: "shared.txt".into(), + hunk_headers: vec![], + }], + "top: modify shared.txt".to_string(), + "source-top".to_string(), + guard.write_permission(), + ) + .unwrap(); + drop(guard); + + // Destination stack: empty. + let mut guard = ctx.exclusive_worktree_access(); + let dest_entry = gitbutler_branch_actions::create_virtual_branch( + ctx, + &BranchCreateRequest { + name: Some("dest-stack".into()), + ..Default::default() + }, + guard.write_permission(), + ) + .unwrap(); + drop(guard); + let dest_id = dest_entry.id; + + let details = stack_details(ctx); + let (_, dest_sd) = details.iter().find(|(id, _)| *id == dest_id).unwrap(); + let dest_branch_name = dest_sd.branch_details[0].name.to_str_lossy().to_string(); + + // Moving the bottom branch must fail — the top branch depends on it + // and would conflict when rebased without it. + let result = gitbutler_branch_actions::move_branch( + ctx, + dest_id, + &dest_branch_name, + source_id, + &bottom_branch_name, + ); + assert!( + result.is_err(), + "move_branch should be rejected: remaining top branch depends on the moved bottom branch" + ); +} + +// --------------------------------------------------------------------------- +// Cross-stack branch moves: destination conflict is rejected +// --------------------------------------------------------------------------- + +/// Moving a branch onto a destination that has already modified the same content +/// must be rejected: the branch commits cannot apply cleanly at the insertion point. +#[test] +fn move_branch_destination_conflict() { + let Test { repo, ctx, .. } = &mut Test::default(); + + std::fs::write(repo.path().join("shared.txt"), "original\n").unwrap(); + repo.commit_all("M"); + repo.push(); + + let mut guard = ctx.exclusive_worktree_access(); + gitbutler_branch_actions::set_base_branch( + ctx, + &"refs/remotes/origin/master".parse().unwrap(), + guard.write_permission(), + ) + .unwrap(); + drop(guard); + + // Source stack: one branch that changes shared.txt to "source". + std::fs::write(repo.path().join("shared.txt"), "source\n").unwrap(); + let mut guard = ctx.exclusive_worktree_access(); + let source_entry = gitbutler_branch_actions::create_virtual_branch( + ctx, + &BranchCreateRequest { + name: Some("source-stack".into()), + ..Default::default() + }, + guard.write_permission(), + ) + .unwrap(); + drop(guard); + let source_id = source_entry.id; + create_commit(ctx, source_id, "source: change shared.txt").unwrap(); + + let details = stack_details(ctx); + let (_, source_sd) = details.iter().find(|(id, _)| *id == source_id).unwrap(); + let source_branch_name = source_sd.branch_details[0].name.to_str_lossy().to_string(); + + // Destination stack: inject a raw commit that also changes shared.txt to "dest". + let mut guard = ctx.exclusive_worktree_access(); + let dest_entry = gitbutler_branch_actions::create_virtual_branch( + ctx, + &BranchCreateRequest { + name: Some("dest-stack".into()), + ..Default::default() + }, + guard.write_permission(), + ) + .unwrap(); + drop(guard); + let dest_id = dest_entry.id; + + let dest_branch_name = { + let details = stack_details(ctx); + let (_, dest_sd) = details.iter().find(|(id, _)| *id == dest_id).unwrap(); + dest_sd.branch_details[0].name.to_str_lossy().to_string() + }; + { + let (_guard, repo, ws, _db) = ctx.workspace_and_db().unwrap(); + let merge_base = ws.lower_bound.unwrap(); + let dest_commit = + super::make_commit_on_file(&repo, merge_base, "shared.txt", b"dest\n").unwrap(); + let ref_name = ws + .stacks + .iter() + .find(|s| s.id == Some(dest_id)) + .unwrap() + .ref_name() + .unwrap() + .to_owned(); + repo.reference( + ref_name.as_ref(), + dest_commit, + gix::refs::transaction::PreviousValue::Any, + "test: set dest commit", + ) + .unwrap(); + } + + // Moving must fail: the branch's commit conflicts at the destination insertion point. + let result = gitbutler_branch_actions::move_branch( + ctx, + dest_id, + &dest_branch_name, + source_id, + &source_branch_name, + ); + assert!( + result.is_err(), + "move_branch should be rejected: branch commits conflict at the destination" + ); +} + +// --------------------------------------------------------------------------- +// Cross-stack branch moves: pre-existing conflict in source does not block +// --------------------------------------------------------------------------- + +/// A source stack whose top branch already has a conflicted commit must not block +/// moving the bottom branch out. Only *new* conflicts introduced by the move matter. +#[test] +fn move_branch_preexisting_conflict() { + let Test { repo, ctx, .. } = &mut Test::default(); + + std::fs::write(repo.path().join("shared.txt"), "original\n").unwrap(); + repo.commit_all("M"); + repo.push(); + + let mut guard = ctx.exclusive_worktree_access(); + gitbutler_branch_actions::set_base_branch( + ctx, + &"refs/remotes/origin/master".parse().unwrap(), + guard.write_permission(), + ) + .unwrap(); + drop(guard); + + // Source stack: bottom branch changes shared.txt to "source" (this is what we'll move). + std::fs::write(repo.path().join("shared.txt"), "source\n").unwrap(); + let mut guard = ctx.exclusive_worktree_access(); + let source_entry = gitbutler_branch_actions::create_virtual_branch( + ctx, + &BranchCreateRequest { + name: Some("source-stack".into()), + ..Default::default() + }, + guard.write_permission(), + ) + .unwrap(); + drop(guard); + let source_id = source_entry.id; + let commit_to_move = create_commit(ctx, source_id, "bottom: change shared.txt").unwrap(); + + let details = stack_details(ctx); + let (_, source_sd) = details.iter().find(|(id, _)| *id == source_id).unwrap(); + let bottom_branch_name = source_sd.branch_details[0].name.to_str_lossy().to_string(); + + // Add a top branch to the source stack, then push a pre-existing conflicted commit onto it. + gitbutler_branch_actions::stack::create_branch( + ctx, + source_id, + gitbutler_branch_actions::stack::CreateSeriesRequest { + name: "source-top".to_string(), + target_patch: None, + preceding_head: None, + }, + ) + .unwrap(); + { + let (_guard, repo, ws, _db) = ctx.workspace_and_db().unwrap(); + let merge_base = ws.lower_bound.unwrap(); + // Competitor: from merge_base, also changes shared.txt differently. + // Cherry-picking it onto commit_to_move produces a 3-way conflict. + let competitor = + super::make_commit_on_file(&repo, merge_base, "shared.txt", b"competitor\n").unwrap(); + drop(_db); + drop(ws); + drop(_guard); + super::push_conflicted_commit_onto(ctx, source_id, commit_to_move, competitor).unwrap(); + } + + // Destination stack: empty. + let mut guard = ctx.exclusive_worktree_access(); + let dest_entry = gitbutler_branch_actions::create_virtual_branch( + ctx, + &BranchCreateRequest { + name: Some("dest-stack".into()), + ..Default::default() + }, + guard.write_permission(), + ) + .unwrap(); + drop(guard); + let dest_id = dest_entry.id; + + let details = stack_details(ctx); + let (_, dest_sd) = details.iter().find(|(id, _)| *id == dest_id).unwrap(); + let dest_branch_name = dest_sd.branch_details[0].name.to_str_lossy().to_string(); + + // Moving the bottom branch must succeed: the top branch's conflict was pre-existing + // and must not be counted as a new conflict caused by this move. + let result = gitbutler_branch_actions::move_branch( + ctx, + dest_id, + &dest_branch_name, + source_id, + &bottom_branch_name, + ); + assert!( + result.is_ok(), + "move_branch should succeed: top branch's conflict is pre-existing, not caused by this move: {result:?}" + ); +} diff --git a/crates/gitbutler-branch-actions/tests/branch-actions/virtual_branches/move_commit_to_vbranch.rs b/crates/gitbutler-branch-actions/tests/branch-actions/virtual_branches/move_commit_to_vbranch.rs index 058c03b19c8..e19cc2cff98 100644 --- a/crates/gitbutler-branch-actions/tests/branch-actions/virtual_branches/move_commit_to_vbranch.rs +++ b/crates/gitbutler-branch-actions/tests/branch-actions/virtual_branches/move_commit_to_vbranch.rs @@ -602,3 +602,323 @@ fn no_branch() { "Destination branch not found" ); } + +// --------------------------------------------------------------------------- +// Cross-stack moves: non-overlapping changes +// --------------------------------------------------------------------------- + +/// Move a commit between two stacks that edit different files. +/// No conflicts should arise. +#[test] +fn move_commit_non_overlapping() { + let Test { repo, ctx, .. } = &mut Test::default(); + + std::fs::write(repo.path().join("base.txt"), "base\n").unwrap(); + repo.commit_all("M"); + repo.push(); + + let mut guard = ctx.exclusive_worktree_access(); + gitbutler_branch_actions::set_base_branch( + ctx, + &"refs/remotes/origin/master".parse().unwrap(), + guard.write_permission(), + ) + .unwrap(); + drop(guard); + + // Source stack: commit edits file-a. + std::fs::write(repo.path().join("file-a.txt"), "source content\n").unwrap(); + + let mut guard = ctx.exclusive_worktree_access(); + let source_entry = gitbutler_branch_actions::create_virtual_branch( + ctx, + &BranchCreateRequest { + name: Some("source-stack".into()), + ..Default::default() + }, + guard.write_permission(), + ) + .unwrap(); + drop(guard); + + let source_id = source_entry.id; + let commit_oid = super::create_commit(ctx, source_id, "source: add file-a").unwrap(); + + // Destination stack: commit edits file-b (non-overlapping). + std::fs::write(repo.path().join("file-b.txt"), "dest content\n").unwrap(); + + let mut guard = ctx.exclusive_worktree_access(); + let dest_entry = gitbutler_branch_actions::create_virtual_branch( + ctx, + &BranchCreateRequest { + name: Some("dest-stack".into()), + ..Default::default() + }, + guard.write_permission(), + ) + .unwrap(); + drop(guard); + + let dest_id = dest_entry.id; + super::create_commit(ctx, dest_id, "dest: add file-b").unwrap(); + + // Move the commit from source to dest. + gitbutler_branch_actions::move_commit(ctx, dest_id, commit_oid, source_id).unwrap(); + + let details = stack_details(ctx); + + let source = details.iter().find(|(id, _)| *id == source_id).unwrap(); + let dest = details.iter().find(|(id, _)| *id == dest_id).unwrap(); + + assert_eq!( + source.1.branch_details[0].commits.len(), + 0, + "source should have no commits after move" + ); + assert_eq!( + dest.1.branch_details[0].commits.len(), + 2, + "dest should have original + moved commit" + ); + + // No conflicts expected. + assert!( + dest.1.branch_details[0] + .commits + .iter() + .all(|c| !c.has_conflicts), + "no conflicts expected for non-overlapping changes" + ); +} + +// --------------------------------------------------------------------------- +// Cross-stack moves: dependent commit is rejected +// --------------------------------------------------------------------------- + +/// A commit that modifies a file created by an earlier commit in the source +/// stack cannot be moved: it depends on context that stays in the source, so +/// the cherry-pick onto the destination would conflict. +#[test] +fn move_commit_with_dependency_rejected() { + let Test { repo, ctx, .. } = &mut Test::default(); + + std::fs::write(repo.path().join("base.txt"), "base\n").unwrap(); + repo.commit_all("M"); + repo.push(); + + let mut guard = ctx.exclusive_worktree_access(); + gitbutler_branch_actions::set_base_branch( + ctx, + &"refs/remotes/origin/master".parse().unwrap(), + guard.write_permission(), + ) + .unwrap(); + drop(guard); + + // Source stack: first commit creates shared.txt, second modifies it. + std::fs::write(repo.path().join("shared.txt"), "alpha\nbravo\ncharlie\n").unwrap(); + + let mut guard = ctx.exclusive_worktree_access(); + let source_entry = gitbutler_branch_actions::create_virtual_branch( + ctx, + &BranchCreateRequest { + name: Some("source-stack".into()), + ..Default::default() + }, + guard.write_permission(), + ) + .unwrap(); + drop(guard); + + let source_id = source_entry.id; + let first_commit = super::create_commit(ctx, source_id, "source: create shared.txt").unwrap(); + + std::fs::write( + repo.path().join("shared.txt"), + "alpha\nbravo_modified\ncharlie\n", + ) + .unwrap(); + super::create_commit(ctx, source_id, "source: modify shared.txt").unwrap(); + + // Destination stack: empty. + let mut guard = ctx.exclusive_worktree_access(); + let dest_entry = gitbutler_branch_actions::create_virtual_branch( + ctx, + &BranchCreateRequest { + name: Some("dest-stack".into()), + ..Default::default() + }, + guard.write_permission(), + ) + .unwrap(); + drop(guard); + + let dest_id = dest_entry.id; + + // Moving the first commit must fail — the second commit depends on it + // and would conflict when rebased without it. + assert!( + gitbutler_branch_actions::move_commit(ctx, dest_id, first_commit, source_id).is_err(), + "move_commit should be rejected: remaining source commits depend on the moved commit" + ); +} + +// --------------------------------------------------------------------------- +// Cross-stack moves: destination conflict is rejected +// --------------------------------------------------------------------------- + +/// Moving a commit onto a destination stack that has already modified the same +/// content must be rejected: the commit cannot apply cleanly at the insertion point. +#[test] +fn move_commit_destination_conflict() { + let Test { repo, ctx, .. } = &mut Test::default(); + + std::fs::write(repo.path().join("shared.txt"), "original\n").unwrap(); + repo.commit_all("M"); + repo.push(); + + let mut guard = ctx.exclusive_worktree_access(); + gitbutler_branch_actions::set_base_branch( + ctx, + &"refs/remotes/origin/master".parse().unwrap(), + guard.write_permission(), + ) + .unwrap(); + drop(guard); + + // Source stack: changes shared.txt to "source". + std::fs::write(repo.path().join("shared.txt"), "source\n").unwrap(); + let mut guard = ctx.exclusive_worktree_access(); + let source_entry = gitbutler_branch_actions::create_virtual_branch( + ctx, + &BranchCreateRequest { + name: Some("source-stack".into()), + ..Default::default() + }, + guard.write_permission(), + ) + .unwrap(); + drop(guard); + let source_id = source_entry.id; + let commit_to_move = super::create_commit(ctx, source_id, "source: change shared.txt").unwrap(); + + // Destination stack: inject a raw commit that also changes shared.txt to "dest", + // simulating content that overlaps with the commit being moved. + let mut guard = ctx.exclusive_worktree_access(); + let dest_entry = gitbutler_branch_actions::create_virtual_branch( + ctx, + &BranchCreateRequest { + name: Some("dest-stack".into()), + ..Default::default() + }, + guard.write_permission(), + ) + .unwrap(); + drop(guard); + let dest_id = dest_entry.id; + + { + let (_guard, repo, ws, _db) = ctx.workspace_and_db().unwrap(); + let merge_base = ws.lower_bound.unwrap(); + let dest_commit = + super::make_commit_on_file(&repo, merge_base, "shared.txt", b"dest\n").unwrap(); + let ref_name = ws + .stacks + .iter() + .find(|s| s.id == Some(dest_id)) + .unwrap() + .ref_name() + .unwrap() + .to_owned(); + repo.reference( + ref_name.as_ref(), + dest_commit, + gix::refs::transaction::PreviousValue::Any, + "test: set dest commit", + ) + .unwrap(); + } + + // Moving must fail: shared.txt was changed by both stacks, so the commit + // cannot apply cleanly at the destination insertion point. + assert!( + gitbutler_branch_actions::move_commit(ctx, dest_id, commit_to_move, source_id).is_err(), + "move_commit should be rejected: moved commit conflicts at the destination" + ); +} + +// --------------------------------------------------------------------------- +// Cross-stack moves: pre-existing conflict in source does not block +// --------------------------------------------------------------------------- + +/// A source stack that already has a conflicted commit (unrelated to the commit +/// being moved) must not block the move. Only *new* conflicts introduced by the +/// move matter. +#[test] +fn move_commit_preexisting_conflict() { + let Test { repo, ctx, .. } = &mut Test::default(); + + std::fs::write(repo.path().join("shared.txt"), "original\n").unwrap(); + repo.commit_all("M"); + repo.push(); + + let mut guard = ctx.exclusive_worktree_access(); + gitbutler_branch_actions::set_base_branch( + ctx, + &"refs/remotes/origin/master".parse().unwrap(), + guard.write_permission(), + ) + .unwrap(); + drop(guard); + + // Source stack: commit_to_move changes shared.txt to "source". + std::fs::write(repo.path().join("shared.txt"), "source\n").unwrap(); + let mut guard = ctx.exclusive_worktree_access(); + let source_entry = gitbutler_branch_actions::create_virtual_branch( + ctx, + &BranchCreateRequest { + name: Some("source-stack".into()), + ..Default::default() + }, + guard.write_permission(), + ) + .unwrap(); + drop(guard); + let source_id = source_entry.id; + let commit_to_move = super::create_commit(ctx, source_id, "source: change shared.txt").unwrap(); + + // Create a "competitor" commit from merge_base that also changes shared.txt to "competitor". + // Cherry-picking it onto commit_to_move will produce a 3-way conflict (both modified + // shared.txt from "original" in different ways) — giving us a pre-existing conflicted commit. + { + let (_guard, repo, ws, _db) = ctx.workspace_and_db().unwrap(); + let merge_base = ws.lower_bound.unwrap(); + let competitor = + super::make_commit_on_file(&repo, merge_base, "shared.txt", b"competitor\n").unwrap(); + drop(_db); + drop(ws); + drop(_guard); + super::push_conflicted_commit_onto(ctx, source_id, commit_to_move, competitor).unwrap(); + } + + // Destination stack: empty. + let mut guard = ctx.exclusive_worktree_access(); + let dest_entry = gitbutler_branch_actions::create_virtual_branch( + ctx, + &BranchCreateRequest { + name: Some("dest-stack".into()), + ..Default::default() + }, + guard.write_permission(), + ) + .unwrap(); + drop(guard); + let dest_id = dest_entry.id; + + // Moving commit_to_move must succeed: the conflicted commit above it in the + // source stack was already conflicted before the move, so it must not block. + assert!( + gitbutler_branch_actions::move_commit(ctx, dest_id, commit_to_move, source_id).is_ok(), + "move_commit should succeed: the conflicted commit in source is pre-existing" + ); +}