|
| 1 | +# Move-commit should prevent inter-stack conflicts + Myers diff false positive causes hard failure |
| 2 | + |
| 3 | +## Problem |
| 4 | + |
| 5 | +There are two related problems with commit moves in GitButler: |
| 6 | + |
| 7 | +### 1. Move-commit allows creating conflicted commits between stacks |
| 8 | + |
| 9 | +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. |
| 10 | + |
| 11 | +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. |
| 12 | + |
| 13 | +### 2. Myers diff false positive causes hard failure on the graph-based path (gitoxide#2475) |
| 14 | + |
| 15 | +A specific file content pattern involving blank lines (`item\n\nitem\nitem\n\n`) triggers a [false conflict in gitoxide's Myers diff implementation](https://github.com/GitoxideLabs/gitoxide/issues/2475) ([fix PR](https://github.com/GitoxideLabs/gitoxide/pull/2476)). When this pattern appears in the 3-way merge base during a cherry-pick, the two code paths behave differently: |
| 16 | + |
| 17 | +- **Graph-based path** (desktop app, `but-workspace::commit::move_commit`): Hard `FailedToMergeBases` error. The operation fails completely. |
| 18 | +- **Legacy path** (`gitbutler-branch-actions::move_commit`): Silently succeeds via `merge_options_force_ours`, but the resulting tree content may be incorrect (the "ours" side wins where the merge should have combined both sides). |
| 19 | + |
| 20 | +## Reproduction |
| 21 | + |
| 22 | +The user sees this in the logs during the failed move: |
| 23 | + |
| 24 | +``` |
| 25 | +Failed to merge bases while cherry picking commit <sha>. |
| 26 | +Encountered a conflict while merging the commit's new bases: <sha>, <sha>. |
| 27 | +Any ids mentioned may be in-memory and inaccessible through the git CLI. |
| 28 | +``` |
| 29 | + |
| 30 | +A reproduction repo is available (ask Mattias), or run the tests below. |
| 31 | + |
| 32 | +## Test coverage |
| 33 | + |
| 34 | +Six tests have been added covering a 3×2 matrix (3 scenarios × 2 code paths): |
| 35 | + |
| 36 | +| Scenario | Graph-based (`but-workspace`) | Legacy (`gitbutler-branch-actions`) | Correct behavior | |
| 37 | +| ------------------------------------------------------ | ----------------------------- | ----------------------------------- | ------------------------------------ | |
| 38 | +| **Myers false conflict** (blank-line trigger) | `FailedToMergeBases` error | Silently clean (wrong tree content) | Should be prevented or merge cleanly | |
| 39 | +| **Non-overlapping edits** (add at top + add at bottom) | Clean merge | Clean merge | Clean merge | |
| 40 | +| **Overlapping edits** (same line modified) | Conflicted commit (accepted) | Conflicted commit (accepted) | **Should be prevented** | |
| 41 | + |
| 42 | +### Run the tests |
| 43 | + |
| 44 | +```bash |
| 45 | +# Graph-based (3 tests) |
| 46 | +cargo test -p but-workspace --test workspace "move_top_commit_to_empty_branch" |
| 47 | + |
| 48 | +# Legacy (3 tests) |
| 49 | +cargo test -p gitbutler-branch-actions --test branch-actions "move_commit_to_vbranch" \ |
| 50 | + -- --skip no_diffs --skip multiple --skip diffs_on --skip locked --skip no_commit --skip no_branch |
| 51 | +``` |
| 52 | + |
| 53 | +### Test files |
| 54 | + |
| 55 | +- **Graph-based**: `crates/but-workspace/tests/workspace/commit/move_commit.rs` |
| 56 | + - `move_top_commit_to_empty_branch_myers_false_conflict` |
| 57 | + - `move_top_commit_to_empty_branch_dependent_changes` |
| 58 | + - `move_top_commit_to_empty_branch_overlapping_changes` |
| 59 | +- **Legacy**: `crates/gitbutler-branch-actions/tests/branch-actions/virtual_branches/move_commit_to_vbranch.rs` |
| 60 | + - `myers_false_conflict_on_move_to_empty_branch` |
| 61 | + - `dependent_changes_move_to_empty_branch` |
| 62 | + - `overlapping_changes_move_to_empty_branch` |
| 63 | +- **Fixtures**: `crates/but-workspace/tests/fixtures/scenario/move-commit-{myers-false-conflict,dependent-changes,overlapping-changes}.sh` |
| 64 | + |
| 65 | +## Proposed fixes |
| 66 | + |
| 67 | +### 1. Prevent moves that would produce inter-stack conflicts |
| 68 | + |
| 69 | +The graph-based rebase engine already has a `conflictable: bool` flag on `Pick` (in `but-rebase::graph_rebase`). Workspace commits use `conflictable: 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, set `conflictable: false` so 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. |
| 70 | + |
| 71 | +For the legacy path, the `cherry_pick_one` function in `but-rebase::cherry_pick` already checks `has_unresolved_conflicts` after the merge — a similar pre-flight check or early-return could prevent persisting a conflicted commit. |
| 72 | + |
| 73 | +### 2. Upstream Myers fix |
| 74 | + |
| 75 | +The false-positive conflict is a bug in gitoxide's Myers diff algorithm, tracked at [GitoxideLabs/gitoxide#2475](https://github.com/GitoxideLabs/gitoxide/issues/2475) with a fix in [#2476](https://github.com/GitoxideLabs/gitoxide/pull/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. |
| 76 | + |
| 77 | +### What to update when gitoxide#2476 lands |
| 78 | + |
| 79 | +1. **Graph-based Myers test**: Change `expect_err` to assert the move succeeds and the commit is NOT conflicted |
| 80 | +2. **Legacy Myers test**: Assert the tree content is correct (`alpha_x\n\ncharlie_x\n\n` — bravo_x removed, alpha_x kept) |
| 81 | + |
| 82 | +## Technical details |
| 83 | + |
| 84 | +### The 3-way merge during cherry-pick |
| 85 | + |
| 86 | +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: |
| 87 | + |
| 88 | +``` |
| 89 | +base: tree of commit 1 (parent of commit 2) |
| 90 | +ours: tree of main (target branch) |
| 91 | +theirs: tree of commit 2 |
| 92 | +``` |
| 93 | + |
| 94 | +For the Myers false-positive case: |
| 95 | + |
| 96 | +``` |
| 97 | +base: \n\nbravo_x\ncharlie_x\n (after commit 1 deleted alpha_x) |
| 98 | +ours: alpha_x\n\nbravo_x\ncharlie_x\n\n (main, with blank-line pattern) |
| 99 | +theirs: \n\ncharlie_x\n (commit 2 deleted bravo_x) |
| 100 | +``` |
| 101 | + |
| 102 | +base→ours adds `alpha_x` at top + trailing newline. base→theirs removes `bravo_x`. These don't overlap, but Myers produces a spurious empty insertion hunk on the blank-line boundaries that collides with the deletion. |
| 103 | + |
| 104 | +### Code path divergence |
| 105 | + |
| 106 | +| | Graph-based | Legacy | |
| 107 | +| ------------- | -------------------------------------------------------------------------------------------- | ------------------------------------------------------------------ | |
| 108 | +| Entry point | `but-api::commit::move_commit::commit_move_only` | `gitbutler-branch-actions::move_commit` | |
| 109 | +| Cherry-pick | `but-rebase::graph_rebase::cherry_pick` (N-to-M generalized) | `but-rebase::cherry_pick::cherry_pick_one` | |
| 110 | +| Merge options | Default | `merge_options_force_ours` | |
| 111 | +| On conflict | `ConflictedCommit` (accepted if `conflictable: true`) or `FailedToMergeBases` (always bails) | Force-resolves (ours wins), then checks `has_unresolved_conflicts` | |
0 commit comments