Code Review Best Practices Using Git and Collaborator
For a while now, Collaborator has had support for creating code reviews from changes located in a Git repository. However, we’ve seen a lot more adoption of Git (and other DVCSes like Mercurial and Bazaar) in the past year. Users setting up Collaborator for use with Git often ask us for recommendations for the best way to do code reviews. I’ll outline best practices in this post. At least, I’ll outline the workflow that we on the Collaborator team use and recommend whenever we’re asked. If you use some other workflow with Git and Collaborator, feel free to reply and let us know about your own process.
First, a caveat
Back in 2008, when the Collaborator team wrote our initial Git integration, we were not ourselves Git users. We still used Subversion to manage the source code of Collaborator itself. We’d had a few requests for some Git support, so we added the simplest command that would provide support for the broadest number of workflows, ccollab addgitdiffs. In Collaborator Version 6, we extended our Git support to include ccollab addchanges, which allows for pre-commit reviews like many of our users perform with Subversion.
It wasn’t until after Collaborator v6 was out the door that the Collaborator team took the time to migrate our source code into Git. Since then, we’ve been doing maintenance releases for v6 and new work for v7 in Git. Not only have we received continued feedback from our customers in that time, but we’ve been using our Git integration for ourselves.
What have we learned since then? Git isn’t like Subversion! Unlike Subversion, Git commits are fast, and they’re local, so there’s no reason not to do a commit of your code before uploading your changes to a review. This means that post-commit reviews are very easy to do with Git, which also means they’re common. In fact, judging from our internal workflow and customer feedback, post-commit review workflows are more common than pre-commit in Git.
Knowing what we know now, we can now see the importance of improving our support for post-commit Git reviews in Collaborator. With the next release of Collaborator version 7, we’ll be adding Git support for post-commit reviews, both in our GUI, and via the ccollab addchangelist command. For example, to add the change you just committed to your local repository to a new review, you will be able to select “Add Commits..” in the Collaborator GUI, or run the command:
ccollab addchangelist new HEAD>
These new commands are able to capture a bit more metadata about your changes than the addgitdiffs command, so you should find that adds of contiguous commits roll up in Collaborator much better than a series of changes added via addgitdiffs. That metadata also includes the full commit ID and commit message, which are automatically attached to your changes in the review.
In the meantime, you can simulate adding a single commit by adding the diff of that commit. So for example, to review the differences introduced in HEAD, you can diff the commit that came before HEAD (HEAD^) with HEAD. So, via the command line:
ccollab addgitdiffs new HEAD^ HEAD
If you’re going to do this a lot, you might try using a script to emulate some the new addchangelist functionality until the upgrade is available.
Workflow Best Practices
Finally, on to the recommendations! So, how do we recommend doing reviews with Git and Collaborator?
1. Use feature branches. (AKA: a “Topic Branches”) One of Git’s main advantages is cheap, fast, local branches. If you’re working on implementing a new feature or fixing a bug, you can separate that work into its own branch until you’re ready to integrate it back into your “master” branch. This works particularly well with Collaborator’s asynchronous reviews. If you want to make sure your code is reviewed before you merge it back into “master”, you can fire off a review, then switch to another branch to do some other work while you wait on reviewers to approve your changes.
2. Post-commit reviews. This is almost self-explanatory from the above. If you’re going to switch your focus and work on other code while your changes are being reviewed, you’re going to want to be able to get back to that code should you need to rework some of it. Doing post-commit reviews also allows Collaborator to include metadata about that commit in the review of that work. Should questions arise about the code in the future, you can easily find the review corresponding to that commit by searching for the commit ID in Collaborator. Or, vice versa, if questions about the review come up, the code that was reviewed
3. Commit rework(s) in your feature branch. If there are defects in your review, you’ll need to rework your code to fix the defects before the review can finish. Perform these reworks in the feature branch you used to create the original review. This way, your changes are always based on the code that is under review. Collaborator will be able to see that commit Y came directly after commit X, and roll up changes accordingly. This keeps the fixes tightly coupled with the code under review so that when you merge it back into your “master” branch, the code AND the fixes are all merged together.
4. Prefer merges over rebasing. When your review has been accepted, it’s time to include your changes in your “master” branch. This brings up the question -- if the branches have diverged, should you use merge, or rebase? Rebasing is a powerful tool in Git, but it comes at a cost. When you rebase, Git creates new commits that have the same code changes and metadata except that the commit IDs are different. This means that the commit IDs in Collaborator will no longer match those in your Git repository. This can make it difficult to tie code reviews to Git commits (and vice versa) in the future. If you like being able to do this (as we do!), avoid rebasing.
One interesting tidbit about code review in Git is that “pre-commit” vs. “post-commit” doesn’t quite capture the important aspects of the above workflow. In Git, commits are cheap. The more interesting characteristic is that we’re reviewing code before we’ve merged it into our “master” branch and pushed it back to our central/authoritative repository. It may be more accurate to call the above workflow “post-commit, pre-push”. You can also do “post-commit, post-push” reviews. You might use this if you need to get a quick bug-fix into your “master” branch without waiting for the results of the code review.
While the above summarizes how we do code reviews with Git on the Collaborator team, we’d love to hear from customers and the Git community about your workflows. Do you do Git reviews differently? Do you have tips to share with other users of Collaborator and Git?Please let us know!