similarity index 79%
rename from source/code-review-guide.md
rename to source/code-review-guide.rst
@@ -1,4 +1,5 @@
-# Code Review Guide
+Code Review Guide
+*****************
This document highlights what reviewers such as maintainers and committers look
for when reviewing your code. It sets expectations for code authors and provides
@@ -14,40 +15,46 @@ provides pointers on code author's and reviewer's workflows.
Sometimes it happens that a submitted patch series made wrong assumptions or has
a flawed design or architecture. This can be frustrating for contributors and
-code reviewers. Note that this document does contain [a section](#problems)
-that provides suggestions on how to minimize the impact for most stake-holders
+code reviewers. Note that this document does contain `a section <problems_>`_
+that provides suggestions on how to minimize the impact for most stake-holders
and also on how to avoid such situations.
This document does **not cover** the following topics:
-* [Communication Best Practice][1]
-* [Resolving Disagreement][2]
-* [Patch Submission Workflow][3]
-* [Managing Patch Submission with Git][4]
-## What we look for in Code Reviews
+* :doc:`Communication Best Practice <communication-practice>`
+* :doc:`Resolving Disagreement <resolving-disagreement>`
+* `Patch Submission Workflow <3_>`_
+* `Managing Patch Submission with Git <4_>`_
+
+What we look for in Code Reviews
+================================
When performing a code review, reviewers typically look for the following things
-### Is the change necessary to accomplish the goals?
+Is the change necessary to accomplish the goals?
+------------------------------------------------
* Is it clear what the goals are?
* Do we need to make a change, or can the goals be met with existing
functionality?
-### Architecture / Interface
+Architecture / Interface
+------------------------
* Is this the best way to solve the problem?
* Is this the right part of the code to modify?
* Is this the right level of abstraction?
* Is the interface general enough? Too general? Forward compatible?
-### Functionality
+Functionality
+-------------
* Does it do what it’s trying to do?
* Is it doing it in the most efficient way?
* Does it handle all the corner / error cases correctly?
-### Maintainability / Robustness
+Maintainability / Robustness
+----------------------------
* Is the code clear? Appropriately commented?
* Does it duplicate another piece of code?
@@ -61,60 +68,75 @@ and/or robustness issues. In such cases, maintainers may ask you to make
additional changes, such that your submitted code does not make things worse or
point you to other patches are already being worked on.
-### System properties
+System properties
+-----------------
In some areas of the code, system properties such as
+
* Code size
* Performance
* Scalability
* Latency
* Complexity
* &c
+
are also important during code reviews.
-### Style
+Style
+-----
* Comments, carriage returns, **snuggly braces**, &c
-* See [CODING_STYLE][5] and [tools/libxl/CODING_STYLE][6]
+* See `CODING_STYLE <5_>`_ and `tools/libxl/CODING_STYLE <6_>`_
* No extraneous whitespace changes
-### Documentation and testing
+Documentation and testing
+-------------------------
* If there is pre-existing documentation in the tree, such as man pages, design
documents, etc. a contributor may be asked to update the documentation
- alongside the change. Documentation is typically present in the [docs][7]
+ alongside the change. Documentation is typically present in the `docs <7_>`_
folder.
* When adding new features that have an impact on the end-user,
- a contributor should include an update to the [SUPPORT.md][8] file.
+ a contributor should include an update to the `SUPPORT.md <8_>`_ file.
Typically, more complex features require several patch series before it is
ready to be advertised in SUPPORT.md
* When adding new features, a contributor may be asked to provide tests or
ensure that existing tests pass
-#### Testing for the Xen Project Hypervisor
+Testing for the Xen Project Hypervisor
+--------------------------------------
Tests are typically located in one of the following directories
-* **Unit tests**: [tools/tests][9] or [xen/test][A]<br>
+
+* **Unit tests**: `tools/tests <9_>`_ or `xen/test <A_>`_
+
Unit testing is hard for a system like Xen and typically requires building a
subsystem of your tree. If your change can be easily unit tested, you should
consider submitting tests with your patch.
-* **Build and smoke test**: see [Xen GitLab CI][B]<br>
+
+* **Build and smoke test**: see `Xen GitLab CI <B_>`_
+
Runs build tests for a combination of various distros and compilers against
changes committed to staging. Developers can join as members and test their
development branches **before** submitting a patch.
-* **XTF tests** (microkernel-based tests): see [XTF][C]<br>
+
+* **XTF tests** (microkernel-based tests): see `XTF <C_>`_
+
XTF has been designed to test interactions between your software and hardware.
It is a very useful tool for testing low level functionality and is executed
as part of the project's CI system. XTF can be easily executed locally on
xen.git trees.
-* **osstest**: see [README][D]<br>
+
+* **osstest**: see `README <D_>`_
+
Osstest is the Xen Projects automated test system, which tests basic Xen use
cases on a variety of different hardware. Before changes are committed, but
**after** they have been reviewed. A contributor’s changes **cannot be
applied to master** unless the tests pass this test suite. Note that XTF and
other tests are also executed as part of osstest.
-### Patch / Patch series information
+Patch / Patch series information
+--------------------------------
* Informative one-line changelog
* Full changelog
@@ -124,14 +146,16 @@ Tests are typically located in one of the following directories
* Reviewed-by’s and Acked-by’s dropped if appropriate
More information related to these items can be found in our
-[Patch submission Guide][E].
+`Patch submission Guide <E_>`_.
-## Code Review Workflow
+Code Review Workflow
+====================
This section is important for code authors and reviewers. We recommend that in
particular new code authors carefully read this section.
-### Workflow from a Reviewer's Perspective
+Workflow from a Reviewer's Perspective
+--------------------------------------
Patch series typically contain multiple changes to the codebase, some
transforming the same section of the codebase multiple times. It is quite common
@@ -147,7 +171,8 @@ Generally there are no hard rules on how to structure a series, as the structure
of a series is very code specific and it is hard to give specific advice. There
are some general tips which help and some general patterns.
-**Tips:**
+Tips
+^^^^
* Outline the thinking behind the structure of the patch series. This can make
a huge difference and helps ensure that the code reviewer understands what the
@@ -161,7 +186,8 @@ are some general tips which help and some general patterns.
to learn the tools, code and deal with a large patch series all together for
the first time.
-**General Patterns:**
+General Patterns
+^^^^^^^^^^^^^^^^
If there are multiple subsystems involved in your series, then these are best
separated out into **sets of patches**, which roughly follow the following
@@ -171,6 +197,7 @@ all subsystems (e.g. headers, macros, documentation) impacting all changed
subsystems which ideally comes **before** subsystem specific changes.
The seven categories typically making up a logical set of patches
+
1. Cleanups and/or new Independent Helper Functions
2. Reorganizations
3. Headers, APIs, Documentation and anything which helps understand the
@@ -189,13 +216,15 @@ patches.
If a series is structured this way, it is often possible to agree early on,
that a significant portion of the changes are fine and to check these in
independently of the rest of the patch series. This means that there is
+
* Less work for authors to rebase
* Less cognitive overhead for reviewers to review successive versions of a
series
* The possibility for different code reviewers to review portions of such
large changes independently
-**Trade-Offs:**
+Trade-Offs
+^^^^^^^^^^
* In some cases, following the general pattern above may create extra patches
and may make a series more complex and harder to understand.
@@ -210,9 +239,12 @@ independently of the rest of the patch series. This means that there is
the change** is back-ported, whereas general cleanups and improvements are
not.
-**Example:**
-* [[PATCH v3 00/18] VM forking][H] is a complex patch series with an exemplary
+Example
+^^^^^^^
+
+* `[PATCH v3 00/18] VM forking <H_>`_ is a complex patch series with an exemplary
cover letter. Notably, it contains the following elements
+
* It provides a description of the design goals and detailed description
of the steps required to fork a VM.
* A description of changes to the user interface
@@ -225,7 +257,8 @@ independently of the rest of the patch series. This means that there is
rest of the series and **4** the substance of the series with additional
information to make it easier for the reviewer to parse the series.
-### Workflow from an Author's Perspective
+Workflow from an Author's Perspective
+-------------------------------------
When code authors receive feedback on their patches, they typically first try
to clarify feedback they do not understand. For smaller patches or patch series
@@ -236,18 +269,22 @@ make sense to send out a new revision earlier.
As a reviewer, you need some system that helps ensure that you address all
review comments. This can be tedious when trying to map a hierarchical e-mail
thread onto a code-base. Different people use different techniques from using
+
* In-code TODO statements with comment snippets copied into the code
* To keeping a separate TODO list
* To printing out the review conversation tree and ticking off what has been
addressed
* A combination of the above
-### <a name="problems"></a>Problematic Patch Reviews
+.. _problems:
+
+Problematic Patch Reviews
+-------------------------
A typical waterfall software development process is sequential with the
following steps: define requirements, analyze, design, code, test and deploy.
Problems uncovered by code review or testing at such a late stage can cause
-costly redesign and delays. The principle of **[Shift Left][D]** is to take a
+costly redesign and delays. The principle of `Shift Left`_ is to take a
task that is traditionally performed at a late stage in the process and perform
that task at earlier stages. The goal is to save time by avoiding refactoring.
@@ -271,43 +308,45 @@ separate difficult and easy portions of a patch series. This will enable
reviewers to progress uncontroversial portions of a patch independently from
controversial ones.
-### Reviewing for Patch Authors
+Reviewing for Patch Authors
+---------------------------
The following presentation by George Dunlap, provides an excellent overview on
how we do code reviews, specifically targeting non-maintainers.
As a community, we would love to have more help reviewing, including from **new
community members**. But many people
+
* do not know where to start, or
* believe that their review would not contribute much, or
* may feel intimidated reviewing the code of more established community members
The presentation demonstrates that you do not need to worry about any of these
concerns. In addition, reviewing other people's patches helps you
+
* write better patches and experience the code review process from the other
side
* and build more influence within the community over time
Thus, we recommend strongly that **patch authors** read the watch the recording
or read the slides:
-* [Patch Review for Non-Maintainers slides][F]
-* [Patch Review for Non-Maintainers recording - 20"][G]
-
-[1]: communication-practice.md
-[2]: resolving-disagreement.md
-[3]: https://wiki.xenproject.org/wiki/Submitting_Xen_Project_Patches
-[4]: https://wiki.xenproject.org/wiki/Managing_Xen_Patches_with_Git
-[5]: https://xenbits.xenproject.org/gitweb/?p=xen.git;a=blob;f=CODING_STYLE
-[6]: https://xenbits.xenproject.org/gitweb/?p=xen.git;a=blob;f=tools/libxl/CODING_STYLE
-[7]: https://xenbits.xenproject.org/gitweb/?p=xen.git;a=tree;f=docs
-[8]: https://xenbits.xenproject.org/gitweb/?p=xen.git;a=blob;f=SUPPORT.md
-[9]: https://xenbits.xenproject.org/gitweb/?p=xen.git;a=tree;f=tools/tests
-[A]: https://xenbits.xenproject.org/gitweb/?p=xen.git;a=tree;f=xen/test
-[B]: https://gitlab.com/xen-project/xen/pipelines
-[C]: https://xenbits.xenproject.org/docs/xtf/
-[D]: https://xenbits.xenproject.org/gitweb/?p=osstest.git;a=blob;f=README
-[E]: https://wiki.xenproject.org/wiki/Submitting_Xen_Project_Patches
-[D]: https://devopedia.org/shift-left
-[F]: https://www.slideshare.net/xen_com_mgr/xpdds19-keynote-patch-review-for-nonmaintainers-george-dunlap-citrix-systems-uk-ltd
-[G]: https://www.youtube.com/watch?v=ehZvBmrLRwg
-[H]: https://lists.xenproject.org/archives/html/xen-devel/2019-12/threads.html#02097
+
+* `Patch Review for Non-Maintainers slides <F_>`_
+* `Patch Review for Non-Maintainers recording - 20" <G_>`_
+
+.. _3: https://wiki.xenproject.org/wiki/Submitting_Xen_Project_Patches
+.. _4: https://wiki.xenproject.org/wiki/Managing_Xen_Patches_with_Git
+.. _5: https://xenbits.xenproject.org/gitweb/?p=xen.git;a=blob;f=CODING_STYLE
+.. _6: https://xenbits.xenproject.org/gitweb/?p=xen.git;a=blob;f=tools/libxl/CODING_STYLE
+.. _7: https://xenbits.xenproject.org/gitweb/?p=xen.git;a=tree;f=docs
+.. _8: https://xenbits.xenproject.org/gitweb/?p=xen.git;a=blob;f=SUPPORT.md
+.. _9: https://xenbits.xenproject.org/gitweb/?p=xen.git;a=tree;f=tools/tests
+.. _A: https://xenbits.xenproject.org/gitweb/?p=xen.git;a=tree;f=xen/test
+.. _B: https://gitlab.com/xen-project/xen/pipelines
+.. _C: https://xenbits.xenproject.org/docs/xtf/
+.. _D: https://xenbits.xenproject.org/gitweb/?p=osstest.git;a=blob;f=README
+.. _E: https://wiki.xenproject.org/wiki/Submitting_Xen_Project_Patches
+.. _Shift Left: https://devopedia.org/shift-left
+.. _F: https://www.slideshare.net/xen_com_mgr/xpdds19-keynote-patch-review-for-nonmaintainers-george-dunlap-citrix-systems-uk-ltd
+.. _G: https://www.youtube.com/watch?v=ehZvBmrLRwg
+.. _H: https://lists.xenproject.org/archives/html/xen-devel/2019-12/threads.html#02097
@@ -12,6 +12,7 @@ Welcome to XenProject Governance's documentation!
code-of-conduct
communication-guide
+ code-review-guide
Indices and tables
==================
Convert titles as approproate. Use inter-doc references for other full docs. Convert other external links to RST-style references, keeping the labels (3-F). One exception to this: sphinx noticed that there were two 'D' labels; rename one to `Shift Left`. Convert internal link to RST-style reference. Add spaces so that lists compile correctly. Remove explicit HTML <br> tags; Make them a separate block to achieve a similar goal. Convert "manual" **subsubsection** sections to RST subsubsuctions (^^^^^). No textual changes. Signed-off-by: George Dunlap <george.dunlap@citrix.com> --- ...-review-guide.md => code-review-guide.rst} | 153 +++++++++++------- source/index.rst | 1 + 2 files changed, 97 insertions(+), 57 deletions(-) rename source/{code-review-guide.md => code-review-guide.rst} (79%)