[Sugar-devel] Code Review process changes

[Bernie Innocenti]

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?


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.


> 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.


> 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.


> 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.


> 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 confused, how are these systems better than the patch review
> report we used to have?
> 
> For those who weren't with us back then:
> 
> http://lists.sugarlabs.org/archive/sugar-devel/2008-July/006903.html

That's nice, but I'd rather let everyone see the actual code right in
the list without delays and reply with their comments inline. As a
bonus, this lightweight process saves a lot of time to developers
posting many small patches.

OTOH, if a developer *likes* to go through Trac for posting a patch,
they're still free to file tickets and then find a reviewer who would
look at it. I've often done it myself for tickets that are already open,
for informational purposes. But of course nobody reviewed my patches
until I posted them to the list.


> > (*) We defined "Sugar developer" as "anybody who has made at least one
> > change that entered mainline".

To move forward: do you agree with this definition or would you prefer a
stricter criteria for people who can approve patches?

It's up to you: I'm ok with any decision you make, as long as patches
are going to land in git within 48h most of the time.

Once we're used to the current levels of latency, one may think a 48h is
way too impatient. However, consider that my very first kernel patch got
merged in Linus' tree in just about *5* minutes from posting it to lkml.
It was very welcoming, very rewarding for me. This may be part of the
reason why they managed to attract over one thousand active
contributors.

-- 
   // Bernie Innocenti - http://codewiz.org/
 \X/  Sugar Labs       - http://sugarlabs.org/