comitters-guide: clarify submitted and approved post git

[kbowling]

Summary

Based on IRC discussions, some of this is my interpretation.

[imp]

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....

documentation/content/en/articles/committers-guide/_index.adoc

2823

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.

[kbowling]

documentation/content/en/articles/committers-guide/_index.adoc

2823

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:

• bare: a full ACK, which implies non-trivial review and testing and a willingness to help fix any fallout and issues
• Concept ACK: I agree with the overall goal, and signaling approval for the submitter to continue down this line of work
• Approach ACK: Stronger than a concept ACK, to include agreement with the implementation details like data structure, algorithm etc but not a full stamp of testing
• utACK: primarily for maintainers to say that the direction of the work is fitting and wont cause architectural issues but the reviewer hasn't done and doesn't intend to do extensive QA at this time.
• NACK: disagree with the changes or goal (can be a concept NACK, approach NACK, bare NACK for overall disagreement). Should be accompanied by an explanation in a linked review or PR.

Reviewed by: jrm, imp (concept ACK), kbowling (utACK), bapt (NACK, PR888#c33)

[jrm]

documentation/content/en/articles/committers-guide/_index.adoc

2806–2817

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.

[kbowling]

documentation/content/en/articles/committers-guide/_index.adoc

2806–2817

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).