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 _______________________________________________ flashrom mailing list -- flashrom@flashrom.org To unsubscribe send an email to flashrom-le...@flashrom.org
