[coreboot-gerrit] Patch set updated for coreboot: Documentation: coreboot Gerrit Etiquette and Guidelines

[Martin Roth (martinroth@google.com)]

Martin Roth (martinroth at google.com) just uploaded a new patch set to gerrit, which you can find at http://review.coreboot.org/12256

-gerrit

commit 23ca6f7db7af0fac8731906c42cf4e072e76909a
Author: Martin Roth <martinroth at google.com>
Date:   Thu Oct 29 12:43:10 2015 -0600

    Documentation: coreboot Gerrit Etiquette and Guidelines
    
    As the community has grown, so has the need to formalize some of the
    guidelines that the community lives by.  When the community was small,
    it was easy to communicate these things just from one person to another.
    
    Now, with more people joining the community every day, it seems that
    it's time to write some of these things down, allowing people to
    understand our policies immediately instead of making them learn our
    practices as they make mistakes.
    
    As it says in the document:
    The following rules are the requirements for behavior in the coreboot
    codebase in gerrit.  These have mainly been unwritten rules up to this
    point, and should be familiar to most users who have been active in
    coreboot for a period of time.  Following these rules will help reduce
    friction in the community.
    
    Change-Id: If80e933fcfb04b86fd5efe6423cda448118d7a3c
    Signed-off-by: Martin Roth <martinroth at google.com>
---
 Documentation/gerrit_guidelines.md | 233 +++++++++++++++++++++++++++++++++++++
 1 file changed, 233 insertions(+)

diff --git a/Documentation/gerrit_guidelines.md b/Documentation/gerrit_guidelines.md
new file mode 100644
index 0000000..e41d9c0
--- /dev/null
+++ b/Documentation/gerrit_guidelines.md
@@ -0,0 +1,233 @@
+coreboot Gerrit Etiquette and Guidelines
+========================================
+
+The following rules are the requirements for behavior in the coreboot
+codebase in gerrit. These have mainly been unwritten rules up to this
+point, and should be familiar to most users who have been active in
+coreboot for a period of time. Following these rules will help reduce
+friction in the community.
+
+
+Summary:
+--------
+These are the expectations for committing, reviewing, and submitting code
+into coreboot git and gerrit. While breaking individual rules may not have
+immediate consequences, the coreboot leadership may act on repeated or
+flagrant violations with or without notice.
+
+* Don't violate the licenses.
+* Let non-trivial patches sit in a review state for at least 24 hours
+before submission.
+* Try to coordinate with platform maintainers when making changes to
+platforms.
+* If you give a patch a -2, you are responsible for giving concrete
+recommendations for what could be changed to resolve the issue the patch
+addresses.
+* Don't modify other people's patches without their consent.
+* Be respectful to others when commenting.
+* Each patch should be kept to one logical change.
+* Don’t submit patches that you know will break other platforms.
+
+
+More detail:
+------------
+* Don't violate the licenses. If you're submitting code that you didn't
+write yourself, make sure the license is compatible with the license of the
+project you're submitting the changes to. If you’re submitting code that
+you wrote that might be owned by your employer, make sure that your
+employer is aware and you are authorized to submit the code. For
+clarification, see the Developer's Certificate of Origin in the coreboot
+[Signed-off-by policy](http://www.coreboot.org/Development_Guidelines#Sign-off_Procedure).
+
+* Let non-trivial patches sit in a review state for at least 24 hours
+before submission. Remember that there are coreboot developers in timezones
+all over the world, and everyone should have a chance to contribute.
+Trivial patches would be things like whitespace changes or spelling fixes.
+In general, small changes that don’t impact the final binary output. The
+24-hour period would start at submission, and would be restarted at any
+update which significantly changes any part of the patch.
+
+* Do not +2 patches that you authored or own, even for something as trivial
+as whitespace fixes. When working on your own patches, it’s easy to
+overlook something like accidentally updating file permissions or git
+submodule commit IDs. Let someone else review the patch.
+
+* Try to coordinate with platform maintainers when making changes to
+platforms. The platform maintainers are the users who initially pushed the
+code for that platform, as well as users who have made significant changes
+to a platform. To find out who maintains a piece of code, please use
+util/scripts/maintainers.go or refer to the original author of the code in
+git log.
+
+* If you give a patch a -2, you are responsible for giving concrete
+recommendations for what could be changed to resolve the issue the patch
+addresses. If you feel strongly that a patch should NEVER be merged, you
+are responsible for defending your position and listening to other points
+of view. Giving a -2 and walking away is not acceptable, and may cause your
+ -2 to be removed by the coreboot leadership after a period of time.
+
+* Don't modify other people's patches unless you are specifically requested
+to do so by the owner of that patch. Not only is this considered rude, but
+your changes could be lost unintentionally when the owner pushes an update
+without realizing that you’ve changed something.
+
+* Be respectful to others when commenting on their patch. Comments should
+be kept to the code, and should be kept in a polite tone. If you feel
+attacked, resist the urge to attack back - this only escalates the issue.
+
+* Each patch should be kept to one logical change, which should be
+described in the title of the patch. Unrelated changes should be split out
+into separate patches. Fixing whitespace on a line you’re editing is
+reasonable. Fixing whitespace around the code you’re working on should be a
+separate ‘cleanup’ patch. Larger patches that touch several areas are fine,
+so long as they are one logical change. Adding new chips and doing code
+cleanup over wide areas are two examples of this.
+
+* Don’t submit code that you know will break other platforms. If your patch
+affects code that is used by other platforms, it should be compatible with
+those platforms. While it would be nice to update any other platforms, you
+must at least provide a path that will allow other platforms to continue
+working.
+
+
+Recommendations for gerrit activity:
+------------------------------------
+These guidelines are less strict than the ones listed above. These are more
+of the “good idea” variety. You are requested to follow the below
+guidelines, but there will probably be no actual consequences if they’re
+not followed. That said, following the recommendations below will speed up
+review of your patches, and make the members of the community do less work.
+
+* Test your patches before submitting them to gerrit. It's also appreciated
+if you add a line to the commit message describing how the patch was
+tested. This prevents people from having to ask whether and how the patch
+was tested.  An example of this sort of comment would be ‘TEST=Built
+platform’ or ‘TEST=Built and booted platform’
+
+* Take advantage of the lint tools to make sure your patches don’t contain
+trivial mistakes. By running ‘make gitconfig’, the lint-stable tools are
+automatically put in place and will test your patches before they are
+committed. As a violation of these tools will cause the jenkins build test
+to fail, it’s to your advantage to test this before pushing to gerrit.
+
+* Don't submit patch trains longer than around 20 patches. Long patch
+trains become unmanageable and tie up the build servers for long periods of
+time. Rebasing a patch train over and over as you fix earlier patches in
+the train can hide comments, and make people review the code multiple times
+to see if anything has changed between revisions.
+
+* Run 'make what-jenkins-does' locally on patch trains before submitting.
+This helps verify that the patch train won’t tie up the jenkins builders
+for no reason if there are failing patches in the train.
+
+* Use a topic when pushing a train of patches. This groups the commits
+together so people can easily see the connection at the top level of
+gerrit. Topics can be set for individual patches in gerrit by going into
+the patch and clicking on the icon next to the topic line. Topics can also
+be set when you push the patches into gerrit. For example, to push a set of
+commits with the the i915-kernel-x60 set, use the command:
+        git push origin HEAD:refs/for/master/i915-kernel-x60
+
+* If one of your patches isn't ready to be merged, make sure it's obvious
+that you don't feel it's ready for merge yet. This can be something like
+giving it a -1 or -2, or marking in the commit message that it’s not ready
+until X. The commit message can be updated easily when it’s ready to be
+pushed.  Examples of this are "WIP: title" or "[NEEDS_TEST]: title".  These
+can also be pushed as drafts as shown in the next guideline.
+
+* When pushing patches that are not for submission, these should be marked
+as such. This can be done in the title ‘[DONOTSUBMIT]’, or can be pushed as
+draft commits, so that only explicitly added reviewers will see them. These
+sorts of patches are frequently posted as ideas or RFCs for the community
+to look at. To push a draft, use the command:
+        git push origin HEAD:refs/drafts/master
+
+* Respond to anyone who has taken the time to review your patches, even if
+it's just to say that you disagree. While it may seem annoying to address a
+request to fix spelling or 'trivial' issues, it’s generally easy to handle
+in gerrit’s built in editor.  It's also acceptable to add fixes for these
+sorts of comments to another patch, but it's recommended that that patch be
+pushed to gerrit before the initial patch gets submitted.
+
+* Consider breaking up large individual patches into smaller patches
+grouped by areas. This makes the patches easier to review, but increases
+the number of patches. The way you want to handle this is a personal
+decision, as long as each patch is still one logical change.
+
+* If you have an interest in a particular area or mainboard, set yourself
+up as a ‘maintainer’ of that area by adding yourself to the MAINTAINERS
+file in the coreboot root directory. Eventually, this should automatically
+add you as a reviewer when an area that you’re listed as a maintainer is
+changed.
+
+* Submit mainboards that you’re working on to the board-status repo. This
+helps others and shows that these mainboards are currently being
+maintained.  At some point, boards that are not up to date in the
+board-status repo will probably end up getting deleted from the coreboot
+tree.
+
+* Abandon patches that are no longer useful, or that you don’t intend to
+keep working on.
+
+* Bring attention to patches that you would like reviewed. Add reviewers,
+or even just rebase it against the current codebase to bring it to the top
+of the gerrit list. If you’re not sure who would be a good reviewer, look
+in the MAINTAINERS file or git history of the files that you’ve changed,
+and add those people.
+
+* Familiarize yourself with the commit message guidelines, and try to
+follow them. This will help to keep annoying requests to fix your commit
+message to a minimum.
+
+* If there have been comments or discussion on a patch, verify that the
+comments have been addressed before giving a +2. If you feel that a comment
+is invalid, please respond to that comment instead of just ignoring it.
+
+* Be conscientious when reviewing patches. If you give a patch a +2 and it
+breaks things, you should feel as responsible as the owner of the patch for
+fixing things, and could be called on by the community to help fix any
+fallout of the patch. This means you shouldn’t +2 a patch just because you
+trust the author of a patch - Make sure you understand what the
+implications of a patch might be, or leave the review to others. Partial
+reviews, reviewing code style, for example, can be given a +1 instead of a
++2. This also applies if you think the patch looks good, but may not have
+the experience to know if there may be unintended consequences.
+
+* If there is still ongoing discussion to a patch, try to wait for a
+conclusion to the discussion before submitting it to the tree. If you feel
+that someone is beating a dead horse, maybe just state that and give a time
+that the patch will be submitted. If no new objections have come up instead
+of submitting the patch immediately and ending the discussion by force.
+
+* When working with patch trains, for minor requests it’s acceptable to
+create a fix addressing a comment in another patch at the end of the patch
+train. This minimizes rebases of the patch train while still addressing the
+request. For major problems where the change doesn’t work as intended or
+breaks other platforms, the change really needs to go into the original
+patch.
+
+
+Expectations contributors should have:
+--------------------------------------
+* Don't expect that people will review your patch unless you ask them to.
+Adding other people as reviewers is the easiest way. Asking for reviews for
+individual patches in the IRC channel, or by sending a direct request to an
+individual through your favorite messenger is usually the best way to get a
+patch reviewed quickly.
+
+* Don't expect that your patch will be submitted immediately after getting
+a +2. As stated previously, non-trivial patches should wait at least 24
+hours before being submitted. That said, if you feel that your patch or
+series of patches has been sitting longer than needed, you can ask for it
+to be submitted on IRC, or comment that it's ready for submission in the
+patch. This will move it to the top of the list where it's more likely to
+be noticed and acted upon.
+
+* Reviews are about the code. It's easy to take it personally when someone
+is criticising your code, but the whole idea is to get better code into our
+codebase. Again, this also applies in the other direction: review code,
+criticize code, but don’t make it personal.
+
+
+Requests for clarification and suggestions for updates to these guidelines
+should be sent to the coreboot mailing list at <coreboot at coreboot.org>.