angular · mhevery · Jun 22, 2018 · Jun 22, 2018 · Jun 22, 2018 · Jun 22, 2018
diff --git a/docs/PR_REVIEW.md b/docs/PR_REVIEW.md
@@ -0,0 +1,155 @@
+# PR Review
+
+## Tools
+
+A better way to do a code-review of a PR is to do it in your IDE.
+Here are two scripts which allow you to perform the review and create local changes which can be appended to the PR.
+
+### 1. Loading PR
+
+Run this command to load the changes into your local repository where your IDE is running.
+
+```
+$ ./scripts/github/review-pr 24623
+```
+
+This will result in output:
+
+```
+Already on 'master'
+Your branch is up to date with 'origin/master'.
+Fetching pull request #24623 with 1 SHA(s) into branch range: pr/24623_base..pr/24623_top
+======================================================================================
+cef93a51b (pr/24623_top) ci: scripts to review PRs locally
+======================================================================================
+Switched to a new branch 'pr/24623'
+On branch pr/24623
+Untracked files:
+  (use "git add <file>..." to include in what will be committed)
+
+        docs/PR_REVIEW.md
+        scripts/github/push-pr
+        scripts/github/review-pr
+
+nothing added to commit but untracked files present (use "git add" to track)
+```
+
+Note that the script created `pr/24623_top` and `pr/24623_base` branches which denote SHAs where the PR start and end.
+
+```
+cef93a51b (pr/24623_top) ci: scripts to review PRs locally
+637805a0c (pr/24623_base) docs: update `lowercase` pipe example in "AngularJS to Angular" guide (#24588)
+```
+
+Knowing `pr/24623_top` and `pr/24623_base` makes it convenient to refer to different SHAs in PR when rebasing or reseting. 
+
+### 2. Review PR
+
+Because the script has reset the `HEAD` of the PR the changes show up as unstaged files.
+
+```
+$ git status
+On branch pr/24623
+Untracked files:
+  (use "git add <file>..." to include in what will be committed)
+
+        docs/PR_REVIEW.md
+        scripts/github/push-pr
+        scripts/github/review-pr
+
+nothing added to commit but untracked files present (use "git add" to track)
+```
+
+Use your IDE to review the untracked files as needed.
+A good trick is to use your IDE to stage the files which were already reviewed.
+When all files are staged the review is done.
+
+### 3. Creating Edits
+
+At any point you can edit any line in the repository.
+The idea is to create edits locally and push them to the PR later.
+This is useful because it is often times easier to make minor changes locally than to request the PR author to change and repush through a comment (often times the comment is larger than the change.)
+
+Example of a local edit.
+```
+echo "# here is a change" >> docs/PR_REVIEW.md
+```
+
+### 4. Creating a Commit From Local Edits
+
+Since the HEAD has been reset to `pr/24623_base` so that changes show up in `git status` we have to reverse the reset to only see our local changes.
+To do that reset the `HEAD` to `pr/24623_top`.
+
+```
+$ git reset pr/24623_top
+```
+
+Doing so will remove all PR changes and only leave your local modifications which you have done.
+You can verify by running `git status` and `git diff` to see only your changes (PR changes have been removed.)
+
+```
+$ git status
+On branch pr/24623
+Changes not staged for commit:
+  (use "git add <file>..." to update what will be committed)
+  (use "git checkout -- <file>..." to discard changes in working directory)
+
+        modified:   docs/PR_REVIEW.md
+
+no changes added to commit (use "git add" and/or "git commit -a")
+```
+```
+$ git diff
+diff --git a/docs/PR_REVIEW.md b/docs/PR_REVIEW.md
+index 184b5aeca..83517fbe0 100644
+--- a/docs/PR_REVIEW.md
++++ b/docs/PR_REVIEW.md
+@@ -8,4 +8,4 @@ A better way to do code review of the PR is to do it in your IDE. Here are two s
+existing text
+-
+\ No newline at end of file
++# here is a change
+```
+
+Next step is to turn your local changes into a `fixup!` commit.
+Run `git commit --all --fixup HEAD` to create a `fixup!` commit.
+
+NOTE: If you added new files they must be added using `git add .` or they will not be picked up by the `git commit --all` flag.
+
+```
+$ git commit --all --fixup HEAD
+[pr/24623 45ae87ce4] fixup! ci: scripts to review PRs locally
+ 1 file changed, 1 insertion(+), 1 deletion(-)
+```
+
+You can verify that the `fixup!` commit with your local modifications was created.
+```
+$ git log --oneline
+45ae87ce4 (HEAD -> pr/24623) fixup! ci: scripts to review PRs locally
+cef93a51b (pr/24623_top) ci: scripts to review PRs locally
+```
+
+### 5. Pushing local edits back to the PR
+
+The last step is to push your local changes back into the PR. 
+Use `./scripts/github/push-pr` script for that.
+
+```
+$ ./scripts/github/push-pr
+Assuming PR #24623
+>>> git push  [email protected]:mhevery/angular.git HEAD:review_pr_script
+Counting objects: 4, done.
+Delta compression using up to 8 threads.
+Compressing objects: 100% (4/4), done.
+Writing objects: 100% (4/4), 392 bytes | 392.00 KiB/s, done.
+Total 4 (delta 3), reused 0 (delta 0)
+remote: Resolving deltas: 100% (3/3), completed with 3 local objects.
+To github.com:mhevery/angular.git
+   cef93a51b..45ae87ce4  HEAD -> review_pr_script
+```
+
+NOTE: Notice that we did not have to specify the PR number since the script can guess it from the branch name.
+
+If you visit https://github.com/angular/angular/pull/24623/commits you will see that your `fixup!` commit has been added to the PR.
+This greatly simplifies the work for many minor changes to the PR. 
+
diff --git a/scripts/github/push-pr b/scripts/github/push-pr
@@ -0,0 +1,56 @@
+#!/usr/bin/env node
+
+const shell = require('shelljs');
+shell.config.fatal = true;
+const util = require('./utils/git_util');
+
+if (require.main === module) {
+  main(process.argv.splice(2)).then(
+    (v) => process.exitCode, 
+    (e) => console.error(process.exitCode = 1, e) 
+  );
+}
+
+async function main(args) {
+  let forceWithLease = '';
+  let prNumber = 0;
+  let printHelp = false;
+
+  args.forEach((arg) => {
+    if (prNumber == 0 && arg > 0) {
+      prNumber = arg;
+    } else if (arg == '--help') {
+      printHelp = true;
+    } else if (arg == '--force-with-lease') {
+      forceWithLease = ' --force-with-lease';
+    } else {
+      shell.echo('Unexpected argument: ', arg);
+    }
+  });
+
+  if (!prNumber) {
+    const branch = util.getCurrentBranch();
+    const maybePr = branch.split('/')[1];
+    if (maybePr > 0) {
+      shell.echo(`PR number not specified. Defaulting to #${maybePr}.`);
+      prNumber = maybePr;
+    }
+  }
+
+  if (!prNumber || printHelp) {
+    shell.echo(`Push the current HEAD into an existing pull request.`);
+    shell.echo(``);
+    shell.echo(`${process.argv[1]} [PR_NUMBER] [--force-with-lease]`);
+    shell.echo(``);
+    shell.echo(`    --force-with-lease    Continues even \if change can\'t be fast-forwarded.`);
+    shell.echo(`    [PR_NUMBER]           If not present the script guesses the PR from the branch name.`);
+    return 1;
+  }
+
+  const prInfo = await util.githubPrInfo(prNumber);
+  const prPushCmd = `git push${forceWithLease} ${prInfo.repository.gitUrl} HEAD:${prInfo.branch}`;
+  shell.echo(`>>> ${prPushCmd}`);
+  shell.exec(prPushCmd);
+
+  return 0;
+ }
diff --git a/scripts/github/review-pr b/scripts/github/review-pr
@@ -0,0 +1,60 @@
+#!/usr/bin/env node
+
+const shell = require('shelljs');
+shell.config.fatal = true;
+const util = require('./utils/git_util');
+
+if (require.main === module) {
+  main(process.argv.splice(2)).then(
+    (v) => process.exitCode = v, 
+    (e) => console.error(process.exitCode = 1, e)
+  );
+}
+
+async function main(args) {
+  let prNumber = 0;
+
+  args.forEach((arg) => {
+    if (prNumber == 0 && arg > 0) {
+        prNumber = arg;
+    } else {
+        shell.echo('Unexpected argument: ', arg);
+    }
+  });
+
+  if (prNumber === 0) {
+    shell.echo('Bring github pull request onto your local repo for review and edit');
+    shell.echo('');
+    shell.echo(`${process.argv[1]} PR_NUMBER`);
+    shell.echo('');
+    return 1;
+  }
+
+  if (util.gitHasLocalModifications()) {
+    shell.echo('Local modification detected. exiting...');
+    return 1;
+  }
+
+  let prShaCount = (await util.githubPrInfo(prNumber)).commits;
+
+  shell.exec(`git checkout master`);
+  if (util.execNoFatal(`git rev-parse --verify --quiet pr/${prNumber}`).code == 0) {
+    shell.exec(`git branch -D pr/${prNumber}`);
+  }
+
+  shell.echo(`Fetching pull request #${prNumber} with ${prNumber} SHA(s) into branch range: pr/${prNumber}_base..pr/${prNumber}_top`);
+  shell.exec(`git fetch -f [email protected]:angular/angular.git pull/${prNumber}/head:pr/${prNumber}_top`);
+
+  shell.exec(`git branch -f pr/${prNumber}_base pr/${prNumber}_top~${prShaCount}`);
+
+  shell.echo(`======================================================================================`);
+  shell.exec(`git log --oneline --color pr/${prNumber}_base..pr/${prNumber}_top`);
+  shell.echo(`======================================================================================`);
+
+  // Reset the HEAD so that we can see changed files for review
+  shell.exec(`git checkout --force -b pr/${prNumber} pr/${prNumber}_top`);
+  shell.exec(`git reset pr/${prNumber}_base`);
+  shell.exec(`git status`);
+
+  return 0;
+}
diff --git a/scripts/github/utils/git_util.js b/scripts/github/utils/git_util.js
@@ -0,0 +1,95 @@
+/**
+ * @license
+ * Copyright Google Inc. All Rights Reserved.
+ *
+ * Use of this source code is governed by an MIT-style license that can be
+ * found in the LICENSE file at https://angular.io/license
+ */
+
+const https = require('https');
+const shell = require('shelljs');
+
+function httpGet(server, path, headers) {
+  return new Promise((resolve, reject) => {
+    const options = {
+      hostname: server,
+      port: 443,
+      path: path,
+      method: 'GET',
+      headers: {'User-Agent': 'script', ...headers}
+    };
+    https
+        .get(
+            options,
+            (res) => {
+              let json = '';
+              res.on('data', (chunk) => json += chunk.toString());
+              res.on('end', () => resolve(json));
+            })
+        .on('error', (e) => reject(e));
+  });
+};
+
+let warnNoToken = true;
+
+async function githubGet(path) {
+  const token = process.env['TOKEN'];
+  const headers = {};
+  if (token) {
+    headers.Authorization = 'token ' + token;
+  } else if (warnNoToken) {
+    warnNoToken = false;
+    console.warn('############################################################');
+    console.warn('############################################################');
+    console.warn('WARNING: you should set the TOKEN variable to a github token');
+    console.warn('############################################################');
+    console.warn('############################################################');
+  }
+
+  return JSON.parse(await httpGet('api.github.com', '/repos/angular/angular/' + path, headers));
+};
+
+async function githubPrInfo(prNumber) {
+  const pr = (await githubGet('pulls/' + prNumber));
+  const label = pr.head.label.split(':');
+  const user = label[0];
+  const branch = label[1];
+  return {
+    commits: pr.commits,
+    repository: {
+      user: user,
+      gitUrl: `[email protected]:${user}/angular.git`,
+    },
+    branch: branch
+  };
+};  // trailing ; so that clang-format is not confused on async function
+
+function gitHasLocalModifications() {
+  return execNoFatal('git diff-index --quiet HEAD --').code != 0;
+}
+
+function execNoFatal(cmd, options) {
+  const fatal = shell.config.fatal;
+  try {
+    shell.config.fatal = false;
+    return shell.exec(cmd, options);
+  } finally {
+    shell.config.fatal = fatal;
+  }
+}
+
+function getCurrentBranch() {
+  return shell.exec('git branch', {silent: true})
+      .stdout.toString()
+      .split('\n')                      // Break into lines
+      .map((v) => v.trim())             // trim
+      .filter((b) => b[0] == '*')       // select current branch
+      .map((b) => b.split(' ')[1])[0];  // remove leading `*`
+}
+
+exports.httpGet = httpGet;
+exports.githubGet = githubGet;
+exports.githubPrInfo = githubPrInfo;
+exports.gitHasLocalModifications = gitHasLocalModifications;
+exports.execNoFatal = execNoFatal;
+exports.getCurrentBranch = getCurrentBranch;