svn commit: r52815 - head/en_US.ISO8859-1/articles/committers-guide

[Brooks Davis]

Author: brooks (src,ports committer)
Date: Tue Feb 12 22:48:39 2019
New Revision: 52815
URL: https://svnweb.freebsd.org/changeset/doc/52815

Log:
  Committers Guide: Add a section encouraging pre-commit review.
  
  This is based on LLVM's Code Review policy.  It differs it not requiring
  review for non-trivial changes.
  
  Reviewed by:	bcr (verbal), allanjude, imp, jhb
  Approved by:	core
  Differential Revision:	https://reviews.freebsd.org/D16730

Modified:
  head/en_US.ISO8859-1/articles/committers-guide/article.xml

Modified: head/en_US.ISO8859-1/articles/committers-guide/article.xml
==============================================================================
--- head/en_US.ISO8859-1/articles/committers-guide/article.xml	Mon Feb 11 18:34:32 2019	(r52814)
+++ head/en_US.ISO8859-1/articles/committers-guide/article.xml	Tue Feb 12 22:48:39 2019	(r52815)
@@ -2380,6 +2380,101 @@ freebsd-mfc-after = 2 weeks</programlisting>
     </sect2>
   </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
+	  <application>Phabricator</application>, or by another
+	  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>
+      </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
+	  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>
+
+    <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
+      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
+      sufficient when coupled with appropriate testing.</para>
+  </sect1>
+
   <sect1 xml:id="commit-log-message">
     <title>Commit Log Messages</title>