Code review before commit
I’ve started implementing code review before commit/push on my team. We theoretically had a code review system in place before this, but it went something like this:
- Write code
- Commit code
- Push code
- Roll a d20
- If die came up greater than 2, no one will review your code.
- Otherwise, someone tries to retroactively review a billion commits in 30 minutes.
That process wasn’t really the best didn’t work. In my experience, once the code is in the tree, there is a near zero chance that it will be reviewed.
Code review before commit (or push depending on your vcs) is a much simpler way to do things, and it helps promote best practices and communication on your team.
By reviewing the code before someone commits it to the tree, you can spot potential problems early, and teach your team to keep their commits small enough to easily review. It’s much easier to glance at a small diff and say, “ok” than to look at a 300 line beast that crosses multiple files.
Code review before commit doesn’t necessarily give you small commits, but it does let whoever is reviewing the code say: “Hey, this diff is too big for me to wrap my head around, can you break it up into smaller ones?” It also gives a chance for the person who worked on the code to explain what they’ve done to someone else on the team. This is very useful when, after deploying, something goes wrong and the person who wrote the code is out sick.
I’ve seen this process work to great effect on several open source projects, where the project developers can easily review a diff submitted by an external contributor before signing off on it’s acceptance to the tree. I’ve also seen plenty of diffs get rejected for being too complex or too large, and the contributor is always asked to please resubmit the changes but as smaller individual bits.
As a developer, it’s quite difficult to keep a multitude of changes in your head at one time. It’s much simpler to see small changes, one at a time, and approve them individually.
I’ve also started using the arc tool to help with this. Arc is part of the phabricator suite of utilities geared at making developers lives easier. Basically it acts as a wrapper around your vcs (svn, hg, and git only) and allows you to submit a patch via the command line to a web interface designed specifically for code review. What this means, is that instead of having to bug another dev to get a change submitted, you can submit your patch to the system, and then go about working on something else until another developer has a chance to review your code.
The basic arc workflow looks like this (example using git):
git checkout -b my-great-feature
hack hack hack hack
git add file1.go
git commit -m "Increase revenue by factor of 10."
arc diff
Arc diff brings up a template similar to what you see when you commit to git:
Increase revenue by factor of 10
Summary: Tweak to landing page to increase sales!
Test Plan: Apply this patch, try the landing page, pay us money!
Reviewers: gguzman
Subscribers:
You can fill out the Summary with information on the patch, if needed. If your commit message is enough, just leave this blank. The Test Plan is required and lets the reviewer know what you’ve already done to test this commit out, and how they can test it as well. The Reviewers section is for specifying who you’d like to review your change, in this case it will be gguzman (me). Subscribers allows you to notify other phabricator users, who might be concerned but won’t be doing the review. I’ve never personally used that field so I don’t know how amazing it is.
Once you save your message, arc will diff your branch against master and submit a patch to phabricator. Phabricator will fire off an email to the person you requested the review from, and that person will receive a link to something that looks like this: https://secure.phabricator.com/D7528
From that interface, you can easily see the changes, make comments on the code, and either accept the diff, or request changes. If you request changes, the original dev will need to apply those changes before running arc diff again to update her original diff. If you accept the commit, the developer will receive an email informing them that their change was accepted. At this point they can simply run arc land and their change will be merged into mater and pushed to the remote repository.
At any time during that process, the developer is free to start a new local development branch and continue working on other features. Phabricator will keep track of the diffs until they are either applied to the tree, or abandoned.
Phabricator can do a host of other things for your development team, and I hope to write more about it in the months to come. Until then, how do you code review?