Arnt Gulbrandsen
About meAbout this blog
2013-04-25

Code review, Gerrit's way

To the New Yorker, this is a neat way to start a story: A few years ago, at a Las Vegas convention for magicians, Penn Jillette, of the act Penn and Teller, was introduced to a soft-spoken young man named Apollo Robbins, who has a reputation as a pickpocket of almost supernatural ability. Jillette, who ranks pickpockets, he says, a few notches below hypnotists on the show-biz totem pole, was holding court at a table of colleagues, and he asked Robbins for a demonstration, ready to be unimpressed. Robbins demurred, claiming that he felt uncomfortable working in front of other magicians. He pointed out that, since Jillette was wearing only shorts and a sports shirt, he wouldn't have much to work with.

Come on, Jillette said. Steal something from me.

Again, Robbins begged off, but he offered to do a trick instead. He instructed Jillette to place a ring that he was wearing on a piece of paper and trace its outline with a pen. By now, a small crowd had gathered. Jillette removed his ring, put it down on the paper, unclipped a pen from his shirt, and leaned forward, preparing to draw. After a moment, he froze and looked up. His face was pale.

Fuck. You, he said, and slumped into a chair.

Robbins held up a thin, cylindrical object: the cartridge from Jillette's pen.


To me, those 200 words are more than an elegant introduction, they're the best part of the entire story, because they show so clearly how even those of us who know about manipulation can be manipulated.

If you use the code review tool Gerrit, you've probably seen something like the screenshot on the left. I had used git rebase -i to compress my work into about a dozen changes, some large, some small, and pushed that to gerrit. I got some things wrong when I grouped the commits, and there were mismerges and the build broke underway. That's irrelevant to today's rant, so I'll pretend it didn't happen (but maybe I'll rant about that later). My subject today is the two changes you see that do not have little tickmarks in the R column. Those two changes received the comments you see on the right.

Those two comments were enough to block actual functionality from being merged. (In real life, the mismerges also hurt, but five commits were blocked solely due to tab/space issues.) I know how to fix it, just make a new branch, git branch, pull from Gerrit, edit, push to a new changeset and get a new round of review. But I don't want to.

Instead I want to rant about how gerrit manipulates sane, clueful developers such as the one who who complained about tabs and spaces, and turns them into mindless pedants. MINDLESS! PEDANTS!

OK. I'll stop. Now I'll fix the tabs and maybe get the code into testing, where it's needed. And yes, we could have done interoperability testing without turning tabs into spaces, and I think we should have, and I think Gerrit is immoral, because it subverts our sense of value.