git and peer review

I have tried many online review tools: cucible, reviewboard and smartcollaborator. The are both great. However, they can't satisfy all my requirements:

1. poor git support
2. When reviewing changes between revision (or commit for git) a and
revision b, they display all changes as a single diff instead of one
diff for each intermediate revision

So finally, i decide to use git itself as the reviewing tool if i can't find better.

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.

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,

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?

On 03/05/2008, Ping Yin <pkufranky@gmail.com> wrote:

This sounds likely, yes.

Assuming this is enforced either through a template (see: commit.template in git-config(1)), or as part of being added by the committer, then in GIT 1.5.4 onwards there's a commit-msg hook which will do this for you. Something like:

test "" = "$(grep '^Reviewed-by: ')" || {
    echo >&2 "Message must have a Reviewed-by line present."
    exit
}

There's an example of that by way of a SOB in the commit-msg hook.

-- Thomas Adam

On Sat, May 3, 2008 at 9:54 PM, Thomas Adam <thomas.adam22@gmail.com> wrote:

Actually, i want a way to rewrite the history or reapply the patch series and add the reviewed-by line then. Before that, i can commit arbitrarily without any limitation.

Sorry, but what does SOB stand for?

On Sat, May 3, 2008 at 7:33 PM, Ping Yin <pkufranky@gmail.com> wrote:

Signed Off By ?

Regards, Frodo B

On Sat, May 3, 2008 at 10:22 PM, Frodo Baggins <frodo.drogo@gmail.com> wrote:

Or, right. THX.

On Sat, May 03 2008, Ping Yin wrote:

[...]

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

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.

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.

On Mon, May 5, 2008 at 4:21 AM, Toby Allsopp <Toby.Allsopp@navman.co.nz> wrote:

Great, very useful for me. THX.

On 2008-05-05 08:21:54 +1200, Toby Allsopp wrote:

Yes, StGit helps here. "stg edit <patchname>" lets you edit the commit message of any patch.

( In the master branch, but not yet released, is an emacs mode for
  StGit. It displays the list of patches (name + first line of commit
  message), and you can press "=" to view the patch (including the
  commit message), and "e" to edit its commit message. )

* On 2008-05-05 at 08:21 +1200 Toby Allsopp wrote:

That's a useful script, thanks for posting it. I'd like to make it a bit safer to use -- the first time I tried it I didn't give any branch limiting args and it started filtering a lot of history :-P

What I would like is a way for the script to determine the appropriate tracking branch. So that the usage would look like:

   git mark-reviewed someone@userprimary.net

and it would figure out whether it should do trunk.. or release-1.3.., etc. Can anyone point me in the right direction?

Seth Falcon <seth@userprimary.net> wrote:

Something like this, but its uh, ugly due to the use of a network connection:

	branch=$(git symbolic-ref HEAD)
	branch=${branch##refs/heads/}

	remote=$(git config branch.$branch.remote)
	merge=$(git config branch.$branch.merge)

	rb=$(git ls-remote $remote $merge | awk '{print $1}')

Then use a filter-branch on "$rb..$branch" as the range.

You may be able to just assume that the remote name is the refs/remotes prefix and instead do:

	branch=$(git symbolic-ref HEAD)
	branch=${branch##refs/heads/}

	remote=$(git config branch.$branch.remote)
	merge=$(git config branch.$branch.merge)
	merge=${merge##refs/heads/}

	rb=refs/remotes/$remote/$merge

but that's an assumption, and we all know what happens when you assume things.

Technically you need to look at remote.$remote.fetch lines in .git/config to figure out the rewriting rules from what branch.$branch.merge contains to what refs/remotes/* might be, and doing that is not quite as trivial as either using ls-remote or assuming your users have the standard layout created by git-clone and git-remote.

* On 2008-05-18 at 23:37 -0400 Shawn O. Pearce wrote:

Hrm. My use case is with an upstream svn repository and git-svn. With my .git/config, remote and merge as above are empty (I guess that is what one would use with a pure git setup).

For the git-svn case, I think what is needed is the ability to ask git-svn about the local upstream tracking branch associated with HEAD. Since this is information already available to git-svn rebase, I tried adding a --dry-run option that prints out what I want. In the patch that follows I'm not sure if I've chosen the right terminology...