Successful code review strategy with SVN and ReviewBoard?

Question

I've managed to introduce ReviewBoard to the coding workflow in my company, while "introduce" means having installed and presented it. We also have a general agreement that we need code reviews sorely, however, we are not quite sure how we'd like to do it.

Our main revision control is SVN, so we rather limited in branching and merging. Some strategies I've thought about:

1. Pre-commit review from the trunk. Pros include having a single patch, having no unreviewed code in the repository. Contras are having to keep your checkout clean or doing poor-man's branching with several checkouts
2. Post-commit review from the trunk. Works fine with Review Board, however it doesn't stop people from committing dirty code and also allows them to ignore review requests.
3. Post-commit review from a feature branch. Pros are obvious since a feature can be worked on independently, however there is a big pain in creating server-based branches and also a much bigger pain of keeping different branches synced. Also see item 2.

I'd like to make this as painless as possible, so there are several possible automated additions to the workflow, like having a robot committing code which earned at least X "Ship it!" votes and making Review Board "follow" a feature branch with commit-hooks. Still, I'm not sure which code review workflow might be the best for our team of about 8 coders. We won't be able to change revision control systems, i.e. git-svn and SVK are out of the question (while the latter is dead anyway).

Can you recommend anything from your experience?

Answer 1

Base you system on trust and accountability and keep it light weight:

1) Use good judgment: 'You can check anything in at any time. Ask for a code review when you need one.' Add 'reviewed by' to the comments if it was reviewed to facilitate quick reviews.

2) Review board is responsible for monitoring changes via commit hooks. If they see something they don't like, take it up with the developer. Different members can review different sections.

3) If a developer continues to check in crap without asking for review, fire them.

4) If there are sections of the system that are unusually complicated / central / easy to mess up - lock those down and require approval to check in.

5) Everyone can monitor check-ins. Reviewing isn't just for the review board.

I've seen this work with 2 developers and with 100.

Answer 2

A combo of your #2 and #3 (perhaps with abbreviated trunk reviews if the feature branch has already been reviewed) may work well. I find pre-commit reviews a bit of a stifling process -- better to have an enthusiasm (kept kindled by you?) for review infect the whole team.

I recommend perusing SmartBear's free book, Best Kept Secrets of Peer Code Review, which is a quite even-handed treatment esp. considering that its author sells a commercial code review suite. (I don't work for them, nor do I use their products, FWIW.)

That book will help you think through both possible workflows for your environment, and how to introduce the workflows, explaining to the team why you might want to aim for reviews of x-hundred LoC or less, or have a bit of a guided tour before reviews, etc.

Answer 3

We are in a similar position.

Do you have svn configured to email all your developers with every commit? This a good first step in keeping everyone honest. We send an email with the log message, the first 200 lines of the svn diff, and a link to the whole diff in trac (which basically only use for showing svn diffs).

If a developer thinks a change needs review after the fact, we use ReviewBoard to perform the review.

On the other hand, developers can also request a review before check in. Whether they developed the changes on a feature branch or in a trunk sandbox, makes no difference. We have considered adapting scripts to upload a review request from the command line, but the process is so simple we have not done so, at least yet.

My overall recommendation is to introduce a manual system, and automate it, and possibly enforce it with pre-commit scripts, once you are happy with the process. With a small team especially it is better to err on the side of peer-pressure enforcement because you want to minimize the number of little productivity killing processes.

Answer 4

We recently introduced ReviewBoard into our process. Before we added ReviewBoard we had the following things in place:

1. SVN with e-mail automatically sent to all developers for every check-in.
2. ViewCV integrated with to allow diffs to be viewed post-commit with a browser.
3. The SCM-bug script integrated with SVN so developers have to include a big id with their checkins.
4. Buildbot integrated with SVN to automatically run tests after every check-in.

Since we already had the post-commit stuff covered fairly well with other stuff, we use ReviewBoard as a pre-commit tool and only after we hit 'feature-complete' for a given release.

Answer 5

In larger developments, feature branches are inevitable. With SVN, it is not possible to "update" the feature branch from trunk as from working copy [TODO link SO question about it]. However, you may have frequent merges and creations of new branches.

By the way, RB knows to handle pre-commit reviews as well.