Tools/CodeReview

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

Code Review

This page describes the tools used for code reviews. The process of contributing and reviewing code is described in Contributing Code.


Doing code reviews 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, "Raise concern" on commits can be used to notify developers and review code after it has been committed. Audit can be used to see pending/ resolved concerns. Note that concerns should not be a replacement for bug reports.

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

Ingredients of a Patch

The ingredients list has moved to the Contributing Code page.

Guidelines for Contributors

Guidelines have moved to the Contributing Code page.

Guidelines for Reviewers

  • The differential text should be usable as the git commit message (see the guidelines for details).
  • Be explicit when some changes are to be addressed before committing, without the need for a review iteration.
  • If the patch is not "green-light" the author is expected to give the patch another iteration.
  • If the patch needs agreement on the design task first, put the patch on hold.
    Note: T72400

Currently, it is only possible via “Close” patch, which sounds negative. But a new state can be introduced.

  • **Wait for reviewers:**
    • Developers are expected to reply to patches in 3 working days.
  • Add relevant modules/projects to tags.
  • Assign individuals (instead of modules/projects) for reviewers, to avoid too much noise.
  • In the future:
    • We will have CI (compile + tests)
    • Maybe support a "merge" button
  • Get newer developers doing code review.
    • We set them as reviewers, unless they have a good reason not to, they should consider it part of their work for BF to do these code reviews to the best of their ability.
    • Encourage developers to review different areas of the code-base for better bus-factor.
    • Encourage pairing for review - 30-45min sessions.
  • (TODO): patch review playbook: e.g: a specialized feature that doesn’t fit, split into multiple patches.. Needs design task… etc.

Use a Local Branch

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

Note: Patches are usually based on the master branch, unless your changes are targeted at a specific release or experimental branch. Ensure you are on the desired branch (e.g. master) by typing

git status

before continuing.

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

Arcanist will prompt you to open the Conduit API Tokens page in your browser to generate a new token. Note: if this web page asks for an App Code, you will need to get your authentication code from your 2FA (Two-Factor Authentication) app.

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 show up automatically in the task. The reviewers are referred to by their username on developer.blender.org.

Update a Patch

To update the revision with modified or new commits, run:

# Dxxxxx is the number of your revision on developer.blender.org
arc diff --update Dxxxxx

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 -U1000 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 backward 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?