So I suggest the following. We could have 2 ways to get a patch approved.
- For patches that have multiple code owners:
- COR+1 (not from patch owner)
- MR+1 (not from patch owner, not from the person setting COR+1)
- For patches that a have single code owner:
- CR+1 (not from patch owner)
- MR+1 (not from patch owner, not from the person setting CR+1)
Well, wouldn't this mean that patches which have a single code owner never need owner approval in any case? I think the intention was to only make a special case if the code owner themselves write the code, right? In that case, you would have to write this slightly differently:
1) Normal case: * COR+1 (not from author) * MR+1 (not from author, not from the person setting COR+1)
2) Special case: * author must have COR rights (or maybe: author must be the only person with COR rights?) * CR+1 (not from author) * MR+1 (not from author, not from the person setting CR+1)
This is what I was aiming at with the suggestion to allow code owners to COR+1 themselves, except that I wasn't sure if a rule like "author must have COR rights" can be implemented in Gerrit. If we can write such a rule, then yes, having the author set the bit explicitly is unnecessary, it's enough to let the framework confirm that they would have the rights to do so.
Note that in some cases, we would need to mix both policies. If a patch modifies several modules, some of them having multiple code owners and others having a single code owner, we would apply one or the other policy for each module.
Right, would have to apply this check for each module individually.
Also, 1) assumes we always want different individuals doing the code-owner review and the maintainer review. I personally don't feel strongly about this split and I would be fine with the same person doing both types of review, as they are typically looking at different criteria.
Either option seems reasonable to me. I expect in most cases the code owners will not also be maintainers anyway, so for the few cases where they are, it's probably only fair to hold them to the same standard (i.e. ensure that at least two people reviewed the patch).