Merge "doc: Improve contribution guidelines" into integration

docs/process/coding-guidelines.rst

@@ -19,6 +19,7 @@ support its functionality through plugins.
 
 Use of the EditorConfig file is suggested but is not required.
 
+.. _automatic-compliance-checking:
 
 Automatic Compliance Checking
 -----------------------------

docs/process/contributing.rst

@@ -29,19 +29,53 @@ Making Changes
 -  Make commits of logical units. See these general `Git guidelines`_ for
    contributing to a project.
 
--  Follow the :ref:`Coding Style` and :ref:`Coding Guidelines`.
-
-   -  Use the checkpatch.pl script provided with the Linux source tree. A
-      Makefile target is provided for convenience.
-
 -  Keep the commits on topic. If you need to fix another bug or make another
    enhancement, please address it on a separate topic branch.
 
+-  Split the patch in manageable units. Small patches are usually easier to
+   review so this will speed up the review process.
+
 -  Avoid long commit series. If you do have a long series, consider whether
    some commits should be squashed together or addressed in a separate topic.
 
--  Make sure your commit messages are in the proper format. If a commit fixes
-   an `issue`_, include a reference.
+-  Ensure that each commit in the series has at least one ``Signed-off-by:``
+   line, using your real name and email address. The names in the
+   ``Signed-off-by:`` and ``Commit:`` lines must match. By adding this line the
+   contributor certifies the contribution is made under the terms of the
+   :download:`Developer Certificate of Origin <../../dco.txt>`.
+
+   There might be multiple ``Signed-off-by:`` lines, depending on the history
+   of the patch.
+
+   More details may be found in the `Gerrit Signed-off-by Lines guidelines`_.
+
+-  Ensure that each commit also has a unique ``Change-Id:`` line. If you have
+   cloned the repository with the "`Clone with commit-msg hook`" clone method
+   (following the :ref:`Prerequisites` document), this should already be the
+   case.
+
+   More details may be found in the `Gerrit Change-Ids documentation`_.
+
+-  Write informative and comprehensive commit messages. A good commit message
+   provides all the background information needed for reviewers to understand
+   the intent and rationale of the patch. This information is also useful for
+   future reference.
+
+   For example:
+
+   -  What does the patch do?
+   -  What motivated it?
+   -  What impact does it have?
+   -  How was it tested?
+   -  Have alternatives been considered? Why did you choose this approach over
+      another one?
+   -  If it fixes an `issue`_, include a reference.
+
+-  Follow the :ref:`Coding Style` and :ref:`Coding Guidelines`.
+
+   -  Use the checkpatch.pl script provided with the Linux source tree. A
+      Makefile target is provided for convenience, see :ref:`this
+      section<automatic-compliance-checking>` for more details.
 
 -  Where appropriate, please update the documentation.
 
@@ -74,49 +108,85 @@ Making Changes
    is the year of most recent contribution. <OWNER> is your name or your company
    name.
 
+-  Ensure that each patch in the patch series compiles in all supported
+   configurations. Patches which do not compile will not be merged.
+
 -  Please test your changes. As a minimum, ensure that Linux boots on the
    Foundation FVP. See :ref:`Arm Fixed Virtual Platforms (FVP)` for more
    information. For more extensive testing, consider running the `TF-A Tests`_
    against your patches.
 
+-  Ensure that all CI automated tests pass. Failures should be fixed. They might
+   block a patch, depending on how critical they are.
+
 Submitting Changes
 ------------------
 
--  Ensure that each commit in the series has at least one ``Signed-off-by:``
-   line, using your real name and email address. The names in the
-   ``Signed-off-by:`` and ``Author:`` lines must match. If anyone else
-   contributes to the commit, they must also add their own ``Signed-off-by:``
-   line. By adding this line the contributor certifies the contribution is made
-   under the terms of the
-   :download:`Developer Certificate of Origin <../../dco.txt>`.
+-  Submit your changes for review at https://review.trustedfirmware.org
+   targeting the ``integration`` branch.
 
-   More details may be found in the `Gerrit Signed-off-by Lines guidelines`_.
+-  Add reviewers for your patch:
 
--  Ensure that each commit also has a unique ``Change-Id:`` line. If you have
-   cloned the repository with the "`Clone with commit-msg hook`" clone method
-   (following the :ref:`Prerequisites` document), this should already be the
-   case.
+   -  At least one code owner for each module modified by the patch. See the list
+      of modules and their :ref:`code owners`.
 
-   More details may be found in the `Gerrit Change-Ids documentation`_.
+   -  At least one maintainer. See the list of :ref:`maintainers`.
 
--  Submit your changes for review at https://review.trustedfirmware.org
-   targeting the ``integration`` branch.
+   -  If some module has no code owner, try to identify a suitable (non-code
+      owner) reviewer. Running ``git blame`` on the module's source code can
+      help, as it shows who has been working the most recently on this area of
+      the code.
 
-   -  The changes will then undergo further review and testing by the
-      :ref:`code owners` and :ref:`maintainers`. Any review comments will be
-      made directly on your patch. This may require you to do some rework. For
-      controversial changes, the discussion might be moved to the `TF-A mailing
-      list`_ to involve more of the community.
+      Alternatively, if it is impractical to identify such a reviewer, you might
+      send an email to the `TF-A mailing list`_ to broadcast your review request
+      to the community.
+
+   Note that self-reviewing a patch is prohibited, even if the patch author is
+   the only code owner of a module modified by the patch. Getting a second pair
+   of eyes on the code is essential to keep up with the quality standards the
+   project aspires to.
+
+-  The changes will then undergo further review by the designated people. Any
+   review comments will be made directly on your patch. This may require you to
+   do some rework. For controversial changes, the discussion might be moved to
+   the `TF-A mailing list`_ to involve more of the community.
 
    Refer to the `Gerrit Uploading Changes documentation`_ for more details.
 
+-  The patch submission rules are the following. For a patch to be approved
+   and merged in the tree, it must get:
+
+   -  One ``Code-Owner-Review+1`` for each of the modules modified by the patch.
+   -  A ``Maintainer-Review+1``.
+
+   In the case where a code owner could not be found for a given module,
+   ``Code-Owner-Review+1`` is substituted by ``Code-Review+1``.
+
+   In addition to these various code review labels, the patch must also get a
+   ``Verified+1``. This is usually set by the Continuous Integration (CI) bot
+   when all automated tests passed on the patch. Sometimes, some of these
+   automated tests may fail for reasons unrelated to the patch. In this case,
+   the maintainers might (after analysis of the failures) override the CI bot
+   score to certify that the patch has been correctly tested.
+
+   In the event where the CI system lacks proper tests for a patch, the patch
+   author or a reviewer might agree to perform additional manual tests
+   in their review and the reviewer incorporates the review of the additional
+   testing in the ``Code-Review+1`` or ``Code-Owner-Review+1`` as applicable to
+   attest that the patch works as expected. Where possible additional tests should
+   be added to the CI system as a follow up task. For example, for a
+   platform-dependent patch where the said platform is not available in the CI
+   system's board farm.
+
 -  When the changes are accepted, the :ref:`maintainers` will integrate them.
 
    -  Typically, the :ref:`maintainers` will merge the changes into the
       ``integration`` branch.
+
    -  If the changes are not based on a sufficiently-recent commit, or if they
       cannot be automatically rebased, then the :ref:`maintainers` may rebase it
       on the ``integration`` branch or ask you to do so.
+
    -  After final integration testing, the changes will make their way into the
       ``master`` branch. If a problem is found during integration, the
       :ref:`maintainers` will request your help to solve the issue. They may