# git and peer review 11 messages from 2008-05-03 to 2008-05-19. Participants: Ping Yin, Thomas Adam, Frodo Baggins, Toby Allsopp, Karl Hasselström, Seth Falcon, Shawn O. Pearce. Thread: https://gitlist.dev/t/13361 ## Ping Yin, 2008-05-03 01:02 Subject: git and peer review Message-ID: <46dff0320805021802i1a29becflcae901315035a77d@mail.gmail.com> URL: https://gitlist.dev/e/46dff0320805021802i1a29becflcae901315035a77d%40mail.gmail.com ``` 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 ``` ## Thomas Adam, 2008-05-03 13:54 Subject: Re: git and peer review Message-ID: <18071eea0805030654j42c21212wd1ccf4df42662000@mail.gmail.com> URL: https://gitlist.dev/e/18071eea0805030654j42c21212wd1ccf4df42662000%40mail.gmail.com In-Reply-To: <46dff0320805021802i1a29becflcae901315035a77d@mail.gmail.com> ``` On 03/05/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. 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 ``` ## Ping Yin, 2008-05-03 14:03 Subject: Re: git and peer review Message-ID: <46dff0320805030703g68de70edtc0371b2261059827@mail.gmail.com> URL: https://gitlist.dev/e/46dff0320805030703g68de70edtc0371b2261059827%40mail.gmail.com In-Reply-To: <18071eea0805030654j42c21212wd1ccf4df42662000@mail.gmail.com> ``` On Sat, May 3, 2008 at 9:54 PM, Thomas Adam 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 ``` ## Frodo Baggins, 2008-05-03 14:22 Subject: Re: git and peer review Message-ID: <1cdff3fa0805030722p3cfed058oae77d0dd9fb31dee@mail.gmail.com> URL: https://gitlist.dev/e/1cdff3fa0805030722p3cfed058oae77d0dd9fb31dee%40mail.gmail.com In-Reply-To: <46dff0320805030703g68de70edtc0371b2261059827@mail.gmail.com> ``` On Sat, May 3, 2008 at 7:33 PM, Ping Yin wrote: > Sorry, but what does SOB stand for? Signed Off By ? Regards, Frodo B ``` ## Ping Yin, 2008-05-03 14:27 Subject: Re: git and peer review Message-ID: <46dff0320805030727x7d7c473ao5e39f077a73f3523@mail.gmail.com> URL: https://gitlist.dev/e/46dff0320805030727x7d7c473ao5e39f077a73f3523%40mail.gmail.com In-Reply-To: <1cdff3fa0805030722p3cfed058oae77d0dd9fb31dee@mail.gmail.com> ``` On Sat, May 3, 2008 at 10:22 PM, Frodo Baggins wrote: > On Sat, May 3, 2008 at 7:33 PM, Ping Yin wrote: > > Sorry, but what does SOB stand for? > Signed Off By ? Or, right. THX. -- Ping Yin ``` ## Toby Allsopp, 2008-05-04 20:21 Subject: Re: git and peer review Message-ID: <87k5i9u8f1.fsf@nav-akl-pcn-343.mitacad.com> URL: https://gitlist.dev/e/87k5i9u8f1.fsf%40nav-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. > 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 } 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. ``` ## Ping Yin, 2008-05-05 00:52 Subject: Re: git and peer review Message-ID: <46dff0320805041752t1190534dy3740c88f5380098f@mail.gmail.com> URL: https://gitlist.dev/e/46dff0320805041752t1190534dy3740c88f5380098f%40mail.gmail.com In-Reply-To: <87k5i9u8f1.fsf@nav-akl-pcn-343.mitacad.com> ``` On Mon, May 5, 2008 at 4:21 AM, Toby Allsopp 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 < Usage: git-add-checked [] > 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 ``` ## Karl Hasselström, 2008-05-05 13:48 Subject: Re: git and peer review Message-ID: <20080505134831.GA12733@diana.vm.bytemark.co.uk> URL: https://gitlist.dev/e/20080505134831.GA12733%40diana.vm.bytemark.co.uk In-Reply-To: <87k5i9u8f1.fsf@nav-akl-pcn-343.mitacad.com> ``` 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 " 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 ``` ## Seth Falcon, 2008-05-17 21:30 Subject: Re: git and peer review Message-ID: <20080517213039.GR396@ziti.local> URL: https://gitlist.dev/e/20080517213039.GR396%40ziti.local In-Reply-To: <87k5i9u8f1.fsf@nav-akl-pcn-343.mitacad.com> ``` * 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/ ``` ## Shawn O. Pearce, 2008-05-19 03:37 Subject: Re: git and peer review Message-ID: <20080519033736.GZ29038@spearce.org> URL: https://gitlist.dev/e/20080519033736.GZ29038%40spearce.org In-Reply-To: <20080517213039.GR396@ziti.local> ``` Seth Falcon 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. ``` ## Seth Falcon, 2008-05-19 14:10 Subject: Re: git and peer review Message-ID: <20080519141036.GV396@ziti.local> URL: https://gitlist.dev/e/20080519141036.GV396%40ziti.local In-Reply-To: <20080519033736.GZ29038@spearce.org> ``` * 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/ ```