rewrite: don't lose content that both sides of a merge contain - #10100
rewrite: don't lose content that both sides of a merge contain#10100jefft wants to merge 1 commit into
Conversation
8399425 to
32d8851
Compare
|
In my codebase this fix not only made lost lines reappear (the |
688b8d0 to
90a50ce
Compare
jefft
changed the title
8d5621b to
55db199
Compare
| .await | ||
| // TODO: indexing error shouldn't be a "BackendError" | ||
| .map_err(|err| BackendError::Other(err.into()))?; | ||
| let base_tree = Box::pin(merge_commit_ids_no_resolve(store, index, &ancestor_ids)).await?; |
There was a problem hiding this comment.
Recursion using the machine stack will crash Google's servers. Can you rewrite this code to keep track of the stack on the heap instead (e.g. in a Vec)?
There was a problem hiding this comment.
Recursion using the machine stack will crash Google's servers.
So it did (crash the CI). For this PR though, could we just Box::pin as in the revised code?
Box::pin(merge_commit_ids_no_resolve(store, index, &commit_ids)).awaitThat fixes the compile-time type depth problem, which is what broke CI.
There was a problem hiding this comment.
No, I strongly suspect that will still stash Google's servers. You can find many examples of PRs fixing similar problems in this list (but not all of them are fixing stack overflows).
| let tree = tree.resolve().await?; | ||
| if tree.tree_ids().is_resolved() { | ||
| return Ok(tree); | ||
| } | ||
| let conflicts: Vec<_> = tree.conflicts().collect(); | ||
| let mut builder = MergedTreeBuilder::new(tree); | ||
| for (path, values) in conflicts { | ||
| builder.set_or_remove(path, Merge::resolved(values?.first().clone())); | ||
| } | ||
| builder.write_tree().await |
There was a problem hiding this comment.
Arbitrarily picking the first value seems wrong. Perhaps we need a tree.resolve() version that also updates the tree with any automatic resolutions. IIRC, tree.resolve() currently either fully resolves a file conflict or leaves it fully unresolved. FYI, we have talked about going in the opposite direction as well, making tree.resolve() leaving the whole tree either fully resolved or fully unresolved (#4152). I think it seems cleaner to have one method for resolving all the way down to hunk level when possible and one method for not resolving anything at all.
There was a problem hiding this comment.
Arbitrarily picking the first value seems wrong.
Agreed.
Please see the revised code, which moves the collapse into a new MergedTree::collapse_conflicts(), right next to resolve(). It applies the automatic resolutions resolve() would apply, then keeps the first side of whatever remains. The policy is now named, documented and unit-tested, instead of implicit in the rewrite logic.
This should play nicely with #4152. E.g. if you implement hunk-level resolving, the change inside collapse_conflicts() is just its first line:
let tree = self.resolve().await?;
// becomes:
let tree = self.resolve_hunks().await?; // #4152 hunk-leveland the first-side fallback stays as the residue policy, firing only on paths even hunk-level merging can't combine.
(Confession: I am a barely sentient PHP developer with a LLM. When I unchecked the "I fully understand the code that I am submitting" checkbox in this PR, I really meant it. I do have an exceedingly merge'y codebase, where every second jj rebase or jj absorb passes through a cursed criss-cross merge, causing conflicts and pain, so I am keen to see this fixed and happy to test follow-ups on real history.)
ca53f1e to
3cc2862
Compare
|
FWIW, we should probably disable the same-change rule during auto-merging. It seems better to require an explicit |
A merge can silently delete content that both of its sides contain. No conflict marker is produced, and because a merge commit's diff is taken against its auto-merged parents, there is no diff either -- nothing records that the content was ever there. git does not behave this way on the same history. Reported as jj-vcs#6369. It takes two fixes that each add the same line, merged together twice, and those two merges later merged. Only two parents are needed at the end, so this is not an octopus-merge problem: ``` base a file WITHOUT some line / \ A B two fixes, each independently ADDING that line |\ /| | \/ | | /\ | AB_for_X AB_for_Y two *separate* merges of A and B; both have the line \ / XY the line disappears here, with no conflict ``` The same-change rule resolves AB_for_X and AB_for_Y to the line being present, so they and their merge bases A and B all carry it. The base is then never collapsed to a single tree: merge_commit_trees_no_resolve_without_repo() asks find_recursive_merge_commits() for a Merge<CommitId> and flattens it, which moves each of the base merge's adds onto the remove side: ``` adds [AB_for_X(line), base(no line), AB_for_Y(line)] removes [A(line), B(line)] ``` trivial_merge() counts values as a signed multiset, +1 per add and -1 per remove. The line nets 0, the version without it nets +1, and exactly one value at +1 is the "resolved cleanly" case. A value that cancels to zero disappears silently: the counting cannot tell "someone deleted this" from "the base assignment put it on the wrong side". Collapse the base instead, as martinvonz suggested in jj-vcs#6369: recurse over trees rather than commit ids, and reduce each recursive base to exactly one tree. The new merge_commit_ids_no_resolve() builds the outer terms directly, and each recursive base is collapsed by MergedTree::collapse_conflicts(), a new method next to MergedTree::resolve(): it applies the automatic resolutions resolve() would apply, then keeps the first side of whatever remains. The merge becomes the ordinary three-way `adds [AB_for_X, AB_for_Y], removes [virtual_base]`, where the line nets +1 and survives. Merge base selection is unchanged. Keeping the first side is what git's "resolve" strategy effectively does when it picks one of several merge bases. The choice of side only affects which diffs the outer merge sees and which borderline merges auto-resolve versus conflict, never which content the outer merge can keep: every parent's own value is still an add. Materializing the leftover conflicts into markers, as git's "recursive"/"ort" strategies do, is not an option: git never parses a conflict back, but jj round-trips conflicts through the working copy, and a base carrying markers can produce an outer conflict whose materialization no longer parses, leaving the next snapshot to store the marker text as ordinary content. A resolve() variant that also resolves down to hunk level would shrink the set of paths needing the first-side fallback; that belongs with jj-vcs#4152. Merges with a single, already-resolved merge base are unaffected. Criss-cross merges get smaller unresolved term lists -- test_merge_criss_cross's goes from five sides to three -- while their resolved results are unchanged. find_recursive_merge_commits() is no longer used by the tree merge, but is left in place and still tested since it is public API. The merge future is boxed at the merge_commit_trees_no_resolve_without_repo() boundary so the deeper future type doesn't reach consumers. The custom-command example already overflowed rustc's default recursion limit under rustc 1.98. Fixes jj-vcs#6369. Part of jj-vcs#7640. Assisted-by: Opus 5 via Claude Code
3cc2862 to
d6d2e07
Compare
So if I have commits A and B in my stack and commit A gets rebased onto trunk by the remote (perhaps with a different change id), then rebasing onto trunk will result in conflicts in my rebased version of A, right? I agree that it's more correct, but I'm also a bit worried that it will be annoying in many cases. Perhaps it's not so bad now that we detect if the change has been rebased when the change id is preserved. |
If we can |
3 participants
A merge can silently delete content that both of its sides contain. No
conflict marker is produced, and because a merge commit's diff is taken
against its auto-merged parents, there is no diff either -- nothing records
that the content was ever there. git does not behave this way on the same
history. Reported as #6369.
It takes two fixes that each add the same line, merged together twice, and
those two merges later merged. Only two parents are needed at the end, so
this is not an octopus-merge problem:
The same-change rule resolves AB_for_X and AB_for_Y to the line being
present, so they and their merge bases A and B all carry it. The base is
then never collapsed to a single tree:
merge_commit_trees_no_resolve_without_repo() asks
find_recursive_merge_commits() for a Merge and flattens it, which
moves each of the base merge's adds onto the remove side:
trivial_merge() counts values as a signed multiset, +1 per add and -1 per
remove. The line nets 0, the version without it nets +1, and exactly one
value at +1 is the "resolved cleanly" case. A value that cancels to zero
disappears silently: the counting cannot tell "someone deleted this" from
"the base assignment put it on the wrong side".
Collapse the base instead, as martinvonz suggested in #6369: recurse over
trees rather than commit ids, and reduce each recursive base to exactly one
tree. The new merge_commit_ids_no_resolve() builds the outer terms
directly, and each recursive base is collapsed by
MergedTree::collapse_conflicts(), a new method next to MergedTree::resolve():
it applies the automatic resolutions resolve() would apply, then keeps the
first side of whatever remains. The merge becomes the ordinary three-way
adds [AB_for_X, AB_for_Y], removes [virtual_base], where the line nets +1and survives. Merge base selection is unchanged.
Keeping the first side is what git's "resolve" strategy effectively does
when it picks one of several merge bases. The choice of side only affects
which diffs the outer merge sees and which borderline merges auto-resolve
versus conflict, never which content the outer merge can keep: every
parent's own value is still an add. Materializing the leftover conflicts
into markers, as git's "recursive"/"ort" strategies do, is not an option:
git never parses a conflict back, but jj round-trips conflicts through the
working copy, and a base carrying markers can produce an outer conflict
whose materialization no longer parses, leaving the next snapshot to store
the marker text as ordinary content. A resolve() variant that also resolves
down to hunk level would shrink the set of paths needing the first-side
fallback; that belongs with #4152.
Merges with a single, already-resolved merge base are unaffected.
Criss-cross merges get smaller unresolved term lists --
test_merge_criss_cross's goes from five sides to three -- while their
resolved results are unchanged. find_recursive_merge_commits() is no longer
used by the tree merge, but is left in place and still tested since it is
public API.
The merge future is boxed at the merge_commit_trees_no_resolve_without_repo()
boundary so the deeper future type doesn't reach consumers. The
custom-command example already overflowed rustc's default recursion limit
under rustc 1.98.
Fixes #6369.
Part of #7640.
Assisted-by: Opus 5 via Claude Code
Checklist
If applicable:
CHANGELOG.mdREADME.md,docs/,demos/)cli/src/config-schema.json)how it works, how it's organized), including any code drafted by an LLM.
an eye towards deleting anything that is irrelevant, clarifying anything
that is confusing, and adding details that are relevant. This includes,
for example, commit descriptions, PR descriptions, and code comments.