fix: range parsing (#125)

* refactor: add diff-utils for diff parsing into Hunk[]

* refactor: use diff-utils to parse diff into Hunk, convert to Range

* refactor: use diff-utils for parsing FileDiffContent into Hunk[]

* chore: fix lint

* test: cleanup patch to range tests

* test: fix test cases

* chore: fix lint

Co-authored-by: Justin Beckwith <[email protected]>

package.json

@@ -46,6 +46,7 @@
     "async-retry": "^1.3.1",
     "diff": "^4.0.2",
     "glob": "^7.1.6",
+    "parse-diff": "^0.7.1",
     "pino": "^6.3.2",
     "yargs": "^16.0.0"
   },

src/github-handler/comment-handler/get-hunk-scope-handler/index.ts

@@ -13,4 +13,3 @@
 // limitations under the License.
 
 export {getPullRequestScope} from './remote-patch-ranges-handler';
-export {getGitHubPatchRanges} from './github-patch-text-handler';

src/github-handler/comment-handler/get-hunk-scope-handler/remote-patch-ranges-handler.ts

@@ -14,8 +14,8 @@
 
 import {Octokit} from '@octokit/rest';
 import {Range, RepoDomain} from '../../../types';
-import {getGitHubPatchRanges} from './github-patch-text-handler';
 import {logger} from '../../../logger';
+import {parseHunks} from '../../diff-utils';
 
 /**
  * For a pull request, get each remote file's patch text asynchronously
@@ -69,6 +69,14 @@ export async function getCurrentPullRequestPatches(
   return {patches, filesMissingPatch};
 }
 
+// This header is ignored for calculating patch ranges, but is neccessary
+// for parsing a diff
+const _DIFF_HEADER = `diff --git a/file.ext b/file.ext
+index cac8fbc..87f387c 100644
+--- a/file.ext
++++ b/file.ext
+`;
+
 /**
  * Given the patch text (for a whole file) for each file,
  * get each file's hunk's (part of a file's) range
@@ -82,7 +90,11 @@ export function patchTextToRanges(
   validPatches.forEach((patch, filename) => {
     // get each hunk range in the patch string
     try {
-      const validLineRanges = getGitHubPatchRanges(patch);
+      const validHunks = parseHunks(_DIFF_HEADER + patch);
+      // TODO: consider returning hunks instead
+      const validLineRanges = validHunks.map(hunk => {
+        return {start: hunk.newStart, end: hunk.newEnd};
+      });
       allValidLineRanges.set(filename, validLineRanges);
     } catch (err) {
       logger.info(

src/github-handler/comment-handler/raw-patch-handler/raw-hunk-handler.ts

@@ -14,34 +14,7 @@
 
 import {Hunk, FileDiffContent} from '../../../types';
 import {logger} from '../../../logger';
-import {structuredPatch, Hunk as RawHunk} from 'diff';
-
-/**
- * Given the old content and new content of a file
- * compute the diff and extract the necessary hunk data.
- * The hunk list is a list of disjoint ascending intervals.
- * @param fileDiffContent
- * @param fileName
- * @returns the hunks that are generated in ascending sorted order
- */
-export function generateHunks(
-  fileDiffContent: FileDiffContent,
-  fileName: string
-): Hunk[] {
-  const rawHunks: RawHunk[] = structuredPatch(
-    fileName, // old file name
-    fileName, // new file name
-    fileDiffContent.oldContent,
-    fileDiffContent.newContent
-  ).hunks;
-  const hunks: Hunk[] = rawHunks.map(rawHunk => ({
-    oldStart: rawHunk.oldStart,
-    oldEnd: rawHunk.oldStart + rawHunk.oldLines,
-    newStart: rawHunk.newStart,
-    newEnd: rawHunk.newStart + rawHunk.newLines,
-  }));
-  return hunks;
-}
+import {getSuggestedHunks} from '../../diff-utils';
 
 /**
  * Given a map where the key is the file name and the value is the
@@ -61,7 +34,10 @@ export function getRawSuggestionHunks(
     if (fileDiffContent.oldContent === fileDiffContent.newContent) {
       return;
     }
-    const hunks = generateHunks(fileDiffContent, fileName);
+    const hunks = getSuggestedHunks(
+      fileDiffContent.oldContent,
+      fileDiffContent.newContent
+    );
     fileHunks.set(fileName, hunks);
   });
   logger.info('Parsed ranges of old and new patch');

src/github-handler/diff-utils.ts

@@ -0,0 +1,71 @@
+// Copyright 2020 Google LLC
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     https://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+import {Hunk} from '../types';
+import * as parseDiff from 'parse-diff';
+import {createPatch} from 'diff';
+
+/**
+ * Given a diff expressed in GNU diff format, return the range of lines
+ * from the original content that are changed.
+ * @param diff Diff expressed in GNU diff format.
+ * @returns Hunk[]
+ */
+export function parseHunks(diff: string): Hunk[] {
+  const chunks = parseDiff(diff)[0].chunks;
+  return chunks.map(chunk => {
+    let oldStart = chunk.oldStart;
+    let newStart = chunk.newStart;
+    let normalLines = 0;
+    let changeSeen = false;
+
+    chunk.changes.forEach(change => {
+      switch (change.type) {
+        case 'add':
+        case 'del':
+          if (!changeSeen) {
+            oldStart += normalLines;
+            newStart += normalLines;
+            changeSeen = true;
+          }
+          break;
+        case 'normal':
+          normalLines++;
+          break;
+      }
+    });
+    const newEnd = newStart + chunk.newLines - normalLines - 1;
+    const oldEnd = oldStart + chunk.oldLines - normalLines - 1;
+    return {
+      oldStart: oldStart,
+      oldEnd: oldEnd,
+      newStart: newStart,
+      newEnd: newEnd,
+    };
+  });
+}
+
+/**
+ * Given two texts, return the range of lines that are changed.
+ * @param oldContent The original content.
+ * @param newContent The new content.
+ * @returns Hunk[]
+ */
+export function getSuggestedHunks(
+  oldContent: string,
+  newContent: string
+): Hunk[] {
+  const diff = createPatch('unused', oldContent, newContent);
+  return parseHunks(diff);
+}

test/diff-utils.ts

@@ -0,0 +1,70 @@
+// Copyright 2020 Google LLC
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//    http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+import {describe, it, before} from 'mocha';
+import {readFileSync} from 'fs';
+import {setup} from './util';
+import {resolve} from 'path';
+import {parseHunks} from '../src/github-handler/diff-utils';
+import {expect} from 'chai';
+
+const fixturePath = 'test/fixtures/diffs';
+
+before(() => {
+  setup();
+});
+
+describe('parseHunks', () => {
+  it('parses one-to-one hunks', () => {
+    const diff = readFileSync(
+      resolve(fixturePath, 'one-line-to-one.diff')
+    ).toString();
+    const hunks = parseHunks(diff);
+    expect(hunks).to.eql([{oldStart: 5, oldEnd: 5, newStart: 5, newEnd: 5}]);
+  });
+  it('parses one-to-many hunks', () => {
+    const diff = readFileSync(
+      resolve(fixturePath, 'one-line-to-many.diff')
+    ).toString();
+    const hunks = parseHunks(diff);
+    expect(hunks).to.eql([{oldStart: 7, oldEnd: 7, newStart: 7, newEnd: 8}]);
+  });
+  it('parses one-to-many hunks with a newline added', () => {
+    const diff = readFileSync(
+      resolve(fixturePath, 'one-line-to-many-newline.diff')
+    ).toString();
+    const hunks = parseHunks(diff);
+    expect(hunks).to.eql([{oldStart: 5, oldEnd: 5, newStart: 5, newEnd: 6}]);
+  });
+  it('parses many-to-many-hunks', () => {
+    const diff = readFileSync(
+      resolve(fixturePath, 'many-to-many.diff')
+    ).toString();
+    const hunks = parseHunks(diff);
+    expect(hunks).to.eql([{oldStart: 2, oldEnd: 5, newStart: 2, newEnd: 3}]);
+  });
+
+  it('parses many-to-one-hunks', () => {
+    const diff = readFileSync(
+      resolve(fixturePath, 'many-to-one.diff')
+    ).toString();
+    const hunks = parseHunks(diff);
+    expect(hunks).to.eql([{oldStart: 2, oldEnd: 5, newStart: 2, newEnd: 2}]);
+  });
+  it('parses deletions', () => {
+    const diff = readFileSync(resolve(fixturePath, 'deletion.diff')).toString();
+    const hunks = parseHunks(diff);
+    expect(hunks).to.eql([{oldStart: 4, oldEnd: 5, newStart: 4, newEnd: 3}]);
+  });
+});

test/fixtures/diffs/deletion.diff

@@ -0,0 +1,12 @@
+diff --git a/cloudbuild.yaml b/cloudbuild.yaml
+index cac8fbc..bc796a0 100644
+--- a/cloudbuild.yaml
++++ b/cloudbuild.yaml
+@@ -1,7 +1,5 @@
+ steps:
+ - name: 'ubuntu'
+   args: ['echo', 'foobar']
+-- name: 'ubuntu'
+-  args: ['sleep', '30']
+ - name: 'ubuntu'
+   args: ['echo', 'done']

test/fixtures/diffs/many-to-many.diff

@@ -0,0 +1,14 @@
+diff --git a/cloudbuild.yaml b/cloudbuild.yaml
+index cac8fbc..87f387c 100644
+--- a/cloudbuild.yaml
++++ b/cloudbuild.yaml
+@@ -1,7 +1,5 @@
+ steps:
+-- name: 'ubuntu'
+-  args: ['echo', 'foobar']
+-- name: 'ubuntu'
+-  args: ['sleep', '30']
++- name: 'foo'
++  args: ['sleep 1']
+ - name: 'ubuntu'
+   args: ['echo', 'done']