sageserpent-open / kineticMerge

Merge a heavily refactored codebase and stay sane.
MIT License
12 stars 2 forks source link

Finesse resolution of coincident insertions / edits. #113

Closed sageserpent-open closed 1 week ago

sageserpent-open commented 2 weeks ago
          It is apparent (and this has been the case before work was started on this ticket) that coincident insertions and edits that are move destinations haven't been resolved taking the source of the move into account for trivial edit resolution - instead just the left and right contributions are used. That two-way resolution is fine when coincident insertions and edits are standalone, but is lacking when they are coincident move destinations.

          Regarding that last point about coincident entries being represented in unresolved form, perhaps we could represent then as insertion versus insertion conflicts or edit versus edit conflicts in the implementation of `MergeResultDetectingMotion`?

          If so, that would allow three-way resolution to be done in [`evaluateSpeculativeSourcesAndDestinations.evaluateSpeculativeSourcesAndDestinations`](https://github.com/sageserpent-open/kineticMerge/blob/d1973bb25fbbea61ba6f8359f04a7ad42b74ec55/src/main/scala/com/sageserpent/kineticmerge/core/MoveDestinationsReport.scala#L124).

          I'm not sure whether to add a pair of substitutions for the left- and right-side contribution to the three-way resolution there, and then to add additional pairs of substitutions referring to the two-way resolution for all the speculative coincident merge destinations that didn't turn out to be moves after all?

Originally posted by @sageserpent-open in https://github.com/sageserpent-open/kineticMerge/issues/90#issuecomment-2466247827

sageserpent-open commented 2 weeks ago

In short, when a coincident insertion or edit is standalone, resolution should be two-way (and it is in the existing code).

When a coincident insertion or edit is a move destination, resolution should be three-way.

sageserpent-open commented 1 week ago

Added test CodeMotionAnalysisExtensionTest.coincidences; this exposes a bug in the existing code introduced in #90 where speculative conflicts are recorded in the core merge algebra when a speculative migration is recorded.

This approach relies on evaluated moves making a set of migrated edit suppressions that pick away at the conflicted merge result until the speculative conflict is removed. The problem is that other content unrelated to the speculative move can also be incorporated into the conflict, and the edit suppression doesn't cause these to become resolved - they stay on just one side of the conflict.

The plan is to revisit the old approach of dynamically resolving conflicts and edits in the merge algebra when a move source is found, but this time to use the resolved moves in the manner introduced in #90.

So there is an initial discovery phase where speculative migration content and speculative move destinations are picked up via a merge algebra, followed by evaluation of moves. The same merge algebra operations that drove the discovery are then replayed through another merge algebra that dynamically resolves conflicts and edits, delegating to a core merge algebra.

A nice touch here is to leave all the content resolution to the core merge algebra, but to decide what to pass in the delegating merge algebra. So a coincident insertion (or edit) that is a coincident move destination can be dynamically resolved into a preservation on the core merge algebra, allowing that to perform the content resolution.

Similarly, plain one-sided moves cause one-sided edits or one-sided insertions to become preservations.

sageserpent-open commented 1 week ago

Commit b35273138e885f7989a7bad19b952956b318c87a delivers most of what was proposed in the previous comment; what is missing is the revised approach to resolution.

Nevertheless, CodeMotionAnalysisExtensionTest.coincidences passes now, so this approach has paid off.

sageserpent-open commented 1 week ago

The plan for #83 treated anchors as demarcating the boundaries of anchored runs, but doesn't include the anchor in the merge of the run...

          6. An anchored run does not include the anchor itself. It is grown away from the anchor, terminating just before it encounters another anchor, or part of a preservation, or part of a non-migrated edit, or part of a migrated edit or part or a migrated deletion.

Why?

Perhaps MoveDestinationsReport.evaluateSpeculativeSourcesAndDestinations.evaluateSpeculativeSourcesAndDestinations should not perform resolution at all for anchors, sticking only to edit / deletion migration. If so, the notion of anchored runs could be extended to include the anchors (even if the run is just an anchor and nothing else) - so the final merge of the anchored runs across the three sides would send the anchors off to the core merge algebra to be resolved there. Nice!

The tricky part is when two anchored runs share the same intervening anchor - we don't want to perform merges that overlap. There is also fun to be had with an anchor forms part of an ambiguous match, and thus there are multiple merges to consider.

Nonetheless, I think this is the way to go, so resolution will be kicked down the road into #83...

sageserpent-open commented 1 week ago

Merged to main in Git commit SHA: f79df93eed13c8e80d345f8ef820abfab96ff1af.