reflecting on 4.10

[Alex Fiestas]

On Saturday 12 January 2013 18:54:23 Aaron J. Seigo wrote:
> On Saturday, January 12, 2013 18:29:22 Alex Fiestas wrote:
> > In the recent past, we have had people giving "ship it" in reviewboard to
> > code that was not maintained by them and what is worst modifications that
> > broke (or still breaks) stuff, we should prevent this from happening.
> 
> we already generally follow maintainence responsibilities in reviews (e.g.
> kwin reviews are pretty well always stamped ShipIt by kwin devs; there's one
> going through this process right now, in fact)
> 
> however, i don't agree that we should discourage broad participation as a
> few things happen when we do:
> 
> * it becomes easier to have reviews drop on the ground as we wait
> patiently/blindly for maintainers (we have dozens of components in kde-
> workspace)
> 
> * fewer people take an active interest in the code because they aren't ever
> reviewing anything, so what should motivate them?
> 
> * some idividual maintainers end up with more than their share of reviews
> and end up with little time for anything else if not careful (i sometimes
> spend entire days doing only patch review..)
> 
> * we do have components that are under- or simply un-maintained .. then
> what?
> :)
> 
> i don't agree that more careful review will catch significantly more issues
> than are already found out as many breakages will not show up to those doing
> initial testing.
> 
> so i'd like to see *more* reviewboard input rather than less. and it's one
> thing i love about feature and bug fix branches going into integration: it
> lets people thumbs-up the review request without any implications for
> master
> 
> ShipIt would no longer mean the changeset goes into master, but schedule it
> for merge into the integration branch. in theory, more people will be
> testing integration than looking at the initial review, which will catch
> breakage that currently sometimes makes its way straight into master.

I have explained myself terribly wrong since you are making statements that I 
haven't. Let me be more clear to address many of your points.

I want to prevent the fact that broken code can go into the integration branch 
because people reviewing it know C++/QML/etc but don't know the code base of 
the destination project. This means that everybody is welcomed to review and 
point mistakes and give "Thumbs up" but only people knowing the project itself 
should be able to mark it to be merged.
How many times have you seen in the current reviewboard something like: "It is 
ok now but let's wait until XXX gives the ship it" ? I have seen it plenty of 
times in almost all KDE project and that's precisely what I'm talking about.

One of the benefits of having a more review based development process is that 
maintainers and code owners can review code BEFORE it goes in instead of what 
happens now that you have to post-review things which is imho more time 
consuming.

For unmaintained projects I agree with you, it is better to make the 
modification available in integration than to let it rot in reviewboard/review 
process.

So, to sump it up:
-The more reviewers the better
  -Something like "Thumbs up" could be implemented
-Give a chance to the people that know the code base to review it before 
marking the change to be merged.