Merge pull request #546 from vidartf/merge-fixes

Merge fixes

nbdime/merging/strategies.py

@@ -429,7 +429,7 @@ def resolve_strategy_inline_outputs(base_path, outputs, decisions):
             # Replace all decisions affecting key with resolution
             local_diff, remote_diff = collect_diffs(base_path, decs)
             inlined_conflict, keep_base = make_inline_output_conflict(
-                outputs[key], local_diff, remote_diff)
+                outputs[key] if outputs else None, local_diff, remote_diff)
             custom_diff = []
             custom_diff += [op_addrange(key, inlined_conflict)]
             if not keep_base:

nbdime/tests/test_merge_notebooks_inline.py

@@ -1053,6 +1053,28 @@ def test_inline_merge_outputs():
     assert merged == expected
 
 
+def test_inline_merge_outputs_conflicting_insert_in_empty():
+    # One cell with two outputs:
+    base = outputs_to_notebook([[]])
+    local = outputs_to_notebook([['local']])
+    remote = outputs_to_notebook([['remote']])
+    expected = outputs_to_notebook([[
+        nbformat.v4.new_output(
+            output_type='stream', name='stderr',
+            text='<<<<<<< local\n'),
+        'local',
+        nbformat.v4.new_output(
+            output_type='stream', name='stderr',
+            text='=======\n'),
+        'remote',
+        nbformat.v4.new_output(
+            output_type='stream', name='stderr',
+            text='>>>>>>> remote\n'),
+    ]])
+    merged, decisions = merge_notebooks(base, local, remote)
+    assert merged == expected
+
+
 def test_inline_merge_cells_insertion_similar():
     base = sources_to_notebook([['unmodified']], cell_type='markdown')
     local = sources_to_notebook([['unmodified'], ['local']], cell_type='markdown')

packages/nbdime/src/diff/diffentries.ts

@@ -195,7 +195,8 @@ function opPatch(key: string | number, diff: IDiffEntry[] | null): IDiffPatch {
 export
 function validateSequenceOp(base: ReadonlyArray<any> | string, entry: IDiffEntry): void {
   if (typeof entry.key !== 'number') {
-      throw new TypeError('Invalid patch sequence op: Key is not a number: ' + entry.key);
+    console.info('Invalid patch details', base, entry);
+    throw new TypeError(`Invalid patch sequence op: Key is not a number: ${entry.key}`);
   }
   let index = entry.key;
   if (entry.op === 'addrange') {
@@ -226,7 +227,8 @@ export
 function validateObjectOp(base: any, entry: IDiffEntry, keys: string[]): void {
   let op = entry.op;
   if (typeof entry.key !== 'string') {
-      throw new TypeError('Invalid patch object op: Key is not a string: ' + entry.key);
+    console.info('Invalid patch details', base, entry, keys);
+    throw new TypeError(`Invalid patch object op: Key is not a string: ${entry.key}`);
   }
   let key = entry.key;
 

packages/nbdime/src/merge/decisions.ts

@@ -22,7 +22,7 @@ import {
 } from '../patch';
 
 import {
-  deepCopy, valueIn, isPrefixArray, findSharedPrefix, splitLines
+  arraysEqual, deepCopy, valueIn, isPrefixArray, findSharedPrefix, splitLines
 } from '../common/util';
 
 export
@@ -534,7 +534,7 @@ export function applyDecisions(base: any, decisions: MergeDecision[]): any {
     let path = spl[0];
     let line = spl[1];
     // We patch all decisions with the same path in one op
-    if (path === prevPath) {
+    if (arraysEqual(path, prevPath)) {
       if (clearParent) {
         // Another entry will clear the parent, so all other decisions
         // should be dropped
@@ -547,7 +547,7 @@ export function applyDecisions(base: any, decisions: MergeDecision[]): any {
         if (line) {
           ad = pushPath(ad, line);
         }
-        diffs = diffs.concat();
+        diffs = diffs.concat(ad);
       }
     } else {
       // Different path, start a new collection

packages/nbdime/test/src/merge/decisions.spec.ts

@@ -632,6 +632,47 @@ describe('merge', () => {
           baseObject.source + 'line 4\n');
       });
 
+      it('should handle multiple decisions with shared path', () => {
+        let decs = [
+          new decisions.MergeDecision({
+            local_diff: [opAddRange(0, ['top '])],
+            remote_diff: [opAddRange(0, ['not '])],
+            common_path: ['metadata', 'secret']
+          }),
+          new decisions.MergeDecision({
+            local_diff: [opAdd('foo', true)],
+            remote_diff: [opAdd('foo', true)],
+            common_path: ['metadata']
+          }),
+          new decisions.MergeDecision({
+            local_diff: [opRemoveRange(0, 1)],
+            common_path: ['seq']
+          }),
+          new decisions.MergeDecision({
+            local_diff: [opRemoveRange(1, 1)],
+            remote_diff: [opRemoveRange(1, 1)],
+            common_path: ['seq']
+          }),
+          new decisions.MergeDecision({
+            local_diff: [opAdd('bar', 43)],
+            remote_diff: [opAdd('bar', 12)],
+            common_path: ['metadata']
+          }),
+        ];
+        decs[0].action = 'local';
+        decs[1].action = 'either';
+        decs[2].action = 'local';
+        decs[3].action = 'either';
+        decs[4].action = 'local';
+        let value = decisions.applyDecisions({...baseObject, seq: ['foo', 'bar']}, decs);
+        expect(value.metadata).to.eql({
+          foo: true,
+          bar: 43,
+          secret: 'top foo!',
+        });
+        expect(value.seq).to.eql([]);
+      });
+
     });
 
   });