FreeIPA requires that all code receive an ACK in a peer code review prior to submission to the main repository.  While some patches can be reviewed inline in the email, more often the patch makes no sense without context.

Heres’ the way I have been reviewing patches recently.  Feed back is encouraged!

Lets assume that Endi Dewata sends me a patch for redoing some aspect of the details page and then another removing dead code.

Create a branch for the patch or series of patches.  Usually this is <username>-<feature>

#save my current work
git stash
#these next three steps could be streamlined, but I like to perform them by hand to maintain context
git checkout master
git fetch
git rebase origin/master
#create the new branch and check it out
git checkout -b edewata-details
git am ~/Documents/IPA/patches/freeipa-edewata-0094-Restructuring-details-page.patch
git am ~/Documents/IPA/patches/freeipa-edewata-0095-Removed-unused-code.patch

Now, to code review the first patch:

 git difftool  HEAD~2 HEAD~1

and the second patch

 git difftool  HEAD~1 HEAD

On my system (Fedora 14 with no customization), difftool grabs the default, which is meld. I also use kompare but I find them both to be equally up to the task. What is important is that they not only show the lines that have changed, but show the changes within the lines via differential highlighting.

Difftool example: meld in action

git difftool will call the graphical application once for each file that has been changed in the patch. A session will look like this:

ayoung@ayoung ui]$ git difftool  HEAD~1 HEAD
merge tool candidates: meld opendiff kdiff3 tkdiff xxdiff kompare gvimdiff diffuse ecmerge p4merge araxis emerge vimdiff

Viewing: 'install/ui/details.js'
Hit return to launch 'meld': 
merge tool candidates: meld opendiff kdiff3 tkdiff xxdiff kompare gvimdiff diffuse ecmerge p4merge araxis emerge vimdiff

Viewing: 'install/ui/test/details_tests.js'
Hit return to launch 'meld':