Code review tools [WAS: Re: Code review required for commits - Discuss.]

[Nadezhda Ivanova]

I have also used  - and administered - ReviewBoard and think it's OK. I
have only used it with Subversion however...

On Fri, Oct 12, 2012 at 8:16 PM, Matthieu Patou <mat at samba.org> wrote:

> On 10/12/2012 07:06 AM, simo wrote:
>
>> On Fri, 2012-10-12 at 15:42 +0200, Jelmer Vernooij wrote:
>>
>>> On Thu, Oct 11, 2012 at 03:48:53PM -0400, Ira Cooper wrote:
>>>
>>>> Jeremy and Simo brought this up in another topic, but it deserves its
>>>> own thread really.
>>>>
>>>> Also, in most serious work I've done, I've always had reviewers before
>>>> committing my code, provided I'm not the only one working on it.  It
>>>> is just standard procedure.
>>>>
>>>> So consider this my +1, to their ideas, and giving them a "rule".
>>>>
>>>> Actual formal suggestion:
>>>>
>>>> No team member commits their own code.  All code will be "signed off"
>>>> by two team members, as a team member you may sign off your own code.
>>>> The "non-author" team member will be responsible for pushing the code.
>>>>   If there are two they can agree among themselves. ;)
>>>>
>>>> Release branches work as they do today, though the + should be treated
>>>> as a sign-off in bugzilla.  All bug commits should now contain their
>>>> bug number in them, so we can track back what happened.
>>>>
>>> In general, I'm in favor of requiring reviews. However, I also think we
>>> should be explicit about what the benefits and costs are.
>>>
>>> We'll spend more time discussing changes with others, more time
>>> chasing reviewers and getting code in better shape to land. That'll
>>> help quality and spread the overall understanding of the
>>> code over more people, but it'll also have an impact on our
>>> overall output. I think reviews can certainly have a positive impact,
>>> but let's keep the process lightweight and not bureaucratic.
>>>
>>>  I have a rate of "bit rot" I expect on Illumos/Solaris.  It's usually
>>>> 1-2 build breaks, and a few minor issues if I walk away for 6 months.
>>>> Yes... I have expectations on just how broken things get.  Sometimes
>>>> we all exceed them.  Sometimes not.  But the rule of thumb: "Master
>>>> won't build 100% right if I haven't touched it in 6 months." is right.
>>>>
>>> Reviews won't fix portability issues. Maybe we'll catch one or two
>>> more because of reviewers that are aware of specific portability
>>> issues. But I don't see how reviews will significantly help with this
>>> problem.
>>>
>>>  No team member commits their own code.
>>>>
>>> I'm puzzled by this. Do we not trust team members to be able to commit
>>> their code and follow the policy? This just seems like something that
>>> will make landing code harder without any benefits.
>>>
>>> Also, can we switch to a different way of doing reviews, like
>>> Gerrit? Bugzilla is horrible for this kind of thing.
>>>
>> We are not proposing to use bugzilla for the reviews for the master
>> tree.
>>
>> Changing subject to avoid hijacking the main thread and keep it to
>> process and not tools.
>>
>> I do like the concept of gerrit, though I do not like the premises of
>> the tool, I have already seen people having issue because (afaik) gerrit
>> doesn't use the official git server but they built their own in order to
>> do special things, this is a *bad* idea imo.
>> However I would be amenable to the idea on 2 conditions:
>> 1. reviews can be acted upon exclusively via email (a mandatory web ui
>> is a full nack from me)
>> 2. we do not let gerrit own the master branch. The gerrit branch get
>> pushed to the real master.
>>
> At work we use reviewboard (http://www.reviewboard.org/), I can't say
> that I love it but it's ok, the main problem we have here is that we can
> have to review Jumbo patches that touch code all around which makes things
> much more difficult to review.
>
> Matthieu.
>
> --
> Matthieu Patou
> Samba Team
> http://samba.org
>
>