What's the best way to review a code before it's committed to the trunk? (SVN)

Question

What's the best way to review a code before it's committed to the SVN trunk? One idea that I am thinking of is to having the developer to commit his code to a branch and then reviewing his code while merging the branch revisions into the trunk. Is this a good practice? If not, what else could be done to review a code before it's committed to the trunk?

Answer 1

There are two schools of though - what you propose or "review before commit". Most of the The differences can be seen as negatives and/or positives. - eg No tracking of changes caused by a review - do you want to see these as discrete commits, or are you only interested in the final work?

Review before commit - no branching required (although it can done if desired), must give reviewers access to working folders. Code can be changed during and after review with no tracking. Corrections caused by the review do not appear in the repository.

Review After Commit (on a branch) - Need to spin a branch for each review (although this may be in the workflow already). Code submitted for review cannot be changed without tracking of the changes. Someone has to merge the reviewed branches, and keep track of whats been reviewed and what hasn't.

It largely depends on the team culture and experience. Whats you trust model, what it the main purpose for reviews? I personally prefer review after commit, as it allows changes as a result of review to be tracked. We now use Git and Gerrit as they provide a nice balance between the two options.

Answer 2

1. Before committing run 'svn diff' to generate a patch file.
2. Send patch file to reviewer who applies it to a clean copy of the trunk.
3. Reviewer goes over changes using difference viewer of choice.
4. Reviewer performs build and runs tests.
5. If all goes well tell developer they can check their changes. If
   there are problems the developer goes back to step 1.

Answer 3

There's the ideal world, and then there's the real world.

In the ideal world, all your code is tested, so you can be sure that anything that gets checked in will either work or you'll know it's broken because it fails one or more tests. Plus, anyone not so experienced will be paired with someone who is experienced, so code reviewing is done on-the-fly (you commit as you go).

In the real world, things are different. Business wants that change live now and will tell you, with a perfectly straight face, that yes, you'll get time to clean up the code and add test cases later. You probably won't have time to code review everything and the percentage of code that is covered by tests continually decreases. The main reason for code review is for junior developers to learn from senior developers (when there's time for that) by having a more experienced person look over changes and suggest "better ways of doing things (TM)". You'll have senior developers just committing unreviewed code. Branching just for a code review and then merging is a huge waste of time. One way to overcome this problem is to declare a regular weekly 2-hour (or so) team meeting where you pick one or two recent changes people worked on at short notice and get their authors to "present" their approach by looking at the code together on a projector or something This can lead to some interesting discussions (typically goes off-topic quite a bit) but generally improves everyone's understanding of how to do it right. Plus the pressure of possibly having to present your code makes some people do it better ;-)

Or you might be lucky and get to work in a real-world environment where it's not so hectic, programmers are actually appreciated for what they do instead of abused, and there's time to do everything right. In which case my answer would be: try some of the different methods suggested in answers here and see which one fits your team and the way you work best.

Answer 4

Branches should work OK, based on my experience of using them in pre-commits reviews at prior job.

_{Note back then, we were using pre-commit reviews only for critical patches to production release candidate code, so there weren't much branches (routine changes were passed through post-commit reviews).}

Since you seem to be going to use pre-commit reviews for all the changes, you might need to manage large amount of branches. If you expect developer to make one "reviewable" change per week on average, you'll end up having about 50 branches every year for every developer in team. If you are using smaller chunks of work - like ones taking 1, 2, 3... days - multiply 50 by 2, 3, 5... accordingly.

Below are few other considerations to take into account if you want it best way.

1. handling cases when delayed review blocks code needed for other team members

Establish, monitor and resolve conflicts related to code review deadlines. Per my recollection of pre-commit reviews to routine changes I dealt with in one of the past projects, reasonable deadline is about 3 days and time to start worrying about is when review isn't completed more than 1 day after submission.

_{For comparison, at post-commit reviews these requirements are much more relaxed (I'm using 2 weeks deadline and start worrying after 1 week) - but since you target pre-commit reviews, this is probably not interesting.}

2. merge conflicts when submitting reviewed code

What to do if commit for reviewed code is blocked by conflicting changes committed by someone else while code was waiting for review?

Couple of options to consider are

• roll-back to beginning and require developers to re-implement and re-review the change
  For that case you might need to address a negative impact on team morale this might (will!) have.
• pass the responsibility of merge to other team member ("merge master")
  For that case, you will also need to decide whether merges per se should go through pre-commit review or not - and if yes, then what to do in case if that merge in turn meets another conflict.
• ignore changes done to reviewed code at the stage of merge
  For that case you might need to address a negative impact on team morale related to the fact that code committed differs from one that was reviewed.
• invent a way to avoid conflicts
  Straightforward approach is to allow only one developer at time to modify particular set of files - although this won't protect you from the kind of changes that don't modify files directly, but impact them through internal API changes. You might also find out that this kind "pessimistic locking" makes system-wide changes and deep refactoring quite troublesome.

_{For comparison, there would be no issues of that kind in post-commit reviews (since these deal with code that is already merged by definition) - but since you target pre-commit reviews, this is probably not interesting.}

3. load developer who is waiting for review

Establish an explicit policy for whether developer who submitted review should switch to new task or do something else (like eg chasing reviewer).

_{For comparison, post-commit reviews hardly need explicit policy (since it's natural to proceed to next task after you committed code and taking into account that review deadline is a week or two) - but since you target pre-commit reviews, this is probably not interesting.}

Answer 5

Any piece of development that needs a review would have to be in a separate branch. So, the branch should already exist before the time comes for review. Then the step would need to be:

1. Review
2. Revise (possibly)
3. loop back to Review (possibly)
4. Merge into trunk

Merging is the difficult bit. The longer the branch stays independent, the harder it is going to be to merge it back into the trunk. (It might also be harder to test.)

Cross-merging is a possible solution. Before merging into the trunk (step 4, or even earlier, say before step 3 or step 1), merge the trunk into the branch. The developer can do it and test it. Then the branch catches up with the trunk and it becomes easier to merge it into the trunk. Merging into the trunk is best done by you, or whoever is in charge of the trunk.

Some people try rebase instead of cross-merge. Some people argue that rebase is evil. That is another debate.