Code review required for commits - formal Team vote.

Michael Adam obnox at samba.org
Sun Oct 14 17:12:45 MDT 2012


On 2012-10-15 at 01:05 +0200, Michael Adam wrote:
> 
> Yes, the proposal has had a very unfortunate start, indeed.
> Let me rephrase what my understanding and view of the proposal is:
> 
> * A couple of us have worked with frequent review and partly
>   non-author push since some time and made very good experiences.
> 
> * Furthermore other projects use this successfully, and maintain a
>   good level of patch quality with it.
> 
> * Therefore I would like to propose that we try out mandatory
>   code review for the samba master branch.
> 
> * I would futhermore popose to require (for the trial period)
>   the pusher of a patch to be different from the author, since
>   pushing as part of reviewing might force the reviewer to review
>   more thoroughly (doing the push seems to demonstrate a stronger
>   commitment/involvement).
> 
> * I believe that such a trial can only work if the team as a
>   whole commits to testing mandatory review for a certain period
>   of time, after which it can be decided whether we want to move
>   forward with rooting mandatory review into a formal policy.
> 
> 
> Here are some details for a proposal:
> 
> 1. Two team members should review patches.
> 
>    - If the author is a team member, then the author counts as
>      reviewer, so one additional review is sufficient.
> 
>    - If the patch is created by pair-programming of two team
>      members, the two authors are also counted as reviewers.
> 
> 2. The review should be indicated in the commit message by a
>    tag like "Signed-off-by: Name <email>".
> 
>    But this can be discussed: A different tags that has been
>    proposed: "Acked-by: ...". I'd also suggest "Reviewed-by: ..."
> 
> 3. The author should not push his/her own patch, so usually the
>    reviewer should push after positively reviewing the patch.
> 
>    - If the patch is done by pair-programming (of team members),
>      i.e. carries the "Pair-programmed-with: ..." tag,
>      any of the authors may push.
> 
> 4. At this time I do explicitly not propose any specific mechanism
>    for reviewing. The important thing is the result in the
>    commits and pushes!
> 
>    A couple of possibilities:
> 
>    - The patch can be posted to the samba-technical mailing list
>      asking for review.
> 
>    - If developed in collaboration, the patch can also be
>      pushed directly by the reviewer / co-author.
>      (Indeed, review can be quite an interactive process.)
> 
>    - If anybody wants to try a special tool for organizing
>      reviewing, that is of course also possible.
> 
> 5. Committing to doing (trying) mandatory code review also
>    means that we commit to doing reviews more reliably and
>    timely that possibly up to now as a part of our regular
>    development work.
> 
> I guess this is not complete yet, but the most important points
> (for me) are covered.
> 
> Christian's concerns about special areas of code like
> super-special VFS modules are not covered yet here, but our trial
> period will tell whether we need special rules for such areas.
> 
> I suggest that we try this for 3-4 months. After that timeframe
> we should evaluate the outcome. If all goes well, we would have
> elaborated a process and formal requirement that we could put
> into a policy. If we meet impossible obstacles before that
> instead, we need to revise and re-discuss earlier of course.
> But I think we need to give it at least a couple of weeks' time
> to make a meaningful statement.

To make it perfectly clear:

The above is my current version of the proposal.
It should be discussed, and if e.g. the majority was against the
non-authore-push rule, then we might as well start out without it.

I deliberately do not propose any tools.
Let's stay flexible and apply common sense.
I think we should not over-formalize it now.

Cheers - Michael

-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 206 bytes
Desc: not available
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20121015/79ad8dd4/attachment.pgp>


More information about the samba-technical mailing list