Skip to content

Commit 9b24e2c

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 rebase the moved commits on top of the destination branch's current HEAD before applying any changes. If any output commit is conflicted, the move is rejected. A conflict here means the moved commits depend on context that remains in the source stack — for example, a commit being moved was built on top of commits that stay behind. Given GitButler's mutual-exclusivity invariant (each hunk belongs to at most one stack), this is the only class of inter-stack conflict that can arise. Properties of the check: - Pre-flight: the repository is unchanged on failure. - Minimal scope: only the moved commits are rebased, not all destination commits. Any conflict is logically between the source and the target only. - Inter-stack only: skipped when source and destination stack are the same, since intra-stack moves cannot produce this class of conflict.
1 parent 60adba7 commit 9b24e2c

9 files changed

Lines changed: 685 additions & 4 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_for_destination_conflict,
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_for_destination_conflict;

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

Lines changed: 53 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,11 +2,64 @@
22
33
use std::collections::HashMap;
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 if any Pick commit lands as conflicted.
14+
///
15+
/// A conflict means the moved commit depends on context that remains in the source stack,
16+
/// which is an inter-stack conflict — the invariant that stack changes are mutually exclusive
17+
/// guarantees no other conflict cause exists.
18+
///
19+
/// This must be called before modifying any stack state so that a failure leaves the
20+
/// repository unchanged.
21+
pub fn check_for_destination_conflict(
22+
ctx: &Context,
23+
steps: Vec<RebaseStep>,
24+
onto: gix::ObjectId,
25+
) -> Result<()> {
26+
let commit_ids: Vec<gix::ObjectId> = steps
27+
.iter()
28+
.filter_map(|s| {
29+
if let RebaseStep::Pick { commit_id, .. } = s {
30+
Some(*commit_id)
31+
} else {
32+
None
33+
}
34+
})
35+
.collect();
36+
if commit_ids.is_empty() {
37+
return Ok(());
38+
}
39+
let repo = ctx.repo.get()?;
40+
let mut rebase = but_rebase::Rebase::new(&repo, Some(onto), None)?;
41+
rebase.rebase_noops(false);
42+
rebase.steps(steps)?;
43+
let output = rebase.rebase(&*ctx.cache.get_cache()?)?;
44+
45+
for old_id in commit_ids {
46+
let new_id = output
47+
.commit_mapping
48+
.iter()
49+
.find(|(_, old, _)| *old == old_id)
50+
.map(|(_, _, new)| *new)
51+
.unwrap_or(old_id);
52+
let commit = but_core::Commit::from_id(new_id.attach(&repo))?;
53+
if commit.is_conflicted() {
54+
bail!(
55+
"This move would cause a conflict: the moved changes depend on commits \
56+
that remain in the source stack."
57+
);
58+
}
59+
}
60+
Ok(())
61+
}
62+
1063
/// Takes a rebase output and returns the commit mapping with any extra
1164
/// mapping overrides provided.
1265
///

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

Lines changed: 15 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_for_destination_conflict, 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};
@@ -45,6 +45,20 @@ pub(crate) fn move_branch(
4545
let destination_stack = vb_state.get_stack_in_workspace(target_stack_id)?;
4646
let destination_merge_base = destination_stack.merge_base(ctx)?;
4747

48+
// Pre-flight (inter-stack only): check that cherry-picking the branch commits onto the
49+
// destination won't conflict. A conflict means the branch depends on context that stays in
50+
// the source stack.
51+
// Must run before any state is modified so a failure leaves the repo unchanged.
52+
if source_stack_id != target_stack_id {
53+
let (subject_branch_steps, _) =
54+
extract_branch_steps(ctx, &repo, &source_stack, subject_branch_name)?;
55+
check_for_destination_conflict(
56+
ctx,
57+
subject_branch_steps,
58+
destination_stack.head_oid(ctx)?,
59+
)?;
60+
}
61+
4862
let (subject_branch_steps, deleted_stacks) = extract_and_rebase_source_branch(
4963
ctx,
5064
source_stack_id,

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

Lines changed: 16 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
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_for_destination_conflict, stack_ext::StackExt};
55
use gitbutler_stack::{StackId, VirtualBranchesHandle};
66
use gitbutler_workspace::branch_trees::{WorkspaceState, update_uncommitted_changes};
77
use serde::Serialize;
@@ -41,8 +41,22 @@ pub(crate) fn move_commit(
4141
repo.find_commit(subject_commit_oid)
4242
.with_context(|| format!("commit {subject_commit_oid} to be moved could not be found"))?;
4343

44-
take_commit_from_source_stack(ctx, &mut source_stack, subject_commit_oid)?;
44+
// Pre-flight (inter-stack only): check that cherry-picking the commit onto the destination
45+
// won't conflict.
46+
// A conflict means the commit depends on context that stays in the source stack.
47+
// Must run before any state is modified so a failure leaves the repo unchanged.
48+
if source_stack_id != target_stack_id {
49+
check_for_destination_conflict(
50+
ctx,
51+
vec![RebaseStep::Pick {
52+
commit_id: subject_commit_oid,
53+
new_message: None,
54+
}],
55+
destination_stack.head_oid(ctx)?,
56+
)?;
57+
}
4558

59+
take_commit_from_source_stack(ctx, &mut source_stack, subject_commit_oid)?;
4660
move_commit_to_destination_stack(&mut vb_state, ctx, destination_stack, subject_commit_oid)?;
4761

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

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

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -73,6 +73,8 @@ mod create_virtual_branch_from_branch;
7373
mod init;
7474
mod list;
7575
mod list_details;
76+
mod move_branch;
77+
mod move_changes;
7678
mod move_commit_to_vbranch;
7779
mod oplog;
7880
mod save_and_unapply_virtual_branch;
Lines changed: 199 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,199 @@
1+
use bstr::ByteSlice;
2+
use but_testsupport::legacy::stack_details;
3+
use gitbutler_branch::BranchCreateRequest;
4+
use gitbutler_stack::VirtualBranchesHandle;
5+
6+
use super::{Test, create_commit};
7+
8+
/// Move a branch that has no dependencies on commits remaining in the source
9+
/// stack. The source stack is left empty and deleted; the destination gains
10+
/// the branch. No conflicts should arise.
11+
#[test]
12+
fn move_branch_non_overlapping() {
13+
let Test { repo, ctx, .. } = &mut Test::default();
14+
15+
std::fs::write(repo.path().join("base.txt"), "base\n").unwrap();
16+
repo.commit_all("M");
17+
repo.push();
18+
19+
let mut guard = ctx.exclusive_worktree_access();
20+
gitbutler_branch_actions::set_base_branch(
21+
ctx,
22+
&"refs/remotes/origin/master".parse().unwrap(),
23+
guard.write_permission(),
24+
)
25+
.unwrap();
26+
drop(guard);
27+
28+
// Source stack: one branch, edits file-a.
29+
std::fs::write(repo.path().join("file-a.txt"), "source content\n").unwrap();
30+
let mut guard = ctx.exclusive_worktree_access();
31+
let source_entry = gitbutler_branch_actions::create_virtual_branch(
32+
ctx,
33+
&BranchCreateRequest {
34+
name: Some("source-stack".into()),
35+
..Default::default()
36+
},
37+
guard.write_permission(),
38+
)
39+
.unwrap();
40+
drop(guard);
41+
let source_id = source_entry.id;
42+
create_commit(ctx, source_id, "source: add file-a").unwrap();
43+
44+
let details = stack_details(ctx);
45+
let (_, source_sd) = details.iter().find(|(id, _)| *id == source_id).unwrap();
46+
let source_branch_name = source_sd.branch_details[0].name.to_str_lossy().to_string();
47+
48+
// Destination stack: one branch, edits file-b.
49+
std::fs::write(repo.path().join("file-b.txt"), "dest content\n").unwrap();
50+
let mut guard = ctx.exclusive_worktree_access();
51+
let dest_entry = gitbutler_branch_actions::create_virtual_branch(
52+
ctx,
53+
&BranchCreateRequest {
54+
name: Some("dest-stack".into()),
55+
..Default::default()
56+
},
57+
guard.write_permission(),
58+
)
59+
.unwrap();
60+
drop(guard);
61+
let dest_id = dest_entry.id;
62+
create_commit(ctx, dest_id, "dest: add file-b").unwrap();
63+
64+
let details = stack_details(ctx);
65+
let (_, dest_sd) = details.iter().find(|(id, _)| *id == dest_id).unwrap();
66+
let dest_branch_name = dest_sd.branch_details[0].name.to_str_lossy().to_string();
67+
68+
let result = gitbutler_branch_actions::move_branch(
69+
ctx,
70+
dest_id,
71+
&dest_branch_name,
72+
source_id,
73+
&source_branch_name,
74+
);
75+
assert!(result.is_ok(), "move_branch should succeed: {result:?}");
76+
77+
let details = stack_details(ctx);
78+
79+
// Source stack was the only branch — it should be deleted.
80+
let result = result.unwrap();
81+
assert!(
82+
result.deleted_stacks.contains(&source_id),
83+
"source stack should be deleted after its only branch was moved"
84+
);
85+
86+
// Destination should now have 2 branches, no conflicts.
87+
let (_, dest_sd) = details
88+
.iter()
89+
.find(|(id, _)| *id == dest_id)
90+
.expect("dest stack should exist");
91+
assert_eq!(
92+
dest_sd.branch_details.len(),
93+
2,
94+
"dest should have 2 branches"
95+
);
96+
for bd in &dest_sd.branch_details {
97+
assert!(
98+
bd.commits.iter().all(|c| !c.has_conflicts),
99+
"no conflicts expected"
100+
);
101+
}
102+
}
103+
104+
/// A branch whose commits depend on commits remaining in the source stack
105+
/// cannot be moved: cherry-picking onto the destination would conflict.
106+
#[test]
107+
fn move_branch_with_dependency_rejected() {
108+
let Test { repo, ctx, .. } = &mut Test::default();
109+
110+
std::fs::write(repo.path().join("base.txt"), "base\n").unwrap();
111+
repo.commit_all("M");
112+
repo.push();
113+
114+
let mut guard = ctx.exclusive_worktree_access();
115+
gitbutler_branch_actions::set_base_branch(
116+
ctx,
117+
&"refs/remotes/origin/master".parse().unwrap(),
118+
guard.write_permission(),
119+
)
120+
.unwrap();
121+
drop(guard);
122+
123+
// Source stack: bottom branch creates shared.txt; top branch modifies it.
124+
std::fs::write(repo.path().join("shared.txt"), "alpha\nbravo\ncharlie\n").unwrap();
125+
let mut guard = ctx.exclusive_worktree_access();
126+
let source_entry = gitbutler_branch_actions::create_virtual_branch(
127+
ctx,
128+
&BranchCreateRequest {
129+
name: Some("source-stack".into()),
130+
..Default::default()
131+
},
132+
guard.write_permission(),
133+
)
134+
.unwrap();
135+
drop(guard);
136+
let source_id = source_entry.id;
137+
create_commit(ctx, source_id, "bottom: create shared.txt").unwrap();
138+
139+
{
140+
let vb_state = VirtualBranchesHandle::new(ctx.project_data_dir());
141+
let mut stack = vb_state.get_stack_in_workspace(source_id).unwrap();
142+
stack
143+
.add_series_top_of_stack(ctx, "source-top".to_string())
144+
.unwrap();
145+
}
146+
147+
std::fs::write(
148+
repo.path().join("shared.txt"),
149+
"alpha\nbravo_modified\ncharlie\n",
150+
)
151+
.unwrap();
152+
let mut guard = ctx.exclusive_worktree_access();
153+
but_workspace::legacy::commit_engine::create_commit_simple(
154+
ctx,
155+
source_id,
156+
None,
157+
vec![but_core::DiffSpec {
158+
previous_path: None,
159+
path: "shared.txt".into(),
160+
hunk_headers: vec![],
161+
}],
162+
"top: modify shared.txt".to_string(),
163+
"source-top".to_string(),
164+
guard.write_permission(),
165+
)
166+
.unwrap();
167+
drop(guard);
168+
169+
// Destination stack: empty.
170+
let mut guard = ctx.exclusive_worktree_access();
171+
let dest_entry = gitbutler_branch_actions::create_virtual_branch(
172+
ctx,
173+
&BranchCreateRequest {
174+
name: Some("dest-stack".into()),
175+
..Default::default()
176+
},
177+
guard.write_permission(),
178+
)
179+
.unwrap();
180+
drop(guard);
181+
let dest_id = dest_entry.id;
182+
183+
let details = stack_details(ctx);
184+
let (_, dest_sd) = details.iter().find(|(id, _)| *id == dest_id).unwrap();
185+
let dest_branch_name = dest_sd.branch_details[0].name.to_str_lossy().to_string();
186+
187+
// Moving the dependent top branch must fail.
188+
let result = gitbutler_branch_actions::move_branch(
189+
ctx,
190+
dest_id,
191+
&dest_branch_name,
192+
source_id,
193+
"source-top",
194+
);
195+
assert!(
196+
result.is_err(),
197+
"move_branch should be rejected: top branch depends on bottom branch that stays in source"
198+
);
199+
}

0 commit comments

Comments
 (0)