On 12.03.22 08:57, Anastasia Klimchuk wrote: >> 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.
That's also what I am afraid of. Looking through the recent Git history, I've seen many Reviewed-by tags of people that most likely wouldn't fall into the group if we formalize it now. It might help if we make plans in advance how to bring them into the group if they want to. > # 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 That's probably why I'm so afraid of rules ;) start with one and you'll have to add a dozen more. We'd have to properly formalize everything around the groups to avoid regressions. >> 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… Technically, I'm beyond the scale already. But I don't want to burden you with that story. There is not much to worry about right now. Today, the most unsettling thing seems to me is that we spend a lot of time to address problems that people may have accidentally imported into the project. > > 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! Well, I'm not sure but something inside of me wants to object. It's hard to put into words, the things I've reverted in the past are not really of a specific kind but still seem to share something. Roughly, when a patch got a decent review there should only be typos left that could cause trouble; it should be easier to fix than to revert. I've noticed something related in reviews over the years, though. Some- times when reviewers give a lot of comments on Gerrit, among them some critical ones about the overall patch and a lot of nits, the author tends to fix the nits and ignore the critical comments. Sure, when somebody ignores comments and then things get reverted, it's techni- cally their fault. But I can well imagine that such incidents would affect future reviews, also for the reviewers. Somehow, everybody learned that they wasted their time :-/ Reading what I just wrote, it sounds like we'd need more review guidelines ._. Nico > 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? :) _______________________________________________ flashrom mailing list -- flashrom@flashrom.org To unsubscribe send an email to flashrom-le...@flashrom.org
