fix: handle addition-only and deletion-only changes

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

@@ -13,6 +13,7 @@
 // limitations under the License.
 
 import {Hunk} from '../../../types';
+import {adjustHunkUp, adjustHunkDown} from '../../hunk-utils';
 
 interface PartitionedHunks {
   validHunks: Map<string, Hunk[]>;
@@ -24,6 +25,13 @@ interface PartitionedFileHunks {
   invalidFileHunks: Hunk[];
 }
 
+function hunkOverlaps(validHunk: Hunk, suggestedHunk: Hunk): boolean {
+  return (
+    suggestedHunk.oldStart >= validHunk.newStart &&
+    suggestedHunk.oldEnd <= validHunk.newEnd
+  );
+}
+
 function partitionFileHunks(
   pullRequestHunks: Hunk[],
   suggestedHunks: Hunk[]
@@ -40,16 +48,33 @@ function partitionFileHunks(
       candidateHunk = pullRequestHunks[i];
     }
     if (!candidateHunk) {
+      invalidFileHunks.push(suggestedHunk);
       return;
     }
+    // if deletion only or addition only
     if (
-      suggestedHunk.oldStart >= candidateHunk.newStart &&
-      suggestedHunk.oldEnd <= candidateHunk.newEnd
+      suggestedHunk.newEnd < suggestedHunk.newStart ||
+      suggestedHunk.oldEnd < suggestedHunk.oldStart
     ) {
+      // try using previous line
+      let adjustedHunk = adjustHunkUp(suggestedHunk);
+      if (adjustedHunk && hunkOverlaps(candidateHunk, adjustedHunk)) {
+        validFileHunks.push(adjustedHunk);
+        return;
+      }
+
+      // try using next line
+      adjustedHunk = adjustHunkDown(suggestedHunk);
+      if (adjustedHunk && hunkOverlaps(candidateHunk, adjustedHunk)) {
+        validFileHunks.push(adjustedHunk);
+        return;
+      }
+    } else if (hunkOverlaps(candidateHunk, suggestedHunk)) {
       validFileHunks.push(suggestedHunk);
-    } else {
-      invalidFileHunks.push(suggestedHunk);
+      return;
     }
+
+    invalidFileHunks.push(suggestedHunk);
   });
   return {validFileHunks, invalidFileHunks};
 }

src/github-handler/diff-utils.ts

@@ -50,14 +50,25 @@ export function parseAllHunks(diff: string): Map<string, Hunk[]> {
       let normalLines = 0;
       let changeSeen = false;
       const newLines: string[] = [];
+      let previousLine: string | null = null;
+      let nextLine: string | null = null;
 
       chunk.changes.forEach(change => {
+        // strip off leading '+', '-', or ' ' and trailing carriage return
+        const content = change.content.substring(1).replace(/[\n\r]+$/g, '');
         if (change.type === 'normal') {
           normalLines++;
+          if (changeSeen) {
+            if (nextLine === null) {
+              nextLine = content;
+            }
+          } else {
+            previousLine = content;
+          }
         } else {
           if (change.type === 'add') {
             // strip off leading '+' and trailing carriage return
-            newLines.push(change.content.substring(1).replace(/[\n\r]+$/g, ''));
+            newLines.push(content);
           }
           if (!changeSeen) {
             oldStart += normalLines;
@@ -68,13 +79,20 @@ export function parseAllHunks(diff: string): Map<string, Hunk[]> {
       });
       const newEnd = newStart + chunk.newLines - normalLines - 1;
       const oldEnd = oldStart + chunk.oldLines - normalLines - 1;
-      return {
+      let hunk: Hunk = {
         oldStart: oldStart,
         oldEnd: oldEnd,
         newStart: newStart,
         newEnd: newEnd,
         newContent: newLines,
       };
+      if (previousLine) {
+        hunk = {...hunk, previousLine: previousLine};
+      }
+      if (nextLine) {
+        hunk = {...hunk, nextLine: nextLine};
+      }
+      return hunk;
     });
     hunksByFile.set(filename, chunks);
   });

src/github-handler/hunk-utils.ts

@@ -0,0 +1,51 @@
+// 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 {Hunk} from '../types';
+
+/**
+ * Shift a Hunk up one line so it starts one line earlier.
+ * @param {Hunk} hunk
+ * @returns {Hunk | null} the adjusted Hunk or null if there is no preceeding line.
+ */
+export function adjustHunkUp(hunk: Hunk): Hunk | null {
+  if (!hunk.previousLine) {
+    return null;
+  }
+  return {
+    oldStart: hunk.oldStart - 1,
+    oldEnd: hunk.oldEnd,
+    newStart: hunk.newStart - 1,
+    newEnd: hunk.newEnd,
+    newContent: [hunk.previousLine, ...hunk.newContent],
+  };
+}
+
+/**
+ * Shift a Hunk up one line so it ends one line later.
+ * @param {Hunk} hunk
+ * @returns {Hunk | null} the adjusted Hunk or null if there is no following line.
+ */
+export function adjustHunkDown(hunk: Hunk): Hunk | null {
+  if (!hunk.nextLine) {
+    return null;
+  }
+  return {
+    oldStart: hunk.oldStart,
+    oldEnd: hunk.oldEnd + 1,
+    newStart: hunk.newStart,
+    newEnd: hunk.newEnd + 1,
+    newContent: hunk.newContent.concat(hunk.nextLine),
+  };
+}

src/types/index.ts

@@ -168,4 +168,6 @@ export interface Hunk {
   readonly newStart: number;
   readonly newEnd: number;
   readonly newContent: string[];
+  readonly previousLine?: string;
+  readonly nextLine?: string;
 }

test/diff-utils.ts

@@ -40,6 +40,8 @@ describe('parseAllHunks', () => {
         newStart: 5,
         newEnd: 5,
         newContent: ["  args: ['sleep', '301']"],
+        nextLine: "- name: 'ubuntu'",
+        previousLine: "- name: 'ubuntu'",
       },
     ]);
   });
@@ -57,6 +59,7 @@ describe('parseAllHunks', () => {
         newStart: 7,
         newEnd: 8,
         newContent: ["  args: ['foobar']", '  id: asdf'],
+        previousLine: "- name: 'ubuntu'",
       },
     ]);
   });
@@ -74,6 +77,7 @@ describe('parseAllHunks', () => {
         newStart: 5,
         newEnd: 6,
         newContent: ["  args: ['sleep', '30']", "  id: 'foobar'"],
+        previousLine: "- name: 'ubuntu'",
       },
     ]);
   });
@@ -91,6 +95,8 @@ describe('parseAllHunks', () => {
         newStart: 2,
         newEnd: 3,
         newContent: ["- name: 'foo'", "  args: ['sleep 1']"],
+        nextLine: "- name: 'ubuntu'",
+        previousLine: 'steps:',
       },
     ]);
   });
@@ -109,6 +115,8 @@ describe('parseAllHunks', () => {
         newStart: 2,
         newEnd: 2,
         newContent: ["- name: 'foo'"],
+        nextLine: "- name: 'ubuntu'",
+        previousLine: 'steps:',
       },
     ]);
   });
@@ -118,7 +126,32 @@ describe('parseAllHunks', () => {
     expect(allHunks.size).to.equal(1);
     const hunks = allHunks.get('cloudbuild.yaml');
     expect(hunks).to.eql([
-      {oldStart: 4, oldEnd: 5, newStart: 4, newEnd: 3, newContent: []},
+      {
+        oldStart: 4,
+        oldEnd: 5,
+        newStart: 4,
+        newEnd: 3,
+        newContent: [],
+        nextLine: "- name: 'ubuntu'",
+        previousLine: "  args: ['echo', 'foobar']",
+      },
+    ]);
+  });
+  it('parses additions', () => {
+    const diff = readFileSync(resolve(fixturePath, 'addition.diff')).toString();
+    const allHunks = parseAllHunks(diff);
+    expect(allHunks.size).to.equal(1);
+    const hunks = allHunks.get('cloudbuild.yaml');
+    expect(hunks).to.eql([
+      {
+        oldStart: 6,
+        oldEnd: 5,
+        newStart: 6,
+        newEnd: 6,
+        newContent: ["  id: 'added'"],
+        nextLine: "- name: 'ubuntu'",
+        previousLine: "  args: ['sleep', '30']",
+      },
     ]);
   });
 });

test/fixtures/diffs/addition.diff

@@ -0,0 +1,11 @@
+diff --git a/cloudbuild.yaml b/cloudbuild.yaml
+index cac8fbc..f4cbead 100644
+--- a/cloudbuild.yaml
++++ b/cloudbuild.yaml
+@@ -3,5 +3,6 @@ steps:
+   args: ['echo', 'foobar']
+ - name: 'ubuntu'
+   args: ['sleep', '30']
++  id: 'added'
+ - name: 'ubuntu'
+   args: ['echo', 'done']

test/fixtures/diffs/deletion.diff

@@ -9,4 +9,4 @@ index cac8fbc..bc796a0 100644
 -- name: 'ubuntu'
 -  args: ['sleep', '30']
  - name: 'ubuntu'
-   args: ['echo', 'done']
+   args: ['echo', 'done']

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

@@ -11,4 +11,4 @@ index cac8fbc..87f387c 100644
 +- name: 'foo'
 +  args: ['sleep 1']
  - name: 'ubuntu'
-   args: ['echo', 'done']
+   args: ['echo', 'done']

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

@@ -10,4 +10,4 @@ index cac8fbc..71fda1b 100644
 -  args: ['sleep', '30']
 +- name: 'foo'
  - name: 'ubuntu'
-   args: ['echo', 'done']
+   args: ['echo', 'done']

test/fixtures/diffs/one-line-to-many-newline.diff

@@ -9,4 +9,4 @@ index 07e5def..ca56e25 100644
 -  args: ['sleep', '30']
 \ No newline at end of file
 +  args: ['sleep', '30']
-+  id: 'foobar'
++  id: 'foobar'

test/hunk-utils.ts

@@ -0,0 +1,88 @@
+// 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 {setup} from './util';
+import {expect} from 'chai';
+import {adjustHunkUp, adjustHunkDown} from '../src/github-handler/hunk-utils';
+
+before(() => {
+  setup();
+});
+
+describe('adjustHunkUp', () => {
+  it('returns a new hunk if there is a previous line', () => {
+    const hunk = {
+      oldStart: 5,
+      oldEnd: 5,
+      newStart: 5,
+      newEnd: 5,
+      newContent: ["  args: ['sleep', '301']"],
+      nextLine: "- name: 'ubuntu'",
+      previousLine: "- name: 'ubuntu'",
+    };
+    const adjustedHunk = adjustHunkUp(hunk);
+    expect(adjustedHunk).to.eql({
+      oldStart: 4,
+      oldEnd: 5,
+      newStart: 4,
+      newEnd: 5,
+      newContent: ["- name: 'ubuntu'", "  args: ['sleep', '301']"],
+    });
+  });
+  it('returns null if there is no previous line', () => {
+    const hunk = {
+      oldStart: 5,
+      oldEnd: 5,
+      newStart: 5,
+      newEnd: 5,
+      newContent: ["  args: ['sleep', '301']"],
+    };
+    const adjustedHunk = adjustHunkUp(hunk);
+    expect(adjustedHunk).to.eql(null);
+  });
+});
+
+describe('adjustHunkDown', () => {
+  it('returns a new hunk if there is a next line', () => {
+    const hunk = {
+      oldStart: 5,
+      oldEnd: 5,
+      newStart: 5,
+      newEnd: 5,
+      newContent: ["  args: ['sleep', '301']"],
+      nextLine: "- name: 'ubuntu'",
+      previousLine: "- name: 'ubuntu'",
+    };
+    const adjustedHunk = adjustHunkDown(hunk);
+    expect(adjustedHunk).to.eql({
+      oldStart: 5,
+      oldEnd: 6,
+      newStart: 5,
+      newEnd: 6,
+      newContent: ["  args: ['sleep', '301']", "- name: 'ubuntu'"],
+    });
+  });
+  it('returns null if there is no previous line', () => {
+    const hunk = {
+      oldStart: 5,
+      oldEnd: 5,
+      newStart: 5,
+      newEnd: 5,
+      newContent: ["  args: ['sleep', '301']"],
+    };
+    const adjustedHunk = adjustHunkDown(hunk);
+    expect(adjustedHunk).to.eql(null);
+  });
+});