[Spice-devel] About decisions and reviews

[Marc-André Lureau]

Hi

On Tue, May 12, 2020 at 10:08 PM Francesco Giudici <fgiudici at redhat.com> wrote:
>
>
>
> 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.

I don't understand the justification, honestly. The spice server is
not overwhelmed with MR, and there is no rush to switch to C++.

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

It's a collective work, not somebody's project with its personal
priority or wishes.

> But my point is, let's make the rules clearer, so to try to avoid
> similar situations in the future.

We have had rules for years. Anything non-trivial has mandatory
reviews. And in practice, we try to go through review all the time.

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

I am not talking about feelings here, but of practices and rules.

But we are also human, and we have feelings.

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

We seem to agree it's a mistake. If it is, then we should correct it.

Reverting the series is a simple commit away.

-- 
Marc-André Lureau