Conflicting merge decision with both addrange and removerange blows up UI

[itamarst]

The nbmerge-web UI TypeScript assumes removerange is only ever coupled with patch, but I've encountered case there's a conflicting local_then_remote merge decision with addrange and removerange for local and remote respectively:

Uncaught (in promise) Error: Invalid merge decision, cannot have null base for deleted cell: [object Object]
    at t.CellMergeModel.applyCellLevelDecision (cell.ts:495)
    at t.CellMergeModel.processDecisions (cell.ts:368)
    at new t.CellMergeModel (cell.ts:134)
    at t.NotebookMergeModel.buildCellList (notebook.ts:237)
    at new t.NotebookMergeModel (notebook.ts:92)
    at merge.ts:75
    at _ (merge.ts:165)

There are two possible approaches to solving this:

1. Fix the web UI so it can handle this pairing.
2. Change the merge algorithm so it doesn't output merge decisions that pair addrange and removerange.

I initially attempted the first approach, but the assumption that removerange is only ever coupled with patch seems to go pretty deep, so I ended up doing the second approach instead.

[itamarst]

@vidartf would love to some feedback / help fixing this, if you have a few minutes.

[vidartf]

@itamarst Hi, just got back from vacation. Do you have a base/local/remote triplet for reproduction? Either full notebooks or relevant parts. I want to trace the decisions all the way from the backend to the frontend to make sure I understand the problem fully. In general, the merge tool is less polished than the other parts, so I'm a little hesitant to fix the other parts based on the behavior of the mergetool unless I'm sure it is the right thing to do.

[itamarst]

You can find a triplet here: https://github.com/dotmesh-io/nbdime/files/3403248/notebooks.zip

[vidartf]

Thanks for that, I was able to reproduce.

WIP notes:

1. The decision to create merge decisions of the A/R type (addrange on one side and removerange on the other) has been in the code base for a long while, and I believe it is the correct behavior. It will e.g. give a clearer view with the inline merge resolution (editing the conflicted notebook directly).
2. I think the correct solution is to have splitCellChunks in the web merge pre-processing code split such decisions (keeping the conflict marker). Ideally, we would have some visual indication that the addition and the removal together form a conflict, but I don't think this is very important.

[itamarst]

I tried the UI-fixing approach and it wasn't clear quite how to make the change. Could you perhaps give me a sketch of the necessary code and I can take it from there?

[vidartf]

I did a first rough like this without checking anything. If you could try it out and test/modify as needed, that would be a great help!

@@ -313,6 +313,24 @@ function splitCellChunks(mergeDecisions: MergeDecision[]): MergeDecision[] {
           'remote', // Check for custom action first?
           md.conflict,
         ));
+      } else if (md.remoteDiff && md.localDiff) {
+        const ops = [md.remoteDiff[0].op, md.localDiff[0].op].sort();
+        if (ops.join(',') === 'addrange,removerange') {
+          // Insertion and deletions on the same index are simply split
+          // but both keep the conflict status
+
+          // Just do local first (alt. do add first)
+          let lmd = new MergeDecision(md);
+          lmd.localDiff = md.localDiff.slice();
+          lmd.remoteDiff = null;
+
+          let rmd = new MergeDecision(md);
+          rmd.localDiff = null;
+          rmd.remoteDiff = md.remoteDiff.slice();
+          output.push(rmd);
+        } else {
+          output.push(md);  // deepCopy?
+        }
       } else {
         output.push(md);  // deepCopy?
       }

[itamarst]

I will try to get to this in next week or two (we're using the PR internally as workaround, and have some urgent stuff to get done, but do want the real fix here in upstream).

[itamarst]

Thanks for the sketch!