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 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.”
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 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?”
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 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.”
Makes sense. I thought “revert first, fix later” is somewhat a general rule?
Especially when you are saying:
“At least for myself
this 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? :)
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
--