Review Flows

You can implement a workflow to review and test each change submitted to your active, mainline version. This requires some extra work from your developers. However, it solves some specific problems and makes your development process more scalable because:

  • You can motivate developers and QA staff to create and run the automated tests that make a continuous integration process work. You do this by refusing to review and accept any code change until after it has been tested. Code review works better than any other method to enforce testing standards.
  • You can include new contributors, virtual team members, partner companies and other distributed contributors. Instead of looking over people's shoulders in the office, you use the review process to vet work from contributors across the globe.
  • Your mainline build is more stable. You spend less time waiting for fixes. You can support team members in many time zones, and you can release more frequently and with less batch testing.

There are a lot of variations on review workflows. I will discuss them here so that you can see why we recommend either a review workflow based on temporary branches (for mainline development), or a branch and merge workflow.

Post-commit reviews

In an old-style review process, team members would gather to critique code after it was committed to the mainline. This is a good way to discuss and enforce coding standards and solve architecture problems. However, it reduces the stability of the mainline and requires extra work, and the recommendations for future changes are easy to ignore. I have seen a lot of teams try this type of review and abandon it. Don't make your team do these "post-commit" reviews. They are annoying.

Pre-commit reviews with patch files

Instead of committing a change directly, a developer can make a patch file containing his or her change and post it to a patch-based review system like ReviewBoard, Mondrian or Crucible. Other developers can then apply the patch, comment on it, and accept or reject it. An even older technique used by some Apache projects is to attach the patch to an email and send it to an email list for review.

This "pre-commit" review is more effective than post-commit review. It can be combined with "preflight" automated tests that run locally. Google has used these tactics very successfully. However, it is annoying to go through several steps to make a patch, post a patch, test a patch, and fix a new patch.

Temporary review branches

Instead of making patches, you can put changes into a temporary branch in the VCS. This is the system used by a Gerrit "change" (called a "task branch" in other systems). It is easier to fix and update a branch than to reconstruct a patch. When the branch passes review, the system can merge it automatically to the mainline.

The folks at Google used to run a mainline continuous process with a bulletin board for reviewing patches. When they adopted Git for Android development they decided to upgrade from patches to review branches. They built a Web-based code contribution system called Gerrit which turns Git commits into "changes" that go into temporary branches. Users can comment on the changes and vote on them. Changes get merged when they receive a minimum number of votes.

Google uses Gerrit to accept contributions from thousands of people working on various parts of Android, and it has become popular for other development projects. Assembla implements a streamlined version of this workflow with protected branches.

The review or task branch gives you a clever place to put automated testing so that code is always tested before it is delivered to reviewers.

Perforce uses a feature called a "shelf" to save changes in a temporary branch for review.

Why use review or task branches?

I believe that the system of putting changes into branches for test and review completely obsoletes the older "pre-commit" and patch-based review systems because:

  • It is much easier to apply automated testing to real branches.
  • There are fewer steps for developers who want to test, review and comment, and they never need to leave their normal code browser.
  • If reviewers want to fix something in the contribution, they just make a change in the review branch.

Reviewing changes in temporary branches is a good next step for any team that is contributing to a centralized trunk or master. The key concept here is that we are using "temporary" review branches. Contributors are working on a shared version, with short-lived branches for review. The advantages of this approach include:

  • A simple transition from a workflow of contributing directly to the mainline. Changes are small for contributors, and for configuring build and test systems.
  • It is easy for contributors to maintain. They always start working with a copy of the mainline. If you have a centralized team of DevOps and tech leads, they can provide more control and support.
  • It is scalable. Multiple people can vote on changes to the same system and merging is automated.

Review branches are the tool of choice for teams that are upgrading from a mainline or trunk-based process, and for teams that have full-time reviewers (tech leads or maintainers).

The review branch process can scale to large numbers of contributors - for example, in Android and Eclipse development. However, contributors cannot work without timely review from the tech lead team, so if you use temporary review or task branches you should have a full-time reviewing team.

You can implement review branches with:

  • Gerrit and Git, or Assembla Git protected branches. These tools create review branches automatically.
  • Perforce changelists. The changelist is an excellent review branch container. You can get the review workflow in Assembla or in the Perforce Swarm product.
  • Git merge requests and pull requests, if you set up testing for these requests.

There is a review branch workflow for Subversion which we will extend to support centralized continuous delivery, but it needs some refinement.