Code Review (part 1)

I love code review

What is code review? This wikipedia quote sounds ok, but who could love anything that includes "formal process" and "scrutinized"? Sounds like a lot of work, right? What's the upside?

Code review is systematic examination (often as peer review) of computer source code intended to find and fix mistakes
http://en.wikipedia.org/wiki/Code_review

A code review (sometimes called a program inspection) is a formal process where a software developer presents the code he or she has written to other software engineers who are familiar with the project. The code is scrutinized carefully to identify potential bugs, design problems, non-compliance with project standards, inconsistencies, and any other problems in the code.
http://sea.ucar.edu/best-practices/code-review

Code review allows developers to collaborate and improve code by reading it early and catching bugs during development. The earlier bugs are caught, the less impact and expense they cause. Code review is a lot more fun before the changes are live than retroactively trying to figure out what change broke everything in production.

I remember code reviews at my first software company. Someone must have heard they were a good idea, so we had to do code reviews of new features. We waited until the feature was done, then printed out all the code and took a few engineers into a room. We'd sit there for a few hours looking over the printouts before making a few token suggestions and calling it quits. We shared some small insights and caught a few bugs, but overall this heavy process was unstructured, late, disorganized and ineffective. We had the right motivations, but we were looking at code too late in too big of a chunk.

At the other end of the scale is the ad-hoc system of emailing around some diffs or code refs and asking for input. Here the lack of formal process is a pain -- emailing diffs around? Another process flowing through (stalling in) my mailbox? Where do I send my comments, how do I archive the results?

Somewhere in the happy middle are tools for "light weight code review." These tools take a diff and present it in a web interface providing the ability to view the diffs and make comments and enforce some sort of workflow. Gerrit (inspired by Rietveld inspired by Mondrian), Review-Board and BarKeep are some of the open source options, github reviews are free and pay software is available from SmartBear (Code Collaborator), Atlassian (Crucible) among others. These systems all make different trade-offs: pre-vs-post commit, forced-vs-optional reviews, VCS agnostic-vs-integrated, inline vs side-by-side diffs.

At work we've been using Gerrit for two years now after switching from Rietveld when we migrated from SVN to git. Gerrit is very opinionated: it is for mandatory, pre-commit reviews and only supports git. Gerrit integrates nicely with Jenkins continuous integration server for running unit-tests before the review. We originally picked Gerrit at [undisclosed startup] and managed to integrate it into Demand after we were acquired.

For open source projects, I'm happy with github pull-request discussions (I still wish I could see side-by-side diffs! I have this huge monitor for a reason) and couldn't see enforcing the gerrit model (even though the android project does) without discouraging drive-by patches from random developers. But at work, I want the small dollop of process that gerrit provides.

I've been super happy with gerrit and can't wait to tell you more about it in "Code Review, Part II. Gerrit FTW".

Modify submit-type for Gerrit project via meta/config

The Submit-type of a gerrit code review project can not be changed in the UI after creation. It can be modified via the hidden meta/config branch. Any setting available to create-project can be edited this way.

Project information is not stored in the gerrit database. The information is stored directly in the git repository in a branch named 'meta/config', in two files 'project.config' and 'groups'. The values from these files are cached in the'project-list' and 'projects' caches.

Steps to make a change:

1. set read and push permissions on refs/meta/config
2. check out the branch,
3. change the files,
4. push the repo back,
5. clear the cache.

Check out the branch:

% git fetch origin refs/meta/config:refs/remotes/origin/meta/config
% git checkout meta/config

Push back the changes:

#directly:
% git push origin meta/config:meta/config
#via review:
% git push origin meta/config:refs/for/refs/meta/config

Flush the caches:

% ssh gerrit gerrit flush-caches --cache project_list
% ssh gerrit gerrit flush-caches --cache projects

project.config

[access "refs/*"]
        owner = group MYgroup
[receive]
        requireChangeId = true
[submit]
        mergeContent = true
        action = merge always

groups

# UUID                                          Group Name
# eca2c52d733e5740a01747e71f018dcfdeadbeef      MYgroup

I found the meta/config mentioned in some posts (post post) in the repo-discuss newsgroup.