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.

When in doubt, check it out

Of course, if you really want to see how a branch works, don't be afraid to do a local checkout of it.

It might be a good idea if you're checking out someone else's branch to make a "tracking branch" so that if you make changes, they aren't lost. Putting the remote in the name makes it easier to remember where it came from:

 git branch joar/fiddlybooks joar-fiddlybooks
 git checkout joar-fiddlybooks

How to add add review notes on a bug?

Since we aren't using a review tool like reviewboard or the like, and it's easiest to keep our information in one place, ,it's best to actually add comments on the ticket itself. You can just talk about sections of the code like so on a ticket:

 This section looks good:
 
 {{{
 def monkeys():
     import bananas
     bananas.peel()
 }}}
 
 but instead of importing in the function, move it to the top of the commits.  Blah blah blah

Quoting smallish sections of code like this is fine. If you're quoting somethig larger, you may want to snip bits of it:

 It looks like you're gathering bananas twice here:
 
 {{{
 def monkeys_long():
     these_bananas = bananas.gather()
     # [...]
     these_bananas = [
         banana.peel()
         for banana in bananas.gather()]
 }}}
 
 ... you could probably just use the one lisp comprehension

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)