Add pre-flight conflict detection for cross-stack commit and branch moves#13086
Add pre-flight conflict detection for cross-stack commit and branch moves#13086
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
mtsgrd
force-pushed
the
mg-branch-11
branch
3 times, most recently
from
1d780c0 to
9b24e2c
Compare
mtsgrd
changed the title
mtsgrd
force-pushed
the
mg-branch-11
branch
3 times, most recently
from
5dd0f36 to
839e204
Compare
There was a problem hiding this comment.
Pull request overview
This PR adds a pre-flight conflict check for cross-stack commit and branch moves to prevent silent data loss caused by auto-resolving conflicts during the underlying cherry-pick/rebase.
Changes:
- Add
check_for_destination_conflict()inbut-workspaceto speculatively rebase proposed steps and fail if any picked commit becomes conflicted. - Wire new preflight checks into
move_commitandmove_branchentrypoints before snapshots/state mutation. - Add new Rust tests covering cross-stack non-overlapping moves and dependency-conflict rejection for both commit and branch moves.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/gitbutler-branch-actions/tests/branch-actions/virtual_branches/move_commit_to_vbranch.rs | Adds cross-stack move_commit tests for success and dependency rejection. |
| crates/gitbutler-branch-actions/tests/branch-actions/virtual_branches/move_branch.rs | New test module for cross-stack move_branch success and dependency rejection. |
| crates/gitbutler-branch-actions/tests/branch-actions/virtual_branches/mod.rs | Registers the new move_branch test module. |
| crates/gitbutler-branch-actions/src/move_commits.rs | Adds commit-move preflight check calling check_for_destination_conflict(). |
| crates/gitbutler-branch-actions/src/move_branch.rs | Adds branch-move preflight check calling check_for_destination_conflict(). |
| crates/gitbutler-branch-actions/src/actions.rs | Invokes preflight checks before snapshotting for move_commit and move_branch. |
| crates/but-workspace/src/legacy/tree_manipulation/utils.rs | Introduces check_for_destination_conflict() utility. |
| crates/but-workspace/src/legacy/tree_manipulation/mod.rs | Re-exports check_for_destination_conflict(). |
| crates/but-workspace/src/legacy/mod.rs | Re-exports check_for_destination_conflict() from legacy API surface. |
mtsgrd
force-pushed
the
mg-branch-11
branch
2 times, most recently
from
f654da0 to
4fb0999
Compare
|
@Byron this is something we discussed in our last eng meeting. It would be good to prevent moves that effectively cause cross-stack conflicts, since resolving them would require you undo each change where there was a dependency. That information isn't available while you're resolving a conflict, and fi you don't get it right you'll just be presented with a rebase error. I'm not in a hurry to land this pr since I'm still validating both what the code does, but also what we should want in the given scenarios. But, meanwhile, it would be great if you take a quick look and let me know if it's largely ok or if there are things that stand out like a sore thumb? |
Byron
left a comment
Byron
left a comment
There was a problem hiding this comment.
I just took a quick look and think it should be Ok in principle, as the check won't be observable.
One concern could be that it seems to preview the operation which might double its cost, but that shouldn't ever be in the way of correctness or even UX.
With that said, the PR shouldn't be merged with the dreaded VBH still in there, but removing it shouldn't be a problem.
| return Ok(()); | ||
| } | ||
| let repo = ctx.repo.get()?; | ||
| let vb_state = VirtualBranchesHandle::new(ctx.project_data_dir()); |
There was a problem hiding this comment.
Let's not introduce more VBH code, my main mission right now is actually to remove it 😅.
This information should be readily enough available in ctx.workspace_*().
There was a problem hiding this comment.
Should be sorted now!
Caleb-T-Owens
left a comment
Caleb-T-Owens
left a comment
There was a problem hiding this comment.
I'm wondering if we really need to implement the rebases twice.
Would it be possible to inspect the output of the rebases before we update any metadata or references. This way there isn't any possible divergence between the check and the real operation, and we can avoid signing commits twice.
I'm also curious about the actual data loss scenario. In gix terms, we are doing a force ours merge, but, any time we see that a force has happened, we then write it out as a conflicted commit which has all the data. We shouldn't end up loosing data. Could you help me understand the scenario better?
|
Sorry, I needed to update the PR description 🤦 This PR is really about preventing moves produce conflicted commits that are difficult to resolve. That data loss thing was exaggerated by Claude during the draft phase of this pr. See the updated description at the top, and this commit message for an up-to-date description: 36bab11 |
Ahh, ok. The conflict scenario does make sense that it's difficult to resolve. I would quite like it if we could modify the operations to avoid needing to do the rebase work twice though. |
It would be clutch if that's possible! |
Some of the operations may need some reorganisation, but you should be able to similarly inspect the output of the rebase before we update any metadata or references |
…oves 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.
|
@Caleb-T-Owens would you mind taking another look at this? It now only does a single pass rather than the speculative rebases. |
Summary
When moving a commit or branch between stacks, GitButler now checks whether the move would produce conflicts before making any changes.
The motivation for preventing these conflicts upfront is that they are particularly hard to resolve: if a moved commit lands in a conflicted state, the only way to successfully save the resolution in the commit editor is to undo the changes that caused the dependency conflict — but nothing in the UI tells you that. Catching the problem before the move is a much better experience than leaving the user to figure out why their edits won't save.
The check runs two speculative rebases without modifying the repository:
If either fails, the move is rejected with a clear error before
create_snapshotis called — so a rejected move leaves the repository completely unchanged.What gets caught:
What is unaffected:
Test matrix
Eight tests covering the cross-stack move outcomes:
*_non_overlapping*_with_dependency_rejected*_destination_conflict*_preexisting_conflictEach row is tested for both commit moves and branch moves (8 total).
Key files
utils.rs—check_rebase_for_conflicts(): shared speculative rebase helpermove_commits.rs,move_branch.rs—preflight_check()for each move typeactions.rs— wires preflight beforecreate_snapshot