[Spice-devel] SPICE: changing the merge rules - a proposal

[Daniel P. Berrangé]

On Fri, May 15, 2020 at 07:05:50PM +0200, Francesco Giudici wrote:
> Hi,
>   the community around the SPICE project always tried to follow one
> fundamental, implicit rule for accepting code contributions to the project:
> every merge request (beside trivial patches) should be reviewed and acked at
> least by one before getting merged.
> While everyone agrees with this fundamental rule, the actual status of some
> SPICE projects makes the rule impractical to let the project move forward.
> Let's consider the spice/spice project as an example: the number of
> contributions is very low, both on the commit side (only 4 different
> contributors with more than 1 commit from the beginning of the year, and a
> single contributor with 90% of commits) and on the review side (in the last
> 40 merge requests before the C++ switch one, 21 had no comments).
> The x11spice project is another example: we have only 4 contributors from
> the beginning of the year (and a single contributor holding 70% of the
> commits) and the reviews on the gitlab merge requests have been provided by
> two people only, each one reviewing the merge requests of the other.

I think these paragraphs here are the highlighting the key issue. There
just is not sufficient contribution to SPICE in general. I think the most
important thing is to consider why the SPICE project has such a low rate
of contribution, and more importantly what steps can be done to make SPICE
more interesting to attract contributors.

For a project that has such a large codebase and featureset, it is not
healthy to have such a small community. The "bus factor" is waay too
low here, such that SPICE would be in serious trouble as a project if
one or two main contributors moved onto to different work.

So for any changes in process, it is wise to consider what effects those
change have on people SPICE would like to attract as new contributors.

For example, if new feature work by an existing contributor is discussed
out of band between two contributors, instead of via merge request comments
this sets up new contributors up as second class citizenships - the out of
band discussions are essentially invisible to them, so they can't see or
understand the rationale for decisions. This makes it harder for them to
learn how to effectively contribute to the project.

This is the big thing that concerned me about the merge request that adopted
C++. Reading between the lines I get the impression this was indeed discussed
between the several contributors but out of band, instead of on the merge
request which had no comments. Thus that discussion is invisible to any 3rd
party contributor interested in SPICE. I think this risks discouraging people
from contributed to SPICE, so it is something to be wary of doing too often,
especially for really big technical changes.  

> For the sake of having the projects being able to move forward with a
> reduced number of contributors/reviewers, the proposal is to *allow* a
> maintainer to merge a Merge Request without an explicit ack if the three
> following conditions are met:
> 1) The Merge Request has been pending for at least 3 weeks without getting
> new comments
> 2) The Merge Request submitter has kept asking a review on a weekly basis

Note since spice is using gitlab, it is possible to explicitly tag people
by name in the comments to ask for reviews, and/or assign people as a
reviewer. I'd suggest to make use of such tagging, so that people can be
aware they they are being explicitly asked for feedback if something has
been sitting for a long time.

> 3) There are no pending nacks on the Merge Request
> 
> Note that having patches reviewed would still be the preferred way. If at
> any time the number of contributors would raise again, we can switch back to
> the mandatory review rule. Until then the priority is to allow the project
> to move forward.

Contributors are unlikely to magically rise again on their own, without
proactive work to make SPICE a more attractive project to contribute to.
This is a very difficult problem for projects in general, not just SPICE,
to which I don't have any magic answers. At least try to think about what
you like to see from projects when you are approaching them as a new
contributor, and then make sure SPICE follows these practices where ever
possible / practical. Code being pushed with no review is a turn-off to
me as a external contributor, unless the project is clearly setup as a
single-author personal project, rather than a team project.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|