git and peer review

git and peer review

[Ping Yin]

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?

-- 
Ping Yin

Re: git and peer review

[Thomas Adam]

On 03/05/2008, Ping Yin <pkufranky@gmail.com> 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.

This sounds likely, yes.

>  One way i want to try is to check in the hook whether every pushed
>  commit has a "Reviewed-by " line .  Any suggestion?

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
}

>  And one question, how to add a "Reviewed-by" line automatically?

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

-- Thomas Adam

Re: git and peer review

[Ping Yin]

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

>  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
>
> }

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.

>
>  >  And one question, how to add a "Reviewed-by" line automatically?
>
>  There's an example of that by way of a SOB in the commit-msg hook.

Sorry, but what does SOB stand for?





-- 
Ping Yin

Re: git and peer review

[Frodo Baggins]

On Sat, May 3, 2008 at 7:33 PM, Ping Yin <pkufranky@gmail.com> wrote:
>  Sorry, but what does SOB stand for?
Signed Off By ?

Regards,
Frodo B

Re: git and peer review

[Ping Yin]

On Sat, May 3, 2008 at 10:22 PM, Frodo Baggins <frodo.drogo@gmail.com> wrote:
> On Sat, May 3, 2008 at 7:33 PM, Ping Yin <pkufranky@gmail.com> wrote:
>  >  Sorry, but what does SOB stand for?
>  Signed Off By ?

Or, right. THX.


-- 
Ping Yin

Re: git and peer review

[Toby Allsopp]

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.

> 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.

> 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.

Re: git and peer review

[Ping Yin]

On Mon, May 5, 2008 at 4:21 AM, Toby Allsopp <Toby.Allsopp@navman.co.nz> wrote:
> On Sat, May 03 2008, Ping Yin wrote:
>
>  > 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).
>

Great, very useful for me. THX.


-- 
Ping Yin

Re: git and peer review

[Karl Hasselström]

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

> 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.

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. )

-- 
Karl Hasselström, kha@treskal.com
      www.treskal.com/kalle

Re: git and peer review

[Seth Falcon]

* On 2008-05-05 at 08:21 +1200 Toby Allsopp wrote:
> 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):

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 | http://userprimary.net/user/

Re: git and peer review

[Shawn O. Pearce]

Seth Falcon <seth@userprimary.net> wrote:
> 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?

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.

-- 
Shawn.

Re: git and peer review

[Seth Falcon]

* On 2008-05-18 at 23:37 -0400 Shawn O. Pearce 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}')

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...

-- 
Seth Falcon | http://userprimary.net/user/