Based on IRC discussions, some of this is my interpretation.
- Group Reviewers
- R9:274b93aba7f1: comitters-guide: clarify submitted and approved post git
I think these are good. One minor comment on one of the changes, but I'm having a hard time turning it into a suggestion so maybe it is fine to enshrine it here for historians....
We've also customarily used it when a MAINTAINER approves the change, but for whatever reason doesn't do a full code review, though this usage hasn't been used much lately. I'm on the fence about whether or not this needs to be called out. The original language was to encourage people to coordinate when they had changes in other parts of the tree than they normally did when we had huge sweeping changes rippling through the tree for things like newbus and smpng to avoid surprises that would result in a lot of extra work....
These days I guess those last use cases are covered by herald rules and reviewed by, so maybe it's fine and noting it here for context will suffice.
I'll wait several days for other comments.
This diff is to solicit and minimally address any doc work left from the git transition and current practices I've discovered or been told so I can back out anything orthogonal for another review.
I can send in another one with some suggestion for moving this case of respect for maintainers and authors into reviewed by and add some guidance on confidence levels if you think it would be useful, something like:
Reviewed by: jrm, imp (concept ACK), kbowling (utACK), bapt (NACK, PR888#c33)
If a maintainer reviewed a change and it's being committed, it certainly means the change was approved, so only Approved by: username (maintainer) or Approved by: Full Name <valid@email> (maintainer) would be included.
Based on IRC conversations, this is not obvious, so maybe we should allow portmgr to weigh in and maybe this should be clarified in the documentation.
My interpretation of the IRC exchange is that approval is implicit if the author equals the maintainer.
For a non-author commit to a maintained port, I think we should move that to reviewed by in all cases with my proposal below but would like some feedback from seasoned devs. An implied utACK would cover cases like the vacation notices to developers lists or explicitly for lack of time, while a the others would indicate the confidence in the change (which might help drive the decision to revert and try again later).