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

[Francesco Giudici]

Hi,

On 18/05/20 12:56, Daniel P. Berrangé wrote:
> 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.
>
I like your analysis, thanks for sharing it. I agree the best solution 
would be to attract contributors (we need to find some ideas on how to 
do this and act on them btw!) but my point is also: let's try to keep 
the contributors we already have in the meanwhile. I would like to avoid 
issues about contributions not merged (or merged without agreement) 
causing us to start arguing among us. This could led to a not-so-happy 
environment that may harm the project and the community.

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

Agreed. I would also add we don't want any change that may also make 
current contributors lose love or even leave the project. Hope this is 
not the effect of this draft "proposal" on anyone, as my intent is 
exactly the opposite!

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

Thanks for sharing this! AFAIK, this did not happened: no out of band 
discussion at all about the C++ branch merging. Glad you brought this up.
There were two legitimate competing desires in that situation: one was 
to have a big rework merged by the main contributor to the project. The 
other was to have proper review and agreement before performing such a 
merge.
My personal point is that such a rework, moving to C++, with so low 
review effort on the spice/spice project, would have caused the branch 
to stay there without getting a fair review for a very long time... btw, 
a warning that it was going to be merged if no reviews would have had 
not harmed.
This is why I think would be good to have rules that help on what to do 
in such cases: committer asks for reviews for few weeks before merging. 
The other contributors will know that if they don't reply, the merge 
will happen. They will have at least to put a comment there asking for 
more time if they are interested in reviewing. Wouldn't be a fair 
"game"? ;-)

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

This is a good idea! Let's add adding explicit people tagged in the 
project! How to pick them? At least 2 maintainers? The 2 most active 
contributors? What else? This could be done.

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

I agree, addressing this may help reducing or even solving the issue in 
the middle/long term if successful. But this is another point, IMHO. I'm 
in favor of it, but it is harder and will require longer planning and 
effort with no immediate return. What we do in the meanwhile?

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.

I don't like the idea of commits merged without reviews too. And what do 
you think about a project where your merge requests doesn't get any 
comment for three weeks or more? The idea here is to put some pressure 
(and responsibility) on reviewers too. Remember, one single comment is 
enough. And the committer has to keep asking for reviews (and maybe 
tagging specific people).

Not saying that the draft proposal here must be the way to go and I 
agree that improving the participation to the project is important and 
may prevent the issue from happening again.
My point here is, let's not dismiss the problem we are facing right now: 
we don't want MR abandoned without even a comment or merged without an 
agreed process. How do we deal with this?

Thanks

Francesco

> 
> Regards,
> Daniel
>