Hi Anastasia, Martin,
many thanks for your thoughts on the matter. \o/
On 12.03.22 22:29, Martin L Roth wrote:
On Sat, Mar 12, 2022 at 12:57 AM Anastasia Klimchuk aklm@chromium.org wrote: ... On Sat, Mar 5, 2022 at 9:17 AM Nico Huber nico.h@gmx.de wrote: ...
“The big difference seems to be that at coreboot merging patches, i.e. hitting the "Submit" button, is more or less considered an administrative task unrelated to one's opinion on the code. While for us it was supposed to replace the gatekeeping. Changing this would imply that we need our own group of reviewers. With this, a +2 would become 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.
I never actually considered merges just administrative and unrelated to the reviews in coreboot. I definitely review every patch before I merge it, though I'm definitely not as picky or thoughtful as some reviewers are. I'm pretty sure it says in the gerrit guidelines that if someone merges a bad patch, they're responsible for helping to fix it.
The coreboot guidelines indeed state one is expected to help. We never made this explicit for flashrom. There is definitely room for improve- ment in our guidelines when we agree on something :)
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. So I'd put it like this: The reviewer's task is to make sure the code is sound (that should include the whole result, existing code + patch). It's something everyone can use a second pair of eyes for. The submitter should make sure things generally move into the right direction (which is much less objective than the review) and take responsibility for a commit.
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.
Just note that the "Reviewed-by" tag is only for people who gave a +1 or +2 in themerged revision of the patch on Gerrit. Any earlier reviewers are lost.The submitter is of course included in the git history as well.
Some background on the mechanics: Git is prepared to distinguish the _author_ and the _committer_ of a commit. The committer is not shown by default in the log. One can make it visible with `git log --pretty=fuller`. Gerrit also shows this information in the diff view of commit messages.
The author information is set when a commit is created and usually never altered again. If a commit is amended or cherry-picked, however, the committer information is updated. Gerrit also does so when somebody clicks "Submit".
Regarding visibility there's a wrinkle: Technically, the committer information is tighter integrated into the commit, compared to the Reviewed-by tag. Only the latter is shown in the log by default, though.
It is possible to remove a -2, but it happens very rarely on the coreboot project, and I think that currently only 3 people have that right. If flashrom would like to set different rules, that can be set up.
There are currently four people set up for flashrom as "owners" (Angel, David, Stefan T. and myself). 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.
“There is one related idea for a rule that crossed my mind: How about we forbid blocking reverts that fix a regression and apply cleanly (i.e. without needing to revert anything else in advance). Also, let them pass after review without waiting 24 hours.”
Makes sense. I thought “revert first, fix later” is somewhat a general rule?
I would have expected that too. But unfortunately not everybody agrees. 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.
Hopefully, if the review process gets better, this wouldn't need to happen very often. I do think that everyone is really trying to do what they think is best for the project, and that miscommunication may be responsible for a lot of the issues. I definitely don't think that any of the significant contributors are looking to cause friction within the project or codebase.
With the words above about the reviewer and submitter roles in mind, I'd say the reverts of the past three years are more of a review issue. I mean we only revert when the code is broken. And so far the things that I see to be moving into a wrong direction seem to be cases where this is nobody's intention. It's mostly that the code was written for another project/fork and nobody had the time yet to adapt it for upstream. At least I hope so :)
Nico