feat(review): allow raw diff for reviewPullRequest (#137)

* refactor: parseAllHunks from a diff

* feat: allow passing in raw diff string to reviewPullRequest

* refactor: use raw diff in CLI

Co-authored-by: Benjamin E. Coe <bencoe@google.com>

src/bin/handle-git-dir-change.ts

@@ -13,7 +13,7 @@
 // limitations under the License.
 
 import {execSync} from 'child_process';
-import {Changes, FileMode, FileData, FileDiffContent} from '../types';
+import {Changes, FileMode, FileData} from '../types';
 import {logger} from '../logger';
 import {readFile} from 'fs';
 import * as path from 'path';
@@ -137,10 +137,6 @@ export function getGitFileData(
   });
 }
 
-function getFileContentsAtHead(gitRootDir: string, filePath: string): string {
-  return execSync(`git show HEAD:${filePath}`, {cwd: gitRootDir}).toString();
-}
-
 /**
  * Get all the diffs using `git diff` of a git directory.
  * Errors if the git directory provided is not a git directory.
@@ -231,53 +227,21 @@ export function getChanges(dir: string): Promise<Changes> {
  * or if there is a git diff parse error
  * @param {string[]} diffs the git diff raw output (which only shows relative paths)
  * @param {string} gitDir the root of the local GitHub repository
- * @returns {Promise<Changes>} the changeset
+ * @returns {string} the diff
  */
-export async function parseDiffContents(
-  diffs: string[],
-  gitDir: string
-): Promise<Map<string, FileDiffContent>> {
-  try {
-    // get updated file contents
-    const changes: Map<string, FileDiffContent> = new Map();
-    const changePromises: Array<Promise<GitFileData>> = [];
-    for (let i = 0; i < diffs.length; i++) {
-      // TODO - handle memory constraint
-      changePromises.push(getGitFileData(gitDir, diffs[i]));
-    }
-    const gitFileDatas = await Promise.all(changePromises);
-    for (let i = 0; i < gitFileDatas.length; i++) {
-      const gitfileData = gitFileDatas[i];
-      const fileDiffContent: FileDiffContent = {
-        oldContent: getFileContentsAtHead(gitDir, gitfileData.path),
-        newContent: gitfileData.fileData.content!,
-      };
-      changes.set(gitfileData.path, fileDiffContent);
-    }
-    return changes;
-  } catch (err) {
-    logger.error('Error parsing git changes');
-    throw err;
-  }
-}
-
-/**
- * Get the git changes of the current project asynchronously.
- * Rejects if any of the files fails to load (if not deleted),
- * or if there is a git diff parse error
- * @param {string[]} diffs the git diff raw output (which only shows relative paths)
- * @param {string} gitDir the root of the local GitHub repository
- * @returns {Promise<Changes>} the changeset
- */
-export function getDiffContents(
-  dir: string
-): Promise<Map<string, FileDiffContent>> {
+export function getDiffString(dir: string): string {
   try {
     validateGitInstalled();
     const absoluteDir = resolvePath(dir);
     const gitRootDir = findRepoRoot(absoluteDir);
-    const diffs = getAllDiffs(gitRootDir);
-    return parseDiffContents(diffs, gitRootDir);
+    execSync('git add -A', {cwd: gitRootDir});
+    const diff = execSync('git diff --staged --no-renames', {
+      cwd: gitRootDir,
+    })
+      .toString() // strictly return buffer for mocking purposes. sinon ts doesn't infer {encoding: 'utf-8'}
+      .trimRight(); // remove the trailing new line
+    execSync('git reset .', {cwd: gitRootDir});
+    return diff;
   } catch (err) {
     if (!(err instanceof InstallationError)) {
       logger.error('Error loadng git changes.');

src/bin/workflow.ts

@@ -63,9 +63,7 @@ async function createCommand() {
 
 async function reviewCommand() {
   const reviewOptions = coerceUserCreateReviewRequestOptions();
-  const diffContents = await git.getDiffContents(
-    yargs.argv['git-dir'] as string
-  );
+  const diffContents = await git.getDiffString(yargs.argv['git-dir'] as string);
   const octokit = new Octokit({auth: process.env.ACCESS_TOKEN});
   await reviewPullRequest(octokit, diffContents, reviewOptions, logger);
 }

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

@@ -15,7 +15,7 @@
 import {Octokit} from '@octokit/rest';
 import {RepoDomain, Hunk} from '../../../types';
 import {logger} from '../../../logger';
-import {parseHunks} from '../../diff-utils';
+import {parsePatch} from '../../diff-utils';
 
 /**
  * For a pull request, get each remote file's patch text asynchronously
@@ -69,14 +69,6 @@ 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
-`;
-
 /**
  * For a pull request, get each remote file's current patch range to identify the scope of each patch as a Map.
  * @param {Octokit} octokit the authenticated octokit instance
@@ -114,7 +106,7 @@ export async function getPullRequestHunks(
         `File ${file.filename} may have a patch that is too large to display patch object.`
       );
     } else {
-      const hunks = parseHunks(_DIFF_HEADER + file.patch);
+      const hunks = parsePatch(file.patch);
       pullRequestHunks.set(file.filename, hunks);
     }
   });

src/github-handler/comment-handler/index.ts

@@ -19,6 +19,7 @@ import {logger} from '../../logger';
 import {getRawSuggestionHunks} from './raw-patch-handler/raw-hunk-handler';
 import {getPullRequestHunks} from './get-hunk-scope-handler/remote-patch-ranges-handler';
 import {partitionSuggestedHunksByScope} from './get-hunk-scope-handler/scope-handler';
+import {parseAllHunks} from '../diff-utils';
 
 /**
  * Comment on a Pull Request
@@ -34,7 +35,7 @@ export async function reviewPullRequest(
   remote: RepoDomain,
   pullNumber: number,
   pageSize: number,
-  diffContents: Map<string, FileDiffContent>
+  diffContents: Map<string, FileDiffContent> | string
 ): Promise<number | null> {
   try {
     // get the hunks from the pull request
@@ -46,7 +47,10 @@ export async function reviewPullRequest(
     );
 
     // get the hunks from the suggested change
-    const allSuggestedHunks = getRawSuggestionHunks(diffContents);
+    const allSuggestedHunks =
+      typeof diffContents === 'string'
+        ? parseAllHunks(diffContents)
+        : getRawSuggestionHunks(diffContents);
 
     // split hunks by commentable and uncommentable
     const {validHunks, invalidHunks} = partitionSuggestedHunksByScope(

src/github-handler/diff-utils.ts

@@ -16,46 +16,69 @@ import {Hunk} from '../types';
 import * as parseDiff from 'parse-diff';
 import {createPatch} from 'diff';
 
+// 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 a diff expressed in GNU diff format, return the range of lines
+ * Given a patch 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;
-    const newLines: string[] = [];
+export function parsePatch(patch: string): Hunk[] {
+  return parseAllHunks(_DIFF_HEADER + patch).get('file.ext') || [];
+}
 
-    chunk.changes.forEach(change => {
-      if (change.type === 'normal') {
-        normalLines++;
-      } else {
-        if (change.type === 'add') {
-          // strip off leading '+' and trailing carriage return
-          newLines.push(change.content.substring(1).replace(/[\n\r]+$/g, ''));
-        }
-        if (!changeSeen) {
-          oldStart += normalLines;
-          newStart += normalLines;
-          changeSeen = true;
+/**
+ * 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 Map<string, Hunk[]>
+ */
+export function parseAllHunks(diff: string): Map<string, Hunk[]> {
+  const hunksByFile: Map<string, Hunk[]> = new Map();
+  parseDiff(diff).forEach(file => {
+    const filename = file.to ? file.to : file.from!;
+    const chunks = file.chunks.map(chunk => {
+      let oldStart = chunk.oldStart;
+      let newStart = chunk.newStart;
+      let normalLines = 0;
+      let changeSeen = false;
+      const newLines: string[] = [];
+
+      chunk.changes.forEach(change => {
+        if (change.type === 'normal') {
+          normalLines++;
+        } else {
+          if (change.type === 'add') {
+            // strip off leading '+' and trailing carriage return
+            newLines.push(change.content.substring(1).replace(/[\n\r]+$/g, ''));
+          }
+          if (!changeSeen) {
+            oldStart += normalLines;
+            newStart += normalLines;
+            changeSeen = true;
+          }
         }
-      }
+      });
+      const newEnd = newStart + chunk.newLines - normalLines - 1;
+      const oldEnd = oldStart + chunk.oldLines - normalLines - 1;
+      return {
+        oldStart: oldStart,
+        oldEnd: oldEnd,
+        newStart: newStart,
+        newEnd: newEnd,
+        newContent: newLines,
+      };
     });
-    const newEnd = newStart + chunk.newLines - normalLines - 1;
-    const oldEnd = oldStart + chunk.oldLines - normalLines - 1;
-    return {
-      oldStart: oldStart,
-      oldEnd: oldEnd,
-      newStart: newStart,
-      newEnd: newEnd,
-      newContent: newLines,
-    };
+    hunksByFile.set(filename, chunks);
   });
+  return hunksByFile;
 }
 
 /**
@@ -69,5 +92,5 @@ export function getSuggestedHunks(
   newContent: string
 ): Hunk[] {
   const diff = createPatch('unused', oldContent, newContent);
-  return parseHunks(diff);
+  return parseAllHunks(diff).get('unused') || [];
 }

src/index.ts

@@ -52,7 +52,7 @@ import * as retry from 'async-retry';
  */
 export async function reviewPullRequest(
   octokit: Octokit,
-  diffContents: Map<string, FileDiffContent>,
+  diffContents: Map<string, FileDiffContent> | string,
   options: CreateReviewCommentUserOptions,
   loggerOption?: Logger | LoggerOptions
 ): Promise<number | null> {
@@ -62,7 +62,7 @@ export async function reviewPullRequest(
   if (
     diffContents === null ||
     diffContents === undefined ||
-    diffContents.size === 0
+    (typeof diffContents !== 'string' && diffContents.size === 0)
   ) {
     logger.info(
       'Empty changes provided. No suggestions to be made. Cancelling workflow.'

test/diff-utils.ts

@@ -16,7 +16,7 @@ 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 {parseAllHunks} from '../src/github-handler/diff-utils';
 import {expect} from 'chai';
 
 const fixturePath = 'test/fixtures/diffs';
@@ -25,12 +25,14 @@ before(() => {
   setup();
 });
 
-describe('parseHunks', () => {
+describe('parseAllHunks', () => {
   it('parses one-to-one hunks', () => {
     const diff = readFileSync(
       resolve(fixturePath, 'one-line-to-one.diff')
     ).toString();
-    const hunks = parseHunks(diff);
+    const allHunks = parseAllHunks(diff);
+    expect(allHunks.size).to.equal(1);
+    const hunks = allHunks.get('cloudbuild.yaml');
     expect(hunks).to.eql([
       {
         oldStart: 5,
@@ -45,7 +47,9 @@ describe('parseHunks', () => {
     const diff = readFileSync(
       resolve(fixturePath, 'one-line-to-many.diff')
     ).toString();
-    const hunks = parseHunks(diff);
+    const allHunks = parseAllHunks(diff);
+    expect(allHunks.size).to.equal(1);
+    const hunks = allHunks.get('cloudbuild.yaml');
     expect(hunks).to.eql([
       {
         oldStart: 7,
@@ -60,7 +64,9 @@ describe('parseHunks', () => {
     const diff = readFileSync(
       resolve(fixturePath, 'one-line-to-many-newline.diff')
     ).toString();
-    const hunks = parseHunks(diff);
+    const allHunks = parseAllHunks(diff);
+    expect(allHunks.size).to.equal(1);
+    const hunks = allHunks.get('cloudbuild.yaml');
     expect(hunks).to.eql([
       {
         oldStart: 5,
@@ -75,7 +81,9 @@ describe('parseHunks', () => {
     const diff = readFileSync(
       resolve(fixturePath, 'many-to-many.diff')
     ).toString();
-    const hunks = parseHunks(diff);
+    const allHunks = parseAllHunks(diff);
+    expect(allHunks.size).to.equal(1);
+    const hunks = allHunks.get('cloudbuild.yaml');
     expect(hunks).to.eql([
       {
         oldStart: 2,
@@ -91,7 +99,9 @@ describe('parseHunks', () => {
     const diff = readFileSync(
       resolve(fixturePath, 'many-to-one.diff')
     ).toString();
-    const hunks = parseHunks(diff);
+    const allHunks = parseAllHunks(diff);
+    expect(allHunks.size).to.equal(1);
+    const hunks = allHunks.get('cloudbuild.yaml');
     expect(hunks).to.eql([
       {
         oldStart: 2,
@@ -104,7 +114,9 @@ describe('parseHunks', () => {
   });
   it('parses deletions', () => {
     const diff = readFileSync(resolve(fixturePath, 'deletion.diff')).toString();
-    const hunks = parseHunks(diff);
+    const allHunks = parseAllHunks(diff);
+    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: []},
     ]);