sageserpent-open / kineticMerge

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

Question: how should code motion of a section of code on one branch versus deletion on another be handled? #5

Open sageserpent-open opened 1 year ago

sageserpent-open commented 1 year ago

This is a generalisation of #4 to when it is just a section of code that is moved or deleted as opposed to an entire file.

An obvious and desirable outcome would be to treat this as an ordinary conflict, where the code either exists in in its new location (could be in the same file, or buried in another file, or maybe in its own new file) - or does not.

If the code is moved around in the same file or into an existing file, then because it either does this or doesn't exist at all, then we can mark this as a kind of conflict where the code is on one side of the conflict and nothing on the other side. Where I'm a little hesitant here is whether Git and your typical post-merge clean up tool (such as IntelliJ's merge resolution) can handle a conflict where 'our' side has code, 'their' side is empty and the common ancestor is empty too. Need to check that this is feasible, I know that a straight deletion of common ancestor code versus an edit is easily represented as a conflict and is handled by the likes of IntelliJ.

The third case where the code is moved to its own new file as reminiscent of #4 and should probably be handled as such.

sageserpent-open commented 1 year ago

Experimented with this by doing a simple Git merge of a file that is unchanged on our branch, but has a line added in the middle on their branch, then doing git merge --no-ff -no-commit and the manually stuffing the staging area so that stage 0 is removed, stages 1 and 2 refer to the SHA of the file as of the base commit and stage 3 refers to the blob of the uncommitted file's blob. The actual file in the working directory is edited with a synthetic conflict marker, so all told this looks like a Git conflict with our side not having made any changes and their side having added a new line.

Git thinks the file is conflicted, as hoped - in fact even if the file in the working directory is deleted, it is still happy and reports a conflict. git diff also shows the synthetic conflict.

IntelliJ is bit more wily and demands that at very least there is a file at the correct path in the working directory - however, like Git it uses the stages as the source of truth about the merge, so if the conflict marker is completely malformed (or even if the file is empty), IntelliJ will show its usual three-way diff conflict resolution window.

So far, so good.

The wrinkle is that IntelliJ is too intelligent - it spots the absence of any genuine conflict and reports the added line is an ordinary diff contributed from their branch only - which is actually the case here.

So yes, Kinetic Merge can represent the code motion versus deletion as a conflict, but IntelliJ will simply treat it is something that can be trivially merged. Most people will probably execute the auto-resolve action prior to dealing with leftover conflicts, so the net result will be to favour code motion over deletion.

Given that the synthetic not-quite-a-real-conflict will often be mixed in with genuine changes from one branch or another, it is obvious that if IntelliJ is used, the user probably won't be made aware of the conflict in this case!

sageserpent-open commented 1 year ago

One approach is to accept this as a necessary evil - at least the merge conflict is reported, even though the markers are ignored and the conflict optimised away by IntelliJ.

Perhaps a synthetic section could be added to represent the deletion, thus forcing the user to have to think about resolving the conflict manually? Conflict markers already mess up the merged code, so one could have something like:

<<<<<<
SECTION_DELETED_IN_ORIGINAL_LOCATION
======
Code that was moved...
>>>>>>

No, this won't work as IntelliJ is reading from the blobs, and we don't want these to have anything other than 'real' code in them.

sageserpent-open commented 1 year ago

For now, accept this as wrinkle - so if a code section is both deleted and moved, then any good merging tool will likely cleanly resolve this in favour of the move.

sageserpent-open commented 1 year ago

After doing the groundwork for #11 , now that it obvious that Git will allow synthetic blobs to be used for staging entries, then we can fudge the marker workaround to represent the deletion - so we add SECTION_DELETED_IN_ORIGINAL_LOCATION into the blob representing the content that doesn't have the section moved into it. Because this blob is synthetic, we aren't actually clobbering any history, and the staged entries will be forgotten once the merge is completed or aborted.

sageserpent-open commented 1 year ago

On reflection after looking at #6 again, why can't this be treated as a straight divergence, analogous to #4?

This time the divergence syntax would be something like DELETED <--- <file>, <line> [- <line>] ---> <file>, <line> [- <line>].

This way, the user is forced to think about the deletion prior to the handover to conflict resolution, just as they would have to for a move-versus-move divergence.

sageserpent-open commented 1 year ago

My mood today is to favour upfront resolution as a divergence.

Not only is it more obvious what is happening, there is also the danger that in going down the marker route, the user may be misled into thinking that accepting, say, our side of

<<<<<<
SECTION_DELETED_IN_ORIGINAL_LOCATION
======
Code that was moved...
>>>>>>

will automagically propagate the deletion - whereas they will be surprised to see SECTION_DELETED_IN_ORIGINAL_LOCATION appearing in their resolved merged file, and will gripe on a regular basis.

Of course, such a divergence can only be resolved by ml or mr - s doesn't make any sense here. In fact, the command prefix m doesn't seem so appropriate here, at least for the deletion side. Hmm...

sageserpent-open commented 1 year ago

Given the entry in the command table for ml/mr in #6 is to accept the change, maybe it should be changed to al/ar, and the existing a for aborting the merge should become q for quitting the merge subject to a confirmation that there may be unresolved divergences (thus implying the merge must be rolled back to quit).

Let's do that... (Updated #6 accordingly).

sageserpent-open commented 4 months ago

Re-reading this, if a section is moved on one side and deleted on the other, isn't this just an extreme example of propagating an edit of a moved section to the destination? So the outcome is simply to propagate the deletion of the section.

There is a twist - if a section moves on one side, but is deleted on the other and it's original containing file still exists, then this naturally can be regarded as extreme editing, thus the deletion is propagated.

If on the other hand the section moves on one side and it's original containing file is deleted, this feels like a divergent move if the section moves out of the file on the first side, and would probably be a simple file deletion if it moves around in the original file on the first side.

sageserpent-open commented 1 month ago

We have tests for a section moving on one side and being deleted on another - the deletion is migrated (propagated in the older terminology).

I don't think there is any test for a section moving out of a file on one side and the entire file being deleted on the other - this is actually a twist on #4...