views:

502

answers:

5

I was wondering what mechanism have people used to enforce and track peer reviews for committing to SVN.

For each commit I want to be able to track if people have had a peer review (I am not sure whether enforcing a rule where they can't commit is a good idea, my gut feeling is that it shouldn't be enforced as there a probably valid reason for committing without a peer review), but the unreviewed commits should be able to be tracked so that before a release we can view all these commits.

+1  A: 

Personally I can often commit to svn close to 50 times per day, so I wouldn̈́'t like it ;)

I know Atlassian's greenhopper plugin for jira can be run as a pre-committ hook to require a jira reference in the commit. If you use greenhopper for planning development tasks too, you can enforce that all commits should be connected to a jira issue.

And even if you're not using greeenhopper you could probably create a pre-committ hook that enforces a commit-comment containing some issue-tracking reference number.

This should probably let you gather the collected changeset associated with a jira issue, which sounds like a better idea to me.

krosenvold
+2  A: 

You could require every developer to add the tag of the reviewing developer to the commit in a fixed format like [REV:XYZ]. This allows you to filter the history. As was mentioned here, you can use the pre-commit hooks of your source code versioning tool to enforce this if you need to.

Then the rule that no code may be committed without a peer review needs to be communicated to all developers. Also show the advantages of reviewing code before committing. The first few months, someone should check the commit history for commits without reviewer tag. If they find some, ask the developers why if they did a review and with whom. When there was no review, remind them of the rule. If some developers violate this rule knowingly several times, there could be repercussions, but peer pressure usually prevents this from happening.

Normally, there is no excuse for not doing a review when this rule is in effect. Especially the "But I had to fix this bug quickly to deliver the customer the fixed version" can't be an excuse, because a developer is more likely to make errors in a rush.

Sebastian Dietz
+1  A: 

This question sounds like an advert for a distributed VCS like git or bzr: You need developers to be able to commit changes often, but on to their own branch, which then gets reviewed before being pulled in to the central repository.

pdc
+1  A: 

Developers could create a branch for every change (not every commit). They could commit as many times as they like in this branch. However before the changes are merged into the trunk you could enforce a peer review. So your trunk will only ever contain peer reviewed code.

You could do this via permissions, so only select people can commit to the trunk, and they can check the code is ok before doing so.

I have been using version control for a while and think the working in branches model (rather than working in the trunk) is good,. DVCS systems like git take this one step further, and seem to be working well for a lot of people.

Jeremy French
In not a branch per-feature, you can have developers work in their own branch as well.
Steve Mitcham
A: 

One way you can do it is with a reviewer-commits rule on your team. That is, developers are not allowed to commit their own code, they have to convince someone else to commit their code on their behalf.

I've worked on projects like this before, and it works fairly well. The downside is that with Subversion, this means that the reviewer's name, not the developer's, shows up in the log. With git, you can keep the developer's name, and the reviewer adds a Signed-off-by: line.

Michael Melanson