Merge pull request #2007 from atom/tt-19-mar-destructure-bug

Fix bug expanding large-diff-gated patches with comments

Author: Tilde Ann Thurium

lib/models/patch/multi-file-patch.js

@@ -24,21 +24,7 @@ export default class MultiFilePatch {
       this.filePatchesByPath.set(filePatch.getPath(), filePatch);
       this.filePatchesByMarker.set(filePatch.getMarker(), filePatch);
 
-      let diffRow = 1;
-      const index = new RBTree((a, b) => a.diffRow - b.diffRow);
-      this.diffRowOffsetIndices.set(filePatch.getPath(), {startBufferRow: filePatch.getStartRange().start.row, index});
-
-      for (let hunkIndex = 0; hunkIndex < filePatch.getHunks().length; hunkIndex++) {
-        const hunk = filePatch.getHunks()[hunkIndex];
-        this.hunksByMarker.set(hunk.getMarker(), hunk);
-
-        // Advance past the hunk body
-        diffRow += hunk.bufferRowCount();
-        index.insert({diffRow, offset: hunkIndex + 1});
-
-        // Advance past the next hunk header
-        diffRow++;
-      }
+      this.populateDiffRowOffsetIndices(filePatch);
     }
   }
 
@@ -210,6 +196,28 @@ export default class MultiFilePatch {
     return Range.fromObject([[selectionRow, 0], [selectionRow, Infinity]]);
   }
 
+  isDiffRowOffsetIndexEmpty(filePatchPath) {
+    const diffRowOffsetIndex = this.diffRowOffsetIndices.get(filePatchPath);
+    return diffRowOffsetIndex.index.size === 0;
+  }
+  populateDiffRowOffsetIndices(filePatch) {
+    let diffRow = 1;
+    const index = new RBTree((a, b) => a.diffRow - b.diffRow);
+    this.diffRowOffsetIndices.set(filePatch.getPath(), {startBufferRow: filePatch.getStartRange().start.row, index});
+
+    for (let hunkIndex = 0; hunkIndex < filePatch.getHunks().length; hunkIndex++) {
+      const hunk = filePatch.getHunks()[hunkIndex];
+      this.hunksByMarker.set(hunk.getMarker(), hunk);
+
+      // Advance past the hunk body
+      diffRow += hunk.bufferRowCount();
+      index.insert({diffRow, offset: hunkIndex + 1});
+
+      // Advance past the next hunk header
+      diffRow++;
+    }
+  }
+
   adoptBuffer(nextPatchBuffer) {
     nextPatchBuffer.clearAllLayers();
 
@@ -332,6 +340,12 @@ export default class MultiFilePatch {
     for (const hunk of filePatch.getHunks()) {
       this.hunksByMarker.set(hunk.getMarker(), hunk);
     }
+
+    // if the patch was initially collapsed, we need to calculate
+    // the diffRowOffsetIndices to calculate comment position.
+    if (this.isDiffRowOffsetIndexEmpty(filePatch.getPath())) {
+      this.populateDiffRowOffsetIndices(filePatch);
+    }
   }
 
   getMarkersBefore(filePatchIndex) {

test/models/patch/multi-file-patch.test.js

@@ -780,6 +780,41 @@ describe('MultiFilePatch', function() {
       assert.strictEqual(multiFilePatch.getBufferRowForDiffPosition('2.txt', 13), 30);
       assert.strictEqual(multiFilePatch.getBufferRowForDiffPosition('2.txt', 15), 32);
     });
+
+    it('set the offset for diff-gated file patch upon expanding', function() {
+      const {multiFilePatch} = multiFilePatchBuilder()
+        .addFilePatch(fp => {
+          fp.setOldFile(f => f.path('1.txt'));
+          fp.addHunk(h => h.unchanged('0 (1)').added('1 (2)', '2 (3)').deleted('3 (4)').unchanged('4 (5)'));
+          fp.addHunk(h => h.unchanged('5 (7)').added('6 (8)', '7 (9)', '8 (10)').unchanged('9 (11)'));
+          fp.addHunk(h => h.unchanged('10 (13)').deleted('11 (14)').unchanged('12 (15)'));
+          fp.renderStatus(TOO_LARGE);
+        })
+        .build();
+      assert.isTrue(multiFilePatch.isDiffRowOffsetIndexEmpty('1.txt'));
+      const [fp] = multiFilePatch.getFilePatches();
+      multiFilePatch.expandFilePatch(fp);
+      assert.isFalse(multiFilePatch.isDiffRowOffsetIndexEmpty('1.txt'));
+      assert.strictEqual(multiFilePatch.getBufferRowForDiffPosition('1.txt', 11), 9);
+    });
+
+    it('does not reset the offset for normally collapsed file patch upon expanding', function() {
+      const {multiFilePatch} = multiFilePatchBuilder()
+        .addFilePatch(fp => {
+          fp.setOldFile(f => f.path('0.txt'));
+          fp.addHunk(h => h.oldRow(1).unchanged('0-0').deleted('0-1', '0-2').unchanged('0-3'));
+        })
+        .build();
+
+      const [fp] = multiFilePatch.getFilePatches();
+      const stub = sinon.stub(multiFilePatch, 'populateDiffRowOffsetIndices');
+
+      multiFilePatch.collapseFilePatch(fp);
+      assert.strictEqual(multiFilePatch.getBuffer().getText(), '');
+
+      multiFilePatch.expandFilePatch(fp);
+      assert.isFalse(stub.called);
+    });
   });
 
   describe('collapsing and expanding file patches', function() {