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

[From: Andrew Flegg]

On Sun, Feb 6, 2011 at 19:37, Alberto Mardegan
<mardy at users.sourceforge.net> wrote:
>
> 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.

Although, to play Devil's Advocate (and not to get into a pissing
environment about whose professional experience is more valid ;-)),
*no* process can prevent more dangerous bugs. That's why there's a
testing release and (at some point) a stable one which will be
advertised much more widely.

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

Please improve said wording :-)

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

Surely these are solved and/or solvable? Your preference may be for
the ML, but I would suspect that's a personal preference.

For example, Gitorious is _supposed_ to allow effective code review:

    http://blog.gitorious.org/2009/11/06/awesome-code-review/

I'd find it hard for you to find a problem with that which wouldn't
affect an email!

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

Occasionally looking at a commit is different to committing (no pun
intended) to review every change so that there's not a bottleneck.

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

Indeed. However, I don't want to put stop energy in the way of making
the CSSU better. Mohammad's taken the initiative and pushed this
forward. Long term, I imagine we're going to want individual
maintainers for each package under http://gitorious.org/community-ssu
and a set of processes for managing and releasing them.

How we transition from one to the other is the question, and this is
definitely a helpful discussion.

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

Well, going back to 80s style code review on a mailing list is a big
change in effort IMHO. However, as noted on the wiki, development
processes are one of the many things to define.

> Now we have the unique opportunity of developing software with an
> infinite amount of time and a fair amount of good developers. [...]

Well, a few good developers have got stuck into the CSSU. No Nokians
yet, AFAICT; although I'd love to see 'em. Given the still limited
number of changes, understanding the changes being made will, I think,
inform the process. Is it primarily:

  * Merging third party patches without their involvement
  * Creating repository clones & merge requests in gitorious
  * Writing code and committing it to the master

Currently I think the second is primarily happening, with Mohammad
acting as reviewer and maintainer for all the packages. Whether or not
his standards correspond to yours is a different question as to
whether or not code review is happening ;-)

Cheers,

Andrew

-- 
Andrew Flegg -- mailto:andrew at bleb.org  |  http://www.bleb.org/
Maemo Community Council member