Code review required for commits - formal Team vote.

[Michael Adam]

Hi Andrew,

On 2012-10-13 at 15:25 +1100, Andrew Bartlett wrote:
> On Sat, 2012-10-13 at 01:13 +0200, Michael Adam wrote:
> 
> > * Productivity need not drop when not pushing ones own
> >   patches, because one can work on a private branch until
> >   patches hit master.
> 
> This assumes there is no work involved in rebasing patches, abandoning
> patches that become too hard to rebase,

No, it does not assume this. I know it is some amount of work.
Because I do it every day, as do many (actually most) of the
developers I know and work with. Notably my colleagues Volker,
Metze, Gregor, and Björn, but also Simo, Andreas, Christian to
name a few more that I know of.

This is some amount of work, but not so much that it
significantly kills productivity. (See the commit counts
of the above ...)

The good thing about the review is that it will force patches
to ripen more before they end up in master. If you push patches
prematurely, you'll have to fix up things later on, which could
also account for decreased productivity, the "extra work" just
happens at a different time and probably less effectively.
(Or you let others do the tidying up behind you to keep up
productivity, which wouldn't be very nice. :-)

Interestingly, Jeremy who has proposed this move at this time
is by himself someone who has not been into the branch-and-rebase
business to an exaggerated level, and rumour is that he has also
pushed patches prematurely in the past (as most of us have).
So guess what his commitment to doing mandatory code reviews
and non-author push also means: that he is not only trying to
discipline others, but also himself... :-)

> I note with interest that nobody proposing this effort is promising to
> pick up patches for review, and certainly not in a reasonable
> time-frame.

I guess you miss the most important point of the idea of moving
to mandatory reviews: It is that saying +1 on this is mostly a
commitment to doing reviews more often, more reliably.

> In my particular situation, the only time in the last year
> that I've had any significant review available for the AD and
> integration changes was when gensec happened to be mutually convenient
> for the SMB3 effort.  I appreciated that co-incidence, but I certainly
> can't count on it for the rest of my work. 

Again, the idea of the proposal is to change this so that you can
count on it (and others as well).

Earlier, you have been "begging for review". With the mandatory
review you can instead demand review. That changes your position.

> That the issue of patches being unable to gain review is handwaved off
> as 'harass your fellow team members enough and they might actually help
> you out' indicates we do not have a mature process here to deal with
> this, and we should not jump from our current processes to mandatory
> author != committer in just one step. 

Of course we don't have a mature process yet!
We are discussing the option to newly establish such a process.
We have to work out how to do it best.

Here is how I understand Jeremy's proposal (possibly wrong ;-) :

The suggestion is to try mandatory code review by committing (as
a team) to doing review and and non-author-push for a certain
time (e.g. a couple of months). If this does not work out after that
time we need to rethink and change the idea. If it does, we can
put it formally into the team policy. But I think it can not
work without the commitment of the team to try it out (without
it being enforced by policy or tools).

> Why can't we take it a step at a time, committing for example to have
> all substantive changes on the list for 3 days (or until reviewed), but
> without rules on author != committer.  This will encourage review
> without creating in one step the level of extra work and dislocation
> this proposal entails. 

Hmm, we could also do this. But why not try the full featured
model directly. If it should really turn out to be unberable
we would re-discuss after a certain period.

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/20121013/c13282db/attachment.pgp>