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