Code Reviews
A code review is required for any change to the central repository. This is regardless of how seemingly trivial the change is! All changes must be reviewed!
We use web-based reviews using the Rietveld code review
site. We upload review requests using the python
script make/upload.py, which was obtained from
http://codereview.appspot.com/static/upload.py. We use a front-end script
make/git/git_review.sh, accessed via the alias git review. Be sure
that you have run the development setup script make/git/devsetup.sh and
that python is on your path.
We support one commit per branch, so use a separate branch to review each piece of a large feature that is intended to be a separate push to the central repository.
We use oauth2 to authenticate to the code review site. By default, each review upload and finalization will auto-launch a web browser where you will need to authenticate. To automate the process, visit https://codereview.appspot.com/get-access-token and store the text token displayed there into a file located at $HOME/.codereview_dr_token. The git review script looks for that file and will pass the token to upload.py. If you receive a 401 error, the token has expired and must be updated.
Make sure you have built and tested your changes using the pre-commit test suite before requesting a review. Re-run the test suite after merging or making any changes during the review process, prior to the final push.
Request a review by running:
git review
The review script will ask you for the email address of the reviewer. You
can pass it directly via -r:
git review -r john.doe@emails.r.us
The git review command creates a new page on the Rietveld site. It
should print some output similar to this:
% git review -r john.doe@emails.r.us
Creating a new code review request.
Uploading the review...
Upload server: codereview.appspot.com (change with -s/--server)
Loaded authentication cookies from /home/john.doe/.codereview_upload_cookies
Issue created. URL: http://codereview.appspot.com/187770044
Uploading base file for make/git/git_review.sh
Uploading base file for make/git/devsetup.sh
[review 39db994] Add new code review front end: "git review"
2 files changed, 160 insertions(+), 1 deletion(-)
create mode 100755 make/git/git_review.sh
The line that says Issue created. URL: gives the URL of the review page
(in this case, http://codereview.appspot.com/187770044). The git review
command automatically sends email to the reviewer and to the drmemory-devs
googlegroups mailing list.
The initial git review command edits your commit message to include a
comment that points at the review page:
Review-URL: https://codereview.appspot.com/187770044
That provides a record of the review and associates it permanently with this set of code changes.
Upon receiving the email requesting a review, the reviewer will visit the page and add comments, including comments on individual lines. Once the reviewer "publishes" the comments, you will receive an email. You should view the comments, address them in your code, and reply to each comment on the review site (typically by saying "Done" if you agree with the request and have made the change in your code).
After making and committing changes in response to review comments, you can
upload a new patchset for the review by simply running git review again.
The script will query you for a summary of the new patchset. This can be
passed directly via the -s flag:
git review -s "Addressed reviewer concerns"
This will add a new patchset to the same review page and again send email to the reviewer and mailing list.
The process continues until you receive a "LGTM" ("Looks Good To Me") comment from the reviewer. At this point, merge your code with any upstream changes, make any additional requested changes, re-run the test suite one last time, and then commit the changes with:
git dcommit
The git dcommit command will edit the review page with your final
patchset and a link to the commit in the central repository, as well as
remove the bookkeeping about which review the current branch is associated
with. Thus, the next time you run git review, a brand-new review page
will be created.
See DynamoRIO Commit Criteria for discussion of breaking large diffs into incremental pieces, and for size guidelines for new committers. See also additional discussion below.
With an open-source project like this where each contributor has a day job with other priorities, code reviews should be expected to take a day or two, and longer if the change is lengthy. However, if the reviewer expects to not have time to complete the review within 2 days he or she should notify the author up front to give a chance to switch to a different reviewer.
If too many bugs or major issues were identified by a reviewer, a second review by that same reviewer is required before committing. We do not require iterating to a fixed point (i.e., zero review comments) due to diminishing returns in each iteration, but we do require iterating until the reviewer marks the diff as "LGTM" (or "Looks Good To Me") with perhaps a few minor comments or suggestions left.
Reviews should be kept to a moderate size for more focused responses and more isolated commits. Large projects should be split into separate logical pieces for review. Review diffs larger than about 1500 lines should be avoided.
For larger or complex changes, multiple code reviews should be obtained from separate reviewers. Suggestions from the previous review should be addressed before requesting the next review. Work-in-progress reviews are recommended, both to keep other developers apprised of ongoing development and to catch contentious issues or design errors early in development. When reviewing a work-in-progress change, it is acceptable to have pieces missing or not fully implemented: the goal is to evaluate the design and the initial work.
