Code review required for commits - formal Team vote.
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
> * 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
Size: 206 bytes
Desc: not available
More information about the samba-technical