Extract relevant changes for code review

Question

I'm working on a project with a team where we check in early and often. Individual checkins are completed to the satisfaction of the developer doing the change (including tests where possible) but sometimes the direction of work changes slightly and previous commits need to be reverted and done another way. Or, stub code is filled out in later commits.

When it comes time for code review, there's a sequence of commits all tagged with the same bug tracking id number. It's easy to get a list of such changes. When a reviewer looks through the changes one by one, sometimes there will be a commit A that is undone or modified by a later commit B as part of the same review. This can make reviewing more difficult.

If only one developer was working on the file for the duration of the change, then it's easy to do a diff between the original state of the file and the final state of the file. The problem arises when another developer happens to have made unrelated changes in the same file, or even in the same functions.

How do you handle this situation? Are there tools that, given a sequence of patches to a file, can give the moral equivalent of a diff between the first and last versions, but only including a subset of those patches?

Come to think of it, I could create a temporary branch in git starting from before the first related change, and cherry-pick the changes relevant to the review. Hopefully there won't be too many conflicts that need to be resolved (and if there are, then the whole lot should be reviewed at once anyway). Any other ideas?

More info: This happens to be a big legacy system where a single change might touch multiple files. The files are big and crufty - too big to just review the final product without an indication of what might have changed.

Answer 1

I think you should be branching to do your changes. See my post here.

Answer 2

Uh, just look at the last state of the file.

(Branching for each change is a PITA. Don't do it.)

Answer 3

It turns out you can do this fairly nicely in IntelliJ IDEA if you've got that tool:

Select Version Control | Show Changes View.

On the left hand side you select repository and click all of the revisions you want to review.

In the right hand pane you will get a list of all the files that are affected by the revisions you have selected. When you choose "diff" you will see the internal changes in the selected changesets. Internal re-works that occur within the commits are not shown (as can be expected)

Answer 4

See our http://www.semdesigns.com/Products/SmartDifferencer/index.html for a tool that is parametrized by language grammar, and produces deltas in terms of language elements (identifiers, expressions, statements, blocks, methods, ...) inserted, deleted, moved, replaced, or has identifiers substituted across it consistently. This tool ignores whitespace reformatting (e.g., different line breaks or layouts) and semantically indistinguishable values (e.g., it knows that 0x0F and 15 are the same value).

This won't disentangle two sets of changes made for fundamentally different reasons. But it will minimize the size of the deltas you have to inspect, and that will surely help.

You say the files are thousands of lines long. This sounds like COBOL. There's a smart differencer tool for C#, PHP, Java, C++ ... and COBOL, with examples for most of these at the web site.

Answer 5

One approach (using subversion terminology, but similar approaches should work with other systems, too):

Check out a working copy at a revision just before the first changes. Then merge all related commits into your working copy.

Now you have a working copy which differs from its base just by the relevant changes. You can review this directly, or create a patch from it for review.

If some unrelated change after the base revision overlaps with the to-reviewed changes, you might get a merge conflict, but this should be rare.

Answer 6

From a Subversion point of view

Given your requirements i would run code reviews by diffing against a tag from the last review; i.e. diff between two tags.

Answer 7

If you have the ability to switch source control systems, you might seriously consider git (or mercurial or other similar tool).

Each developer creates their own local repository.

For each bug they work on, they create a branch.

When they're done with the changes, they can play with their history using rebase commands to bundle up changes into coherent checkpoints.

The developer then submits the rebased changeset for review.

The integrator or reviewer can then accept the changes as-is, cherry-pick to grab what they want to keep, and/or do more rebasing to break up commits that shouldn't have been grouped together.

Answer 8

The patchutils package contains a combinediff tool that can "create a cumulative unified patch from two incremental patches".

Answer 9

I would do pretty much what you suggested. Create a review branch. Get the developer(s) whos changes are being reviewed to do the cherry-picking into that branch. Diff the root of the branch with the head.

I'd probably also create a little tool to grab all the checkins that match a given bug tracking id tag and mash them together into an approximate script to do the cherry picking. I'd definately give the user a chance to edit this script before running it though as I'll bet some stuff gets miss-tagged.

Do the review on the branch. Make any edits on the branch. Cherry pick the edits back to the trunk. Discard the review branch if you don't want it anymore.