[v4,5/7] Add Code Review Guide

Commit Message

Lars Kurth Dec. 30, 2019, 7:32 p.m. UTC

From: Lars Kurth <[e-mail]>

This document highlights what reviewers such as maintainers and committers look
for when reviewing code. It sets expectations for code authors and provides
a framework for code reviewers.

Changes since v3
* Added example under *Workflow from a Reviewer's Perspective* section
* Fixed typos in text introduced in v2

Changes since v2 (introduced in v2)
* Extend introduction
* Add "Code Review Workflow" covering
  - "Workflow from a Reviewer's Perspective"
  - "Workflow from an Author's Perspective"
  - "Problematic Patch Reviews"
* Wrap to 80 characters
* Replace inline links with reference links to make
  wrapping easier

Signed-off-by: Lars Kurth <[e-mail]>
---
Cc: [e-mail]
Cc: [e-mail]
Cc: [e-mail]
Cc: [e-mail]
Cc: [e-mail]
---
 code-review-guide.md | 313 +++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 313 insertions(+)
 create mode 100644 code-review-guide.md

Comments

Jürgen Groß Jan. 6, 2020, 7:03 a.m. UTC | #1

On 30.12.19 20:32, Lars Kurth wrote:
> From: Lars Kurth <[e-mail]>
> 
> This document highlights what reviewers such as maintainers and committers look
> for when reviewing code. It sets expectations for code authors and provides
> a framework for code reviewers.
> 
> Changes since v3
> * Added example under *Workflow from a Reviewer's Perspective* section
> * Fixed typos in text introduced in v2
> 
> Changes since v2 (introduced in v2)
> * Extend introduction
> * Add "Code Review Workflow" covering
>    - "Workflow from a Reviewer's Perspective"
>    - "Workflow from an Author's Perspective"
>    - "Problematic Patch Reviews"
> * Wrap to 80 characters
> * Replace inline links with reference links to make
>    wrapping easier
> 
> Signed-off-by: Lars Kurth <[e-mail]>
> ---
> Cc: [e-mail]
> Cc: [e-mail]
> Cc: [e-mail]
> Cc: [e-mail]
> Cc: [e-mail]
> ---
>   code-review-guide.md | 313 +++++++++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 313 insertions(+)
>   create mode 100644 code-review-guide.md
> 
> diff --git a/code-review-guide.md b/code-review-guide.md
> new file mode 100644
> index 0000000..b2c08d2
> --- /dev/null
> +++ b/code-review-guide.md
> @@ -0,0 +1,313 @@
> +# 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

duplicate "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 reorganisations
> +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 subsytems (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. Reorganisations
> +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 reorganisations 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** reorganisations 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, analyse, 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

s/read the//

> +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

Juergen

Patch

diff --git a/code-review-guide.md b/code-review-guide.md
new file mode 100644
index 0000000..b2c08d2
--- /dev/null
+++ b/code-review-guide.md
@@ -0,0 +1,313 @@ 
+# 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 reorganisations
+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 subsytems (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. Reorganisations
+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 reorganisations 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** reorganisations 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, analyse, 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