]> xenbits.xensource.com Git - governance.git/commitdiff
Convert code-review-guide.md to rst
authorGeorge Dunlap <george.dunlap@citrix.com>
Thu, 10 Sep 2020 11:42:50 +0000 (12:42 +0100)
committerGeorge Dunlap <george.dunlap@citrix.com>
Thu, 10 Sep 2020 17:00:50 +0000 (18:00 +0100)
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>
source/code-review-guide.md [deleted file]
source/code-review-guide.rst [new file with mode: 0644]
source/index.rst

diff --git a/source/code-review-guide.md b/source/code-review-guide.md
deleted file mode 100644 (file)
index ed4f9d6..0000000
+++ /dev/null
@@ -1,313 +0,0 @@
-# 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
-a framework for code reviewers.
-
-Before we start, it is important to remember that the primary purpose of a
-a code review is to identify any bugs or potential bugs in the code. Most code
-reviews are relatively straight-forward and do not require re-writing the
-submitted code substantially.
-
-The document provides advice on how to structure larger patch series and
-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
-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
-
-When performing a code review, reviewers typically look for the following things
-
-### 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
-
-* 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
-
-* 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
-
-* Is the code clear? Appropriately commented?
-* Does it duplicate another piece of code?
-* Does the code make hidden assumptions?
-* Does it introduce sections which need to be kept **in sync** with other
-  sections?
-* Are there other **traps** someone modifying this code might fall into?
-
-**Note:** Sometimes you will work in areas which have identified maintainability
-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
-
-In some areas of the code, system properties such as
-* Code size
-* Performance
-* Scalability
-* Latency
-* Complexity
-* &c
-are also important during code reviews.
-
-### Style
-
-* Comments, carriage returns, **snuggly braces**, &c
-* See [CODING_STYLE][5] and [tools/libxl/CODING_STYLE][6]
-* No extraneous whitespace changes
-
-### 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]
-  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.
-  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
-
-Tests are typically located in one of the following directories
-* **Unit tests**: [tools/tests][9] or [xen/test][A]<br>
-  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>
-  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 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 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
-
-* Informative one-line changelog
-* Full changelog
-* Motivation described
-* All important technical changes mentioned
-* Changes since previous revision listed
-* 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].
-
-## 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
-
-Patch series typically contain multiple changes to the codebase, some
-transforming the same section of the codebase multiple times. It is quite common
-for patches in a patch series to rely on the previous ones. This means that code
-reviewers review  patches and patch series **sequentially** and **the structure
-of a patch series guides the code review process**. Sometimes in a long series,
-patches {1,2}/10 will be clean-ups, {3-6}/10 will be general reorganizations
-which don't really seem to do anything and then {7-10}/10 will be the substance
-of the series, which helps the code reviewer understand what {3-6}/10 were
-about.
-
-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:**
-
-* 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
-  series is trying to achieve and which portions are addressing which problems.
-* Try and keep changes that belong to a subsystem together
-* Expect that the structure of a patch series sometimes may need to change
-  between different versions of a patch series
-* **Most importantly**: Start small. Don't submit a large and complex patch
-  series as the first interaction with the community. Try and pick a smaller
-  task first (e.g. a bug-fix, a clean-up task, etc.) such that you don't have
-  to learn the tools, code and deal with a large patch series all together for
-  the first time.
-
-**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
-seven categories. In other words: you would end up with **7 categories x N
-subsystems**. In some cases, there is a **global set of patches** that affect
-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
-   substance of a series
-4. The substance of the change
-5. Cleanups of any infelicities introduced temporarily
-6. Deleting old code
-7. Test code
-
-Note that in many cases, some of the listed categories are not always present
-in each set, as they are not needed. Of course, sometimes there are several
-patches describing **changes of substance**, which could be ordered in different
-ways: in such cases it may be necessary to put reorganizations in between these
-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:**
-
-* In some cases, following the general pattern above may create extra patches
-  and may make a series more complex and harder to understand.
-* Crafting a more extensive cover letter will be extra effort: in most cases,
-  the extra time investment will be saving time during the code review process.
-  Verbosity is not the goal, but clarity is. Before you send a larger series
-  in particular: try and put yourself into the position of a code reviewer and
-  try to identify information that helps a code reviewer follow the patch
-  series.
-* In cases where changes need to be back-ported to older releases, moving
-  general cleanups last is often preferable: in such cases the **substance of
-  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
-  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
-  * It contains some information about the test status of the series including
-    some performance information.
-  * It maps the series onto the categories listed above. As expected, not
-    all categories are used in this case. However, the series does contain
-    elements of **1** (in this case preparation to enable the functionality),
-    **2** reorganizations and other non-functional changes that enable the
-    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
-
-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
-it makes sense to wait until receiving feedback on the entire series before
-sending out a new version addressing the changes. For larger series, it may
-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
-
-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
-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.
-
-Typically, problematic patch reviews uncover issues such as wrong or missed
-assumptions, a problematic architecture or design, or other bugs that require
-significant re-implementation of a patch series to fix the issue.
-
-The principle of **Shift Left** also applies in code reviews. Let's assume a
-series has a major flaw: ideally, this flaw would be picked up in the **first
-or second iteration** of the code review. As significant parts of the code may
-have to be re-written, it does not make sense for reviewers to highlight minor
-issues (such as style issues) until major flaws have been addressed of the
-affected part of a patch series. In such cases, providing feedback on minor
-issues reviewers cause the code author and themselves extra work by asking for
-changes to code, which ultimately may be changed later.
-
-To make it possible for code reviewers to identify major issues early, it is
-important for code-authors to highlight possible issues in a cover letter and
-to structure a patch series in such a way that makes it easy for reviewers to
-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
-
-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
diff --git a/source/code-review-guide.rst b/source/code-review-guide.rst
new file mode 100644 (file)
index 0000000..9c0040e
--- /dev/null
@@ -0,0 +1,352 @@
+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
+a framework for code reviewers.
+
+Before we start, it is important to remember that the primary purpose of a
+a code review is to identify any bugs or potential bugs in the code. Most code
+reviews are relatively straight-forward and do not require re-writing the
+submitted code substantially.
+
+The document provides advice on how to structure larger patch series and
+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
+and also on how to avoid such situations.
+
+This document does **not cover** the following topics:
+
+* :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 it clear what the goals are?
+* Do we need to make a change, or can the goals be met with existing
+  functionality?
+
+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
+-------------
+
+* 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
+----------------------------
+
+* Is the code clear? Appropriately commented?
+* Does it duplicate another piece of code?
+* Does the code make hidden assumptions?
+* Does it introduce sections which need to be kept **in sync** with other
+  sections?
+* Are there other **traps** someone modifying this code might fall into?
+
+**Note:** Sometimes you will work in areas which have identified maintainability
+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
+-----------------
+
+In some areas of the code, system properties such as
+
+* Code size
+* Performance
+* Scalability
+* Latency
+* Complexity
+* &c
+
+are also important during code reviews.
+
+Style
+-----
+
+* Comments, carriage returns, **snuggly braces**, &c
+* See `CODING_STYLE <5_>`_ and `tools/libxl/CODING_STYLE <6_>`_
+* No extraneous whitespace changes
+
+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_>`_
+  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.
+  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
+--------------------------------------
+
+Tests are typically located in one of the following directories
+
+* **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_>`_
+  
+  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_>`_
+  
+  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_>`_
+  
+  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
+--------------------------------
+
+* Informative one-line changelog
+* Full changelog
+* Motivation described
+* All important technical changes mentioned
+* Changes since previous revision listed
+* 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_>`_.
+
+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
+--------------------------------------
+
+Patch series typically contain multiple changes to the codebase, some
+transforming the same section of the codebase multiple times. It is quite common
+for patches in a patch series to rely on the previous ones. This means that code
+reviewers review  patches and patch series **sequentially** and **the structure
+of a patch series guides the code review process**. Sometimes in a long series,
+patches {1,2}/10 will be clean-ups, {3-6}/10 will be general reorganizations
+which don't really seem to do anything and then {7-10}/10 will be the substance
+of the series, which helps the code reviewer understand what {3-6}/10 were
+about.
+
+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
+^^^^
+
+* 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
+  series is trying to achieve and which portions are addressing which problems.
+* Try and keep changes that belong to a subsystem together
+* Expect that the structure of a patch series sometimes may need to change
+  between different versions of a patch series
+* **Most importantly**: Start small. Don't submit a large and complex patch
+  series as the first interaction with the community. Try and pick a smaller
+  task first (e.g. a bug-fix, a clean-up task, etc.) such that you don't have
+  to learn the tools, code and deal with a large patch series all together for
+  the first time.
+
+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
+seven categories. In other words: you would end up with **7 categories x N
+subsystems**. In some cases, there is a **global set of patches** that affect
+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
+   substance of a series
+4. The substance of the change
+5. Cleanups of any infelicities introduced temporarily
+6. Deleting old code
+7. Test code
+
+Note that in many cases, some of the listed categories are not always present
+in each set, as they are not needed. Of course, sometimes there are several
+patches describing **changes of substance**, which could be ordered in different
+ways: in such cases it may be necessary to put reorganizations in between these
+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
+^^^^^^^^^^
+
+* In some cases, following the general pattern above may create extra patches
+  and may make a series more complex and harder to understand.
+* Crafting a more extensive cover letter will be extra effort: in most cases,
+  the extra time investment will be saving time during the code review process.
+  Verbosity is not the goal, but clarity is. Before you send a larger series
+  in particular: try and put yourself into the position of a code reviewer and
+  try to identify information that helps a code reviewer follow the patch
+  series.
+* In cases where changes need to be back-ported to older releases, moving
+  general cleanups last is often preferable: in such cases the **substance of
+  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
+  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
+  * It contains some information about the test status of the series including
+    some performance information.
+  * It maps the series onto the categories listed above. As expected, not
+    all categories are used in this case. However, the series does contain
+    elements of **1** (in this case preparation to enable the functionality),
+    **2** reorganizations and other non-functional changes that enable the
+    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
+-------------------------------------
+
+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
+it makes sense to wait until receiving feedback on the entire series before
+sending out a new version addressing the changes. For larger series, it may
+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
+
+.. _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`_ 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.
+
+Typically, problematic patch reviews uncover issues such as wrong or missed
+assumptions, a problematic architecture or design, or other bugs that require
+significant re-implementation of a patch series to fix the issue.
+
+The principle of **Shift Left** also applies in code reviews. Let's assume a
+series has a major flaw: ideally, this flaw would be picked up in the **first
+or second iteration** of the code review. As significant parts of the code may
+have to be re-written, it does not make sense for reviewers to highlight minor
+issues (such as style issues) until major flaws have been addressed of the
+affected part of a patch series. In such cases, providing feedback on minor
+issues reviewers cause the code author and themselves extra work by asking for
+changes to code, which ultimately may be changed later.
+
+To make it possible for code reviewers to identify major issues early, it is
+important for code-authors to highlight possible issues in a cover letter and
+to structure a patch series in such a way that makes it easy for reviewers to
+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
+---------------------------
+
+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_>`_
+
+.. _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
index e0139fa1439118cfc5c432a6b9028a3020aac588..d1152c4aee9918aa1b63728cfad534730eebc760 100644 (file)
@@ -12,6 +12,7 @@ Welcome to XenProject Governance's documentation!
 
    code-of-conduct
    communication-guide
+   code-review-guide
 
 Indices and tables
 ==================