Merging the Contact List and using reviewboard

[Andrunko]

On Wed, Mar 23, 2011 at 11:40 AM, George Goldberg
<grundleborg at googlemail.com> wrote:
> 2011/3/23 David Edmundson <david at davidedmundson.co.uk>:
>> This conversation came up in IRC and I wanted to bring it to the mailing
>> list:
>> - I want to merge Martin's contact list into the main repository.
>>  -Martin is worried that if he has to start going through reviewboard for
>> patches it's really going to slow down development.
>> I can see where he's coming from, when I see one line patches on reviewboard
>> it wastes my time going through sorting them out, and it does slow things
>> down. Especially when everyone hacking is doing this just in the few moments
>> of their freetime.
>> On the other hand I have fixed a lot of code in reviewboard before it's
>> merged, and had a lot of my mistakes spotted too.
>> Personally I just want his stuff merged, and I'm happy to leave the choice
>> of whether to enforce reviewboard or not to the discretion of the component
>> leader.
>
> I have no strong feelings on using reviewboard or not, but I'd
> encourage people to use it for any patches that are more than just a
> couple of lines. However, the thing I would really not want to see is
> code being merged without review. So, if you have a quick change, ping
> someone on IRC and get them to just check your branch. It is very
> clear from the stuff on reviewboard so far (as well as experience
> upstream in Telepathy) that unreviewed code == bad code. No matter how
> well you think you know a code base, review is still beneficial
> because other people may spot things you missed, or spot things that
> aren't clear (to others, even if you personally find it very clear).
> This is very important in a project with so many different developers
> working on it.
>
> If anyone is worried about how to make this work without slowing down
> development too much, I'd encourage you to talk to the upstream
> Telepathy developers in our channel (e.g. andrunko, oggis etc) who can
> explain how Telepathy handles code review - it really works well for
> them, so I think it can work well for us too.
Just to explain how we do it in tp-qt4 and other telepathy components:

All components have at least one maintainer (in tp-qt4 it's me and
oggis) and one maintainer or whoever is _really_ familiar with the
code (active developer) needs to review _any_ new code before this
code is merged upstream. Even when the maintainers have patches we ask
for review, even if we think that our code is correct.

So our workflow is basically:
- Each developer has a public git repo where he keeps a master branch
that is the copy of upstream/master.
- Once a developer has a new feature/bugfix he/she pushes a new branch
implementing such a feature to his/her personal repo and ping someone
on IRC to do the review (or file a bug report and add the branch to
the bug URL field together with a "patch" keyword in the bug Keywords
field).
- The reviewer goes on and either approves the branch with ++ or
request changes.
- This branch is then updated with review suggestions and published
again for review, where the developer has to ping a reviewer again or
update the bug if applicable. Once the reviewer approves the branch
(++) the developer can go and merge it.

Ps.: One other thing we do here is to always edit the "merge" commit
of the new branch into master and add a "Reviewed-By: Foo" stamp, so
we can keep track of who implemented and who reviewed the code.
I always use git merge --no-ff when merging branches, which will
forcibly create a "merge" commit which can then be edited adding
"Reviewed-By:".
   $ git checkout master
   $ git fetch upstream
   $ git reset --hard upstream/master
   $ git merge --no-ff mybranch
   $ git commit --amend # here we add "Reviewed-By: Foo"
   $ git push upstream master:master

Where upstream is the remote for the project upstream repo.

This method proves to be reliable and a fast way to get reviews done.

And my opinion about the subject is that all code needs to be
reviewed, either on IRC or using bugzilla/reviewboard, no matter who
wrote it. In some rare cases when the patch is _really_ simple we
allow it to be merged without any review (for example adding/editing
docs or adding some useful debug), but we try to avoid it as much as
we can.

BR
Andre

-- 
Andre Moreira Magalhaes (andrunko)
--------------------------------------------------------
Jabber: andrunko at gmail.com
MSN: andrunko at hotmail.com
Skype: andrunko
IRC: andrunko
Blog: http://andrunko.blogspot.com