diff --git a/docs/PR_REVIEW.md b/docs/PR_REVIEW.md new file mode 100644 index 0000000000..5a4278513a --- /dev/null +++ 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 ..." 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 ..." 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 ..." to update what will be committed) + (use "git checkout -- ..." 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 git@github.com: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 new file mode 100755 index 0000000000..a32eceaaae --- /dev/null +++ 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 new file mode 100755 index 0000000000..d8883d0381 --- /dev/null +++ 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 git@github.com: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 new file mode 100644 index 0000000000..35124df18e --- /dev/null +++ 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: `git@github.com:${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; \ No newline at end of file