[maemo-developers] Proposal: code review process for community SSU

[From: Alberto Mardegan]

Hi Andrew,

2011/2/6 Andrew Flegg <andrew at bleb.org>:
> As to be expected, as this is a *testing* release to start defining
processes and get more involved.

Indeed. But I would stress that the more stable it is, the more people
will get involved with that.
The fact that I am a developer myself doesn't help me in trying the
CSSU: if it screws things up, I'm as troubled as any other user.
Therefore, if the N900 is my primary phone, I won't use the community
SSU.

> I don't think it's critical at all. Certainly not compared with the
community-ssu-enabler postinst bug before the announcement which
required editing /var/lib/dpkg/community-ssu-enabler.postinst to do the
upgrade.

Sorry, I didn't weight my words correctly: I didn't mean to write
"critical" in the sense of "makes the phone unusable", I just meant
something affecting many users. But the point is that with the current
situation, much more dangerous bugs can emerge.

 > This was my bad: the fix was in gitorious but not in the repo. 
However, I'd say this is the purpose of having a testing repo; and the 
clear warnings both at install time and on the Community_SSU page which 
say that currently it's only for those "willing to risk a reflash."

That's fine as a disclaimer, but I insist that one thing is being
honest and clear with your users, and another thing is having more
community support on the CSSU. The first we have, more or less (the
wiki page is not that clear about it being potentially dangerous
software, though). Also the latter started well, I think, given the
number of users who started using it, but I'm afraid many users will
stop using it soon.

 > My own thought is that a comprehensive set of test scripts can cover 
the main bases before something gets pushed to the stable repo. These 
can be crowdsourced at both writing and testing:

No, here I have to disagree completely -- I might be too pessimist,
but I don't believe you'll ever be able to create a test suite which
would make you feel even a little safer. Manual testing is the way to
go (I'm talking of end user experience here), but to get testers you
need either to pay them, or to give them a product with a reasonable
quality.

 >> PROPOSAL: MAILING LIST FOR CODE REVIEW
 >
 > I don't think this is the best approach though. There are many 
problems to overcome:
 >
 >  * It throws away the streamlined workflow supported
 >    by gitorious.org and its "merge request" functionality.

I've been using gitorious for years now, but I still don't like it.
The review process is not better than a ML (because there's no easy
way to navigate from one diff to see the full code), and I would claim
that it's even worse because of missing notifications.

 >  * Who would volunteer to review the code? Currently
 >    we have a surfeit of people working on the code of
 >    the CSSU, not a surplus. Given they're doing it in their
 >    spare time, drudgerous formal code review is not going
 >    to be high on their list.

People in maemo-developers. I'd be one, for sure. Besides, as I wrote
before, there are several gurus there who have always been helpful and
that happen to be the original writers of that software.

 >  * In my professional experience, monolithic code reviews
 >    are rubbish at detecting bugs. One certainly wouldn't've
 >    caught #11813.

Now I'm sure I'll sound arrogant, but I bet that I would have caught it. :-)
And if by "monolithic code review" you mean reviewing a big chunk of
code, then my objection is that such a patch would have been discarded
in the first place.

 > Having said that, doing something informally should be fine. 
Gitorious offers "watch" and (IIRC) RSS functionality. If you, or anyone 
else, wanted to watch the commits and provide comments on 
maemo-developers, I think that'd be very useful.

That would be a pain. It's true that it could help in spotting some
bugs, but reviewing code "a posteriori" (after it has been merged into
the master branch) it's demotivating for everyone. At least it won't
help code quality: if I ask "please split this function into two",
when the function is perfectly functional as it is, who would do that?
And why would I bother doing that, given the the repository history
has already recorded that?

 > I thought the wiki pages did. However, it's a wiki, feel free to 
iteratively improve it!

I will :-)

BTW, I don't mean to underestimate you, Mohammad or any other
contributor -- I'm complaining about the development process. I have
some experience with leading the development of a project, and I see
how this CSSU could be very easily improved with very little effort.

<politician-speech>When we were developing for the N900 within Nokia,
we had to make compromises, reduce the scope of some features and
cancel others, just for the sake of delivering it in time and
minimizing risks of missing deadlines. Now we have the unique
opportunity of developing software with an infinite amount of time and
a fair amount of good developers. We should aim at raising the quality
of the original software, and not accept any
compromises!</politician-speech>

:-)

Ciao,
   Alberto