[pulseaudio-discuss] Rethinking how we do reviews

[Arun Raghavan]

On Tue, 2016-03-29 at 09:52 +0200, David Henningsson wrote:
> 
> On 2016-03-28 09:13, Arun Raghavan wrote:
> > 
> > Hello,
> > I'm sure it's not shocking or surprising to state that our patch review
> > bandwidth is significantly lower than the rate at which contributions
> > are coming in.
> > 
> > It is also quite demotivating to send patches to the list and have to
> > wait weeks to hear about them, and kudos to all our patient
> > contributors and everyone who's been pitching in on the review process,
> > especially Tanu who's taken up the bulk of the heavy-lifting on this
> > front.
> > 
> > While having Patchwork in place helps keep track of patches so we don't
> > lose some through the cracks, it does not help decrease our review
> > turnaround time by a whole lot.
> > 
> > To this end, I propose that we ease our review policy a bit. My
> > proposal is that:
> > 
> > * Committers should have the ability to go ahead and push out their own
> > changes without review, except ...
> > 
> > * User-facing changes should have some announcement and/or discussion
> > (changing dependencies, new modules, etc.)
> > 
> > * Changes to API or protocol should undergo review at least to the
> > extent of the API/protocol change
> > 
> > * Large infrastructure changes should go through a full review
> > (slightly subjective, but I think we can leave this to individual
> > judgment)
> > 
> > Our current way of doing things is good for keeping up code quality,
> > but I think over time, with such a large patch backlog, we end up
> > spending more and more time performing reviews, and less and less time
> > working on features. This becomes quite draining and drops our overall
> > productivity in contributing to the project.
> > 
> > At this point, I guess this is mostly for Tanu to buy into, and maybe
> > David if he'd like to continue contributing at least on the ALSA side.
> > Thoughts and suggestions from others are still welcome, of course.
> I agree in large with the problem statement, but I'm not really seeing 
> how your suggestion addresses the problem. Giving committers a fast path 
> seems to rather encourage committers to work on their own stuff rather 
> than reviewing other people's patches.


I don't think committers review patches because they're blocked on
other people reviewing their patches. :)

We do reviews because we want to, so having a faster path to having our
own code in master is not directly related to patch review bandwidth.

However, there *is* a way in which it is indirectly related, which is
motivation. If I'm writing code and then waiting weeks for it to be
reviewed, this is massively demotivating to me, and the overall energy
I'm expending to do the same set of things in the project is higher.

If there's a shorter path to me doing things and having them have the
desired effect, then I'm feeling more productive, and that translates
into higher motivation to work on all aspects of the project.

> Also, it's not the trivial "fix-a-typo" patches that are the problem, 
> those are easy to review and commit - it's the large patchsets (such as 
> memfd, RAOP2, etc) that take time. And those would still require manual 
> review and design thinking.

And this would not change significantly in my current proposal. Large
changes need discussion etc. and that will need to continue to happen.
I'm hoping that allowing committers greater autonomy on their own work
will increase the overall energy available to spend on these things as
well.

> My preferences is that we should instead be less picky about patches. 
> And then I mean less picky about things that don't cause bugs; like 
> coding style, variable names, that sort of stuff. If the overall 
> architecture of a patch (or patch set) is good and does not cause 
> regressions, then let it go in. If then someone wants to rename a 
> variable or fix some coding style issue, that can then be done as a 
> separate patch.

This is a tricky thing too. On the one hand, you don't want to be too
nitpicky. On the other, code readibility and consistency do have a long
term impact on maintainability and ease of contribution. I think in
recent times the balance we have struck has been okay (this is, of
course, subjective).

-- Arun