Code review required for commits - formal Team vote.

Stefan (metze) Metzmacher metze at samba.org
Wed Oct 17 02:27:01 MDT 2012


> I am not sure that we are still in the stage of official proposals
> to be voted on. I think we are somewhat back at discussing things.
> 
> If we are proposing a trial period for mandatory code review,
> then we need to have some form of consensus. It can only work
> if the team commits to seriously trying it. I don't think
> a simple majority vote can lead us to a meaningful trial period,
> since this would assume good will and motivation to work in this
> mode from virtually all of the team.

I agree to this.

> The idea was brought up and quickly a vote was called,
> but the discussion has shown that there is (imho) not enough
> consensus right now to reasonably do it.

.. and this...

> I originally thought the matter was rather simple, but it
> has turned out to have created a big wave. Talking to various people
> also off the mailing list thread, I see that there are various
> concerns about the switching to mandatory code review and non
> author push even with a trial period. We should take these
> concerns seriously!

.. and this...

> So, since we are discussing anyways, and the discussion takes
> a lot longer and gets a lot more heated than I had anticipated,
> I think we should take some momentum out of it and not force
> things.

.. and this...

> SUGGESTION:
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 1. I suggest that we defer a team vote on a commited trial period
>    or even a policy change for some time, at least a couple of weeks
>    until we have thought and discussed things through thoroughly
>    and without hastening, maybe until after 4.0 is released.
> 
> 2. In the meanwhile, we encourage team members to do reviews
>    more often on a voluntary basis, which means both asking for
>    review before pushing and giving review for others.
> 
> 3. We also encourage to have the reviewer push.
> 
> 4. We can also try any mechanisms/tools on a voluntary basis.
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Sounds good.

> This allows us to concentrate on shipping 4.0 which is the most
> important goal currently! The worst we can do now is delay or
> spoil the 4.0 effort by working ourselves into the ground over
> heated policy discussions.

I agree to this.

I wish that all the energy that went into this thread(s) would have been put
into fixing important bugs for the 4.0.0 release. That's why I'm currently
ignoring it as much as I can.

> And it allows us on a completely voluntary basis to gain more
> experience and confidence in the reviewing business and possibly
> get an idea of the load and mechanisms.
> 
> This will certainly move towards mandatory reviews much slower
> than the hard trial period, but it is more respectful for the
> folks that have serious concerns. I think that we will still make
> progress, based on the following observations:
> 
> - I think we can safely assume that we all want to sustain and
>   improve the quality of our commits and would consider changing
>   our mode of operation on the master branch if suitable.
> 
> - I got from mails and discussions that nobody actually
>   thinks that doing reviews is a bad thing!
> 
> The concerns we should address are these:
> 
> - How will we provide the ressources to review 6000 patches / year?
>   (or will it imply a drop in productivity?)

I don't think I have much spare bandwidth to handle all the review all
patches I'll very likely get then.

> - Do we need a tracking tool for reviews so patches don't get lost?
>   (this is roughly a.k.a how do I get a reviewer for my patch?)
> 
> - Can we find *two* team reviewers even for the most exotic areas
>   in code ore do we need to establish more flexible rules?
> 
> - and probably more I forgot to mention, please fill this up...

For me it very helpful that I review what I'm going to push
by myself in the same way I'd review patches from others.
See https://wiki.samba.org/index.php/CodeReview for what I typically do.

> I hope that even a voluntary period (as has been suggested by
> others) will lead us to some insight about theses issues in the
> next couple of weeks and that we can move forward with voting
> about something more concrete and with a better feeling attached
> to it after 4.0 is out.

I agree to this.

> Cheers - Michael
> 
> 
> PS: Jeremy, I apologize for back-pedalling here. I still think
> we should do it, but not by all means. This is the only way
> I see how we can continue in a reasonable and respectful way.

I also agree to this.

metze

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 259 bytes
Desc: OpenPGP digital signature
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20121017/2f8db4f7/attachment.pgp>


More information about the samba-technical mailing list