ITK/Gerrit/ReviewPrimer

Gerrit code reviews can be relatively straightforward, but also give the reviewer a chance to look deep into proposed changes building and testing those changes locally. This primer is intended to guide the reader through the process of a deep review of a code change.

Prerequisites

This primer assumes a working knowledge of how to build ITK. No details will be given as to how to build and test the code (but they can be found here). This primer also assumes a working knowledge of git. Please see the ITK git pages and/or git books (ProGit, Community Book, or Version Control with Git).

Also required is a Gerrit change to review. We will go through a real life code review of a change submitted to Gerrit.

Select an open change

The primary review site for Gerrit/ITK is http://review.source.kitware.com. From this page, changes may be reviewed, etc. Our review will be of Change I5e76253f: Removed unecessary commented out debugging code commited on Sept. 17, 2010. Because Gerrit is dynamic, this primer will include many screenshots, YMMV.

The screen below is from Gerrit and contains several sections of interest.

A This section gives details of the change author (Hans Johnson), project(ITK), destination branch (master), topic (removeUnnecessaryComments), dates and status.
B This section is the commit message. The standard format for a commit is a single line summary, blank line, and longer description of the change.
C This section gives the details of the reviewers. A developer may request certain registered developers as reviewers for the change, and any Gerrit registered user may comment.
D This section gives details on the submitted code. It provides details on how to pull the changes from Gerrit. At the bottom is a list of files changed and links to diffs.

Review steps

• Review the code in Gerrit
• Use git to pull the changes
• Build, test, review locally
• Optionally, modify the commit and push changes back into Gerrit for review
• Score the change with comments
• If the change is ready, push it into the official repository

Review the code in Gerrit

The Gerrit developers have created a very useful system for code review that includes helpful shortcuts. The available shortcuts can be accessed by pressing the '?' key.

From the change we are reviewing, press <enter> or "o" to open the diff viewer. You may also press the "Side-by-Side" link next to "Commit Message". This brings up the difference viewer.

The first file to review is the commit message. At the top of the screen are some options controlling the display of the diffs. Change to suit your tastes.

Since there is not much to see, press ] to move to the next file:

Lines 12-14 have been commented out in the change. Individual lines in the diff may be commented on by double clicking the line (when you are logged in to Gerrit). These comments are preserved in Gerrit and shown to the author and reviewers when the review is completed.

Let's comment on line 14:

Clicking "Save" when finished:

Useful navigation commands

• n Move to next diff within a file.
• p Move to previous diff within a file.
• ] Move to next file.
• [ Move to previous file.
• f Show/Hide list of files.

Use git to pull the changes

Press u or click "Up to changes" to return to the main change page.

Gerrit implements a full git server and provides hints as to how to pull this particular change. Be aware, that the hints are mainly command lines.

Gerrit allows git to use anonymous HTTP, SSH and authenticated HTTP to pull changes. The protocol is set using the radio buttons on the right hand side of the Download section of Patch Set 1. Git has several different ways to pull the changes:

• checkout Pull the change locally and set the current branch to be this change.
• pull Pull the change locally, but don't do anything else (current branch is unchanged).
• cherry-pick Pull the change and cherry-pick it into your current branch. Cherry-picking adds the changes as a commit on your current branch. NB: cherry-pick adds a commit to your current branch!
• patch Pull the changes, and apply them to the current branch, but don't commit.

In our case, let's do a checkout using anonymous HTTP (the default). Click the clipboard icon to the right of the git command to copy the proper command to the local clipboard and paste into a terminal:

git fetch http://review.source.kitware.com/p/ITK refs/changes/79/79/1 && git checkout FETCH_HEAD

If you notice my command prompt:

revelation:ITK((no branch)) blezek$

I have added a little bash magic to have the current git branch displayed in the prompt. Very helpful as a reminder to what I'm working on. The magic is:

# Git branch on the prompt
function parse_git_branch {
 git branch --no-color 2> /dev/null | sed -e '/^[^*]/d' -e 's/* \(.*\)/(\1)/'
}
PS1='\h:\W$(parse_git_branch) \u\$ '

So why didn't we end up on a branch after our pull? When we pulled from Gerrit, we simply copied the information necessary to construct the change set. Any information about branches, etc. didn't get pulled from Gerrit. When get performs a fetch, the commit fetched is pointed to by a special FETCH_HEAD tag. In our case, we grabbed the changes and switched to them. Since there is no branch available at this commit, our prompt reports that we are not on a branch (and are in the "detached head" state). This is confusing, but reading a good git book a few times helps. For now, just trust me that we are in good shape.

Making the fetch better

If we look at the git log we see:

git log
commit 5e76253f0ae70dec8e0b849bbb25799a3cd9c530
Author: Hans Johnson <hans-johnson@uiowa.edu>
Date:   Fri Sep 17 19:31:37 2010 -0500

    Removed unecessary commented out debugging code.

This is helpful, but we don't have the Gerrit Change-Id, so it's hard to push changes back into Gerrit. It is possible to use hooks written by Kitware to add the Change-Id lines to the log automatically, and this is the advised way to work. If the Change-Id line is not present in any commits you pull there is no problem unless a commit is rebased or edited. In that case you can simply copy and paste the full Change-Id line into the last line of the commit. Assuming you would like to insert the Change-Id line into the last commit,

$ git commit --amend

Then copy the Change-Id line from Gerrit into your editor, save and close to commit the updated message.

Build, test, review locally

It should be noted that if you are reviewing a topic branch in Gerrit, then you can find the tip of the topic branch (the last commit in the series) by clicking on "Needed-by" in Dependencies until "Needed-by" is blank (meaning no other commits in Gerrit need this patch. Following the checkout instructions above on the final commit in the series will fetch all of the other commits into your local tree and check them out. To verify what commits are in your local checkout that are not merged,

$ git log --stat origin/master..

If the topic branch you are reviewing is from a contributor who does not have push access to the main repository a reviewer will need to stage and merge the topic too. This can be done by making your detached head into a local topic branch,

$ git checkout -b topic-name

This can then be treated as any other topic branch. You can push it to the topic stage and merge it as normal.

Optionally, modify the commit and push changes back into Gerrit for review

Hmmm, I don't actually know how to do this... Sorry... Hopefully Brad King or Brad Lowekamp can fill this in.

Score the change with comments

Back on the change page, we click the "Review" button to enter our review.

Typically, a change requires a score of +2 to be incorporated into the official repo. Certain reviewers are granted the +2 ability, while normal reviewers can give a score of +1. In this case, the changes were straightforward and so I scored a +2.

If the change is ready, push it into the official repository

Now that this change is ready, we can push it to the official ITK repository. There are two methods for pushing changes, depending on your rights as an ITK developer.

Staging changes

Changes may pass through the ITK staging area. The basic instructions for this step are documented elsewhere.

First create a topic branch based on the changes we want to publish.

git checkout -b removeUnecessaryComments

Next, we setup a remote corresponding to the staging area for ITK.

git remote add stage git://itk.org/stage/ITK.git
git config remote.stage.pushurl git@itk.org:stage/ITK.git

The first line adds a remote called "stage" to our repo, and the second line configures that remote to push via SSH.

Before we can push our changes, we need to be up to date with the staging area, so we fetch from the stage remote.

git fetch stage

Now our repo is in sync with the staging area, and we are ready to push the changes.

git push stage HEAD

Kitware has setup a mechanism for interacting with the staging area through SSH. The first step is to print the staged topics:

ssh git@itk.org stage ITK print

Running this command:

revelation:ITK(removeUnecessaryComments) blezek$ ssh git@itk.org stage ITK print
 removeUnecessaryComments | master=0

So we can see our branch (removeUnecessaryComments) is now staged. The next step is to ask the server to merge this topic into the mainline.

ssh git@itk.org stage ITK merge removeUnecessaryComments

Running this command:

revelation:ITK(removeUnecessaryComments) blezek$  ssh git@itk.org stage ITK merge removeUnecessaryComments
Fetching upstream master
Merge topic 'removeUnecessaryComments'

5e76253 Removed unecessary commented out debugging code.

Pushing upstream master
To ../../ITK.git
Deleting fully merged topic 'removeUnecessaryComments' (5e76253) from stage.

The remote server added this commit into the ITK master. To check, we ask the server to print staged topics:

revelation:ITK(removeUnecessaryComments) blezek$ ssh git@itk.org stage ITK print
No topics staged!

Direct commit to the ITK repo

Rather than use the somewhat unwieldy staging process, properly authorized developers can directly push to the ITK official repo. Since the changes are already pushed into the official repo, I'll just present the commands and not actually execute them.

In my workflow, I have a public GitHub clone of the official ITK repo. My local repo is cloned from my GitHub clone of ITK. To push changes up to the official ITK repo, I can make a remote to have a shorthand.

• Create the remote
• Fetch remote changes
• Create a topic branch
• Merge local changes
• Push new changes to the official ITK repo

Create the remote

A remote is a git shorthand for a URL. We need to create the ITK remote:

git remote add ITK git://itk.org/ITK.git           # The ITK remote
git config remote.ITK.pushurl git@itk.org:ITK.git  # Change the push side to use SSH

By default, the pull and push URLs of ITK are git://itk.org/ITK.git. The git protocol is read only, so we need to configure the push side of the ITK remote to use SSH (git@itk.org:ITK.git is understood to be SSH).

Fetch remote changes

Let's fetch in the latest changes from the official ITK repo to make sure we are in sync:

revelation:ITK(master) blezek$ git fetch ITK
remote: Counting objects: 78, done.
remote: Compressing objects: 100% (60/60), done.
remote: Total 64 (delta 36), reused 1 (delta 1)
Unpacking objects: 100% (64/64), done.
From git://itk.org/ITK
 * [new branch]      dashboard  -> ITK/dashboard
 * [new branch]      hooks      -> ITK/hooks
 * [new branch]      master     -> ITK/master
 * [new branch]      nightly-master -> ITK/nightly-master
 * [new branch]      release    -> ITK/release

Create a topic branch

Since I used fetch, the ITK repo's changes are stored locally. I next need to check them out, and create a new branch:

revelation:ITK((no branch)) blezek$ git checkout -b GerritPrimer ITK/master
Branch GerritPrimer set up to track remote branch master from ITK.
Switched to a new branch 'GerritPrimer'

I've now created a branch called GerritPrimer. git reminds me that it will now track ITK/master when I do fetches or pulls. This may or may not be a good thing...

Merge local changes

Edit, test, commit (see Workflow Primer).

Push new changes to the official ITK repo

Pushing changes is really straight forward. Remember that GerritPrimer was configured to be a tracking branch? This means that if we have GerritPrimer checked out, pushes and pulls interact with the ITK/master branch on the official repo. Neat huh?

 git push    # Nothing will come of this

Did it all work?

Let's check back into Gerrit to see if our push to the staging area and official repo was properly merged and picked up by Gerrit.

Indeed, it is. We can see the status is now Merged and an automatic comment has been added that the change has been successfully pushed to the official repo.