pre-commit code review

Question

I'd like to have such scenario: I have a repository and lots of folks committing to it. For some of them I prefer looking at their code first, before allowing them to submit their code.

There are many good code-review tools, but none actually allows me to easy support this workflow:

• Edit your code.
• Easily submit your code from within your-tool-of-choice (i.e. Eclipse) to review system.
• Have the code approved by someone else in review system.
• Submit the code. Repository will revoke it if it's not approved.

I want to avoid situations when people have to create diffs, send them to me over email, I will have to patch them... That should be a usable, intutive workflow that does not add overhead to having your code reviewed. Any suggestions?

This used to be a subversion question, but since lots of people are suggesting using something different than svn, I'll try to pursue this direction.

Answer 1

I've used PVCS for source control and it does what you want, but it would be a big change for a team used to SVN and it costs money - probably not worth it in general.

Answer 2

Perhaps you'd benefit from switching to a distributed version control system like Git or Bazaar. You should be able to maintain a system where you can check merges into the main repo.

The tools for each aren't as advanced as SVN's (yet) but they're quite alright. Just something to consider.

Answer 3

In Bazaar you can have a Human GateKeeper in workflow that would work as reviewer.

Commands in Bazaar are very similar as in SVN plus additional, excellent, functionalities (eg. shelving changes, etc.).

Bazaar has basic support for importing SVN repo with full history, supports pluggable modules and TortoiseBzr is in the works. You might also want to peek at QBzr.

Answer 4

My feeling is that this sort of workflow should be tracked in your bug tracking software. Trac, for example, will let you construct custom workflows in order to implement a code review step. It also has an XMLRPC plugin which would allow you to prevent ticket branches from being merged unless their associated ticket has progressed to the correct point in the workflow.

Answer 5

I'm not sure that there's an "ideal" way of doing this -- I have looked!

The two options that I have used are:

• A branch for every change. As long as you have a naming convention this isn't as onerous as it initially sounds. This breaks down if changes are often very small bug fixes or especially large features.

• The low-tech solution: review the code on your colleagues computer before he checks in the code, view on a shared drive or whatever is appropriate in your environment.

I have also considered options such as svk, which is a distributed version of subversion, but have no experience actually using it.

It's also possible to sync from svn to git. This would allow you to keep svn and gain your review workflow at the expense of complexity. Also sounds pretty fragile.

Answer 6

send patches to the mailing list of your project?

Maybe it would be a mission to develop something for this. Using two repositories and some web software which merges code from the "pre-commit" repos to the repos, when certain guidelines are met.

Answer 7

Code Collaborator ( http://www.smartbear.com/codecollab.php ) does everything you want. We use it with Perforce. Creating reviews are easily done from within the perforce client, or from within Eclipse if you like. Reviews are simply handled with a web interface. You don't have to make your own diffs. It tracks defects for you.

The only thing it can't do is physically prevent a checkin without a review, but if you don't trust your developers not to bypass the system completely then you've got bigger problems. Plus there's always that one time when you're coding alone at night and you HAVE to check the code in.

Answer 8

Team Foundation Server has the concept of "shelving code" which essentially creates a mini-branch that you can then pull down for review. At that point, you can then merge the shelved code into the main branch after approval. You can also set up the rights such that someone can only shelve code, and not check into the trunk. You would then grant rights to certain individuals to be able to do the merges, presumably after the proper review cycle.