Re: git and peer review

From: Toby Allsopp <toby.allsopp@navman.co.nz>
Date: May 4, 2008, 20:21 UTC
Message-ID: <87k5i9u8f1.fsf@nav-akl-pcn-343.mitacad.com>
In-Reply-To: <46dff0320805021802i1a29becflcae901315035a77d@mail.gmail.com>

On Sat, May 03 2008, Ping Yin wrote:

[...]

> I am in a company environment and i want to enforce a policy that
> every commit must be reviewed before pushed to central repository. I
> think i can use hooks to enforce such kind of policy.

I'm in a similar environment, although it's only me using git (via git-svn) at the moment.

Show 8 quoted lines

> One way i want to try is to check in the hook whether every pushed
> commit has a "Reviewed-by " line .  Any suggestion?
>
> And one question, how to add a "Reviewed-by" line automatically?
>
> The reviewers sit near each other, so we do face-to-face peer review
> and don't pass patches by email.
> Say,  i have prepared a patch series,

I'm very interested in good ways of doing this face-to-face review.

At the moment I'm using gitk to step through the patch series along with the patch to gitk that adds a context-menu entry to lauch an external diff tool when a side-by-side diff is easier to read.

This is okay, but it's a bit of a pain to make changes while the review is in progress (git rebase -i, s/pick/edit on the appropriate line, make changes, git commit --amend, git rebase --continue). Perhaps stgit or guilt would help with this.

Show 10 quoted lines

> Case 1
>     I ask someone to review my patches at my machine. If the review
> passes, i have to add Reviewed-by line to each commit and then merge
> it to the master branch. However, i find no easy way to add
> reviewed-by line. Maybe adding --reviewed-by  option to cherry-pick or
> rebase or merge?
>
> Case 2
>    The reviewer is the maintainer, so i ask him to pull and review. So
> now it is his turn to add review-by line. But still, how?

I do something similar using git filter-branch --msg-filter. I have a little shell script call git-add-checked (our convention is to have a "checked: " line in the commit message):

--8<---------------cut here---------------start------------->8--- #!/bin/sh

usage() {
    cat <<EOF
Usage: git-add-checked <checker> [<filter-branch options>] <rev-list options>
EOF
}

checker="$1" [ -n "$checker" ] || { usage >&2; exit 2; } shift

set -x git filter-branch --msg-filter "sed '\$a\\ \\ checked: $checker'" "$@" --8<---------------cut here---------------end--------------->8---

Then, after getting my changes reviewed, I just do:

$ git-add-checked joe.bloggs trunk..

This adds a "checked: joe.bloggs" line at the end of the commit message for all of the commits on the current branch since trunk (which is a remote branch maintained by git-svn).

Regards, Toby.