There are several questions raised in this thread, but good news is that at the meeting yesterday one of the questions was decided on!
We decided to have a Reviewers group for flashrom. Martin, can we ask you to help with creating a group? If you could help that would be great! Initial content of the group (as it is suggested in the starter email) is a copy of “flashrom developers”, and then we can add people on the top. Thank you! Given the existing "flashrom developers" , maybe a new group can be called "flashrom reviewers"? On Sat, Mar 26, 2022 at 9:11 PM Anastasia Klimchuk <a...@chromium.org> wrote: > > Hello! > > > and I vote 100% against it. FWIW, this feature wakes our worst in- > > stincts. No matter how convinced we are that review is also for our > > own good, getting a commit merged always feels rewarding. And many > > people act like it's a necessity to set the resolved tick to gain > > that reward. I've seen people doing so with every single comment they > > wrote since day 1 when this feature was activated for coreboot. Let's > > not repeat every mistake made there. > > > > Also, today Gerrit warns about unresolved comment threads when one > > tries to submit. Enforcing it brings a marginal convenience but makes > > the review process less friendly in my experience. > > > > Sometimes comment threads are opened after review. So the `merged` > > status never implies all-comments-resolved. And that's not bad, IMHO. > > Sometimes things come up after review and Gerrit makes it easy to > > continue to discuss changes even after they are merged. > > I have mixed feelings about this. On one hand, I understand what you > are saying about worst instincts, especially since you are saying you > observed examples how the feature can be workarounded. On the other > hand, it seems logical to me to have two types of comments: major ones > that need to be resolved before merging the patch, and everything else > (non-blocking). > Also, I am totally fine to leave as is: my own workflow won’t change. > I learned over the years that people have a variety of opinions about > yellow and grey comments, my workflow is trying to cover all of that > anyway :) > > > There also seems to be a misunderstanding about patches. Some people > > seem to believe that every patch should get merged eventually. > > I think, you gave a perfect explanation to this just above: “getting a > commit merged always feels rewarding” > > > IMO, problematic patches should not be escalated but avoided in the > > first place. In our wiki it says in multiple places that one should > > discuss things on the mailing list early so nobody gets frustrated > > later. Such problematic reviews is exactly why it's stated there. > > Open-source development is nothing new. Many problems have been fixed > > in the past already. Please always encourage everybody to try the > > established ways before experimenting with new ones. > > That’s all correct, for sure! But sometimes, established ways do not > work (for whatever reasons). And boom, we have a problematic patch. It > should not exist, but here it is. You called this “escalation” but > what if it is only reviewers discussing, it is not an escalation, it’s > the same people who are on the patch, just talking instead of typing? > And yes, if everything goes normally , it’s not needed. > > Re: encouragement, I always do what I can! > > > If we don't revert first, it > > will create pressure to fix things right now (which may break things > > again). > > Agree with that! I agreed with that from the very beginning :) > > > something like "Do you want to write > > that for upstream, there's the development guidelines: https://...";. > > Believe it or not, guidelines are something that we recently discussed > (guidelines for people who send patches upstream). But I need to work > on this a bit, I will tell later! > > However one topic I am not sure how to guide is: gerrit comments. The > only thing I understand by now, there are various opinions on gerrit > comments. > > Also, returning back to initial questions, an idea of having a > Reviewers group for flashrom. > Does this look good for everyone? > This comes together with a question: what are criteria to add someone > to Reviewers and Committers? > > This probably needs some brainstorming, so I am going to start, I give > it 5 mins, everything that comes to mind: > > Number of patches owned [for last X months/years] > Number of patches reviewed [for last X months/years] > Number of patches merged [for last X months/years] > Last time the patch was reviewed > Last time patch was send to review > Last time patch was merged > How long active on the project > Any topics on the mailing list > Reachable via email or channel > ….. > > > On Fri, Mar 18, 2022 at 2:44 AM Nico Huber <nic...@gmx.de> wrote: > > > > Hi Anastasia, > > > > On 17.03.22 12:32, Anastasia Klimchuk wrote: > > >> 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. > > > > > > Oh this explains! I was wondering where those “patches merged with > > > unresolved comments” are coming from. I am 100% voting for this setting to > > > be enabled. It does not prevent from just clicking Ack on all comments, if > > > needed. But at least, comments won’t be lost unintentionally. > > > > > > > and I vote 100% against it. FWIW, this feature wakes our worst in- > > stincts. No matter how convinced we are that review is also for our > > own good, getting a commit merged always feels rewarding. And many > > people act like it's a necessity to set the resolved tick to gain > > that reward. I've seen people doing so with every single comment they > > wrote since day 1 when this feature was activated for coreboot. Let's > > not repeat every mistake made there. > > > > Also, today Gerrit warns about unresolved comment threads when one > > tries to submit. Enforcing it brings a marginal convenience but makes > > the review process less friendly in my experience. > > > > Sometimes comment threads are opened after review. So the `merged` > > status never implies all-comments-resolved. And that's not bad, IMHO. > > Sometimes things come up after review and Gerrit makes it easy to > > continue to discuss changes even after they are merged. > > > > > And then, we can for example agree (if we agree) that if the Reviewer > > > wants > > > a response on the comment, they mark it Unresolved (it will be yellow > > > colour). > > > It doesn’t solve all the problems, but at least, it guards unintentional > > > mistakes. > > > > > >> it might help to have a weekly or bi-weekly meeting *just* to discuss > > >> patches face-to-face. > > > > > > I would agree with that. This is *another meeting* yes! But looking at how > > > things are going right now, a problematic patch takes so much unnecessary > > > time, it can be weeks or even months, as I said, unnecessary extra time. > > > > IMO, problematic patches should not be escalated but avoided in the > > first place. In our wiki it says in multiple places that one should > > discuss things on the mailing list early so nobody gets frustrated > > later. Such problematic reviews is exactly why it's stated there. > > Open-source development is nothing new. Many problems have been fixed > > in the past already. Please always encourage everybody to try the > > established ways before experimenting with new ones. > > > > There also seems to be a misunderstanding about patches. Some people > > seem to believe that every patch should get merged eventually. This > > is practically impossible. We can't merge every single line that was > > and will be written about flashrom into one project. Not everything > > is correct and not everything that is is compatible. Sometimes the > > most efficient thing to do with a patch is to abandon it. > > > > Sorry if I seem worked up over this. FWIW, such problematic reviews > > are one thing that's been congesting the project over the past three > > years. Of course, we can all try to learn from such reviews, but it > > needs to pay out for the project somehow. Otherwise, it would be more > > efficient and less frustrating if the reviewers would write everything. > > > > > And it starts involving lots of people, many of those people should not be > > > involved at all. And worst of all, it can cause emotional damage to people > > > involved. This is all wrong. We need to ask others for sure, but for > > > myself, I am happy to have another meeting, because it has a chance to > > > remove those problems. A patch should be a purely technical question. > > > > > >> Comparing the two roles there is another important point: One can't > > >> review their own commits but a core developer can submit their own > > >> commits. > > > > > > This is an important point. Reviewer is a very important second pair of > > > eyes. > > > I see my previous point about “Reviewed-by” tag is not that strong :) But > > > your one is. > > > > > >> It's kind of admin access but without > > >> being admin for the whole Gerrit instance. I never tried, but I assume > > >> we could remove a -2. In any case this group can change the rules, so > > >> it's only a question of how convenient it would be to override a -2. > > > > > > This is good, so there is a way to remove a -2. > > > I am still in the same position that “merging” and “block from merging” > > > are > > > similar powers with + and - sign, and it makes sense to have it in one > > > group. If this group (very small!) has no trust in each other, let’s solve > > > it. > > > > > >> And there is also a huge psychological component. When somebody knows > > >> they have the time to fix things after a revert, that seems fine. But if > > >> one doesn't have the time, it can really hurt to see something reverted. > > > > > > But wait, one needs to fix things if a patch breaks something? It is > > > technically a choice between “revert and fix” or “fix”? In both cases, > > > there is a “fix” involved… > > > > It's a matter of time and responsibility. Who will fix it and when? Will > > the one who broke things fix them? or the one who discovers the problem? > > or maybe even a non-involved maintainer? If we don't revert first, it > > will create pressure to fix things right now (which may break things > > again). > > > > > > > >> 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. > > > > > > I looked once again in my “what can go wrong” scenarios, and now I think > > > that #2 and #3 can be solved by having clear rules on when someone can be > > > added to Reviewers and Committers (and those clear rules are published so > > > everyone can see). Let’s brainstorm some clear rules? :) > > > The point #1 can be solved by a nice message which explains everything and > > > calms down people ;) > > > So actually, maybe it’s not so bad :) > > > > > >> 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. > > > > > > If I can help by listening, just tell me! > > > > > >> 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 think this is a connection to a second topic “Release preparations” that > > > I will respond to next :) > > > But what I think, the problems need to be listed, described and assigned. > > > Lots of these you did already! > > > > > >> 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. > > > > > > We can formalise this. Gerrit has yellow and grey comments. Yellow are > > > supposed to be blocking (major) and grey are non-blocking (minor). We can, > > > at the very least, state very clearly that yellow comments cannot be > > > ignored (and Martin said it can be enforced in Gerrit!). It is still > > > possible to give some random answer, but… better than nothing. > > > > How about we try education first? A hunch tells me that half-baked rules > > can't fix someone's attitude towards review. Taking Google for instance, > > there were some patches by CrOS developers since Edward started that > > endeavor. I don't know what they are told before they push upstream. But > > I assume it makes a huge difference if you tell them "Do you want to > > push that upstream?" compared to something like "Do you want to write > > that for upstream, there's the development guidelines: https://...";. > > > > > > > > And that’s what Greg said as well, findings can be major and minor. > > > Although in my head I see it as “comments that I expect a reply” and “just > > > comments”. This is why I almost always have yellow comments: I expect a > > > reply :P Unless it is something like “Awesome work thanks!” etc. > > > > > > And in addition to everything that I said, I have another thought. I hear > > > the words “right direction” and “wrong direction” from time to time (in > > > gerrit comments, and in this thread). And the thing with those words… They > > > are deceptive, because everyone has their own idea in their own head on > > > what is right / wrong direction, and it seems so obvious to the person, > > > and > > > no one thinks of explaining what exactly “right direction” means. But it > > > means different things to different people. > > > > It can be subjective but doesn't have to be. Unless something is > > obviously objective, figuring out what is what is impossible without > > talking about it. That's why we point people to the mailing list. > > We all should exercise this more, IMHO. If things turn out to be > > subjective and people disagree, it's definitely something for a > > meeting, though ;) > > > > Cheers, > > Nico > > > > -- > Anastasia. -- Anastasia. _______________________________________________ flashrom mailing list -- flashrom@flashrom.org To unsubscribe send an email to flashrom-le...@flashrom.org
