Require TWO team members review for externally written patches?

[Andrew Bartlett]

On Mon, 2013-06-03 at 00:50 +0200, Michael Adam wrote:
> On 2013-06-03 at 08:25 +1000, Andrew Bartlett wrote:
> > On Mon, 2013-06-03 at 00:16 +0200, Michael Adam wrote:
> > > 
> > > Well, Andrew already pushed the patches. Now they are not only
> > > missing the signoff but also a second review by a team member.  :-)
> > 
> > While I've seen Jeremy doing that, I never understood that to be the
> > process,
> 
> See what we have written down as a summary of our discussions
> last year:
> 
> https://wiki.samba.org/index.php/CodeReview
> 
> There was always the proposed requirement to have two team
> members review, with the special case tha the author's signoff
> would count as review if the author happens to be a team member.

Indeed, I just looked that up. 

I'm quite disappointed we agreed to that, but I don't really want to
have to go over the archives again. 

It's curious that this has come up now, because I've been merging
patches on this basis for months now :-)

> > and if we must discuss that again I would oppose it, as it only
> > adds friction to the typically small and useful patches that our
> > external contributors bring. 
> > 
> > The purpose of the review process is to ensure someone else, other than
> > the author, thinks it's OK.  That reviewer may want extra review (I
> > asked that a patch go past kai or amitay as well recently, despite being
> > a one-liner), but I see NO value in *requiring* the involvement in two
> > team members.  It just makes extra work for value.
> 
> Here we are again at the fundamental question what is a small
> patch. What is a harmless and/or useful patch? It is not possible
> to draw a line here. So we need a clean and simple rule that
> applies always. If the patch is so simple and harmless than it
> will be very easy to review and get additional review. The hard
> part is the review of the complex patches.

It is easy to argue that 'getting reviews is easy', and that has been
put often.  I would argue that it doesn't matter how easy it is to get
reviews, that getting reviews is killing the joy of working on Samba.

Certainly by this graph *something* happened in October 2012:
https://www.ohloh.net/p/samba/commits/summary

As a professional software developer, as long as the process keeps
working (and I have come very close a number of times to saying that it
isn't working, and opting out), I'm currently willing to continue in it.
I do feel very sorry for those who don't have all day to contribute to
Samba however, because this process tipped me over the edge to make
other changes in my professional arrangements now that working on Samba
has become so much less enjoyable. 

I would also argue that the 'clean and simple' rules we have applied so
far have made Samba worse, not better.  That even doc/comment typo fixes
go up for review (let alone the double-review to get into stable)
creates work, and certainly in my case means I don't bother pro-actively
fixing 'little stuff' any more, because it isn't worth writing the
change, and spending even longer writing the mail, chasing up for review
etc. 

That, in the case of external patches, for the benefit of 'clear and
simple rules' we put aside totally the experience and trust in members
of the Samba Team to decide 'this patch doesn't need more than my
review' seems totally wasteful.

The resource of developer enthusiasm is not infinite, and we should be
incredibly careful not to drain it except when absolutely required.  We
should ensure our processes uplift our developers, be they on the team
or off it, rather than make them feel they have to work past an
obstinate policy system that only costs everybody time.  

We should do this because if we burn off developers early, they won't
come back, but if we encourage them early, we can look forward to many
more contributions, and this is what will make Samba grow again. 

Thanks,

Andrew Bartlett
-- 
Andrew Bartlett                                http://samba.org/~abartlet/
Authentication Developer, Samba Team           http://samba.org