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

[From: Sebastian Lauwers]

On 7 February 2011 19:46, Alberto Mardegan <mardy at users.sourceforge.net> wrote:
> I didn't mention any rules for maintainers (the "rule" that I'd like to add
> is "wait a few days before merging a merge request into master"): I'd first
> wait to see how this discussion evolves, and get some agreement about it.

Basically, the way I see things:

1/ Submitter needs to ensure (as a commodity) that said patch is as
up-to-date with the stable branch as possible[1].
2/ Because 1/ can't be enforced, the maintainer or integrator needs to
double check the coding practises, apply some form of regression
testing[2], and probably also encourage unit testing[3].
3/ Once the conflicts / regressions have been satisfied, and the
coding practises have been validated, the MR can be merged into the
target branch.

Because there isn't any dependency on any work unit (or at least, if
MRs are created and treated sequentially, there isn't), and code has
been branched off a relatively stable source, the maintainer can focus
on *validating* code rather than *reviewing the effort*. It may be a
pedantic difference, but I still feel it needs to be made. What it
also means is that merge requests could for example remain open for a
whole release cycle, or simply put: "This merge request is
interesting, however requires a lot of maintenance / housekeeping /
work to be done; we will only apply it in version $x.$y.$z".

I fully agree with you: a merge request definitely doesn't require an
immediate integration, and we should aim towards allowing people to
work on bits of code that 1/ do not break things, 2/ are difficult to
integrate.

[1]: Note that "stable" doesn't mean last released version. The way we
use git at work is by having some form of functional regression
testing after pulling a merge request, we can safely merge a request
into the master branch, without tagging it as a release, because we
*know* that every completed work unit (merge request) is stable
(enough) on its own. This obviously stems from the fact that we have a
closely knit group of people communicating and working together; I
truthfully don't know up to what point this could be applicable to
Open Source.

[2]: Regression testing can in cases be as simple as "Does it compile?
[Yes / No]". In other cases, it requires some form of functional tests
that ensure the previous functionalities haven't been affected in a
significant way that could break internal and/or external
compatibility.

[3]: Unit testing is hard to implement correctly, and in most cases
project-dependent. *Enforcing* some form of unit-testing globally is
the best way to shy away developers.

Sorry for the slightly illogical message.

Good night,

-S.

-- 
question = ( to ) ? be : ! be;
      -- Wm. Shakespeare