On 12/05/20 19:13, Marc-André Lureau wrote:
Hi
On Tue, May 12, 2020 at 5:07 PM Francesco Giudici <fgiudici@xxxxxxxxxx> 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
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel