Code review tips

So you want to help with code review. Great! We have a review queue and if you have enough code review experience, maybe you'd like to help us there.

We don't use github though, and for the most part it's probably easiest to do "local" code review: pull down the remote of the person's branch you're reviewing, then use local tools to read through things.

For this set of examples we'll assume you pulled down Joar Wandborg's repository as "joar", and are reviewing the branch "fiddlybooks"

Basic local code review

This gives a set of suggestions on how to do code review for sane, command-line running people. (Crazy Chris Webber emacs using suggestions come later ;))

Reviewing commits local to this branch

First, install tig. tig is a command line git history browser.

You could view the branch:

 tig joar/fiddlybooks

That will show you the whole history. But what if you want to *just* show the commits that are new to this branch?

 tig master..joar/fiddlybooks

This will show you a narrowed log that only contains the branch that are new to this branch and not found in master. This means even if things are changing in master or it's been a while or there's been a bit of merging back and forth, this will make it much easier to read the history!

Reviewing the whole branch as one diff

But what if you want to review *everything* that's changed at once, in one big diff?

Diffing with master is not really ideal... what happens if there are new commits in master? You'll see way more in the diff than is useful. Add this to your ~/.bashrc file:

 function git-diff-commonbase() {
   git diff `git merge-base $2 $1` $2
 }

Now open a new terminal, change to your mediagoblin directory, and try running this:

 git-diff-commonbase master joar/fiddlybooks

This will show a diff between master and joar/fiddlybooks that's *only* the things that have changed since master.

Crazy emacs code review workflow

If you're an emacs user, Magit is super great. Some tips:

• If you're interested in seeing all of the branches that have new commits, you can use the built-in magit-wazzup.
• Chris Webber wrote something based on magit-wazzup for people who are reviewing an insane amount of branches, called magit-review. It's like magit-wazzup except you can move things in and out of review states and tell magit-review to ignore certain branches.
• When viewing things in the git diff view, if you're on a file that you'd like to see what it looked like before and after the commits, hit "e" for magit-ediff. Note: ediff takes a bit to learn, and by default it uses a crappy external frame with a horizontal split (you can switch to vertical by hitting "|"). Here are some better default settings for ediff:

 ; Don't break out a separate frame for ediff
 (setq ediff-window-setup-function 'ediff-setup-windows-plain)
 
 ; Horizontal splitting really ought to be the default, honestly.
 (setq ediff-split-window-function 'split-window-horizontally)