Is Your Source Control Usage Conducive to Code Review?
Editorial Note: I originally wrote this post for the SmartBear blog. Head over to their site and check out the original. While you’re there, have a look around at posts by some other authors as well.
I can think back to times in my career that the source control that I was using (or not using) made me a cranky, unhappy human being. Years and years ago, there was the time that a coworker accidentally left all of the files in the codebase checked out through Visual SourceSafe and went on vacation. I distinctly remember enlisting a sysadmin and the two of us going into the source control server with admin credentials and hacking at settings until we could undo that and I could work. You see, Visual SourceSafe employed a pessimistic locking strategy by which his checkout meant I couldn’t do anything with the code.
There was also the time, a few years later, when I was suffering through a project that used Rational Clear Case. On a normal day, delivering code to the official branch or stream or whatever took half an hour. If I had to work from home, it took all morning.
And then there was the time that I was switched onto a project with no source control at all. The C source code was stored on a production server — a production server that controlled physical machinery in the real world. To “check things in,” you would modify the C code, turn off the physical machine, load the modified kernel modules, turn the machine on, and then revert real quick if things started blowing up. I’m not kidding. This was the commit/rollback strategy when I arrived (I did actually migrate this).
Tools Affect Behavior
These things make for fun war stories, but they also serve to illustrate how source control dictates behavior. With Visual SourceSafe, we implemented some kind of out of band email protocol to remind people to check in. With Rational Clear Case, I implemented a homegrown SVN for day to day version control and delivered/integrated only a few times per month. With the machine server, there was extensive historical commenting in every single source file. These tools spur you toward behaviors, and, in these cases, toward wasteful or bad behaviors.
For the examples I listed, I was steered toward useless process, steered away from continuous integration, and steered toward neurotic documentation. But the steering can apply to almost anything, and that includes having a healthy code review process.
There have been studies conducted that demonstrate the importance of code review. It is uniquely effective when it comes to catching defects earlier than later, and it promotes collective code ownership, thus reducing “bus factor.” I could go on, but let’s take it as axiomatic in this post that you want to do it.
Does your source control situation make it easy for you to conduct code reviews? Or does it discourage you, making life tough if you do them, and thus making you less likely to do them. If it’s the latter, that’s not a good situation.
I’m going to list some considerations here to look for in your source control setup and process. Please bear in mind that I’m not talking simply about whether you use Git, SVN, Mercurial, etc. I’m talking about the entire setup, including the tool, tool plugins, workflow around the tool, etc.
The Ever-Important Diff
If a coworker delivers code and wants you to take a look, how easy is it to do so? Does the coworker have to email you and enumerate the files and line numbers changed? Do you then go in and look?
Or does your setup allow for the concept of “diff,” meaning that you can see for yourself what was changed between two versions. Done well, the diff will allow you to see a list of the files that have been touched, and also to see the before and after of each file. In fact, some source control plugins even allow for what’s known as a semantic diff that not only calls out that text has been changed, but what sort of change it is within the context of the language (e.g. this commit moved a method from class A to class B).
If your setup does not grant you diffs, it becomes genuine work to figure out what is different. That work translates to friction for the reviewer, and that friction makes reviews less likely.
Accessibility for Reads
When I was using Clear Case, it wasn’t just commits that were onerous. Though not as time consuming as the delivery, simply reading files that someone else has delivered was a hassle as well. With that setup, the prudent course of action prior to looking at a given changed file was to open the diff and then go grab a cup of coffee.
And given that the setup in the first place drove us toward large batches, imagine this repeated over all of the files touched in 2-3 weeks worth of writing code. Performing a code review thus became a chore that consumed hours and hours of time. We did our best to be diligent about it, but even resolving to go through exhaustively did not mean we weren’t a little punchy and hasty by the time we got to the last few files. It was often too tiresome to do anything but apply a rubber stamp.
This also can apply to another scenario: offline review. Do you have a centralized source control implementation that creates overhead in retrieving the files? I travel a lot, and reviewing code in a taxi or on the airplane is not at all unusual for me. I’m a lot more likely to do reviews if it’s easy for me to get at the files when and how I prefer.
Integration with Issue Tracking
A more advanced consideration is the degree to which your source control setup integrates with an issue/work item tracking tool, like JIRA or TFS. If your organization has artifacts like bug tickets or user stories that trigger programmer work, then you’re probably used to a certain kind of workflow. It might go something like this.
- You get an email that something is assigned to you.
- Click the link, review the work item, make initial notes.
- Implement what’s needed for the work item.
- Deliver the code.
- Update work item notes, and mark complete (or whatever the next state is).
But now, let’s imagine that you can tightly integrate the code review process here. Basically, you add a step after (3) for “request a code review,” which generates a code review work item for someone on your team and presents that person with your changeset. They log in and do the code review, and then mark it as complete, which restores the flow to you on number (4).
Think of the difference. In the original scenario, you have to remember to request and schedule code reviews, and it’s somewhat ad-hoc. In the latter scenario, it’s an enforced and ingrained part of your day to day process.
Going Further
These are just a few examples that I’ve called out. There are plenty of other ways that your code review process might be affected by your source control tool. Hopefully you can think of some that apply specifically to you and start turning version control war stories into success stories, even if it means reconsidering your tooling.
