Skip to content

Commit 4fb0999

Browse files
committed
Add pre-flight conflict detection for cross-stack commit and branch moves
When moving a commit or branch to a different stack, speculatively simulate the impact on both sides before applying any changes: Source check: rebase the remaining source commits without the moved commit(s). If any would conflict, the move is rejected — those commits depend on the changes being moved away. Destination check: rebase the moved commit(s) onto the destination insertion point. If any would conflict, the move is rejected — the moved changes do not apply cleanly in the target context. 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. The check is a pure read: no refs, no snapshot, no worktree state is modified on rejection. It is invoked before create_snapshot so a failure leaves the repository completely unchanged.
1 parent 222bcce commit 4fb0999

9 files changed

Lines changed: 943 additions & 10 deletions

File tree

crates/but-workspace/src/legacy/mod.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,7 @@ pub use stacks::{
1818
local_and_remote_commits, stack_branches, stack_details_v3, stack_heads_info, stacks_v3,
1919
};
2020
pub use tree_manipulation::{
21-
MoveChangesResult,
21+
MoveChangesResult, check_rebase_for_conflicts,
2222
move_between_commits::move_changes_between_commits,
2323
remove_changes_from_commit_in_stack::remove_changes_from_commit_in_stack,
2424
split_branch::{split_branch, split_into_dependent_branch},

crates/but-workspace/src/legacy/tree_manipulation/mod.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -38,3 +38,4 @@ pub(super) mod remove_changes_from_commit_in_stack;
3838
pub(super) mod split_branch;
3939
pub(super) mod split_commit;
4040
mod utils;
41+
pub use utils::check_rebase_for_conflicts;

crates/but-workspace/src/legacy/tree_manipulation/utils.rs

Lines changed: 67 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,78 @@
11
//! Utility types related to discarding changes in the worktree.
22
3-
use std::collections::HashMap;
3+
use std::collections::{HashMap, HashSet};
44

5+
use anyhow::{Result, bail};
6+
use but_ctx::Context;
57
use but_rebase::{RebaseOutput, RebaseStep};
8+
use gix::prelude::ObjectIdExt as _;
69

710
// Re-export from non-legacy location for backward compatibility
811
pub use crate::tree_manipulation::{ChangesSource, create_tree_without_diff};
912

13+
/// Speculatively rebase `steps` onto `onto` and bail with `error_message` if any
14+
/// Pick commit lands as conflicted.
15+
///
16+
/// This is the shared building block for cross-stack move pre-flight checks.
17+
/// It must be called before modifying any stack state so that a failure leaves
18+
/// the repository unchanged.
19+
pub fn check_rebase_for_conflicts(
20+
ctx: &Context,
21+
steps: Vec<RebaseStep>,
22+
onto: gix::ObjectId,
23+
error_message: &str,
24+
) -> Result<()> {
25+
let commit_ids: Vec<gix::ObjectId> = steps
26+
.iter()
27+
.filter_map(|s| {
28+
if let RebaseStep::Pick { commit_id, .. } = s {
29+
Some(*commit_id)
30+
} else {
31+
None
32+
}
33+
})
34+
.collect();
35+
if commit_ids.is_empty() {
36+
return Ok(());
37+
}
38+
let repo = ctx.repo.get()?;
39+
40+
// Commits already conflicted before our speculative rebase must not block
41+
// the move — they were conflicted for unrelated reasons.
42+
let already_conflicted: HashSet<gix::ObjectId> = commit_ids
43+
.iter()
44+
.filter(|&&id| {
45+
but_core::Commit::from_id(id.attach(&repo))
46+
.map(|c| c.is_conflicted())
47+
.unwrap_or(false)
48+
})
49+
.copied()
50+
.collect();
51+
52+
let mut rebase = but_rebase::Rebase::new(&repo, Some(onto), None)?;
53+
rebase.rebase_noops(false);
54+
rebase.steps(steps)?;
55+
let output = rebase.rebase(&*ctx.cache.get_cache()?)?;
56+
57+
let old_to_new: HashMap<gix::ObjectId, gix::ObjectId> = output
58+
.commit_mapping
59+
.iter()
60+
.map(|(_, old, new)| (*old, *new))
61+
.collect();
62+
63+
for old_id in commit_ids {
64+
if already_conflicted.contains(&old_id) {
65+
continue;
66+
}
67+
let new_id = old_to_new.get(&old_id).copied().unwrap_or(old_id);
68+
let commit = but_core::Commit::from_id(new_id.attach(&repo))?;
69+
if commit.is_conflicted() {
70+
bail!("{error_message}");
71+
}
72+
}
73+
Ok(())
74+
}
75+
1076
/// Takes a rebase output and returns the commit mapping with any extra
1177
/// mapping overrides provided.
1278
///

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

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -386,6 +386,7 @@ pub fn move_commit(
386386
ctx.verify(guard.write_permission())?;
387387
ensure_open_workspace_mode(ctx, guard.read_permission())
388388
.context("Moving a commit requires open workspace mode")?;
389+
move_commits::preflight_check(ctx, target_stack_id, commit_oid, source_stack_id)?;
389390
let _ = ctx.create_snapshot(
390391
SnapshotDetails::new(OperationKind::MoveCommit),
391392
guard.write_permission(),
@@ -410,6 +411,13 @@ pub fn move_branch(
410411
ctx.verify(guard.write_permission())?;
411412
ensure_open_workspace_mode(ctx, guard.read_permission())
412413
.context("Moving a branch requires open workspace mode")?;
414+
crate::move_branch::preflight_check(
415+
ctx,
416+
target_stack_id,
417+
target_branch_name,
418+
source_stack_id,
419+
subject_branch_name,
420+
)?;
413421
let _ = ctx.create_snapshot(
414422
SnapshotDetails::new(OperationKind::MoveBranch),
415423
guard.write_permission(),

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

Lines changed: 52 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@ use anyhow::{Context as _, Result};
22
use but_core::ref_metadata::StackId;
33
use but_ctx::{Context, access::RepoExclusive};
44
use but_rebase::{Rebase, RebaseStep};
5-
use but_workspace::legacy::stack_ext::StackExt;
5+
use but_workspace::legacy::{check_rebase_for_conflicts, stack_ext::StackExt};
66
use gitbutler_reference::{LocalRefname, Refname};
77
use gitbutler_stack::{StackBranch, VirtualBranchesHandle};
88
use gitbutler_workspace::branch_trees::{WorkspaceState, update_uncommitted_changes};
@@ -21,6 +21,57 @@ pub struct MoveBranchResult {
2121
pub unapplied_stacks: Vec<StackId>,
2222
}
2323

24+
/// Pre-flight check for cross-stack branch moves.
25+
///
26+
/// Checks both sides of the move:
27+
/// 1. **Source**: rebases remaining source commits without the moved branch —
28+
/// bails if any would conflict (they depend on the branch being moved away).
29+
/// 2. **Destination**: rebases the moved branch commits onto the target branch head —
30+
/// bails if any would conflict at the insertion point.
31+
///
32+
/// Must be called before `create_snapshot` so a rejection leaves no side-effects.
33+
pub(crate) fn preflight_check(
34+
ctx: &Context,
35+
target_stack_id: StackId,
36+
target_branch_name: &str,
37+
source_stack_id: StackId,
38+
subject_branch_name: &str,
39+
) -> Result<()> {
40+
if source_stack_id == target_stack_id {
41+
return Ok(());
42+
}
43+
let repo = ctx.repo.get()?;
44+
let vb_state = VirtualBranchesHandle::new(ctx.project_data_dir());
45+
let source_stack = vb_state.get_stack_in_workspace(source_stack_id)?;
46+
let (subject_branch_steps, remaining_steps) =
47+
extract_branch_steps(ctx, &repo, &source_stack, subject_branch_name)?;
48+
49+
// Source side: remaining commits must rebase cleanly without the moved branch.
50+
check_rebase_for_conflicts(
51+
ctx,
52+
remaining_steps,
53+
source_stack.merge_base(ctx)?,
54+
"This move would cause a conflict in the source stack: \
55+
other commits depend on the changes being moved.",
56+
)?;
57+
58+
// Destination side: the moved branch commits must apply cleanly at the insertion point.
59+
let destination_stack = vb_state.get_stack_in_workspace(target_stack_id)?;
60+
let target_branch_oid = destination_stack
61+
.branches()
62+
.into_iter()
63+
.find(|b| b.name == target_branch_name)
64+
.context("Target branch not found in destination stack")?
65+
.head_oid(&repo)?;
66+
check_rebase_for_conflicts(
67+
ctx,
68+
subject_branch_steps,
69+
target_branch_oid,
70+
"This move would cause a conflict in the destination stack: \
71+
the branch does not apply cleanly at the target location.",
72+
)
73+
}
74+
2475
pub(crate) fn move_branch(
2576
ctx: &Context,
2677
target_stack_id: StackId,

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

Lines changed: 60 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,13 +1,72 @@
11
use anyhow::{Context as _, Result, anyhow, bail};
22
use but_ctx::{Context, access::RepoExclusive};
33
use but_rebase::RebaseStep;
4-
use but_workspace::legacy::stack_ext::StackExt;
4+
use but_workspace::legacy::{check_rebase_for_conflicts, stack_ext::StackExt};
55
use gitbutler_stack::{StackId, VirtualBranchesHandle};
66
use gitbutler_workspace::branch_trees::{WorkspaceState, update_uncommitted_changes};
77
use serde::Serialize;
88

99
use crate::VirtualBranchesExt;
1010

11+
/// Pre-flight check for cross-stack commit moves.
12+
///
13+
/// Checks both sides of the move:
14+
/// 1. **Source**: rebases remaining source commits without the moved commit —
15+
/// bails if any would conflict (they depend on the commit being moved away).
16+
/// 2. **Destination**: rebases the moved commit onto the destination stack head —
17+
/// bails if it would conflict at the insertion point.
18+
///
19+
/// Must be called before `create_snapshot` so a rejection leaves no side-effects.
20+
pub(crate) fn preflight_check(
21+
ctx: &Context,
22+
target_stack_id: StackId,
23+
subject_commit_oid: gix::ObjectId,
24+
source_stack_id: StackId,
25+
) -> Result<()> {
26+
let repo = ctx.repo.get()?;
27+
repo.find_commit(subject_commit_oid)
28+
.with_context(|| format!("commit {subject_commit_oid} to be moved could not be found"))?;
29+
if source_stack_id == target_stack_id {
30+
return Ok(());
31+
}
32+
let vb_state = ctx.virtual_branches();
33+
34+
// Source side: remaining commits must rebase cleanly without the moved commit.
35+
let source_stack = vb_state
36+
.try_stack(source_stack_id)?
37+
.ok_or(anyhow!("Source stack not found"))?;
38+
let remaining_steps: Vec<RebaseStep> = source_stack
39+
.as_rebase_steps(ctx)?
40+
.into_iter()
41+
.filter(|s| match s {
42+
RebaseStep::Pick { commit_id, .. } => *commit_id != subject_commit_oid,
43+
_ => true,
44+
})
45+
.collect();
46+
check_rebase_for_conflicts(
47+
ctx,
48+
remaining_steps,
49+
source_stack.merge_base(ctx)?,
50+
"This move would cause a conflict in the source stack: \
51+
other commits depend on the changes being moved.",
52+
)?;
53+
54+
// Destination side: the moved commit must apply cleanly at the insertion point.
55+
let destination_stack = vb_state
56+
.try_stack(target_stack_id)?
57+
.ok_or(anyhow!("Destination branch not found"))?;
58+
check_rebase_for_conflicts(
59+
ctx,
60+
vec![RebaseStep::Pick {
61+
commit_id: subject_commit_oid,
62+
new_message: None,
63+
}],
64+
destination_stack.head_oid(ctx)?,
65+
"This move would cause a conflict in the destination stack: \
66+
the commit does not apply cleanly at the target location.",
67+
)
68+
}
69+
1170
/// move a commit from one stack to another
1271
///
1372
/// commit will end up at the top of the destination stack
@@ -20,8 +79,6 @@ pub(crate) fn move_commit(
2079
) -> Result<Option<MoveCommitIllegalAction>> {
2180
let old_workspace = WorkspaceState::create(ctx, perm.read_permission())?;
2281
let mut vb_state = ctx.virtual_branches();
23-
let repo = ctx.repo.get()?;
24-
2582
let applied_stacks = vb_state
2683
.list_stacks_in_workspace()
2784
.context("failed to read virtual branches")?;
@@ -38,11 +95,7 @@ pub(crate) fn move_commit(
3895
.try_stack(target_stack_id)?
3996
.ok_or(anyhow!("Destination branch not found"))?;
4097

41-
repo.find_commit(subject_commit_oid)
42-
.with_context(|| format!("commit {subject_commit_oid} to be moved could not be found"))?;
43-
4498
take_commit_from_source_stack(ctx, &mut source_stack, subject_commit_oid)?;
45-
4699
move_commit_to_destination_stack(&mut vb_state, ctx, destination_stack, subject_commit_oid)?;
47100

48101
let new_workspace = WorkspaceState::create(ctx, perm.read_permission())?;

crates/gitbutler-branch-actions/tests/branch-actions/virtual_branches/mod.rs

Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -73,6 +73,7 @@ mod create_virtual_branch_from_branch;
7373
mod init;
7474
mod list;
7575
mod list_details;
76+
mod move_branch;
7677
mod move_commit_to_vbranch;
7778
mod oplog;
7879
mod save_and_unapply_virtual_branch;
@@ -82,6 +83,60 @@ mod undo_commit;
8283
mod update_commit_message;
8384
mod workspace_migration;
8485

86+
/// Create a raw git commit parented to `parent` that sets `filename` to `content`,
87+
/// bypassing GitButler's stack API. Used in tests to construct competitor commits
88+
/// or to inject a known tree state without going through the worktree.
89+
pub fn create_raw_commit(
90+
repo: &gix::Repository,
91+
parent: gix::ObjectId,
92+
filename: &str,
93+
content: &[u8],
94+
) -> anyhow::Result<gix::ObjectId> {
95+
let blob_id = repo.write_blob(content)?.detach();
96+
let parent_commit = repo.find_commit(parent)?;
97+
let mut editor = parent_commit.tree()?.edit()?;
98+
editor.upsert(filename, gix::object::tree::EntryKind::Blob, blob_id)?;
99+
let tree_id = editor.write()?.detach();
100+
Ok(repo
101+
.write_object(gix::objs::Commit {
102+
tree: tree_id,
103+
parents: [parent].into(),
104+
message: "raw test commit".into(),
105+
..parent_commit.decode()?.to_owned()?
106+
})?
107+
.detach())
108+
}
109+
110+
/// Cherry-pick `competing_oid` (which shares an ancestor with `onto_oid` and modifies the
111+
/// same content) onto `onto_oid` to produce a conflicted commit, then update the source
112+
/// stack head to that conflicted commit. Returns the conflicted commit's OID.
113+
///
114+
/// The conflicted commit is parented to `onto_oid`, so after this call the source stack
115+
/// history is: merge_base → onto_oid → conflicted.
116+
pub fn push_conflicted_commit(
117+
ctx: &but_ctx::Context,
118+
stack_id: gitbutler_stack::StackId,
119+
onto_oid: gix::ObjectId,
120+
competing_oid: gix::ObjectId,
121+
) -> anyhow::Result<gix::ObjectId> {
122+
let repo = ctx.repo.get()?;
123+
let conflicted_oid = but_rebase::cherry_pick_one(
124+
&repo,
125+
onto_oid,
126+
competing_oid,
127+
but_rebase::cherry_pick::PickMode::Unconditionally,
128+
but_rebase::cherry_pick::EmptyCommit::Keep,
129+
)?;
130+
assert!(
131+
but_core::Commit::from_id(repo.find_commit(conflicted_oid)?.id())?.is_conflicted(),
132+
"cherry_pick_one must have produced a conflicted commit for the test to be meaningful"
133+
);
134+
let mut vb_state = gitbutler_stack::VirtualBranchesHandle::new(ctx.project_data_dir());
135+
let mut stack = vb_state.get_stack_in_workspace(stack_id)?;
136+
stack.set_stack_head(&mut vb_state, &repo, conflicted_oid)?;
137+
Ok(conflicted_oid)
138+
}
139+
85140
pub fn list_commit_files(
86141
ctx: &Context,
87142
commit_id: gix::ObjectId,

0 commit comments

Comments
 (0)