[Sugar-devel] Code Review process changes

[Tomeu Vizoso]

On Thu, Apr 29, 2010 at 18:49, Bernie Innocenti <bernie at codewiz.org> wrote:
> El Wed, 28-04-2010 a las 18:41 +0200, Tomeu Vizoso escribió:
>> > Several contributors are hindered or even put off by the current
>> > process. It often takes more time to handle mere technicalities (save
>> > patch to file, create ticket in Trac, attach patch, wait for review,
>> > push) than it takes to fix the bug.
>>
>> We talked before of using XML-RPC to automate it, what happened to that?
>>
>> I'm using this for GNOME and Freedesktop projects and works very well:
>>
>> http://git.fishsoup.net/man/git-bz.html
>>
>> I also heard about a trac<->email gateway, any news?
>
> What's the problem with plain email reviews that we're trying to solve
> with bug trackers and fancy review tools?

It's useful to have the patches tracked and related to the bug report.

> On every release cycle (lasting ~2 months), the Linux kernel merges some
> 10000 (ten thousands!) patches totaling 1000000 (one million!) lines of
> code, making it by far the fastest growing software project on the
> planet. See http://lwn.net/Articles/348445/ for more details.
>
> One may look at these numbers and wonder: what special tools are being
> used to manage such a gargantuan volume of patches?
>
> 1) git
> 2) email
>
> There's no 3. Projects much bigger than us, such as the Linux kernel and
> GCC have similarly been doing email reviews for the past 15 years
> without any trouble.
>
> The fear of loosing patches is really a red herring: if a patch got
> ignored, the poster (or anyone else) can simply ping it after 2-3 days.
> In fact, we're loosing more patches *now* by hiding them in Trac, away
> from the many eyeballs of all sugar-devel subscribers.

I don't have time to waste discussing how our system could be
completely different. After we changed to use the same system as
Linux, someone would start complaining we should switch to Mozilla's.

We have a system modelled after GNOME's and it used to work when we
had maintainers, and of course it works for the dozens of modules in
GNOME and Freedesktop. Sure, we can make changes to adapt to the
specifics of Sugar's community, but that request for change should
come from the maintainers, who are the ones that will be most directly
affected by them.

>> If we have such a thing as module peers is solely for helping out with reviews:
>>
>> http://wiki.sugarlabs.org/go/Development_Team/Release/Modules
>
> Can peers also approve patches?
>
> If so, then I think we've already solved our issue.

Well, we don't have enough peers, many of the listed them are not
active any more.

>> But we also should have more than one maintainer per module.
>
> If peers can approve patches, I'd not be concerned if we have only one
> maintainer. Actually, two maintainers on the same module may even get
> into fights.
>
> Regarding sugar, sugar-toolkit, sugar-base and sugar-artwork... I don't
> see them as really separate modules: you can't really use patches often
> span across multiple modules and you can't really use them
> independently.
>
> Being so tightly coupled, these 4 modules should probably share the same
> set of maintainers and peers.

That doesn't match my actual experience maintaining Sugar. You are
guessing about what some people have actually experienced, please stop
doing that.

>> This seems to imply that the sole purpose of reviews is QA, but I
>> think it has two more important purposes: transferring the burden of
>> maintenance and sharing best practices and conventions.
>
> Indeed! This would make another good argument for carrying on reviews in
> the direct sunlight: so everyone would get to learn from each other's
> code.

Sure, I'm a bit tired of repeating to you that I'm in favor of that.

>> In practical terms, to me this means that reviews need to come from
>> those who can appreciate the actual impact on maintenance of a patch,
>> and that also have a direct interest in keeping the maintenance burden
>> low.
>
> Let's distinguish the act of *reviewing* a patch, which anyone could do
> regardless of experience, from the act of approving it.
>
> I'm all running patches through maintainers for approval, IF THEY ARE
> RESPONSIVE.
>
> If not, we can't afford to block the entire development pipeline of
> Sugar due to lack of maintainer bandwidth. There are many healthy
> projects out there which managed to do well even without clear
> maintainers in the way of all patches. One very well known example of
> the anarchic model is Xorg. Others I participated in were uClinux, BDM
> and AROS.
>
> I'm not saying it's ideal, I'm just saying it's better than pissing off
> all new contributors by ignoring their patches. Note that what we're
> proposing is not totally anarchic: we're just proposing to enable more
> people to approve patches when the maintainer is unavailable or too
> busy.
>
> Upstream being unresponsive is the #1 complaint I hear about Sugar Labs
> lately, from many many people. We have a very serious problem that needs
> to be addressed ASAP.
>
> Hopefully we'll grow fresh new full-time maintainers from today's
> newbies, but only if we make it possible for them to contribute now.

I'm not interested in discussing changes to the system as long as we
have Sugar in such unmaintained state. I have tried at least a dozen
of times to start a discussion on this resourcing problem and have
been ignored.