Repository navigation
Disable rename detection when merging stack trees - #13445
Conversation
There was a problem hiding this comment.
Pull request overview
This PR prevents gix rename detection from producing false-positive renames when tree_from_merging_commits pairwise-merges independent stack tip trees (workspace multi-parent scenarios), which could otherwise hide delete-vs-modify conflicts and silently drop deletions in the resulting workspace tree.
Changes:
- Disable rename tracking/rewrites for the stack-tip tree merge in
tree_from_merging_commitsby switching tomerge_options_no_rewrites_fail_fast(). - Add a regression scenario fixture reproducing the silent-deletion bug (delete + add-similar-content across stacks).
- Add a regression test (with snapshot) asserting the conflict is surfaced as
FailedToMergeBasesinstead of producing an incorrect merged tree.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| crates/but-rebase/src/graph_rebase/cherry_pick.rs | Switch stack-tip tree merge to no-rewrite merge options to avoid cross-stack false renames. |
| crates/but-rebase/tests/rebase/graph_rebase/cherry_pick.rs | Add regression test validating conflict surfacing for delete-vs-modify instead of silent deletion loss. |
| crates/but-rebase/tests/fixtures/scenario/cherry-pick-rename-detection.sh | Add fixture repository topology and file edits/deletes to reproduce the rename-detection failure mode. |
| crates/but-rebase/tests/rebase/graph_rebase/snapshots/rebase__graph_rebase__cherry_pick__rename_detection_does_not_swallow_file_deletion.snap | Snapshot of the fixture commit graph used by the new regression test. |
|
@Byron would you be able to help me validate this? |
| // Rename detection must be disabled here: stack tips are independent, so | ||
| // renames detected across them are always false positives. With detection | ||
| // enabled, gix can silently swallow clean file deletions. | ||
| let (options, conflicts) = repo.merge_options_no_rewrites_fail_fast()?; |
There was a problem hiding this comment.
This makes sense to me!
I might also add that assuming the rename is not a false positive, then merge can get smart with how it treats these files and it's definitely not what we want for a megamerge.
The worst here is that the rename detection is not as elaborate as the one in Git, merely 'good enough', but even if it was great it's probably still not what we want here.
The question might still be if a cherry-pick would benefit from this, and also what Git does on cherry-pick. Does it run rename-detection to get better picks, maybe? If they also turn it off, it's a clear answer though.
There was a problem hiding this comment.
Git does seem to use rename detection for cherry-pick, so we now only disable rename detection for workspace commits with TreeMergeMode::Workspace passed through cherry_pick. Regular merge commits within stacks use TreeMergeMode::Standard with rename detection enabled. Let me know what you think?
There was a problem hiding this comment.
That sounds better, let's do what Git does, and workaround the workspace-commit cherry-pick issue like this for now.
If you have the time, you might want to setup a testcase where Git cherry-picks such a merge-commit (can it even do that?), and if it's different/better, it's a bug in gix.
There was a problem hiding this comment.
I'd say it's working as intended?
I tested what Git does in this scenario. Git's cherry-pick -m 1 works correctly here because it operates fundamentally differently from our approach: it extracts a diff relative to the selected parent and applies it as a 3-way merge. In that diff, file-a.txt is untouched, so it's never affected by rename detection.
Our merged_tree_from_commits works differently — it merges the full trees of the parent commits pairwise (not diffs). This is specific to how we construct workspace commits from multiple stack heads. In that full-tree merge, gix's rename detection matches file-b → file-combined and silently absorbs the deletion of file-a.txt.
So this isn't a gix bug — rename detection is working as designed for tree merges. The issue is that rename detection doesn't make sense for our workspace merges, where stacks are independent and a "rename" across stacks is coincidental. Disabling it only for TreeMergeMode::Workspace keeps standard cherry-pick/rebase behavior unchanged.
3f1a0b3 to
0b0c0fe
Compare
|
@Byron if you're happy with these changes can we go ahead and merge? |
gix's rename detection can match deleted files to newly added files based on content similarity. When merging independent stack tip trees in `merged_tree_from_commits`, this causes false-positive renames across stacks, silently swallowing clean file deletions and hiding real delete-vs-modify conflicts. Introduce a `TreeMergeMode` enum (`Standard` / `Workspace`) threaded through `cherry_pick` and `merged_tree_from_commits`. In `Workspace` mode, rename detection is disabled via `merge_options_no_rewrites_fail_fast`. In `Standard` mode, rename detection remains active to keep parity with git for regular merge commits. Also rename `tree_from_merging_commits` to `merged_tree_from_commits` for clarity, and add a regression test that reproduces the scenario and verifies the conflict is correctly surfaced. Rename TreeMergeMode variants and update defaults Clarify and rename TreeMergeMode variants to WithRenames/WithoutRenames to make their behavior explicit and update all usages and tests to the new names. Adjust default values and comments to indicate rename detection is enabled by default, update merge option lookups accordingly, and refresh a test snapshot that reflects the expected commit graph output. This improves code readability and makes the workspace/no-rename semantics unambiguous.
Byron
left a comment
There was a problem hiding this comment.
LGTM!*
*The PR mostly affects but-rebase so my approval can only be for the changes to the way merges are computed, i.e. to skip rename tracking for what looks like workspace commits.
Problem
When rebuilding the workspace commit,
merged_tree_from_commitsmerges stack tip trees pairwise using a 3-way merge. gix's rename detection matches deleted files to newly added files based on content similarity — and this produces false-positive renames across independent stacks, silently corrupting the workspace tree.Example scenario
Two stacks share a base with
file-a.txtandfile-b.txt:file-b.txtfile-combined.txt(content similar tofile-b.txt)When merging these stack trees, gix sees
file-b.txt → file-combined.txtas a rename (high content similarity). This has two effects:file-b.txtis hidden — gix treats it as a rename+content merge insteadfile-a.txt's clean deletion is silently dropped from the result treeWithout the fix, the workspace tree contains:
With the fix, the delete-vs-modify conflict on
file-b.txtis correctly surfaced asFailedToMergeBases, preventing the silent data loss.Fix
Introduce a
TreeMergeModeenum (WithRenames/WithoutRenames) threaded throughcherry_pickandmerged_tree_from_commits:TreeMergeMode::WithoutRenames— disables rename detection viamerge_options_no_rewrites_fail_fast. Used for workspace commits whose parents are independent stack heads.TreeMergeMode::WithRenames— keeps rename detection enabled, maintaining parity with git for regular merge commits.Why is this safe?
merged_tree_from_commitsis only called when merging multiple parents of a commit. For single-parent (linear) commits, the merge loop never executes and the options are never used.The actual cherry-pick 3-way merge (base → onto → target) uses
merge_options_force_ours, which is a completely separate code path unaffected by this change. Rename detection remains fully active there.Since stacks are independent by definition, a rename detected between two stack trees is always a false positive. Disabling it here is correct.
Other changes
tree_from_merging_commits→merged_tree_from_commitsfor clarityconflictablefield docs to specify it's checked during rebase, not mergeTest plan
workspace_merge_surfaces_delete_vs_modify_conflict) with a fixture that reproduces the exact scenario