On Sat, Mar 12, 2022 at 12:57 AM Anastasia Klimchuk aklm@chromium.org wrote: ... On Sat, Mar 5, 2022 at 9:17 AM Nico Huber nico.h@gmx.de wrote: ...
“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.
I never actually considered merges just administrative and unrelated to the reviews in coreboot. I definitely review every patch before I merge it, though I'm definitely not as picky or thoughtful as some reviewers are. I'm pretty sure it says in the gerrit guidelines that if someone merges a bad patch, they're responsible for helping to fix it.
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.
Just note that the "Reviewed-by" tag is only for people who gave a +1 or +2 in themerged revision of the patch on Gerrit. Any earlier reviewers are lost.The submitter is of course included in the git history as well.
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 :)
I Absolutely agree. The flashrom project should decide how to decide who gets what rights, and a new set of groups can be created. Even though all of the projects managed at coreboot.org have a similar set of rules, that's not by necessity. For example, the flashrom project doesn't require that all comments be resolved before merge. That can be enabled if you'd like, but currently it isn't.
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 :)
I think that the change just needs to be announced, along with the requirements for flashrom +2. I don't see why people would expect to be able to +2 flashrom patches if they haven't submitted anything in a long time. I just removed a few people's +2 rights from coreboot - anyone who hadn't submitted a patch in the past 4 years or so. I'll also occasionally go through and remove submit rights, though I typically talk to the person before doing that.
“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?
It is possible to remove a -2, but it happens very rarely on the coreboot project, and I think that currently only 3 people have that right. If flashrom would like to set different rules, that can be set up.
Depending on how fast you want to be able to get patches merged at flashrom, it might help to have a weekly or bi-weekly meeting *just* to discuss patches face-to-face. It seems like there's some miscommunication, and as we saw at the flashrom meeting last week, people can discuss things better vocally. Even just an impromptu meeting between two reviewers to hash out the points would be better than what's happening now, where people seem to be talking past each other.
“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?
Hopefully, if the review process gets better, this wouldn't need to happen very often. I do think that everyone is really trying to do what they think is best for the project, and that miscommunication may be responsible for a lot of the issues. I definitely don't think that any of the significant contributors are looking to cause friction within the project or codebase.
I hope it's not too much to digest at once ;)
This is all I can digest for now, so I'm ending my response here. To anyone following on, please go back to the previous emails to read the rest of the discussion.
Cheers, Nico
-- Anastasia.
Take care, Martin
On Sat, Mar 12, 2022 at 12:57 AM Anastasia Klimchuk aklm@chromium.org wrote:
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? :)
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
-- Anastasia. _______________________________________________ flashrom mailing list -- flashrom@flashrom.org To unsubscribe send an email to flashrom-leave@flashrom.org