Move-commit should prevent inter-stack conflicts + Myers diff false positive causes hard failure#13048
Draft
Move-commit should prevent inter-stack conflicts + Myers diff false positive causes hard failure#13048
Conversation
Add a 3×2 test matrix (3 scenarios × 2 code paths) covering move-commit behavior for both the graph-based (but-workspace) and legacy (gitbutler-branch-actions) paths: - Myers false conflict: blank-line pattern triggers a spurious conflict in gitoxide's Myers diff (gitoxide#2475). Graph-based path fails with FailedToMergeBases; legacy path silently produces wrong tree content. - Non-overlapping (dependent) edits: add at top + delete in middle. Both paths merge cleanly. - Overlapping edits: same line modified differently. Both paths currently accept the conflicted commit, but should be prevented. These tests document current behavior to support two planned fixes: 1. Prevent inter-stack moves that would produce conflicted commits 2. Update assertions once gitoxide#2476 (Myers fix) lands upstream
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
There are two related problems with commit moves in GitButler:
1. Move-commit allows creating conflicted commits between stacks
When a user moves a commit from one stack to another (e.g. dragging from stack A to empty stack B), the cherry-pick can produce a conflicted commit on the destination stack. GitButler currently allows this — the move succeeds and a conflicted commit silently lands on the target branch.
This is a problem because GitButler's core value proposition is managing multiple branch checkouts in the same working directory. Inter-stack conflicts are extremely difficult to resolve in this context and should be prevented at the operation level, not surfaced after the fact. Ensuring stacks remain mutually exclusive should be our top priority.
2. Myers diff false positive causes hard failure on the graph-based path (gitoxide#2475)
A specific file content pattern involving blank lines (
item\n\nitem\nitem\n\n) triggers a false conflict in gitoxide's Myers diff implementation (fix PR). When this pattern appears in the 3-way merge base during a cherry-pick, the two code paths behave differently:but-workspace::commit::move_commit): HardFailedToMergeBaseserror. The operation fails completely.gitbutler-branch-actions::move_commit): Silently succeeds viamerge_options_force_ours, but the resulting tree content may be incorrect (the "ours" side wins where the merge should have combined both sides).Reproduction
The user sees this in the logs during the failed move:
A reproduction repo is available (ask Mattias), or run the tests below.
Test coverage
Six tests have been added covering a 3×2 matrix (3 scenarios × 2 code paths):
but-workspace)gitbutler-branch-actions)FailedToMergeBaseserrorRun the tests
Test files
crates/but-workspace/tests/workspace/commit/move_commit.rsmove_top_commit_to_empty_branch_myers_false_conflictmove_top_commit_to_empty_branch_dependent_changesmove_top_commit_to_empty_branch_overlapping_changescrates/gitbutler-branch-actions/tests/branch-actions/virtual_branches/move_commit_to_vbranch.rsmyers_false_conflict_on_move_to_empty_branchdependent_changes_move_to_empty_branchoverlapping_changes_move_to_empty_branchcrates/but-workspace/tests/fixtures/scenario/move-commit-{myers-false-conflict,dependent-changes,overlapping-changes}.shProposed fixes
1. Prevent moves that would produce inter-stack conflicts
The graph-based rebase engine already has a
conflictable: boolflag onPick(inbut-rebase::graph_rebase). Workspace commits useconflictable: false, which causes the rebase to bail if the workspace merge itself conflicts. The same mechanism could be used for move-commit operations: when cherry-picking the moved commit onto the destination branch, setconflictable: falseso the rebase aborts rather than creating a conflicted commit. The move operation can then return an error to the UI, allowing the user to see that the move isn't possible.For the legacy path, the
cherry_pick_onefunction inbut-rebase::cherry_pickalready checkshas_unresolved_conflictsafter the merge — a similar pre-flight check or early-return could prevent persisting a conflicted commit.2. Upstream Myers fix
The false-positive conflict is a bug in gitoxide's Myers diff algorithm, tracked at GitoxideLabs/gitoxide#2475 with a fix in #2476. Once that fix lands and we update our gitoxide dependency, the Myers-specific tests should start failing (indicating the bug is fixed) and can be updated to assert clean merges.
What to update when gitoxide#2476 lands
expect_errto assert the move succeeds and the commit is NOT conflictedalpha_x\n\ncharlie_x\n\n— bravo_x removed, alpha_x kept)Technical details
The 3-way merge during cherry-pick
When commit 2 is moved from stack A to empty stack B, it is cherry-picked onto
main(the merge base). The 3-way merge is:For the Myers false-positive case:
base→ours adds
alpha_xat top + trailing newline. base→theirs removesbravo_x. These don't overlap, but Myers produces a spurious empty insertion hunk on the blank-line boundaries that collides with the deletion.Code path divergence
but-api::commit::move_commit::commit_move_onlygitbutler-branch-actions::move_commitbut-rebase::graph_rebase::cherry_pick(N-to-M generalized)but-rebase::cherry_pick::cherry_pick_onemerge_options_force_oursConflictedCommit(accepted ifconflictable: true) orFailedToMergeBases(always bails)has_unresolved_conflicts