{"thread":{"id":"13361","subject":"git and peer review","startedAt":"2008-05-03T01:02:41Z","lastAt":"2008-05-19T14:10:36Z","messageCount":11,"participants":["Ping Yin","Thomas Adam","Frodo Baggins","Toby Allsopp","Karl Hasselström","Seth Falcon","Shawn O. Pearce"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"75899","messageId":"46dff0320805021802i1a29becflcae901315035a77d@mail.gmail.com","threadId":"13361","inReplyTo":null,"subject":"git and peer review","fromName":"Ping Yin","fromEmail":"pkufranky@gmail.com","sentAt":"2008-05-03T01:02:41Z","receivedAt":"2008-05-03T01:02:41Z","isPatch":false,"sender":{"key":"pkufranky@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5346?v=4"},"body":"I have tried many online review tools: cucible, reviewboard and\nsmartcollaborator. The are both great. However, they can't satisfy all\nmy requirements:\n\n1. poor git support\n2. When reviewing changes between revision (or commit for git) a and\nrevision b, they display all changes as a single diff instead of one\ndiff for each intermediate revision\n\nSo finally, i decide to use git itself as the reviewing tool if i\ncan't find better.\n\nI am in a company environment and i want to enforce a policy that\nevery commit must be reviewed before pushed to central repository. I\nthink i can use hooks to enforce such kind of policy.\n\nOne way i want to try is to check in the hook whether every pushed\ncommit has a \"Reviewed-by \" line . Any suggestion?\n\nAnd one question, how to add a \"Reviewed-by\" line automatically?\n\nThe reviewers sit near each other, so we do face-to-face peer review\nand don't pass patches by email.\nSay, i have prepared a patch series,\n\nCase 1\n I ask someone to review my patches at my machine. If the review\npasses, i have to add Reviewed-by line to each commit and then merge\nit to the master branch. However, i find no easy way to add\nreviewed-by line. Maybe adding --reviewed-by option to cherry-pick or\nrebase or merge?\n\nCase 2\n The reviewer is the maintainer, so i ask him to pull and review. So\nnow it is his turn to add review-by line. But still, how?\n\n-- \nPing Yin\n"},{"id":"75925","messageId":"18071eea0805030654j42c21212wd1ccf4df42662000@mail.gmail.com","threadId":"13361","inReplyTo":"46dff0320805021802i1a29becflcae901315035a77d@mail.gmail.com","subject":"Re: git and peer review","fromName":"Thomas Adam","fromEmail":"thomas.adam22@gmail.com","sentAt":"2008-05-03T13:54:12Z","receivedAt":"2008-05-03T13:54:12Z","isPatch":false,"sender":{"key":"thomas.adam22@gmail.com","avatar":"https://gravatar.com/avatar/137f9858bc6bfd5b2f743aefd988c81ce0cbd306248889df80e269519cfc8741?d=mp&s=160"},"body":"On 03/05/2008, Ping Yin wrote:\n> I am in a company environment and i want to enforce a policy that\n> every commit must be reviewed before pushed to central repository. I\n> think i can use hooks to enforce such kind of policy.\n\nThis sounds likely, yes.\n\n> One way i want to try is to check in the hook whether every pushed\n> commit has a \"Reviewed-by \" line . Any suggestion?\n\nAssuming this is enforced either through a template (see:\ncommit.template in git-config(1)), or as part of being added by the\ncommitter, then in GIT 1.5.4 onwards there's a commit-msg hook which\nwill do this for you. Something like:\n\ntest \"\" = \"$(grep '^Reviewed-by: ')\" || {\n echo >&2 \"Message must have a Reviewed-by line present.\"\n exit\n}\n\n> And one question, how to add a \"Reviewed-by\" line automatically?\n\nThere's an example of that by way of a SOB in the commit-msg hook.\n\n-- Thomas Adam\n"},{"id":"75931","messageId":"46dff0320805030703g68de70edtc0371b2261059827@mail.gmail.com","threadId":"13361","inReplyTo":"18071eea0805030654j42c21212wd1ccf4df42662000@mail.gmail.com","subject":"Re: git and peer review","fromName":"Ping Yin","fromEmail":"pkufranky@gmail.com","sentAt":"2008-05-03T14:03:33Z","receivedAt":"2008-05-03T14:03:33Z","isPatch":false,"sender":{"key":"pkufranky@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5346?v=4"},"body":"On Sat, May 3, 2008 at 9:54 PM, Thomas Adam wrote:\n\n> Assuming this is enforced either through a template (see:\n> commit.template in git-config(1)), or as part of being added by the\n> committer, then in GIT 1.5.4 onwards there's a commit-msg hook which\n> will do this for you. Something like:\n>\n> test \"\" = \"$(grep '^Reviewed-by: ')\" || {\n> echo >&2 \"Message must have a Reviewed-by line present.\"\n> exit\n>\n> }\n\nActually, i want a way to rewrite the history or reapply the patch\nseries and add the reviewed-by line then. Before that, i can commit\narbitrarily without any limitation.\n\n>\n> > And one question, how to add a \"Reviewed-by\" line automatically?\n>\n> There's an example of that by way of a SOB in the commit-msg hook.\n\nSorry, but what does SOB stand for?\n\n\n\n\n\n-- \nPing Yin\n"},{"id":"75936","messageId":"1cdff3fa0805030722p3cfed058oae77d0dd9fb31dee@mail.gmail.com","threadId":"13361","inReplyTo":"46dff0320805030703g68de70edtc0371b2261059827@mail.gmail.com","subject":"Re: git and peer review","fromName":"Frodo Baggins","fromEmail":"frodo.drogo@gmail.com","sentAt":"2008-05-03T14:22:57Z","receivedAt":"2008-05-03T14:22:57Z","isPatch":false,"sender":{"key":"frodo.drogo@gmail.com","avatar":null},"body":"On Sat, May 3, 2008 at 7:33 PM, Ping Yin wrote:\n> Sorry, but what does SOB stand for?\nSigned Off By ?\n\nRegards,\nFrodo B\n"},{"id":"75938","messageId":"46dff0320805030727x7d7c473ao5e39f077a73f3523@mail.gmail.com","threadId":"13361","inReplyTo":"1cdff3fa0805030722p3cfed058oae77d0dd9fb31dee@mail.gmail.com","subject":"Re: git and peer review","fromName":"Ping Yin","fromEmail":"pkufranky@gmail.com","sentAt":"2008-05-03T14:27:35Z","receivedAt":"2008-05-03T14:27:35Z","isPatch":false,"sender":{"key":"pkufranky@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5346?v=4"},"body":"On Sat, May 3, 2008 at 10:22 PM, Frodo Baggins wrote:\n> On Sat, May 3, 2008 at 7:33 PM, Ping Yin wrote:\n> > Sorry, but what does SOB stand for?\n> Signed Off By ?\n\nOr, right. THX.\n\n\n-- \nPing Yin\n"},{"id":"76041","messageId":"87k5i9u8f1.fsf@nav-akl-pcn-343.mitacad.com","threadId":"13361","inReplyTo":"46dff0320805021802i1a29becflcae901315035a77d@mail.gmail.com","subject":"Re: git and peer review","fromName":"Toby Allsopp","fromEmail":"toby.allsopp@navman.co.nz","sentAt":"2008-05-04T20:21:54Z","receivedAt":"2008-05-04T20:21:54Z","isPatch":false,"sender":{"key":"toby.allsopp@navman.co.nz","avatar":null},"body":"On Sat, May 03 2008, Ping Yin wrote:\n\n[...]\n\n> I am in a company environment and i want to enforce a policy that\n> every commit must be reviewed before pushed to central repository. I\n> think i can use hooks to enforce such kind of policy.\n\nI'm in a similar environment, although it's only me using git (via\ngit-svn) at the moment.\n\n> One way i want to try is to check in the hook whether every pushed\n> commit has a \"Reviewed-by \" line . Any suggestion?\n>\n> And one question, how to add a \"Reviewed-by\" line automatically?\n>\n> The reviewers sit near each other, so we do face-to-face peer review\n> and don't pass patches by email.\n> Say, i have prepared a patch series,\n\nI'm very interested in good ways of doing this face-to-face review.\n\nAt the moment I'm using gitk to step through the patch series along with\nthe patch to gitk that adds a context-menu entry to lauch an external\ndiff tool when a side-by-side diff is easier to read.\n\nThis is okay, but it's a bit of a pain to make changes while the review\nis in progress (git rebase -i, s/pick/edit on the appropriate line, make\nchanges, git commit --amend, git rebase --continue). Perhaps stgit or\nguilt would help with this.\n\n> Case 1\n> I ask someone to review my patches at my machine. If the review\n> passes, i have to add Reviewed-by line to each commit and then merge\n> it to the master branch. However, i find no easy way to add\n> reviewed-by line. Maybe adding --reviewed-by option to cherry-pick or\n> rebase or merge?\n>\n> Case 2\n> The reviewer is the maintainer, so i ask him to pull and review. So\n> now it is his turn to add review-by line. But still, how?\n\nI do something similar using git filter-branch --msg-filter. I have a\nlittle shell script call git-add-checked (our convention is to have a\n\"checked: \" line in the commit message):\n\n--8<---------------cut here---------------start------------->8---\n#!/bin/sh\n\nusage() {\n cat < [] \nEOF\n}\n\nchecker=\"$1\"\n[ -n \"$checker\" ] || { usage >&2; exit 2; }\nshift\n\nset -x\ngit filter-branch --msg-filter \"sed '\\$a\\\\\n\\\\\nchecked: $checker'\" \"$@\"\n--8<---------------cut here---------------end--------------->8---\n\nThen, after getting my changes reviewed, I just do:\n\n$ git-add-checked joe.bloggs trunk..\n\nThis adds a \"checked: joe.bloggs\" line at the end of the commit message\nfor all of the commits on the current branch since trunk (which is a\nremote branch maintained by git-svn).\n\nRegards,\nToby.\n"},{"id":"76056","messageId":"46dff0320805041752t1190534dy3740c88f5380098f@mail.gmail.com","threadId":"13361","inReplyTo":"87k5i9u8f1.fsf@nav-akl-pcn-343.mitacad.com","subject":"Re: git and peer review","fromName":"Ping Yin","fromEmail":"pkufranky@gmail.com","sentAt":"2008-05-05T00:52:38Z","receivedAt":"2008-05-05T00:52:38Z","isPatch":false,"sender":{"key":"pkufranky@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5346?v=4"},"body":"On Mon, May 5, 2008 at 4:21 AM, Toby Allsopp wrote:\n> On Sat, May 03 2008, Ping Yin wrote:\n>\n> > Case 1\n> > I ask someone to review my patches at my machine. If the review\n> > passes, i have to add Reviewed-by line to each commit and then merge\n> > it to the master branch. However, i find no easy way to add\n> > reviewed-by line. Maybe adding --reviewed-by option to cherry-pick or\n> > rebase or merge?\n> >\n> > Case 2\n> > The reviewer is the maintainer, so i ask him to pull and review. So\n> > now it is his turn to add review-by line. But still, how?\n>\n> I do something similar using git filter-branch --msg-filter. I have a\n> little shell script call git-add-checked (our convention is to have a\n> \"checked: \" line in the commit message):\n>\n> --8<---------------cut here---------------start------------->8---\n> #!/bin/sh\n>\n> usage() {\n> cat < Usage: git-add-checked [] \n> EOF\n> }\n>\n> checker=\"$1\"\n> [ -n \"$checker\" ] || { usage >&2; exit 2; }\n> shift\n>\n> set -x\n> git filter-branch --msg-filter \"sed '\\$a\\\\\n> \\\\\n> checked: $checker'\" \"$@\"\n> --8<---------------cut here---------------end--------------->8---\n>\n> Then, after getting my changes reviewed, I just do:\n>\n> $ git-add-checked joe.bloggs trunk..\n>\n> This adds a \"checked: joe.bloggs\" line at the end of the commit message\n> for all of the commits on the current branch since trunk (which is a\n> remote branch maintained by git-svn).\n>\n\nGreat, very useful for me. THX.\n\n\n-- \nPing Yin\n"},{"id":"76125","messageId":"20080505134831.GA12733@diana.vm.bytemark.co.uk","threadId":"13361","inReplyTo":"87k5i9u8f1.fsf@nav-akl-pcn-343.mitacad.com","subject":"Re: git and peer review","fromName":"Karl Hasselström","fromEmail":"kha@treskal.com","sentAt":"2008-05-05T13:48:31Z","receivedAt":"2008-05-05T13:48:31Z","isPatch":false,"sender":{"key":"kha@treskal.com","avatar":"https://gravatar.com/avatar/f0120c734b5279b345075a28521e1ac66acb20c9913ffe9bf6ae97e53f7f3f13?d=mp&s=160"},"body":"On 2008-05-05 08:21:54 +1200, Toby Allsopp wrote:\n\n> At the moment I'm using gitk to step through the patch series along\n> with the patch to gitk that adds a context-menu entry to lauch an\n> external diff tool when a side-by-side diff is easier to read.\n>\n> This is okay, but it's a bit of a pain to make changes while the\n> review is in progress (git rebase -i, s/pick/edit on the appropriate\n> line, make changes, git commit --amend, git rebase --continue).\n> Perhaps stgit or guilt would help with this.\n\nYes, StGit helps here. \"stg edit \" lets you edit the commit\nmessage of any patch.\n\n( In the master branch, but not yet released, is an emacs mode for\n StGit. It displays the list of patches (name + first line of commit\n message), and you can press \"=\" to view the patch (including the\n commit message), and \"e\" to edit its commit message. )\n\n-- \nKarl Hasselström, kha@treskal.com\n www.treskal.com/kalle\n"},{"id":"77208","messageId":"20080517213039.GR396@ziti.local","threadId":"13361","inReplyTo":"87k5i9u8f1.fsf@nav-akl-pcn-343.mitacad.com","subject":"Re: git and peer review","fromName":"Seth Falcon","fromEmail":"seth@userprimary.net","sentAt":"2008-05-17T21:30:39Z","receivedAt":"2008-05-17T21:30:39Z","isPatch":false,"sender":{"key":"seth@userprimary.net","avatar":"https://gravatar.com/avatar/1db807504c1f8fb0a13bf1056a1e4d5096d17f3a09a1e9540f3af5f8b5ee009c?d=mp&s=160"},"body":"* On 2008-05-05 at 08:21 +1200 Toby Allsopp wrote:\n> I do something similar using git filter-branch --msg-filter. I have a\n> little shell script call git-add-checked (our convention is to have a\n> \"checked: \" line in the commit message):\n\nThat's a useful script, thanks for posting it. I'd like to make it a\nbit safer to use -- the first time I tried it I didn't give any branch\nlimiting args and it started filtering a lot of history :-P\n\nWhat I would like is a way for the script to determine the appropriate\ntracking branch. So that the usage would look like:\n\n git mark-reviewed someone@userprimary.net\n\nand it would figure out whether it should do trunk.. or release-1.3..,\netc. Can anyone point me in the right direction?\n\n\n-- \nSeth Falcon | http://userprimary.net/user/\n"},{"id":"77249","messageId":"20080519033736.GZ29038@spearce.org","threadId":"13361","inReplyTo":"20080517213039.GR396@ziti.local","subject":"Re: git and peer review","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2008-05-19T03:37:36Z","receivedAt":"2008-05-19T03:37:36Z","isPatch":false,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Seth Falcon wrote:\n> What I would like is a way for the script to determine the appropriate\n> tracking branch. So that the usage would look like:\n> \n> git mark-reviewed someone@userprimary.net\n> \n> and it would figure out whether it should do trunk.. or release-1.3..,\n> etc. Can anyone point me in the right direction?\n\nSomething like this, but its uh, ugly due to the use of a network\nconnection:\n\n\tbranch=$(git symbolic-ref HEAD)\n\tbranch=${branch##refs/heads/}\n\n\tremote=$(git config branch.$branch.remote)\n\tmerge=$(git config branch.$branch.merge)\n\n\trb=$(git ls-remote $remote $merge | awk '{print $1}')\n\nThen use a filter-branch on \"$rb..$branch\" as the range.\n\nYou may be able to just assume that the remote name is the\nrefs/remotes prefix and instead do:\n\n\tbranch=$(git symbolic-ref HEAD)\n\tbranch=${branch##refs/heads/}\n\n\tremote=$(git config branch.$branch.remote)\n\tmerge=$(git config branch.$branch.merge)\n\tmerge=${merge##refs/heads/}\n\n\trb=refs/remotes/$remote/$merge\n\nbut that's an assumption, and we all know what happens when you\nassume things.\n\nTechnically you need to look at remote.$remote.fetch lines\nin .git/config to figure out the rewriting rules from what\nbranch.$branch.merge contains to what refs/remotes/* might be,\nand doing that is not quite as trivial as either using ls-remote or\nassuming your users have the standard layout created by git-clone\nand git-remote.\n\n-- \nShawn.\n"},{"id":"77272","messageId":"20080519141036.GV396@ziti.local","threadId":"13361","inReplyTo":"20080519033736.GZ29038@spearce.org","subject":"Re: git and peer review","fromName":"Seth Falcon","fromEmail":"seth@userprimary.net","sentAt":"2008-05-19T14:10:36Z","receivedAt":"2008-05-19T14:10:36Z","isPatch":false,"sender":{"key":"seth@userprimary.net","avatar":"https://gravatar.com/avatar/1db807504c1f8fb0a13bf1056a1e4d5096d17f3a09a1e9540f3af5f8b5ee009c?d=mp&s=160"},"body":"* On 2008-05-18 at 23:37 -0400 Shawn O. Pearce wrote:\n> Something like this, but its uh, ugly due to the use of a network\n> connection:\n> \n> \tbranch=$(git symbolic-ref HEAD)\n> \tbranch=${branch##refs/heads/}\n> \n> \tremote=$(git config branch.$branch.remote)\n> \tmerge=$(git config branch.$branch.merge)\n> \n> \trb=$(git ls-remote $remote $merge | awk '{print $1}')\n\nHrm. My use case is with an upstream svn repository and git-svn.\nWith my .git/config, remote and merge as above are empty (I guess that\nis what one would use with a pure git setup).\n\nFor the git-svn case, I think what is needed is the ability to ask\ngit-svn about the local upstream tracking branch associated with HEAD.\nSince this is information already available to git-svn rebase, I tried\nadding a --dry-run option that prints out what I want. In the patch\nthat follows I'm not sure if I've chosen the right terminology...\n\n-- \nSeth Falcon | http://userprimary.net/user/\n"}]}