I always struggled at reviewing code.
Specially when the code to be reviewed is in reality a patch inlined in some e-mail… I hate monospaced fonts in my e-mail reader, and with all the context switches I got in my daily work, I simply can’t concentrate properly in order to follow what’s been proposed with that one patch out of many, in that long long patch series.
In the past, I used to apply them manually, then go over the code using Source Navigator and later cscope.
I still miss the ability to jump between symbol definition and use that cscope does the best, but I have a much more streamlined way of reviewing patches today, thanks to git, meld, and claws-mail.
The first thing is about git. Nowadays I use git in every coding project I use – even if the upstream project is not using git as SCM itself (I simply create a local repository and import). And this is not only for making reviewing patches easier, but all sorts of things, like fast branching and merging, easy cherry-picking, rebasing, commit amending, modern utilities et al. It’s really the 21st century version control system.
The second thing is meld. Meld is one good example of an intuitive interface that doesn’t get in the way. It can compare, merge and edit files (up to 3-way merge if needed). Supports all the major SCMs such as git, hg, cvs and svn (although I can’t find a reason why would anyone still use the last two, at least locally).
The forth thing, and where actually everything makes sense, is Claws-mail, which has the very useful (and unique?) ability to create custom actions to process messages.
Guess what happens when you combine Claws-Mail’s actions with a script that uses git and Meld? A very point-and-click way of reviewing patches:
The trick is in configuring an action in Claws-Mail that opens a terminal and calls a script. The script uses git-am to apply the patch contained within the selected mail message to some branch in your local git repository. After applying, it calls git-difftool to show the differences. git-difftool then calls any diff tool you might like (my suggestion stays with Meld).
I’m attaching the script for reference below:
#!/bin/sh ## git-review-step ## (C) Copyright 2010 Klaus Heinrich Kiwi ## Licensed under CreativeCommons Attribution-ShareAlike 3.0 Unsupported ## http://creativecommons.org/licenses/by-sa/3.0/ for more info. ## dirname is where the git tree is located. dirname=$HOME/sandbox/ock/sourceforge-git/opencryptoki if [ "$#" -lt 1 ]; then echo "Invalid number of parameters" echo "usage: $(basename $0) <patch1> [patch2] [patch3] [...]" exit 1 fi messages=($@) cd $dirname oldbranch=`git branch | grep -e '^* ' | cut -d " " -f 2` # Save any uncommitted changes in the working dir or index if git stash | grep HEAD; then savedchanges="yes" fi function restore() { echo "Reverting to original branch..." git checkout --force $oldbranch if [ -n "$savedchanges" ]; then echo "Restoring un-committed changes..." git stash pop fi } # Get branch to apply to git branch echo "Select branch to apply patches:" echo " Enter \"<branchname>\" to apply to an existing branch" echo " Enter \"<newname> [origref]\" to create a new branch from \"origref\"" echo " reference (use current branch and HEAD if left blank)" read -p "Apply patch(es) to branch (default is current):" -e -i $oldbranch newbranch origbranch if [ -n "$newbranch" ]; then if git branch | grep -e "\b${newbranch}$"; then echo "Applying to existing branch \"$newbranch\"" # Checkout if ! git checkout $newbranch; then echo "Error checkout out \"$newbranch\" - Aborting" restore read -p "Press Enter to continue" exit 1 fi else if [ -n "$origbranch" ]; then echo "Applying to new branch \"$newbranch\" created from \"$origbranch\" branch..." else echo "Applying to new branch \"$newbranch\" created from \"$oldbranch\" branch..." fi if ! git checkout -b $newbranch $origbranch; then echo "Error creating \"$newbranch\" from \"$oldbranch\" - Aborting" restore read -p "Press Enter to continue" exit 1 fi # if ! git checkout ... fi # if `git branch | grep ... fi # if [ -n $newbranch ... # Apply patches to working dir using git-apply amparams="--whitespace=error-all" while ! git am $amparams ${messages[@]}; do git am --abort echo "git-am failed. Retry (the whole chunk) with additional parameters?" read -p "git-am parameters (empty aborts):" -e -i $amparams amparams if [ -z "$amparams" ]; then echo "Aborting..." restore read -p "Press Enter to continue" exit 1 fi done for (( i=${#messages[@]}; i > 0; i-- )); do PAGER='' git log --stat HEAD~${i}..HEAD~$((i-1)) if git diff --check HEAD~${i}..HEAD~$((i-1)); then echo "WARNING: Commit introduces whitespace or indenting errors" fi git difftool HEAD~${i}..HEAD~$((i-1)) done echo "Restoring working tree to original state" restore read -p "Press Enter to continue" |
git-review-step by Klaus Heinrich Kiwi is licensed under a Creative Commons Attribution-ShareAlike 3.0 Unported License.
Based on a work at blog.klauskiwi.com.
