[Spice-devel] About decisions and reviews

[Marc-André Lureau]

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.

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

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