Tools/CodeReview

< Tools(Redirected from Dev:Doc/Tools/Code Review)

Code Review

Differential on developer.blender.org is an online tool for code review, used by developers with commit access to review each others code, and by other developers to submit patches.

Doing code review improves the quality of commits. With more eyes looking at the code we can prevent bugs, and having your code reviewed also generally keeps you on your toes. If mistakes slip through anyway, Audit on developer.blender.org can be used to notify developers and review code after it has been committed.

Ingredients of a Patch

Before adding a new feature (even a small one), it needs to be discussed and motivated. As a minimum, the patch description should include the following where applicable:

  1. Description of the problem that is addressed in the patch.
  2. Description of the proposed solution, and a motivation as to why this is the best solution.
  3. List of alternative solutions, and a motivation as to why these are undesirable.
  4. Limitations of the proposed solution.
  5. Mock-up of the proposed user interface and a description of how users are going to interact with the new feature.

These ingredients don’t have to be full essays. A few well-written sentences can be very illuminating as well. A simple example that still adheres to the above can be found in D6352.

The order in which these are listed is also relevant, as it helps to guide the reader towards understanding what the patch is doing. To give a counter-example, starting a patch description with “This drop-down menu allows the artist to…” can be hard to understand at first, as the reader doesn’t know anything about the underlying problem yet.

Every day developers have to dive into the history to find rationales on changes. When this results in a commit or patch with no rationales at all, developers end up having to make guesses and assumptions, which tend to backfire and result in bugs. The alternative is to spend a significant amount of time understanding the changes and testing corner cases. By writing down some (brief) background information and rationales for a change, it helps to make better, more informed decisions later.

Guidelines

The intention is that code review is used for most non-trivial commits. Some guidelines:

  • The typical reviewers with final approval should be module owners, but anyone can and is encouraged to participate.
  • Code reviewers should try to either accept, request changes, or reassign to another reviewer quickly, to unblock the developers working on their features. This makes the process more inviting to new developers and more usable for existing developers.
  • Developers submitting code should make it easy for people to review their code. That means you do not submit huge patches, but break up your work into multiple smaller commits and reviews. Code refactoring, new features, bug fixes, etc. should generally be submitted as separate code reviews. You can have them as separate commits in the same local Git branch.
  • If there are bigger design question to solve, open a Patch or Design task first, so that code review can focus mostly on the implementation.

Use a Local Branch

When you are going to submit code for review, it is best to commit that code in a local Git branch.

git checkout -b some-new-feature
# ... make changes to code ...
git commit .

The next step is to create a revision in Differential, and there are two different ways of doing this.


Use Arcanist

Phabricator comes with a command line tool called arc which can be used to quickly submit and apply revisions. For developers that do code reviews often this is much more efficient than uploading and downloading diffs, but requires some setup and use of command line tools.

Installation

See the Arcanist Quick Start.

Basically you need to install php, get arcanist from its git repository, and put the arc command in your PATH.

You do not need to add a .arcconfig, this is already in the repository.

Next you need to authenticate yourself. Run this command and follow the instructions:

arc install-certificate

Send a Patch

When arc is installed and all your code changes are committed in a branch, sending the changes for review is quick, simply run:

git checkout some-new-feature
arc diff

You will be prompted to add a title on the first line, and an optional extra summary and reviewers. If this revision relates to a particular task, you should mention the task ID in the summary, for example "This commit fixes T123" to reference task T123. The revision will then shown up automatically in the task. The reviewers are referred to by their username on developer.blender.org.

To update the revision with modified or new commits, simply run the same arc diff command again. If updating a revision you may want to get the latest changes from master on your local branch (should not rebase on public branch) before submitting a new diff, this can be done with:

# commit code to your branch if not already done so
git checkout master
git pull origin master
git checkout some-new-feature
git rebase master

Apply a Patch

A reviewer or other interested party can download and apply a patch to their local repository with a command like this, which will be shown on the revision page:

arc patch D123

This will download the patch and apply it in a newly created branch. Alternatively, you can apply a patch to the current branch.

arc patch --nobranch D123

Commit a Patch

When the revision is approved and you have commit rights, you can push to the main repository. If you do not have commit rights, the reviewer will do this for you.

This command can be used to move it to the master branch.

arc land --hold some-new-feature

Then you can inspect the code changes and commit message, run the automated tests and if necessary amend it:

# show commit log and diff
git show HEAD
# change commit log
git commit --amend

Do not use arc land without --hold. This will not give you the opportunity to verify the commit before pushing it.

Some Other Useful Commands

This section explained the basic workflow for working with arc, however it is quite a flexible tool and it can be used in different ways. For example you can send a specified range of commits, or even uncommitted changes rather than always creating a branch, it's a matter of preference.

arc can also be used to close revisions, close tasks, upload or download files, and many more things. See the user guide for more details. Here are a few useful commands:

# list local branches that have an associated revision on differential
arc feature
# list open tasks assigned to you
arc tasks
# upgrade to the latest version of arc (can be needed when we update the website)
arc upgrade

Further Reading

Upload a Diff

As an alternative to using Arcanist, you can also send in a patch file. This is an easier way to submit your code, but as it lacks certain context that Arcanist adds, it also makes it harder for people to review. Create a Diff and upload a diff file there. For detailed information on how to work with such files, see the Patches and Diffs page.

In short, Differential expects standard git diff or git show output. If your changes are committed in a local branch, this is the command that will work.

# generate a diff from some-new-feature branch
git diff master...some-new-feature > some-new-feature.diff

When the reviewer requests changes, you can use Update Diff on the code review page to upload a new patch.

Quality Checklist

There are a number of common mistakes in patches. This checklist can help both contributors and review help verify the patch takes everything into account.

  • Is backwards compatibility with existing .blend files and the Python API preserved?
  • Is the naming and user interface consistent with related functionality in Blender?
  • Can the feature be made easier or more efficient to use?
  • Does the patch have a negative impact on performance?
  • Can a little refactoring help make the code easier to understand and maintain?
  • Does the code have comments?
  • Was clang-format used to format the code to follow the conventions?
  • Do the automated tests still pass?