[SCM] Samba Shared Repository - branch master updated

[Simo]

On Thu, 2013-09-12 at 10:27 +0200, Kai Blin wrote:
> On 2013-09-12 10:06, Michael Adam wrote:
> 
> Moin Michael,
> 
> > Since commit 9e7bce53707732700928eaf2bb53a5f1cc5d7784
> > which was on 2012-10-19, and was the first commit to
> > be pushed under the new proposed reviewed-by/signed-off-by
> > rules, we have:
> > 
> > - a total of 2772 commits
> > - Signed-off-by:  1995 tags by 24 different team members
> > - Reviewed-by:    2771 tags by 25 different team members
> >
> > So your impression that nobody cares does not seem to be justified.
> 
> Right, and apparently it works on a voluntary basis. So why do we need a
> technical solution for something that's not even broken?
> 
> > Of course this slows things down. Doing thorough reviews
> > takes time. But the resulting patches are almost always
> > much better and don't need to be amended afterwards as much
> > as before. So that's worth the pain, imho.
> 
> Do we have a metric for that?

I do not have to tell you that objectively evaluating code quality is as
easy as automatically finding very subtle bugs ... not very.

However subjectively I can tell you I've reviewed a (small
unfortunately) number of patches and found bugs or inconsistencies and
asked for amends. Each time that happens, it is a ton less work 3 months
down the road when a bug pops up, nobody remembers what that bit of code
did and you have to a) do all the costly analysis to find out where is
the bug and b) go in and fix the stuff and c) assess if it needs
backporting and backport and possibly d) release it in haste if it was a
security issue.

It is much more efficient to find, discuss and resolve bugs when the
code is fresh in the mind of the author and he can explain exactly why
and what he did in a specific patch rather than having to hunt a bug
later.

A clear example was a very good patchset from Jeremy on the memory limit
stuff we have in talloc. It was good, but with one *subtle* bug. It
would have been a beast to try to find it out later what was going on,
as that thing would hit rarely and in a way that would not have
necessarily been obvious.

For area that have little contribution this is even more important,
because the code is probably not very much exercised so bugs will
probably take a while before being reported, and because there are fewer
people familiar with the code so having the author available to explain
the patch while fresh in his mind, if necessary is even more important.
Also the less people familiar the easier it is that they are not around
if the bug hits at a critical moment.

> > Do you have any patches out there pending review? (as most of
> > us do, btw) Then just ping again. Doesn't that work?
> > Of course, reviews are not designed for the impatient. ;-)
> 
> Do you have any example of people purposely pushing code that wasn't
> reviewed after we started doing reviews?
> 
> Again, while I currently don't have a problem, my original worry
> remains: Having a mandatory review policy will divide the developer team
> into the people working on popular areas and the people working on areas
> nobody really cares about. The voluntary review model we currently have
> avoids this.
> 
> If we really need a technical solution and mandatory reviews, we also
> need mandatory response times. I don't think we can give these, so the
> alternative would be the "Reviewed-by: Nobody Cared <nope at samba.org>" tag.
> 
> TLDR; The current voluntary system works. Don't fix it if it ain't broken.

It was proposed, this time, as a technical solution to someone
forgetting to add tags, but what you are saying is: even though it seem
we all agree on doing reviews and in effect all the last ~2700 patches
each had a review, you want to be able to go ahead and forcibly push
un-reviewed patches on the off chance that nobody will care at all for
weeks to review one of your patches for mysterious reasons.

All this said I do not think we really have to make checks in autobuild,
for this, but I do not see that it would cause any issue if we really
did. As you say worst case (everybody left the project w/o disabling the
autobuild check) you can always push with yourself as signed off and an
arbitrary reviewer or three.

Simo.