Code review tips

From GNU MediaGoblin Wiki
Jump to navigation Jump to search
The printable version is no longer supported and may have rendering errors. Please update your browser bookmarks and please use the default browser print function instead.

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