09:32 am, 23 Mar 07
code review is great
At Google, all code must be reviewed before you can check it in. This means that another engineer needs to look over your change and give it their approval. Traditionally we've done it via email and some wrapper scripts, but more recently people have been switching to Guido's fancy tool.
When I first started doing this I was a bit skeptical. On many other projects I've worked on, you instead use post-commit emails (or equivalents) to watch what everyone is doing. You can contact the submitters if something went wrong, so it seemed to me to be more or less equivalent to review without the overhead.
But in practice, I find that after-the-fact emails let a lot more shady code to get through. It's surprisingly common (both in the code I write and the code by others I review) for something to be written in a slightly messy way -- like with an unclearly named variable, or with an overlong function that could be split into two -- and even though the author knows it's not ideal they'll still submit it. Then, in a post-commit email world, others will see the commit and not be happy with it, but they won't be unhappy enough to warrant requiring a subsequent change for cleanliness.
Instead, in a code-review world, nothing's been committed yet so it seems like it's not that much more trouble to just get it right in the first place. My most common sort of feedback in a review is a request to clarify a bit of code by either restructuring or with comments. Especially when you have a lot of people consuming the same code, this sort of polish is really quite nice and worth the pain of reviewing. I now fully endorse this sort of workflow.
One of the things that Mondrian (linked above) adds to the process is that each attempted commit is a sort of microbranch. Perforce is actually pretty weird as compared to all the other VCSes I've used, but you can sorta map the code review process to more "normal" systems and call sending code for review just sending someone a diff. In the past, when the submitter then updates their code in response to feedback, the reviewer just gets a second diff, and doesn't get any sort of diff-diff to see what changes were actually made between the diffs.
Mondrian adds this diff-diff feature by actually snapshotting the file state at different review points and (within its own UI) letting you see diffs between the files at different points in the review. In fact, what's it's done is implement its own sort of VCS outside of Perforce! It even identifies the different file states with hashes instead of changelist numbers (a Perforce-ism).
Identifying file states with hashes and micro-branches are the bread and butter of monotone, and in writing this post I could immediately sorta imagine how you could implement this sort of code-review process completely within monotone: you'd add an extra cert on revisions that had passed the review process, and then make it so all the normal commands (like "update") only pull revisions that passed review. And then I remembered that they had already done exactly this. (There are a lot of details necessary to making this not be totally painful, and since I haven't looked at their implementation I can't vouch for it.)
monotone is a seriously sweet piece of software, primarily in its powerful design. I wish there was some way to make it less confusing and complicated. I wonder how much of its complexity is just fundamental to the problem space it attacks...
When I first started doing this I was a bit skeptical. On many other projects I've worked on, you instead use post-commit emails (or equivalents) to watch what everyone is doing. You can contact the submitters if something went wrong, so it seemed to me to be more or less equivalent to review without the overhead.
But in practice, I find that after-the-fact emails let a lot more shady code to get through. It's surprisingly common (both in the code I write and the code by others I review) for something to be written in a slightly messy way -- like with an unclearly named variable, or with an overlong function that could be split into two -- and even though the author knows it's not ideal they'll still submit it. Then, in a post-commit email world, others will see the commit and not be happy with it, but they won't be unhappy enough to warrant requiring a subsequent change for cleanliness.
Instead, in a code-review world, nothing's been committed yet so it seems like it's not that much more trouble to just get it right in the first place. My most common sort of feedback in a review is a request to clarify a bit of code by either restructuring or with comments. Especially when you have a lot of people consuming the same code, this sort of polish is really quite nice and worth the pain of reviewing. I now fully endorse this sort of workflow.
One of the things that Mondrian (linked above) adds to the process is that each attempted commit is a sort of microbranch. Perforce is actually pretty weird as compared to all the other VCSes I've used, but you can sorta map the code review process to more "normal" systems and call sending code for review just sending someone a diff. In the past, when the submitter then updates their code in response to feedback, the reviewer just gets a second diff, and doesn't get any sort of diff-diff to see what changes were actually made between the diffs.
Mondrian adds this diff-diff feature by actually snapshotting the file state at different review points and (within its own UI) letting you see diffs between the files at different points in the review. In fact, what's it's done is implement its own sort of VCS outside of Perforce! It even identifies the different file states with hashes instead of changelist numbers (a Perforce-ism).
Identifying file states with hashes and micro-branches are the bread and butter of monotone, and in writing this post I could immediately sorta imagine how you could implement this sort of code-review process completely within monotone: you'd add an extra cert on revisions that had passed the review process, and then make it so all the normal commands (like "update") only pull revisions that passed review. And then I remembered that they had already done exactly this. (There are a lot of details necessary to making this not be totally painful, and since I haven't looked at their implementation I can't vouch for it.)
monotone is a seriously sweet piece of software, primarily in its powerful design. I wish there was some way to make it less confusing and complicated. I wonder how much of its complexity is just fundamental to the problem space it attacks...
trivia
That's actually the source of the name: it's a device for constructing monotonically improving branches, by permitting a QA filter like a buildbot or a code reviewer define a new branch strictly as a view of the high-quality revisions in some other branch. You can wire your mtn client to ignore revisions that "broke the build" or otherwise "got worse".It's also the source of using certs to describe branches: branches inside monotone can overlap. This is necessary because if I want to checkout from a "high quality" branch, make some changes and commit, I want those changes to also be children of the underlying "low quality" branch that may churn back and forth between working and not-working. Essentially you commit into a position-in-the-history-graph (independent of branches), and then you bless it into a branch. QA bots or code reviewers can bless the same rev into many "higher quality" branches without forking the history.
It's also the source of its distributed-ness: constructing a QA branch "in house" is only partly useful; often people outside your particular org want to make comments on your code indicating its quality, and some users -- even your own devs -- might want to use those quality annotations. So for example an independent testsuite vendor could define a superimposed branch on the GCC trunk called "GCC trunk versions that pass my testsuite", just by emitting a constant-sized cert into the cloud every few days. To support that you need to break the chokehold that your org has on "write permission to the repository", and let everyone write into the shared cloud.
Surprisingly, almost everything odd about monotone falls out of this QA design goal. Unfortunately 4 years into its life monotone still doesn't make this mode of operation easy out of the box. You have to turn some knobs and write some lua. I've become lazy and am not really hacking on it anymore.