Peer Code Reviews in a Mercurial World
Mercurial, or Hg, is a brilliant DVCS (Distributed Version Control System). Personally I think it’s much better than Git, but that’s a whole religious war in itself. If you’re not familiar with at least one of these systems, do yourself a favour and take a read of hginit.com.
Massive disclaimer: This worked for our team. Your team is different. Be intelligent. Use what works for you.
The Need for Peer Code Reviews
I’ve previously worked in a number of environments which required peer code reviews before check-ins. For those not familiar, the principle is simple – get somebody else on the team to come and validate your work before you hit the check-in button. Now, before anybody jumps up and says this is too controlling, let me highlight that this was back in the unenlightened days of centralised VCSs like Subversion and TFS.
This technique is just another tool in the toolbox for finding problems early. The earlier you find them, the quicker and cheaper they are to fix.
- If you’ve completely misinterpreted the task, the reviewer is likely to pick up on this. If they’ve completely misinterpreted the task, it spurs a discussion between the two of you that’s likely to help them down the track.
- Smaller issues like typos can be found and fixed immediately, rather than being relegated to P4 bug status and left festering for months on end.
- Even if there aren’t any issues, it’s still useful as a way of sharing knowledge around the team about how various features have been implemented.
On my current project we’d started to encounter all three of these issues – we were reworking code to fit the originally intended task, letting small issues creep into our codebase and using techniques that not everybody understood. We identified these in our sprint retrospective and identified the introduction of peer code reviews as one of the techniques we’d use to counter them.
Peer Code Reviews in a DVCS World
One of the most frequently touted benefits for DVCS is that you can check-in anywhere, anytime, irrespective of network access. Whilst you definitely can, and this is pretty cool, it’s less applicable for collocated teams.
Instead, the biggest benefit I perceive is how frictionless commits enables smaller but more frequent commits. Smaller commits provide a clearer history trail, easier merging, easier reviews, and countless other benefits. That’s a story for a whole other post though. If you don’t already agree, just believe me that smaller commits are a good idea.
Introducing a requirement for peer review before each check-in would counteract these benefits by introducing friction back into the check-in process. This was definitely not an idea we were going to entertain.
The solution? We now perform peer reviews prior to pushing. Developers still experience frictionless commits, and can pull and merge as often as possible (also a good thing), yet we’ve been able to bring in the benefits of peer reviews. This approach has been working well for us for 3 weeks so far (1.5 sprints).
It’s a DVCS. Why Not Forks?
We’ve modelled our source control as a hub and spoke pattern. BitBucket has been nominated as our ‘central’ repository that is the source of truth. Generally, we all push and pull from this one central repository. Because our team is collocated, it’s easy enough to just grab the person next to you to perform the review before you push to the central repository.
Forks do have their place though. One team member worked from home this week to avoid infecting us all. He quickly spun up a private fork on BitBucket and started pushing to there instead. At regular intervals he’d ask one of us in the office for a review via Skype. Even just using the BitBucket website, it was trivial to review his pending changesets.
The forking approach could also be applied in the office. On the surface it looks like a nice idea because it means you’re not blocked waiting on a review. In practice though, it just becomes another queue of work which the other developer is unlikely to get to in as timely a manner. “Sure, I’ll take a look just after I finish this.” Two hours later, the code still hasn’t hit the central repository. The original developer has moved on to other tasks. By the time a CI build picks up any issues, ownership and focus has long since moved on. An out-of-band review also misses the ‘let’s sit and have a chat’ mentality and knowledge sharing we were looking for.
What We Tried
To kick things off, we started with
hg out. The
outgoing command lists all of the changesets that would be pushed if you ran
hg push right now. By default it only lists the header detail of each changeset, so we’d then run though
hg exp 1234,
hg exp 1235,
hg exp 1236, etc to review each one. The downsides to this approach were that we didn’t get colored diff outputs, we had to review them one at a time and it didn’t exclude things like merge changesets.
Next we tried
hg out -p. This lists all of the outgoing changesets, in order, with their patches and full colouring. This is good progress, but we still wanted to filter out merges.
One of the cooler things about Mercurial is revsets. If you’re not familiar with them, it’d pay to take a look at
hg help revsets. This allows us to use the
hg log command, but pass in a query that describes which changesets we want to see listed:
hg log -pr "outgoing() and not merge()".
Finally, we added a
cls to the start of the command so that it was easy to just scroll back and see exactly what was in the review. This took the full command to
cls && hg log -pr "outgoing() and not merge()". It’d be nice to be able to do
hg log -pr "outgoing() and not merge()" | more but the
more command drops the ANSI escape codes used for coloring.
What We Do Now
To save everybody from having to remember and type this command, we added a file called
review.cmd to the root of our repository. It just contains this one command.
Whenever we want a review we just type
review and press enter. Too easy!
One Final Tweak
When dealing with multiple repositories, you need to specify which path
outgoing() applies to in the revset. We updated the contents of
cls && hg log -pr "outgoing(%1) and not merge()". If
review.cmd is called with an argument,
%1 carries it through to the revset. That way we can run
review myfork as required.