Code Reviews
First, please read the Workflow and Code Style wiki pages.
A code review is required for any merge to the master branch. We use shared feature branches stored in Github, and use pull requests from a feature branch to master using squash-and-fast-forward-merge as our review mechanism.
If you are not (yet) a member of the Committers group for DynamoRIO and thus do not have write access to the repository, you can contribute an individual source code change by having an existing developer commit it. Rather than using a feature branch on a clone of DynamoRIO/drmemory and using git review as described below, you'd clone DynamoRIO/drmemory into your personal github account and submit a pull request from there to the master branch of DynamoRIO/drmemory. You are expected to follow our Code Style.
We recommend submitting a few small patches in this manner to demonstrate your work before asking to be added as a project member.
You should have already cloned the repository and run the devsetup.sh script as described on our Workflow page.
For a typical single-commit review, first develop and test your feature or bug fix locally. Be sure to give your branch a name following the conventions described in Workflow. When the code is ready to be reviewed and has been cleaned up and squashed into one commit (see below for multi-commit larger features), push your local branch to Github via:
git review
That command will only work if you've run the devsetup script (which you should have done!).
Now use Github's web interface to create a pull request. We do not currently have command line support set up for this. Immediately after pushing your branch, there is a convenience "Compare & pull request" under "Your recently pushed branches" on the Code main page. In general, you can click on "New pull request" and select your branch on the right side "compare: ". Finalize by clicking "Create pull request". You can select a particular reviewer if desired.
Commit messages should include an initial title line separated from the body of the message by a blank line. The title line should be a concise summary and should start with the issue number followed by a colon which is followed by the description of the issue fix. If the issue is fixed by more than one commit, the title should instead start with the issue number followed by a space and a short issue description, then a colon, and then a description of this particular fix. In either case, the body of the message should elaborate on the contents of the commit using complete sentences.
An example of a single commit fixing an issue:
i#1434: fix crash in get_base_named_obj_dir_name() for earliest injection
The PEB fields used there are not yet initialized, so we construct the
string using what we've observed on different Windows versions.
Fixes #1434
And an example of a commit contributing to a larger issue:
i#1729 offline traces: fix thread entry order issue
Fixes a raw2trace problem where the first bb for a thread switch can be
incorrectly attributed to the prior thread.
For multi-commit solutions to an Issue, adding a "part N" type of label can make it easier to see that each one is incremental:
i#837 AVX2 ISA update, part 4: add VSIB support
Adds support for the new addressing mode, the Vector SIB or VSIB. This
references multiple addresses based on indices in an xmm or ymm register.
For a commit that fixes an Issue, be sure to resolve the Issue with a message indicating the commit revision number. If you use the exact string "Fixes #NNNNN" (where NNNNN is replaced by an Issue number) with nothing else on that line in the commit message, the git push will auto-update the Issue to say "Fixed", avoiding any need to manually close the issue. You can also auto-close a Dr. Memory issue with "Fixes DynamoRIO/drmemory#NNN". You can see other options in the GitHub documentation.
Note that there is no "i" before the "#" for issue references when communicating with Github, but that our convention is to prefix the "i" for our own references to make it clearer that it's an issue and not a pull request or some other hashtag.
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.
We do not want enormous diffs that are impractical to review because an entire polished feature was developed in isolation. Larger features should be split into pieces and either committed incrementally onto a project branch or merge into master in incremental steps. For sharing work and providing visibility into ongoing projects, we prefer using project branches. For incremental experimental work, the typical approach is to introduce the new code in a disabled state, either via runtime option or ifdef or both, until stable. Unfinished parts that are committed should be clearly labeled as such.
For new committers, code review and commit sizes should be small: less than 100 lines for the first few commits. We may reject reviews that are larger.
Pull requests are integrated into our continuous integration system, providing pre-commit testing of all commits. Both Travis and AppVeyor are immediately run when a pull request is created. The request can't be merged until the tests complete and pass.
Please note that our test suite is NOT THE SAME AS RUNNING "make test".
The pre-commit test suite builds multiple builds and enables more tests
than "make test". Running tests in just one build is not sufficient. See
TestSuite for information on setting up and running the test suite locally
via the suite/runsuite.cmake script.
In addition to Travis and Appveyor, we have several machines running test suites at scheduled times and reporting results to our CDash dashboard. Be sure to look at any emails about build or test breakages in the day or so after your commit and either help to fix it or consider reverting if something is broken that the continuous integration tests did not catch (e.g., currently they do not cover ARM or Android).
Upon receiving the email requesting a review, the reviewer will visit the pull request page and add comments to the code as part of a Github review, including comments on individual lines. An email will be sent with these comments. 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). Please read and reply to every comment.
Reply to comments individually at the point in the code view where they appear. Do not simply reply at the top level, as such comments are more difficult to follow as a conversation thread with context.
After making changes in response to review comments, commit those changes locally as a new commit on top of your original commit. If the reviewer requested changes to the commit message, use a new proposed final commit message as the message for this change commit so that the reviewer can review the new message.
Then push the new commit to the pull request:
git review
Do not use a force push to change the history of the shared branch! Use a new, separate commit.
The final squash-and-merge step will squash these additional commits with the original to make a single commit that will be fast-forward-merged into master (see below for more details).
During local development before requesting review, i.e., before your feature branch is on the server, rebasing to pull in new changes from master is the preferred workflow. But once you've pushed your branch and it's shared, avoid rebasing as it will destroy history in the pull request. Instead, merge changes from master if an update is needed. These merge commits will disappear when the feature branch is fast-forward-merged to master.
Generally, it's better to not update at all once a review has started, and let Github do the update at the final step before merging into master.
Only once the reviewer marks the proposed code changes as accepted and all of the continuous integration tests pass can the change be merged into master. If you have merge privileges, when performing the squash-and-merge, be sure to edit the final commit message (after pressing the squash-and-merge button, Github presents the final message in an edit box) to create a clean description of the commit without the messy incremental steps from the multiple commits fixing small issues during the review process. (You can resize the edit box by dragging the control in the bottom right.) Especially be sure to remove all commit message titles from the body of the final squash-and-merge message, as the commit title is in a separate edit box (and defaults to the title of the first commit, so update it as well if the title changed during review). Also be sure to avoid truncation of the commit title, as Github likes to replace the end with "...".
Leave in place the pointer to the pull request, which Github automatically adds to the end of the commit message title, in parentheses, for its suggested final squash-and-merge title.
Generally, the feature branch should be deleted from the repository using the button that Github provides immediately after merging.
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.
For larger, more experimental features that do not easily fit into the one-commit-per-merge model, project branches are used. The review process is identical to above but substituting the project branch for master: i.e., single-commit feature branches are still used and reviewed, but they are squash-and-fast-forward-merged into the project branch.
Clean, reviewed commits then accumulate in the project branch until they are all ready to be merged into master. Here, a rebase is used rather than a squash-and-merge.
