[Spice-devel] About decisions and reviews

[Francesco Giudici]

On 12/05/20 19:13, Marc-André Lureau wrote:
> Hi
> 
> On Tue, May 12, 2020 at 5:07 PM Francesco Giudici <fgiudici at redhat.com> wrote:
>>
>>
>>
>> On 12/05/20 11:24, Daniel P. Berrangé wrote:
>>> On Mon, May 11, 2020 at 12:16:24PM +0200, Marc-André Lureau wrote:
>>>> Hi,
>>>>
>>>> About "Move code to C++":
>>>> https://gitlab.freedesktop.org/spice/spice/-/merge_requests/62
>>>>
>>>> I would like to know how the decision happened. As long as I have been
>>>> involved in the Spice project, we had open discussions and did
>>>> mandatory review for anything non-trivial.
>>>>
>>>> This change is substantial, and impacts the work and contribution of
>>>> others. Where did the discussion happen? Who reviewed the code
>>>> changes?
>>>
>>> Looking at that merge request, it is pretty surprising to see a 100
>>> patch long series merged with no review comments visible on the code
>>> from other maintainers.
>>>
>>> Regards,
>>> Daniel
>>>
>>
>> I see your points: a proper discussion and fair review on the branch
>> would have been the way to go.
>> To have a fair overall picture btw, I think we should also consider some
>> other points:
>> $> git shortlog --since 01/01/2019 -s -n
>>      411  Frediano Ziglio
>>       29  Victor Toso
>>       20  Uri Lublin
>>       14  Francois Gouget
>>       11  Christophe Fergeau
>>       10  Snir Sheriber
>>        6  Eduardo Lima (Etrunko)
>>        6  Jonathon Jongsma
>>        6  Kevin Pouget
>>        6  Lukáš Hrázký
>>        5  James Le Cuirot
>>        3  Thiago Mendes
>>        2  Rosen Penev
>>        1  Benjamin Tissoires
>>        1  Christian Ehrhardt
>>        1  Douglas Paul
>>        1  Gilmar Santos Jr
>>        1  Jeremy White
>>        1  worldofpeace
>>        1  谢 昆明
> 
> To be "fair", the number of commits in spice server alone over 1.5y is
> not a very good metric. Certainly, Frediano's work is consequent,
> thank you, but it's also part of his job afaik.
My point here is that in the last 1.5y there was very little 
contribution to the project apart from Frediano. Same on the review 
side. In this context, I can understand why the branch was merged. I 
have my opinion, but here I don't want to say he did it right or he did 
it wrong: I'm just saying I can understand why he did.

> 
>>
>> Frediano's branches don't get much reviews (if any... see the full list
>> of merged/closed MR in gitlab for the spice/spice project). I think we
>> all agree that his intention is good, which is to just move the project
>> forward. Wondering who would have looked into his 100 patches branch to
>> do a fair review in a reasonable time-frame.
>> I feel (at least partially) guilty for this situation.
>>
>> That said... at this point the branch has been merged. What are the
>> proposals now?
>>
>> Draft a more formal review/commit policy? What if a branch doesn't get a
>> fair review in an acceptable time-frame? Who will have the last word if
>> unanimity is not reached?
> 
> What does an acceptable time-frame mean? If the work is important, the
> project maintainers should do a review in a timely manner. If nobody
> has enough time to review changes, isn't it because we, collectively,
> have more important things to do? The contributor who proposed such a
> change in the first place should realize that.

Well, agreeing on what is "important" is not easy, as it is personal. 
Any contribution is important to the submitter ;-)
But my point is, let's make the rules clearer, so to try to avoid 
similar situations in the future.

> 
>>
>> Do you want to do a post-merge review to consider reverting the commits?
>> Do you want to have a detached "C" branch with the former code to be
>> kept as a "stable C" one?
>> Or what else?
> 
> I am a past contributor to the Spice server, but I regularly have to
> read or debug the code. I keep co-maintaining the spice-gtk client,
> although it seems others are doing a pretty good job at helping me. Is
> there a problem with the Spice server maintenance? I don't know.
> 
> What do you want me to say... I am disappointed by this change, the
> nature of the change, switching to c++, and the way it happened. It
> doesn't reflect practices I like in open-source projects and the way
> the Spice project has been developed in the past.
> 
> Overall, the way it happened is detrimental to the project imho, and I
> would indeed revert the change if nobody reviewed it openly. The spice
> server should not to be taken so lightly, it doesn't deserve it
> either.

I like the love you have for the project. Really, great to see it. It is 
also ok to disagree and argue. But when you say "taken so lightly, it 
doesn't deserve it" you are putting blame there... Hey, we have all the 
same goal to make SPICE better and better. Frediano merged the branch to 
improve the project, not to make it worse... and he proved to love the 
project too. We are all on the same side!
So, let's move forward... Frediano merged it, this is done. What would 
you propose to do now? Let's move the discussion on what to do now :-)

Francesco