search

sageserpent-open / kineticMerge

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

Issue a diagnostic message when a diverging move is detected. #38

Closed sageserpent-open closed 6 months ago

sageserpent-open commented 7 months ago

This follows from #37.

As part of this, the diverging move destinations should be reported with the side they are on, an excerpt of the text (if longer than some predefined limit) and the line number, character offset and token index of the start and the finish of the destination.

The move source should also be shown with the appropriate commit.

A subtlety is when the divergent move is part of a set of mutually ambiguous matches. What should we do there?

sageserpent-open commented 7 months ago

Perhaps all moves should be reported, with any propagated details if applicable?

sageserpent-open commented 7 months ago

Yes, let’s note all moves in preparation for #30.

sageserpent-open commented 7 months ago

Sections are rendered in a more readable fashion with line number and character offset information as of Git commit SHA: 530c7d6736077152e25b6cf86229c09f45284625.

sageserpent-open commented 7 months ago

Umming and ahhing here...

  1. Divergence in itself is probably OK - while there is a conflict in which move destination to pick, one can just go with both and leave it to the user to fix up post-merge. As long as it is reported, it's OK.
  2. Divergence does not mix with propagation of edits or deletions - the fact that the moved section exists on both sides means that any deletion of edit made at the move source can't be though of as being associated with the moved section. That was mulled over in #37, so no attempt is made to propagate an edit or deletion through a divergent move.
  3. It is an open question as to whether an edit or deletion should be propagated through a resolved divergent move. For now, I'm leaning in favour of not doing this. Consider an edit of a section that is moved on the same side as the edit - side A. That would be considered to be an insertion rather than an edit, because the original section still exists, albeit at a different location. If on the other side - side B - there is a move of that original section elsewhere (thus causing divergence), then even if it is the chosen move, the intent on side A was not to edit the moved scetion, so we should honour that in the resolution in favour of side B's move.
  4. If divergence is OK in itself, then a divergent insertion of the same section on both sides should also be tolerated. Again, it is worth reporting, but that's all. As this is an insertion and not a move, there can be no associated edit or deletion to worry about.
sageserpent-open commented 7 months ago

Produced a fairly hokey report of moves in 343f2a5b3a7324b0f295dee0d25d92e9aaccbcc5 - this has no automated tests for either the report generation or how instances of MoveDestinations are built up. Manual inspection of the report and of the logs shows some discrepancies, so it's worth the diligence of adding automated tests.

sageserpent-open commented 7 months ago

Another fly in the ointment is how coincident insertions / edits are reported - although the inserted sections coincide from the point of view of the merge, they are expected to lie in different locations in the left and right file. So we should show both sides of the insertion - but the API for merge.MergeAlgebra only takes the left hand side's element.

Should MergeAlgebra be cutover to take the element from both sides? Or should MatchesContext consult the implied left-right match to obtain the right hand side's element, avoiding a load of upstream rework? There is a precendent for sticking with just one element in MergeAlgebra, namely the preservation method which also takes just the left hand side's element - or should that be cutover too?

sageserpent-open commented 7 months ago

Bug in report generation fixed in 4af6bedadbd964f5ccd968678173715dd99980c2 - still missing automated tests for this, but I'm not satisfied with the design of this; need to think about what this should look like.

sageserpent-open commented 6 months ago

Bug reproduced by automated tests in: ea97c1965d6f449306d86bab49658bbf05c7ddd5 and the fix merged with the updated tests in: 99d6a81069d76bb059709de7d92ed53691c7c7a2.

sageserpent-open commented 6 months ago

Added support for showing both sides of a coincident insertion / edit in: 7a0d75d3f8f09c7d02e4092046243544240910ec. The report doesn't look stunning, but it will do for now. Feedback from the general public should guide further work on it.

For now, it's a wrap.

sageserpent-open commented 6 months ago

This went out in release 1.2.0, Git commit SHA: 4d1935beef2b5923417904895884de8c2e749c6e.