sageserpent-open / kineticMerge

Merge a heavily refactored codebase and stay sane.
MIT License
9 stars 1 forks source link

Question: how to automatically resolve whitespace differences where feasible? #9

Open sageserpent-open opened 1 year ago

sageserpent-open commented 1 year ago

This is essentially duplicating what git merge can do with its various options:

That are passed to its ort strategy. Before proceeding with this, it would be nice to know exactly where in Git's merge plumbing this stuff gets used and how it could be applied to Kinetic Merge.

Given that code may move between files, just how sacred is the whitespace anyway? I'm not sure that duplicating exactly what Git does is the best option here.

sageserpent-open commented 8 months ago

What are Git's merge's rules for precedence when merging with whitespace changes when an option to ignore whitespace is in force?

OURS THEIRS OUTCOME
Whitespace changes Whitespace changes Our whitespace changes
Whitespace changes Non-trivial changes Their non-trivial changes
Non-trivial changes Whitespace changes Our non-trivial changes
No change Whitespace changes No change
Whitespace changes No change Our whitespace changes
No change Non-trivial changes Their non-trivial changes
Non-trivial changes No change Our non-trivial changes
Non-trivial changes Non-trivial changes Conflict
sageserpent-open commented 1 month ago

This came up in #30 when reviewing the results of MainTest.anEditAndADeletionPropagatingThroughAFileSplit - see anEditAndADeletionPropagatingThroughAFileSplit.

Need to review that in the context of the ticket. Speaking as a user, I would want to see the edit on the right in the tested merge make its way into the move destination contributed by the left, which isn't the case with the current dominance rule in the code (ca10050d36cbc7e956d287f6d905130d9decffa5).

Something to be mindful of - the example discussed in #30 is for a merge where our branch - the left is the one that has the condensation, their - the right branch has the whitespace edit.

This is the first test case executed by that test.

If the sense of merging is reversed, so the condensation is merged into the edits, then I'd expect a better looking result.

sageserpent-open commented 3 days ago

The behaviour we want in the last comment - namely to propagate the whitespace change on the right (theirs) to the move destination on the left (ours) goes against what Git specifies in the table above - well, assuming that table fairly summarizes what Git does.

Given the merge would look better with the desired behavour, let's go with it and not follow the table to the letter...

sageserpent-open commented 2 days ago

Scouting in the codebase, it seems that are at least two parts to this last piece of work, corresponding to two use-cases:

  1. Consider a simple edit of whitespace only in their branch - now we allow breaking strict compatibility with Git, this means the edit's changes should make their way into the merge result. This means in turn that the preservation code in the underlying merge algebra should sniff at the two alternative contributions and decide which one has whitespace differences wrt to the base contribution. If both do, then the left contribution is taken, which aligns with what Git does, as is also the case when just the left contribution has a whitespace change. If only the right contribution is different, then it goes through to the merge result. This is a bit icky, because this whole notion of whitespace is completely abstracted away by the time we get to the merge algebra. Also, what about the base contribution? It is not passed to the preservation operation - although this could be arranged.
  2. Consider an edit of whitespace only in their branch with a move of that section in our branch - we have to use the same logic as for the simple edit case, but for the destination's contribution versus their edit. Similarly for when the edit is on our side and the move is on theirs. So the leftDeletion code and rightDeletion code need to be cutover to find the destination and then do the required sniffing.

For the sake of centralising logic (and testing of that logic), I'm wondering whether to treat the first situation as being a kind of edit (because from the user's point of view, that's what it is) when just one side makes a whitespace edit wrt the base. If both or neither do, then it gets treated as a preservation. Then again, what about coincident edits? Do we do the same thing thing there? Or push it all down into the underlying merge algebra? Um, ah...

sageserpent-open commented 2 days ago

Musing ... consider using an abstract strategy to fuse the left and right contributions and pass this into the underlying merge algebra. Cutover MergeTest so that it double checks that the resulting merge actually uses the strategy by re-using the strategy in its expectations - or switching from a left-bias to a right-bias in different trials. This allows preservations and coincident edits to be handled in the usual fashion.

The code up in the merge algebra for MatchesContext needs to use the same strategy to build a edit to be migrated to the move destination when handling a deletion (specifically when one side has made a whitespace-only edit and the other has moved the section). The edit is the fusion of the move destination's section with the whitespace-edited section.

It would be nice to suppress any migrated whitespace-only edits when the fusion leads to the same result as the move destination. Could this be done with a simple equality check?

sageserpent-open commented 1 day ago

TODO: fix the logging message for preservations in merge.of - it currently refers to the left side only, this needs to change given this ticket...

sageserpent-open commented 1 day ago

As of b5188269cb933218c85f8dbd73235f1bd49ecd86, got support for the simple edit case without code motion.