Dear coreboot,
this week the Gerrit review score -2 was discussed again in #coreboot@irc.freenode.net.
Several times, during the last two years, a review score of -2 was given and dealing with it caused a lot of discussion and friction.
This thread is not about the usefulness of -2. Currently it is the way it is set up and certain Gerrit users, those that can also submit change sets, have the right to assign a -2. This prohibits submitting the change set. The score of -2 has to be removed, and I think every Gerrit user with submit rights can do that, so that the change set can be submitted.
In contrast to a review score of -1, which is removed after every new upload of a patch set, -2 is sticky.
To formalize that a little bit, I propose the following addition to the developer guidelines.
1. A review score of -2 has to be assigned with a comment explaining, why the change set is blocked. If such a comment is not published within 24 hours after assigning -2, the -2 can be removed.
2. After answering to the comment, blocking the change set, a (useful) respond has to be published within *four* weeks. If there is no response, the change set can be submitted, if there are at least twice as many scores of +2 than -2.
3. If the review discussion lasts longer than two months, the topic should be brought up on the coreboot mailing list and, if there is no consensus, a vote should be announced, where within a week people can vote. There is no minimum participation number. The simple majority wins. If the votes equal, then the change set is rejected and won’t be submitted.
Thanks,
Paul
Paul Menzel wrote:
this week the Gerrit review score -2 was discussed again in #coreboot@irc.freenode.net.
Several times, during the last two years, a review score of -2 was given and dealing with it caused a lot of discussion and friction.
..
To formalize that a little bit
I think the last thing we need is more formality. I think we need to understand why -2 has caused problems, and then address that.
Adding a time limit to -2 scores is not helpful, then we can just as well remove the -2 score completely, which I hope noone thinks is a good idea.
A review-driven workflow is always slower than a commit-at-will free-for-all bazaar, and that's a highly desirable feature, because it increases code quality.
If someone is in a hurry with some change then it is their own responsibility to invest resources (time, money, or both) into getting this change reviewed. If they are clever, they invest more in working carefully with the community *before* creating that change, because that is *always* a net win.
Put another way: If a reviewer learns about a change for the first time when the change is pushed, then the person pushing the change has already made the mistake of not communicating before starting the work.
If a reviewer already knows about a change when it is pushed, then the reviewer is able to review this change MUCH faster, and as a bonus the change is much more likely to be exactly right on the first try.
Handling this pre-push pre-coding communication well is a distinct skill from doing the actual coding, and I have seen many many projects where amazing developers simply aren't able or willing to do it, and in those cases the solution isn't formalities but instead mentoring.
I'm not saying that there is no problem. I'm saying that formalities most likely is not an effective solution.
//Peter
-
Paul Menzel -
Peter Stuge