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
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 ;)