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 review.cmd to 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 or review myfork as required.
Tatham Oddie 
If you’re using feature branches or personal branches (as we are), it might make more sense to review when you’re merging back in to default/master, rather than on each push.
One of the things we’ve found very handy about having personal branches is that you can commit completely broken, crazy stuff, push (so it’s backed up) and not effect anyone else. I wouldn’t like to add any friction to that.
The downside of ‘merge’ reviews compared to push reviews is the possibility for a large set of changes needing to be reviewed. That’s not insurmountable though.
Another good approach.
We have a small team on this project, highly focussed on collaboratively delivering one backlog item at a time. As such, if we used branches we’d all be working on the same branch anyway. 🙂
If you guys are almost all collocated and have the “Need for Peer Code Reviews” why not doing “Pair programming” in the first place?
+1 to posts about experience from real world!
Good question Peter.
We do pair programming when we’re forging new ground, but fall back to individual work when we’re doing more grunt work (implement another screen like that one).
Also, it’s hard to divide 3 devs into pairs. 🙂
FYI… You should be able to get colors in less using the following command
hg log -pr “outgoing() and not merge()” –color=always |less -r
Hey Tatham,
Nice stuff mate. It is something we have also adopted. Wasn’t aware of the outgoing() function, thx for that. However, some members of our team feel compelled to push before end of day to ensure it is committed to a non local repo. Obviously forking is a way around this but seems excessive, or perhaps even branching. Either way creates a ‘noisy’ repo, What do you guys do?