Changeset View
Standalone View
en_US.ISO8859-1/articles/committers-guide/article.xml
Show First 20 Lines • Show All 2,296 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 FreeBSD Project encourages all committers to follow these | |||||
guidelines:</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>Phabrictor</application>, or by other | |||||
mechanism. Where possible, reviews should be public.</para> | |||||
</listitem> | |||||
bcr: s/Phabrictor/Phabricator/ | |||||
<listitem> | |||||
Done Inline ActionsPerhaps 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… | |||||
<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 “looks good” before it is committed.</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’re | |||||
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 | |||||
impUnsubmitted Done Inline ActionsIs 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… | |||||
build on each other. The smaller your patch, the higher | |||||
the probability that somebody will take a quick look at | |||||
it.</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 anyone is welcome to review and give feedback on a patch, | |||||
but only people with an appropriate commit bit access can | |||||
jhbUnsubmitted Done Inline ActionsThe '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 ActionsI'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 ActionsWe 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… | |||||
approve it.</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> | ||||
Done Inline ActionsMaybe tweak this as: "Note that while anyone is welcome to review and give feedback on a patch, jhb: Maybe tweak this as:
"Note that while anyone is welcome to review and give feedback on a patch… | |||||
<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> | ||||
<para>This information consists of one or more lines | <para>This information consists of one or more lines | ||||
containing the key word or phrase, a colon, tabs for formatting, | containing the key word or phrase, a colon, tabs for formatting, | ||||
and then the additional information.</para> | and then the additional information.</para> | ||||
Done Inline Actionss/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". | |||||
<para>The key words or phrases are:</para> | <para>The key words or phrases are:</para> | ||||
<informaltable frame="none" pgwide="1"> | <informaltable frame="none" pgwide="1"> | ||||
<tgroup cols="2"> | <tgroup cols="2"> | ||||
Done Inline ActionsMissing word, maybe "is sufficient when" instead of "is when"? jhb: Missing word, maybe "is sufficient when" instead of "is when"? | |||||
<tbody> | <tbody> | ||||
<row> | <row> | ||||
<entry><literal>PR:</literal></entry> | <entry><literal>PR:</literal></entry> | ||||
<entry>The problem report (if any) which is affected | <entry>The problem report (if any) which is affected | ||||
(typically, by being closed) by this commit. | (typically, by being closed) by this commit. | ||||
Multiple PRs may be specified on one line, separated by | Multiple PRs may be specified on one line, separated by | ||||
commas or spaces.</entry> | commas or spaces.</entry> | ||||
</row> | </row> | ||||
▲ Show 20 Lines • Show All 2,899 Lines • Show Last 20 Lines |
s/Phabrictor/Phabricator/