[Sugar-devel] Fwd: Proposal on how to speed up patch reviews

[Daniel Narvaez]

Forgot to reply all...


---------- Forwarded message ----------
From: Daniel Narvaez <dwnarvaez at gmail.com>
Date: 25 March 2013 21:12
Subject: Re: [Sugar-devel] Proposal on how to speed up patch reviews
To: Simon Schampijer <simon at schampijer.de>


On 25 March 2013 12:37, Simon Schampijer <simon at schampijer.de> wrote:
> To improve that situation, I think we have to put some lights on all those
> roles and I think often the situation of the maintainer is not understood
> well enough. I can at least say that we had this discussions for the last
> years in Sugar, with different maintainers and I have seen it happening in
> many other projects as well. It is a known issue, and it is not an easy one,
> never less I think we can improve.

In my experience Sugar has been much worst than both GNOME and mozilla
though. I know it's not easy but we should keep trying.

(I hope it's absolutely clear that I'm not blaming anyone for the
situation, just pointing out the importance of fixing it or at least
trying to)

> [bugfix] A bugfix is the simplest case. The submitter is interested in
> solving the specific issue he is working on. It is not hard to find a
> reviewer or tester. Either someone from the community that has been annoyed
> by the same issue or the maintainer himself because he is interested in
> seeing the issue solved, his interest is a working code base in the end.
> Here it is easy as well for a maintainer to trust another reviewer and
> acknowledge based on his positive review.
>
> In my experience those patches are not laying around for long if there is
> not a technical blocker in the patch itself. Evaluation is relatively easy
> here.

I had at least one obvious bug fix patch unreviewed for months. Maybe
I was unlucky. Anyway I agree this is the easiest of the cases and
where things work best.

> [feature] When it gets to Features things get more tricky. For a Feature
> first of all the high level goals are important: what need does the Feature
> address, is it wanted by the community, is the technical approach taken a
> good one, basically the maintainer has to decide if it is worth taking on
> maintainership of this feature or not. In the end it might be him who has to
> deal with arising bug fixes and who is blamed if the software is not a solid
> product.

While I agree with you in general, I think maybe we are exagerating a
bit the responsibility of the maintainers a bit. I tend to think it's
the whole community which will get the blame if things goes wrong...
Maintainers have of course a very important role, but they should not
feel like they alone into this.

> That might explain a general bigger resistance to features by maintainers.
> How can we help each other to make this a better process?

I think narrowing the focus is the best we can do, but I'll elaborate
more about that while answering Gonzalo email.

> [feature/UI] All what have been said above applies to the feature that adds
> UI as well. I have separated that item because it adds another complexity.
> We have the UI process for this (as written in the feature policy [1]).
> Basically it adds more care taking of all the roles involved.

Even if we go with my propiosal, I think maintainers should keep a
strong supervision role on features, UI or not. I'd say it's
responsibility of the reviewer to make sure the maintainer is happy in
this regard... we should add it to our review checklist (well we don't
have one yet afaik, but we should).

>> Online services patches, unreviewed  for more than one month
>> http://lists.sugarlabs.org/archive/sugar-devel/2013-February/041808.html
>> Unreviewed for more than one month
>
>
> This one is part of the [feature] category. It can be mostly explained with
> the maintainers having their heads still in bug fixing for 0.98.
>
> Here applies as well what I said with high level descriptions of features
> and the feature process [2].

In an ideal world, a reviewer which is not busy with 0.98 should have
given a first pass to the patches and at the same time started a
discussion, involving the maintainer, about the oppurtunity of adding
such a feature etc.

>> * Let's separate the maintainer from the reviewer roles. Maintainers
>> should be very expert because ultimately the future of the project is
>> in their hands. Those kind of people are very rare. Though I think
>> there are many more people that could do reviews on areas of the code
>> they feel comfortable with.
>
>
> That sounds good to me. We actually are doing this more or less already. We
> can maybe make this more explicit and foster that principle. Especially for
> bug fixes I do not see a reason why I should not merge a patch that has
> r+/t+ from a trusted person if there is not an obvious issue.

There a couple of important differences compared to what we are doing:

* The reviewers are explicitly entrusted by the maintainers. So they
know if they review the patches will most likely go in. It's often not
very motivating to do reviews without being able to approve.
* The reviewers takes care of merging, so there is no blocking on the
maintainers (if not for an high level discussion on feature changes).

>> * We go back requiring all the patches to be posted on the mailing
>> list (that's still the officlal policy but in practice a lot of code
>> is going through trac these days). This is necessary so that
>> maintainers can see all that is being reviewed and can comment if
>> required.
>
>
> This works if we make our process fast enough :) The issue here is that we
> do not have a good way of tracking unmerged patches. If we merge quickly
> that works.
>
> As well, we still have the bug tracker. When we go into bug fixing mode
> things get messed up. I still do not have a good answer to that.

I thought about this a lot in the past and honestly I don't have a
good answer either. I think it's less urgent than adding people that
can do reviews. Hopefully we will figure it out at some point!


-- 
Daniel Narvaez