Differential D16730 Diff 53846 en_US.ISO8859-1/articles/committers-guide/article.xml

Changeset View

Standalone View

en_US.ISO8859-1/articles/committers-guide/article.xml

Show First 20 Lines • Show All 2,374 Lines • ▼ Show 20 Lines	<tbody>
<entry><filename>ports/svnadmin/conf/mentors</filename></entry>		<entry><filename>ports/svnadmin/conf/mentors</filename></entry>
</row>		</row>
</tbody>		</tbody>
</tgroup>		</tgroup>
</informaltable>		</informaltable>
</sect2>		</sect2>
</sect1>		</sect1>

		<sect1 xml:id="pre-commit-review">
		<title>Pre-Commit Review</title>

		<para>Code review is one way to increase the quality of software.
		The following guidelines apply to commits to the
		<literal>head</literal> (-CURRENT) branch of the
		<literal>src</literal> repository. Other branches and the
		<literal>ports</literal> and <literal>docs</literal> trees have
		their own review policies, but these guidelines generally apply
		to commits requiring review:</para>
		<itemizedlist>
		<listitem>
		<para>All non-trivial changes should be reviewed before they
		are committed to the repository.</para>
		</listitem>

		<listitem>
		<para>Reviews may be conducted by email, in
		<application>Bugzilla</application>, in
		bcrUnsubmitted Done Inline Actions s/Phabrictor/Phabricator/ bcr: s/Phabrictor/Phabricator/
		<application>Phabricator</application>, or by another
		jhbUnsubmitted Done Inline Actions Perhaps either "another mechanism" or "other mechanisms"? I probably have a slight preference for the first. jhb: Perhaps either "another mechanism" or "other mechanisms"? I probably have a slight preference…
		mechanism. Where possible, reviews should be public.</para>
		</listitem>

		<listitem>
		<para>The developer responsible for a code change is also
		responsible for making all necessary review-related
		changes.</para>
		</listitem>

		<listitem>
		<para>Code review can be an iterative process, which continues
		until the patch is ready to be committed. Specifically,
		once a patch is sent out for review, it should receive an
		explicit <quote>looks good</quote> before it is committed.
		So long as it is explicit, this can take whatever form makes
		sense for the review method.</para>
		</listitem>

		<listitem>
		<para>Timeouts are not a substitute for review.</para>
		</listitem>
		</itemizedlist>

		<para>Sometimes code reviews will take longer than you would hope
		for, especially for larger features. Accepted ways to speed up
		review times for your patches are:</para>

		<itemizedlist>
		<listitem>
		<para>Review other people's patches. If you help out,
		everybody will be more willing to do the same for you;
		goodwill is our currency.</para>
		</listitem>

		<listitem>
		<para>Ping the patch. If it is urgent, provide reasons why
		it is important to you to get this patch landed and ping
		it every couple of days. If it is not urgent, the common
		courtesy ping rate is one week. Remember that you are
		asking for valuable time from other professional
		developers.</para>
		impUnsubmitted Done Inline Actions Is there some way to suggest people work in small patch chunks so they can easily generate the small patches? imp: Is there some way to suggest people work in small patch chunks so they can easily generate the…
		</listitem>

		<listitem>
		<para>Ask for help on mailing lists, IRC, etc. Others
		may be able to either help you directly, or suggest a
		reviewer.</para>
		</listitem>

		<listitem>
		<para>Split your patch into multiple smaller patches that
		build on each other. The smaller your patch, the higher
		jhbUnsubmitted Done Inline Actions The 'an appropriate commit bit access' phrase reads a bit odd to me. Perhaps either 'an appropriate commit bit' or 'appropriate commit access'? jhb: The 'an appropriate commit bit access' phrase reads a bit odd to me. Perhaps either 'an…
		impUnsubmitted Done Inline Actions I'm not sure about this detail... There are times that I include non-committers on the review list because I know they are domain experts for something or another. This happens more with ports than with src since most domain experts are already src committers. I suspect this will cause heart-ache when we post it to developers. Also, I wouldn't think of committing a non-trivial vm fix w/o running it by alc, kib, jeffr, etc and having at least one of them sign off on it. I would accept the review from others, but I wouldn't consider even jhb's 'approval' of a phab review to be enough to commit it, even though he has appropriate commit access. I'm at a lost, though, for how to cast this into words we can use here. imp: I'm not sure about this detail... There are times that I include non-committers on the review…
		brooksAuthorUnsubmitted Done Inline Actions We could say "an appropriate subject-matter expert (usually a committer)." which would let use handle things like numerics where the de facto maintainer resigned his commit bit. I might also add something like: Some areas of the tree have more strict review requirements, e.g areas documented in the MAINTAINERS file of the source tree or ports infrastructure in the Mk subdirectory. brooks: We could say "an appropriate subject-matter expert (usually a committer)." which would let use…
		the probability that somebody will take a quick look at
		it.</para>

		<para>When making large changes, it is helpful to keep this
		in mind from the beginning of the effort as breaking large
		changes into smaller ones is often difficult after the
		fact.</para>
		</listitem>
		</itemizedlist>
		jhbUnsubmitted Done Inline Actions Maybe tweak this as: "Note that while anyone is welcome to review and give feedback on a patch, only an appropriate subject matter expert can approve a change." jhb: Maybe tweak this as: "Note that while anyone is welcome to review and give feedback on a patch…

		<para>Developers should participate in code reviews as both
		reviewers and reviewees. If someone is kind enough to review
		your code, you should return the favor for someone else.
		Note that while anyone is welcome to review and give feedback
		on a patch, only an appropriate subject-matter expert can
		jhbUnsubmitted Done Inline Actions s/an/a/ I would be less hesitant and say "is sufficient" vs "may suffice". jhb: s/an/a/ I would be less hesitant and say "is sufficient" vs "may suffice".
		approve a change. This will usually be a committer who works
		with the code in question on a regular basis.</para>

		<para>In some cases, no subject-matter expert may be available.
		In those cases, a review by an experienced developer is
		jhbUnsubmitted Done Inline Actions Missing word, maybe "is sufficient when" instead of "is when"? jhb: Missing word, maybe "is sufficient when" instead of "is when"?
		sufficient when coupled with appropriate testing.</para>
		</sect1>

<sect1 xml:id="commit-log-message">		<sect1 xml:id="commit-log-message">
<title>Commit Log Messages</title>		<title>Commit Log Messages</title>

<para>This section contains some suggestions and traditions for		<para>This section contains some suggestions and traditions for
how commit logs are formatted.</para>		how commit logs are formatted.</para>

<para>As well as including an informative message with each		<para>As well as including an informative message with each
commit, some additional information may be needed.</para>		commit, some additional information may be needed.</para>
▲ Show 20 Lines • Show All 2,946 Lines • Show Last 20 Lines