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
This commit is contained in:
parent
57915394a7
commit
6aedd1788e
Notes:
svn2git
2020-12-08 03:00:23 +00:00
svn path=/head/; revision=52815
1 changed files with 95 additions and 0 deletions
|
|
@ -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>
|
||||
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue