Differential D30969 Diff 92247 documentation/content/en/articles/committers-guide/_index.adoc

Changeset View

Standalone View

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

	Show First 20 Lines • Show All 2,797 Lines • ▼ Show 20 Lines
	\|The problem report (if any) which is affected (typically, by being closed) by this commit. Multiple PRs may be specified on one line, separated by commas or spaces.			\|The problem report (if any) which is affected (typically, by being closed) by this commit. Multiple PRs may be specified on one line, separated by commas or spaces.

	\|`Reported by:`			\|`Reported by:`
	\|The name and e-mail address of the person that reported the issue; for developers, just the username on the FreeBSD cluster.			\|The name and e-mail address of the person that reported the issue; for developers, just the username on the FreeBSD cluster.
	Typically used when there is no PR, for example if the issue was reported on			Typically used when there is no PR, for example if the issue was reported on
	a mailing list.			a mailing list.

	\|`Submitted by:`			\|`Submitted by:`
	\|The name and e-mail address of the person that submitted the fix; for developers, just the username on the FreeBSD cluster.			\|This has been deprecated with git; submitted patches should have the author set by using `git commit --author` with a full name and valid email.

	Typically not used with Git; submitted patches should			\|`Reviewed by:`
	have the author set by using `git commit --author`.			\|The name and e-mail address of the person or people that reviewed the change; for developers, just the username on the FreeBSD cluster. If a patch was submitted to a mailing list for review, and the review was favorable, then just include the list name. If the reviewer is not a member of the project, provide the name, email, and if ports an external role like maintainer:
				a\|

	If the submitter is the maintainer of the port being committed, include "(maintainer)" after the email address.			Reviewed by a developer:
				[source,shell]
				....
				Reviewed by: username

	Avoid obfuscating the email address of the submitter as this adds additional work when searching logs.			Reviewed by a ports maintainer that is not a developer:
				[source,shell]
				....
				Reviewed by: Full Name <valid@email> (maintainer)
				jrmUnsubmitted Not Done Inline Actions 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. jrm: If a maintainer reviewed a change and it's being committed, it certainly means the change was…
				kbowlingAuthorUnsubmitted Not Done Inline Actions 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). kbowling: My interpretation of the IRC exchange is that approval is implicit if the author equals the…
				....

	\|`Reviewed by:`
	\|The name and e-mail address of the person or people that reviewed the change; for developers, just the username on the FreeBSD cluster. If a patch was submitted to a mailing list for review, and the review was favorable, then just include the list name.

	\|`Tested by:`			\|`Tested by:`
	\|The name and e-mail address of the person or people that tested the change; for developers, just the username on the FreeBSD cluster.			\|The name and e-mail address of the person or people that tested the change; for developers, just the username on the FreeBSD cluster.

	\|`Approved by:`			\|`Approved by:`
	a\|			a\|

	The name and e-mail address of the person or people that approved the change; for developers, just the username on the FreeBSD cluster.			The name and e-mail address of the person or people that approved the change; for developers, just the username on the FreeBSD cluster.

	There are several cases where approval is customary:			There are several cases where approval is customary:

	* while a new committer is under mentorship			* while a new committer is under mentorship
	* commits to an area of the tree to which you do not usually commit			* commits to an area of the tree covered by the LOCKS file (src)
				impUnsubmitted Not Done Inline Actions 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. imp: We've also customarily used it when a MAINTAINER approves the change, but for whatever reason…
				kbowlingAuthorUnsubmitted Not Done Inline Actions 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) kbowling: I'll wait several days for other comments. This diff is to solicit and minimally address any…
	* during a release cycle			* during a release cycle
	* committing to a repo where you do not hold a commit bit (e.g. src committer committing to docs)			* committing to a repo where you do not hold a commit bit (e.g. src committer committing to docs)

	While under mentorship, get mentor approval before the commit. Enter the mentor's username in this field, and note that they are a mentor:			While under mentorship, get mentor approval before the commit. Enter the mentor's username in this field, and note that they are a mentor:

	[source,shell]			[source,shell]
	....			....
	Approved by: username-of-mentor (mentor)			Approved by: username-of-mentor (mentor)
	▲ Show 20 Lines • Show All 1,159 Lines • Show Last 20 Lines