“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 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. 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… “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. 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. On Mon, Mar 14, 2022 at 1:59 AM Greg Troxel <g...@lexort.com> wrote: > > > 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 :-/ > > In formal software engineering culture, there is a notion of findings > being major vs minor, where major is failure to meet specification and > minor is something else, usually style. > > I think the basic issue is that flashrom, quite reasonably, is trying to > achieve a far higher level of software quality than is typical in open > source. > > It might be reasonable to expect that after a review which had a major > finding, to expect it to be re-reviewed before merging. > > -- Anastasia.
_______________________________________________ flashrom mailing list -- flashrom@flashrom.org To unsubscribe send an email to flashrom-le...@flashrom.org
