Nico, and everyone,
Thanks for many details, and for historical information, always interesting for me to read :)
*“The big difference seems to be that at coreboot mergingpatches, i.e. hitting the "Submit" button, is more or less consideredan administrative task unrelated to one's opinion on the code. Whilefor us it was supposed to replace the gatekeeping. Changing this wouldimply that we need our own group of reviewers. With this, a +2 wouldbecome both rarer and stronger.”*
I didn’t even know that the Reviewers group is inherited from coreboot, I only learned this very recently! :) Probably because I didn’t work on coreboot, so I don’t know people from there. And when I am adding reviewers that I know, they all work on flashrom.
In general, Reviewers and Committers are two groups to go through to merge a patch, and it makes sense that at least one of them knows very well what they are doing. If one of them is doing an “administrative task”, then the other one should do a real task. To me, it makes sense that the real task is for Reviewers. Because their names are immortalised by the “Reviewed-by” tag, so you can always find from commit history who did it.
It makes sense to me to have a Flashrom Reviewers group (and as I said, I thought this is the case already). Although it needs to be very clearly discussed and agreed, what is the responsibility of being a Reviewer. And now my mind immediately starts to generate ideas about what can go wrong. Please don’t see this as a criticism, it is just how an engineering mind works :)
# New group is smaller than the previous group, so some people are losing their +2 on flashrom. People get very upset and offended. # Shortly after, lots of people are added to the new Flashrom Reviewers group, and we get back to square one. # Committing patches is now considered a truly “administrative task” and so lots of people are added to that group because why not # And all of the above can happen together :)
I still think the idea of the Flashrom Reviewers group makes sense, but it needs to be well discussed and agreed upon. Good thing we have a meeting now :)
*“The "flashrom developers" group also grants the privilege to block"-2" a change. I have very conflicting feelings about this. On onehand, this is a tool that is only ever necessary when people in saidgroup can't trust each other. On the other, that seems to be thestatus quo already. Maybe we should have another group on top forthis?”*
With this, I disagree, and the reason is that the real problem is “people don’t trust each other”. We need to fix this real problem! “We” as a team. Merging patches and blocking patches from merging seems to me to be equivalent powers, just with + and - signs. One missing piece of information for me (I simply don’t know): is it possible to remove -2 which is made by someone else? Like one person blocks the patch, another person unblocks? Is it possible?
*“There is one related idea for a rule that crossed my mind: How aboutwe forbid blocking reverts that fix a regression and apply cleanly(i.e. without needing to revert anything else in advance). Also, letthem pass after review without waiting 24 hours.”*Makes sense. I thought “revert first, fix later” is somewhat a general rule?
Especially when you are saying:
*“At least for myselfthis would reduce a lot pressure to fall back to gatekeeping ”* When I hear “gatekeeping” to me it is equivalent to “enormous responsibility”. Such things burn people :( I don’t know how far you are on the “burning” scale, but I don’t want you to get further…
I have some more thoughts about it. I think people can be opposed to reverts because reviews are very long, and so people are waiting for so long for patches to get merged… and then revert?? NO!! These are not my thoughts, I never opposed the revert :) this is what I imagine. So… reviews take long, and people are opposed to reverts. But then, reviews are long because people are opposed to reverts! Vicious circle yes. Which we need to break. “We” as a team. Good thing we have a meeting now :) Oh wait… I am saying this for the second time? :)
On Sun, Mar 6, 2022 at 3:17 AM Nico Huber nico.h@gmx.de wrote:
Dear fellow flashrom developers,
I've been reflecting lately about written and unwritten rules around our review process and our related privileges on Gerrit. I couldn't identify anything that works well, or let's say well enough. So it might be time we discuss some things and make changes :)
Before the dawn of our Git repository and using Gerrit as a review plat- form, we had a Subversion repository. With very few people, usually one or two I think, that had write access. So practically we had gatekeepers that watched over the code. Eventually, they became a bottleneck so we widened that a little when starting the work on Gerrit. Today, I often catch myself falling back into such a gatekeeper mode. Which is futile anyway because I'm not the only one who can write to the repository. But I still wonder why this happens and if it's necessary.
I guess first everyone needs to understand how we imagined things with Gerrit and how it looks like today. As mentioned above we set up a group "flashrom developers" which has exclusive write access to the reposi- tory. The idea was to only have people in this group who are long-term (I was thinking about years) contributors to flashrom, know each other from real-time communication (IRC) and trust each other. For the review right "+2", we used the "Reviewers" group from coreboot. IIRC, I pushed for the latter. I was hoping to grow the communities a little together that seemed to have gone separate ways in the years before. After all, a lot of coreboot developers use flashrom in their everyday work. Combined with the "flashrom developers" group it shouldn't have been an issue that people with little flashrom knowledge would review.
Since the original setup (about 2017) things went on rather uncon- trolled. I think the gist of it is that people didn't expect that flashrom would need/have different rules compared to coreboot. At some point somebody whom I only knew from their work on coreboot sud- denly merged patches for flashrom. And I guess people pushing patches today pretty much expect that there is nothing more to be done once they see the first +2. No matter if the reviewer knows anything about flashrom.
When reviewers disagree then, it can become frustrating for everyone. So currently, I lean towards aligning our process more with that of coreboot. The big difference seems to be that at coreboot merging patches, i.e. hitting the "Submit" button, is more or less considered an administrative task unrelated to one's opinion on the code. While for us it was supposed to replace the gatekeeping. Changing this would imply that we need our own group of reviewers. With this, a +2 would become both rarer and stronger. If this idea meets some consent, I would propose to copy the "flashrom developers" group for a start. And when we identify somebody over time who shows in-depth knowledge about at least a part of flashrom and good self-judgement if they are experienced enough for a +2, we could add them. Right now, some candidates come to mind already, Peter, Nikolai, and Thomas.
The "flashrom developers" group also grants the privilege to block "-2" a change. I have very conflicting feelings about this. On one hand, this is a tool that is only ever necessary when people in said group can't trust each other. On the other, that seems to be the status quo already. Maybe we should have another group on top for this? This time really only for trusted, established, long-term contributors? Or maybe for those who maintain the codebase beyond what they are paid for?
There is one related idea for a rule that crossed my mind: How about we forbid blocking reverts that fix a regression and apply cleanly (i.e. without needing to revert anything else in advance). Also, let them pass after review without waiting 24 hours. At least for myself this would reduce a lot pressure to fall back to gatekeeping if I knew I could keep things safe and sound without discussion. For maintainers, the patches that are already merged matter the most, cause the most work. This should also be the case for contributors, but alas many don't see it like this and don't help to fix things after a patch is merged. Easing reverts would push the hard work back to the people who break things.
Looks a bit longer than what I intended to write, as usual when one starts writing ;) If we come to a conclusion wrt. Gerrit privileges, we could also discuss more rules for each role. While I'm not a fan of too many rules and would rather keep things to common sense, these things depend much on the people we add to the respective groups.
I hope it's not too much to digest at once ;) Cheers, Nico _______________________________________________ flashrom mailing list -- flashrom@flashrom.org To unsubscribe send an email to flashrom-leave@flashrom.org