ContributionReviews.md - wiki - Git at Google

 The [Melange project](MelangeIntro.md) has a variety of opportunities for the
 community to contribute, including:

   * [Filing an issue](http://code.google.com/p/soc/issues/entry) reporting a defect or requesting a new feature
   * Contributing a change to Melange's code implementing new behavior

 Each of these contributions is subject to review by other members of the
 Melange community.  The guidelines for the review of various types of
 contributions are described here.

 ## CLA

 <h4>All contributors must <a href='ContributorLicenseAgreements.md'>sign the CLA</a> before we can accept their contributions.<h4>

 <h2>Committers</h2>

 Committers in the Melange community are those individuals who have been<br>
 made project members or project owners in the<br>
 <a href='http://code.google.com/p/soc/'>SoC project</a> on<br>
 <a href='http://code.google.com'>Google Code</a>.  These individuals have been given<br>
 commit privileges as a result of their previous contributions to the<br>
 Melange community, usually in the form of<br>
 <a href='ContributionReviews#Patches.md'>patches</a>.  These individuals are the<br>
 "gatekeepers" of the community, helping to insure that standards such<br>
 as <a href='PythonStyleGuide.md'>programming style</a> and<br>
 <a href='TestingGuidelines.md'>testing</a> that are important to the community as a<br>
 whole are adhered to by each contributor and for each contribution.<br>
 <br>
 <a href='Hidden comment:
 Each committer will:

 * be a project member (or possibly project owner) at http://code.google.com/p/soc/
 * be a [MailingListsGuidelines#Developer_list developer mailing list] manager (or possibly list owner) at http://groups.google.com/group/melange-soc-dev
 * have an account at at [http://mail.google.com/a/googleopensourceprograms.com googleopensourceprograms.com]
 * be able to [ContributionReviews#Using_Rietveld_for_code_reviews conduct code reviews] at http://codereviews.googleopensourceprograms.com/
 * have ACCESS template VOP (AUTOVOICE) in the [IrcChannelGuidelines `#melange` IRC channel]
 '></a><br>
 <br>
 <h3>Casual contributors</h3>

 Casual developers can contribute to the <a href='MelangeIntro.md'>Melange project</a>
 primarily through the <a href='ContributionReviews#Patches.md'>patch process</a>
 described below.  Casual contributors still need to agree to the<br>
 <a href='ContributionTerms.md'>guidelines for contributors</a> before their patch<br>
 can be accepted and commmitted by a<br>
 <a href='ContributionReviews#Committer.md'>committer</a>.<br>
 <br>
 <h2>Code reviews</h2>

 Code reviews are handled in a variety of ways, depending on the size of<br>
 the change and the role of the contributor in the Melange community.  Most<br>
 contributions will be in the form of small<br>
 <a href='ContributionReviews#Suggested_patches_from_casual_contributors.md'>patches</a> and<br>
 <a href='ContributionReviews#Reviews_after_committed_patches.md'>commits</a>. Larger<br>
 contributions, such as entire new features, probably require a different<br>
 approach, such as incremental patches and communication with the other<br>
 developers.  These have similar review approaches, but tailored to the<br>
 specific situation.<br>
 <br>
 <h3>Code changes should be atomic</h3>

 The commit of a source code change should result in a source repository<br>
 where the software <a href='BuildingMelange.md'>builds</a> and passes any existing<br>
 <a href='TestingGuidelines.md'>tests</a>.  So, all related changes need to be committed<br>
 in such a way that each patch passes the test suite.  That usually means<br>
 that new functionality is introduced in one patch, 'enabled' in another one,<br>
 and the old functionality removed in a third one.<br>
 <br>
 <h3>Patches</h3>

 The term "patches" is used to refer to relatively small changes to the<br>
 code (and, yes, this definition is intentionally vague).<br>
 <br>
 Patches should be sent for review via our <a href='Gerrit.md'>Gerrit</a> instance.<br>
 <br>
 <h3>Patchfiles</h3>

 It is prefered that all Patches be sent via <a href='Gerrit.md'>Gerrit</a>, but if you need to<br>
 create a traditional Patchfile, you can follow these instructions:<br>
 <br>
 Usually these<br>
 are in the form of output from the <code>git diff</code> command.<br>
 <br>
 Patches should be sent for review to the<br>
 <a href='MailingListsGuidelines#Developer_list.md'>developer mailing list</a> and<br>
 presented in the "unified <code>diff</code>" format, which is de-facto standard<br>
 for many tools and the output from the <code>git diff</code> command (the same<br>
 format as that produced by <code>diff -uNr</code>).  Using Git:<br>
 <br>
 <pre><code>git diff --no-prefix &gt; your-patch-name.patch<br>
 </code></pre>

 The reviewer can then apply the patch using something like:<br>
 <br>
 <pre><code>patch -p0 &lt; your-patch-name.patch<br>
 </code></pre>

 <h4>Patch emails</h4>

 Patch emails sent to the developer mailing list should begin with<br>
 <code>[PATCH]</code> as the first text in the email subject line, followed by a<br>
 brief (one line) description of the change.<br>
 <br>
 For "small" patches (for some definition of "small", please use good<br>
 judgment), include the diff in the email body, to facilitate inline<br>
 commenting in the code review email thread.  For <i>all</i> patches, including<br>
 both "small" and "non small" ones, include the diff itself as an email<br>
 attachment, to make it easier for reviewers to patch the change into a<br>
 working copy with <code>patch</code>.  Such a diff attachment can be produced with<br>
 the methods described above.<br>
 <br>
 <a href='Hidden comment:
 ==== Suggested patches from casual contributors ====

 The Melange community is interested in patches and other contributions
 from casual developers.  To that end, casual developers can submit
 patches for review in a couple of ways:

 * attach a patch to an [http://code.google.com/p/soc/issues/list issue in the issue tracker]
 * mailing a patch to the [MailingListsGuidelines#Developer_list developer mailing list] (membership in the list is required to post to it; see the [MailingListsGuidelines#Developer_list list guidelines])

 Committers can then discuss the contribution with the author on the
 [MailingListsGuidelines#Developer_list developer mailing list], or on the
 [MailingListsGuidelines#General_discussion_list general discussion list]
 if the contributor is not on the developer list.  Since the
 [MelangeIntro Melange project] and the [SoCOverview SoC framework] is mostly
 about code, there are likely to be at least a few code discussions on the
 general announcement list.  Serious discussions will likely result in the
 casual contributor being invited to the developer list to move the discussion
 there.

 Once the patch is accepted, a committer will patch a working copy and commit
 the changed (or added or deleted) source files to the repository,
 supplying a suitable commit log message.
 '></a><br>
 <br>
 <h4>Code Review</h4>

 Code review happens in the <a href='Gerrit.md'>Gerrit</a> system.<br>
 <br>
 All patches need a +2 from a Comitter before they can be submitted.<br>
 <br>
 <a href='Hidden comment:
 ==== Reviews after committed patches ====

 Contributors with [ContributionReviews#Committers commit access] can submit
 changes prior to review.  The participants on the
 [MailingListsGuidelines#Developer_list developer mailing list] are then
 encouraged to review the change, which will be present in the commit
 email that was sent to the mailing list.  At least one other person with
 commit access should acknowledge the change with a brief "looks good to me" (LGTM)
 message in response (sent back to the list).  This does of course not prevent
 other list participants from being able to make suggestions, request
 changes, etc.

 If one or more of the list participants requests changes to the submitted
 code, they can either discuss it and have the original committer submit a
 subsequent change, or the they can make the change themselves.  Please
 avoid conflict by trying to reach a consensus before making commits.  One
 way to do this is to
 [ContributionReviews#Patches follow the patch contribution method] for
 reviewing a change on the developer mailing list, even if the contributor
 has commit access.

 ===== Review after commit using Google Code code reviews =====

 Google Code now has a feature that enables
 [http://code.google.com/p/support/wiki/CodeReviews code reviews of committed code].
 This feature can also be used by project owners and members to do review-after-commit
 style code reviews.

 '></a><br>
 <br>
 <h3>Committing</h3>

 Once all of the<br>
 [TestingGuidelines#All_changes_should_pass_smoke_tests_before_review<br>
 pre-commit tests] pass <b>and</b> the [ContributionReviews#Patches patch<br>
 has been code-reviewed] a <a href='ContributionReviews#Committers.md'>committer</a>
 (who may or<br>
 [ContributionReviews#Suggested_patches_from_casual_contributors may<br>
 not] be the actual contributor), the submit button in <a href='Gerrit.md'>Gerrit</a> or `git<br>
 merge && git push` can be used to commit the change into the<br>
 <a href='http://code.google.com/p/soc/source/browse'>SoC project repository</a>.<br>
 <br>
 <h4>commit messages</h4>

 Committers should use the template below for <code>git commit</code> log messages:<br>
 <br>
 <pre><code>One line, under 68 characters giving a short description of the change<br>
 <br>
 After a blank line, a more detailed log message explaining the reason<br>
 of the change (not necessarily what was changed, as that can be seen<br>
 by readin the patch). Please wrap your lines at 70 characters again.<br>
 </code></pre>

 If committing contributor patch (outside of Gerrit) please include:<br>
 <br>
 <pre><code>Reviewed by: Rick Reviewer<br>
 </code></pre>

 Note that previously the person listed with "Patch by:" was the<br>
 contributor (that is, author) of the patch, however we have switched<br>
 to Git and now we are committing patch from contributors as if<br>
 they would have commit access using command:<br>
 <br>
 <pre><code>git commit -u "FirstName LastName &lt;somebody@example.com&gt;"<br>
 </code></pre>

 Patches from<br>
 [ContributionReviews#Suggested_patches_from_casual_contributors casual<br>
 contributors] will, by definition, need to be committed by someone<br>
 other than the author but Git repository history will indicate<br>
 contributor as committer.<br>
 <br>
 <h2>Documentation</h2>

 Code files should include enough internal documentation in the form of<br>
 <code>__doc__</code> strings, comments and functions with self-explanatory names<br>
 (see PythonStyleGuide) that the automatic documentation suffices.<br>
 This is the recommended way to ensure that code documentation and the<br>
 code itself stay in step.<br>
 <br>
 The wiki is in the transition zone between dialog (IRC and mailing<br>
 lists) and formally reviewed content (code and its comments).  Changes<br>
 to the wiki are 'review after commit'.  Any committer can log comments<br>
 for a wiki page and participate in review.  At the moment rights to<br>
 create pages and edit pages directly are restricted to committers and<br>
 regular contributors to documentation.<br>
 <br>
 <hr />
 <i>Copyright 2008 Google Inc.</i>
 <i>This work is licensed under a</i>
 <a href='http://soc.googlecode.com/svn/wiki/html/licenses/cc-by-attribution-2_5.html'>Creative Commons Attribution 2.5 License</a>.<br>
 <a href='http://creativecommons.org/licenses/by/2.5/'><img src='http://soc.googlecode.com/svn/wiki/html/licenses/cc-by-2_5-88x31.png' /></a>
	The [Melange project](MelangeIntro.md) has a variety of opportunities for the
	community to contribute, including:

	* [Filing an issue](http://code.google.com/p/soc/issues/entry) reporting a defect or requesting a new feature
	* Contributing a change to Melange's code implementing new behavior

	Each of these contributions is subject to review by other members of the
	Melange community. The guidelines for the review of various types of
	contributions are described here.

	## CLA

	<h4>All contributors must <a href='ContributorLicenseAgreements.md'>sign the CLA</a> before we can accept their contributions.<h4>

	<h2>Committers</h2>

	Committers in the Melange community are those individuals who have been<br>
	made project members or project owners in the<br>
	<a href='http://code.google.com/p/soc/'>SoC project</a> on<br>
	<a href='http://code.google.com'>Google Code</a>. These individuals have been given<br>
	commit privileges as a result of their previous contributions to the<br>
	Melange community, usually in the form of<br>
	<a href='ContributionReviews#Patches.md'>patches</a>. These individuals are the<br>
	"gatekeepers" of the community, helping to insure that standards such<br>
	as <a href='PythonStyleGuide.md'>programming style</a> and<br>
	<a href='TestingGuidelines.md'>testing</a> that are important to the community as a<br>
	whole are adhered to by each contributor and for each contribution.<br>
	<br>
	<a href='Hidden comment:
	Each committer will:

	* be a project member (or possibly project owner) at http://code.google.com/p/soc/
	* be a [MailingListsGuidelines#Developer_list developer mailing list] manager (or possibly list owner) at http://groups.google.com/group/melange-soc-dev
	* have an account at at [http://mail.google.com/a/googleopensourceprograms.com googleopensourceprograms.com]
	* be able to [ContributionReviews#Using_Rietveld_for_code_reviews conduct code reviews] at http://codereviews.googleopensourceprograms.com/
	* have ACCESS template VOP (AUTOVOICE) in the [IrcChannelGuidelines `#melange` IRC channel]
	'></a><br>
	<br>
	<h3>Casual contributors</h3>

	Casual developers can contribute to the <a href='MelangeIntro.md'>Melange project</a>
	primarily through the <a href='ContributionReviews#Patches.md'>patch process</a>
	described below. Casual contributors still need to agree to the<br>
	<a href='ContributionTerms.md'>guidelines for contributors</a> before their patch<br>
	can be accepted and commmitted by a<br>
	<a href='ContributionReviews#Committer.md'>committer</a>.<br>
	<br>
	<h2>Code reviews</h2>

	Code reviews are handled in a variety of ways, depending on the size of<br>
	the change and the role of the contributor in the Melange community. Most<br>
	contributions will be in the form of small<br>
	<a href='ContributionReviews#Suggested_patches_from_casual_contributors.md'>patches</a> and<br>
	<a href='ContributionReviews#Reviews_after_committed_patches.md'>commits</a>. Larger<br>
	contributions, such as entire new features, probably require a different<br>
	approach, such as incremental patches and communication with the other<br>
	developers. These have similar review approaches, but tailored to the<br>
	specific situation.<br>
	<br>
	<h3>Code changes should be atomic</h3>

	The commit of a source code change should result in a source repository<br>
	where the software <a href='BuildingMelange.md'>builds</a> and passes any existing<br>
	<a href='TestingGuidelines.md'>tests</a>. So, all related changes need to be committed<br>
	in such a way that each patch passes the test suite. That usually means<br>
	that new functionality is introduced in one patch, 'enabled' in another one,<br>
	and the old functionality removed in a third one.<br>
	<br>
	<h3>Patches</h3>

	The term "patches" is used to refer to relatively small changes to the<br>
	code (and, yes, this definition is intentionally vague).<br>
	<br>
	Patches should be sent for review via our <a href='Gerrit.md'>Gerrit</a> instance.<br>
	<br>
	<h3>Patchfiles</h3>

	It is prefered that all Patches be sent via <a href='Gerrit.md'>Gerrit</a>, but if you need to<br>
	create a traditional Patchfile, you can follow these instructions:<br>
	<br>
	Usually these<br>
	are in the form of output from the <code>git diff</code> command.<br>
	<br>
	Patches should be sent for review to the<br>
	<a href='MailingListsGuidelines#Developer_list.md'>developer mailing list</a> and<br>
	presented in the "unified <code>diff</code>" format, which is de-facto standard<br>
	for many tools and the output from the <code>git diff</code> command (the same<br>
	format as that produced by <code>diff -uNr</code>). Using Git:<br>
	<br>
	<pre><code>git diff --no-prefix > your-patch-name.patch<br>
	</code></pre>

	The reviewer can then apply the patch using something like:<br>
	<br>
	<pre><code>patch -p0 < your-patch-name.patch<br>
	</code></pre>

	<h4>Patch emails</h4>

	Patch emails sent to the developer mailing list should begin with<br>
	<code>[PATCH]</code> as the first text in the email subject line, followed by a<br>
	brief (one line) description of the change.<br>
	<br>
	For "small" patches (for some definition of "small", please use good<br>
	judgment), include the diff in the email body, to facilitate inline<br>
	commenting in the code review email thread. For <i>all</i> patches, including<br>
	both "small" and "non small" ones, include the diff itself as an email<br>
	attachment, to make it easier for reviewers to patch the change into a<br>
	working copy with <code>patch</code>. Such a diff attachment can be produced with<br>
	the methods described above.<br>
	<br>
	<a href='Hidden comment:
	==== Suggested patches from casual contributors ====

	The Melange community is interested in patches and other contributions
	from casual developers. To that end, casual developers can submit
	patches for review in a couple of ways:

	* attach a patch to an [http://code.google.com/p/soc/issues/list issue in the issue tracker]
	* mailing a patch to the [MailingListsGuidelines#Developer_list developer mailing list] (membership in the list is required to post to it; see the [MailingListsGuidelines#Developer_list list guidelines])

	Committers can then discuss the contribution with the author on the
	[MailingListsGuidelines#Developer_list developer mailing list], or on the
	[MailingListsGuidelines#General_discussion_list general discussion list]
	if the contributor is not on the developer list. Since the
	[MelangeIntro Melange project] and the [SoCOverview SoC framework] is mostly
	about code, there are likely to be at least a few code discussions on the
	general announcement list. Serious discussions will likely result in the
	casual contributor being invited to the developer list to move the discussion
	there.

	Once the patch is accepted, a committer will patch a working copy and commit
	the changed (or added or deleted) source files to the repository,
	supplying a suitable commit log message.
	'></a><br>
	<br>
	<h4>Code Review</h4>

	Code review happens in the <a href='Gerrit.md'>Gerrit</a> system.<br>
	<br>
	All patches need a +2 from a Comitter before they can be submitted.<br>
	<br>
	<a href='Hidden comment:
	==== Reviews after committed patches ====

	Contributors with [ContributionReviews#Committers commit access] can submit
	changes prior to review. The participants on the
	[MailingListsGuidelines#Developer_list developer mailing list] are then
	encouraged to review the change, which will be present in the commit
	email that was sent to the mailing list. At least one other person with
	commit access should acknowledge the change with a brief "looks good to me" (LGTM)
	message in response (sent back to the list). This does of course not prevent
	other list participants from being able to make suggestions, request
	changes, etc.

	If one or more of the list participants requests changes to the submitted
	code, they can either discuss it and have the original committer submit a
	subsequent change, or the they can make the change themselves. Please
	avoid conflict by trying to reach a consensus before making commits. One
	way to do this is to
	[ContributionReviews#Patches follow the patch contribution method] for
	reviewing a change on the developer mailing list, even if the contributor
	has commit access.

	===== Review after commit using Google Code code reviews =====

	Google Code now has a feature that enables
	[http://code.google.com/p/support/wiki/CodeReviews code reviews of committed code].
	This feature can also be used by project owners and members to do review-after-commit
	style code reviews.

	'></a><br>
	<br>
	<h3>Committing</h3>

	Once all of the<br>
	[TestingGuidelines#All_changes_should_pass_smoke_tests_before_review<br>
	pre-commit tests] pass <b>and</b> the [ContributionReviews#Patches patch<br>
	has been code-reviewed] a <a href='ContributionReviews#Committers.md'>committer</a>
	(who may or<br>
	[ContributionReviews#Suggested_patches_from_casual_contributors may<br>
	not] be the actual contributor), the submit button in <a href='Gerrit.md'>Gerrit</a> or `git<br>
	merge && git push` can be used to commit the change into the<br>
	<a href='http://code.google.com/p/soc/source/browse'>SoC project repository</a>.<br>
	<br>
	<h4>commit messages</h4>

	Committers should use the template below for <code>git commit</code> log messages:<br>
	<br>
	<pre><code>One line, under 68 characters giving a short description of the change<br>
	<br>
	After a blank line, a more detailed log message explaining the reason<br>
	of the change (not necessarily what was changed, as that can be seen<br>
	by readin the patch). Please wrap your lines at 70 characters again.<br>
	</code></pre>

	If committing contributor patch (outside of Gerrit) please include:<br>
	<br>
	<pre><code>Reviewed by: Rick Reviewer<br>
	</code></pre>

	Note that previously the person listed with "Patch by:" was the<br>
	contributor (that is, author) of the patch, however we have switched<br>
	to Git and now we are committing patch from contributors as if<br>
	they would have commit access using command:<br>
	<br>
	<pre><code>git commit -u "FirstName LastName <somebody@example.com>"<br>
	</code></pre>

	Patches from<br>
	[ContributionReviews#Suggested_patches_from_casual_contributors casual<br>
	contributors] will, by definition, need to be committed by someone<br>
	other than the author but Git repository history will indicate<br>
	contributor as committer.<br>
	<br>
	<h2>Documentation</h2>

	Code files should include enough internal documentation in the form of<br>
	<code>__doc__</code> strings, comments and functions with self-explanatory names<br>
	(see PythonStyleGuide) that the automatic documentation suffices.<br>
	This is the recommended way to ensure that code documentation and the<br>
	code itself stay in step.<br>
	<br>
	The wiki is in the transition zone between dialog (IRC and mailing<br>
	lists) and formally reviewed content (code and its comments). Changes<br>
	to the wiki are 'review after commit'. Any committer can log comments<br>
	for a wiki page and participate in review. At the moment rights to<br>
	create pages and edit pages directly are restricted to committers and<br>
	regular contributors to documentation.<br>
	<br>
	<hr />
	<i>Copyright 2008 Google Inc.</i>
	<i>This work is licensed under a</i>
	<a href='http://soc.googlecode.com/svn/wiki/html/licenses/cc-by-attribution-2_5.html'>Creative Commons Attribution 2.5 License</a>.<br>
	<a href='http://creativecommons.org/licenses/by/2.5/'><img src='http://soc.googlecode.com/svn/wiki/html/licenses/cc-by-2_5-88x31.png' /></a>