Date: Tue, 12 Feb 2019 22:48:39 +0000 (UTC) From: Brooks Davis <brooks@FreeBSD.org> To: doc-committers@freebsd.org, svn-doc-all@freebsd.org, svn-doc-head@freebsd.org Subject: svn commit: r52815 - head/en_US.ISO8859-1/articles/committers-guide Message-ID: <201902122248.x1CMmdvP035595@repo.freebsd.org>
next in thread | raw e-mail | index | archive | help
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>
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201902122248.x1CMmdvP035595>