Misko Hevery
Miško Hevery
parent
f229449c67
commit
f841e36543
|
|
@ -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 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.
|
||||
|
||||
|
|
@ -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;
|
||||
}
|
||||
|
|
@ -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;
|
||||
}
|
||||
|
|
@ -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;
|
||||
Loading…
Reference in New Issue