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).
Meld side-by-side diff
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.
Claws-Mail Actions
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:
Claws-Mail, git and Meld in action
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.
