Sometimes, I feel that my career as a coder is demarcated by the tech stacks I used to write software. Partly that’s about the programming language—Smalltalk in college, C# and Python at Fog Creek—but it’s also about all the other tools you use to get the job done. I spent eight years working for Fog Creek, and in that capacity, I had a pretty consistent stack: FogBugz for bugs, customer support, and documentation; Trello for general feature development; Kiln for code review; Mercurial for source control; Vim and Visual Studio for actual coding; and our in-house tool, Mortar, for continuous integration.[1] While those tools of course changed a bit over my time at Fog Creek, they only ever changed very gradually, component-by-component, so my effective workflow remained largely unchanged.

About a month ago, when I joined Knewton, my entire stack changed all at once.[2] Out with Visual Studio; in with IntelliJ. Out with Mortar; in with Jenkins. Out with Mercurial; in with Git. Out with FogBugz; in with JIRA.

While you might think that’d be a headache-inducing amount of churn, it really hasn’t been, because most of these minimally impacted my workflow. Git and Mercurial end up being surprisingly similar, JIRA is just a kind of half-finished FogBugz, and IntelliJ is at worst Visual Studio’s equal. Maybe I had to relearn some keystrokes and button placements, but the actual pattern of how I wrote code didn’t really change.

With one exception: I hate using Gerrit for code review. I don’t hate it because it’s poorly written; I hate it because its workflow encourages bad habits.

Knewton is really, really big on code review. That’s awesome, because so am I, to the point that I created a whole tool around it. So it’s certainly not the idea of code review that I object to.

Further, Gerrit’s design is actually almost exactly identical to the original Kiln prototype. There are fundamentally two ways to do code review: pre-merge, meaning that you review code before it’s in the main repository, and post-commit, which means you review it afterwards. Modern Kiln permits both, but back in 2008, when Tyler and I won the Django Dash with what became Kiln, we were all about the pre-merge workflow. Pushing to the main repository was forbidden; instead, you’d create a review workspace, push your changes there, discuss them, and, on approval, we’d automatically merge them. That’s still my preferred workflow, which is why Kiln still supports it (via the “Read and Branch” permission), and since that happens to be the only workflow supported by Gerrit, I ought to love it.

Kiln's Early UI

And I almost do, except for one, fatal flaw: the granularity of a review is wrong. In all versions of Kiln, reviews are on a series of related changes. In Gerrit, reviews are on a single patch. Thus, in Kiln, many commits may be involved in a single review, and the review approves or rejects the collection of them, whereas Gerrit’s reviews are single, isolated commits.

Each of these two methodologies patterns has lots of tools in its camp. GitHub and Bitbucket both join Kiln in the review-a-collection-of-patches camp, while Review Board, Barkeep, and Phabricator all land in the review-a-single-patch camp. So it’s not as if I’m saying one particular tool got it right, and all of the others got it wrong. But I sure am going to say that one collection of tools got it right, and the others got it wrong, because single-patch systems encourage bad habits.

There are two fundamental problems with single-patch review systems:

  1. They encourage lumping at-best-weakly-related changes together. Frequently, when I start implementing a new feature, there are at least three steps: first, refactor the existing code to make it clean to add the new feature; next, add the new feature; and finally, add unit tests. The bigger the feature, the more likely each of these steps is likely to itself consist of several logical steps. If you can store several distinct commits in a single review, then you can simply keep these commits grouped together. But if I’m in a single-patch system, I’m going to be strongly encouraged to do everything in one massive commit. That’s especially frustrating because refactoring existing code, and adding new code, get lumped together, demanding much more mental energy on my part to figure out what’s actually going on in a given review.

    You might argue that you can keep the patches split, creating one review per commit, but that’s actually worse. At best, you’re now separating the tests from the feature, and the refactoring from the motivation for the refactoring. But the real issue is that many single-patch systems make it very easy to approve any one of the commits in isolation, which is exactly the opposite of what you want. Thus, one-review-per-commit switches the balance from “annoying” to “dangerous.” Not really an improvement.

  2. They encourage you to hide your history. The whole point of a source control system is to tell you the history of how code got to be the way it was. I want to be able to see what it looked like yesterday, and last February at 2 PM, and anything in between. Sometimes that’s because I know that the code worked then and doesn’t now, and I want to know why, but lots of the time, it’s because I want to know why a given change was done. What was the context? What was the motivation? When you just keep updating one single patch the whole time it’s under review, I’m losing tons of history: all I’ll get is a single finished product as a single patch, without any of the understanding of how it got that way.[3]

And that’s why I’m finding myself extremely frustrated with Gerrit. It’s not that Gerrit’s a bad piece of software; it’s that it’s encouraging me to develop bad habits in how I use source control. And that’s why, for all the parts of my stack that I’ve switched out, the only one I’m truly frustrated to give up is Kiln.

  1. Given the amount of love Mortar’s gotten since I wrote it as part of a hackathon, I think it should be renamed “Moribund”, but that’s neither here nor there. ↩︎

  2. Well, almost. We still use Trello heavily, and if I have anything to say about it, it’ll stay that way. ↩︎

  3. I’m aware that many people do this in Git anyway via aggressive rebasing, but at least GitHub, Bitbucket, and Kiln all allow the agglutinative workflow. It’s not even possible in a single-patch system. ↩︎