Hi On Tue, May 12, 2020 at 10:08 PM Francesco Giudici <fgiudici@xxxxxxxxxx> wrote: > > > > 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. 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 _______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel