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 indentify 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](communication-practice.md)
* [Resolving Disagreement](resolving-disagreement.md)
More information related to these items can be found in our
[Patch submission Guide](https://wiki.xenproject.org/wiki/Submitting_Xen_Project_Patches).
-## Reviewing for Patch Authors
+## Code Review Workflow
+
+This section is important for code authors and reviewers. We recomment 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 serties, 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. Cleaninups 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
+indepentendtly
+
+**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.
+
+**Examples:**
+TODO:
+* We should have some examples of a well structured cover letter for a complex series.
+A candidate may be:
+https://lore.kernel.org/xen-devel/20190928151305.127380-1-wipawel@amazon.de/T/#t
+(or earlier versions)
+* We should have an example which shows a patch with a good logical structure
+
+### 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 he;ps 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](https://devopedia.org/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 diffifcult 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.
> I think this is a good feature proposal
It is also entirely fine to highlight specific changes as good. The best place to
-do this, is at top of a patch, as addressing code review comments typically requires
+do this, is at the top of a patch, as addressing code review comments typically requires
a contributor to go through the list of things to address and an in-lined positive
comment is likely to break that workflow.
> physically and spiritually)
This is not something one is used to hearing from dictionary entries. Once you
-investigate the practice foot-binding, it is hard to disagree with the dictionart entry.
+investigate the practice foot-binding, it is hard to disagree with the dictionary entry.
However, the statement does not contain much information. If you read it without
knowing what foot-binding is, it is hard to be convinced by this statement. The main
take-away is that the author of the dictionary entry had strong opinions about this topic.
-It does not tell you, why you should have the same opinion.
+It does not tell you why you should have the same opinion.
Compare this to the (Wikipedia entry)[https://en.wikipedia.org/wiki/Foot_binding]
If you tend to use a terse commenting style and you do not know whether the author
is OK with it, it is often a good idea to compensate for it in the code review opening
(where you express appreciation) or when there is a need for verbose expression.
+However, when you know are working with a seasoned code author, it is also entirely
+acceptable to drop niceties such as expressing appreciation with the goal to save
+the author and reviewer time.
It is also entirely fine to mention that you have a fairly terse communication style
and ask whether the author is OK with it. In almost all cases, they will be: by asking
you are showing empathy that helps counteract a negative impression.
+### Clarity over Verbosity
+When reading this document, you may get the impression that following the guidance
+outlined here takes more effort and time for both code reviewers and code authors.
+This is not the intention: much of this document aims to create clearer communication,
+which ultimately saves time by reducing unnecessary iterations during communication.
+We value **clarity over verbosity**.
+
+Areas which often create unecessary back-and-forth between reviewers and authors
+are
+* Unstated assumptions and goals
+* Leave suggestions, examples, and resources (such as links to existing code)
+* There is nothing more helpful for the thought process than example. It guarantees that you have a shared understanding and reduces the questions asked on a comment.
+
### Code Review Comments should be actionable
Code review comments should be actionable: in other words, it needs to be clear
what the author of the code needs to do to address the issue you identified.
> thoughts about both solutions by e-mail first, such that we can decide what works
> best
-### Identify the severity of an issue or disagreement
+### Identify the severity and optionality of review comments
By default, every comment which is made **ought to be addressed** by the author.
-However, often reviewers note issues, which would be nice if they were addressed,
-but are not mandatory.
+However, sometimes reviewers note issues, which would be nice if they were
+addressed, but are not mandatory to fix.
Typically, reviewers use terminology such as
> This would be a nice-to-have
Some maintainers use
> NIT: XXX
-however, it is sometimes also used to indicate a minor issue that **must** be fixed.
+however, it is sometimes also used to indicate a minor issue that **must** be fixed.
+Also terminology such as **this is not a blocker** could be misinterpreted. It is
+important that **reviewers** use language that make clear whether
+a comment is an optional suggestion. Examples may be
+> NIT (optional): XXX
+> I think it would be good if X also did Y, not a requirement but nice-to-have
+### Identify the severity of a disagreement
During a code review, it can happen that reviewer and author disagree on how to move
forward. The default position when it comes to disagreements is that **both parties
want to argue their case**. However, frequently one or both parties do not feel that
it does not make sense to do a detailed code review. In such cases, it is best to
focus on the major issues first and deal with style and minor issues in a subsequent
-review. This reduces the workload on both the reviewer and patch author. However,
-reviewers should make clear that they have omitted detailed review comments and
-that these will come later.
+review. Not all series have significant flaws, but most series have different classes of
+changes that are required for acceptance: covering a range of major code
+modifications to minor code style fixes. To avoid misunderstandings between
+reviewers and contributors, it is important to establish and agree whether a series or
+part of a series has a significant flaw and agree a course of action.
+
+A pragmatic approach would be to
+* Highlight problematic portions of a series in the cover letter
+* For the patch author and reviewer(s) to agree that for problematic to omit style and
+minor issues in the review, until the significant flaw is addressed
+
+This saves both the patch author and reviewer(s) time. Note that some background
+is covered in detail in [Problematic Patch Reviews](resolving-disagreement.md#problems).
+
-### Welcome newcomers
+### Reviewers: Welcome newcomers
When reviewing the first few patches of a newcomer to the project, you may want
spend additional time and effort in your code review. This contributes to a more
**positive experience**, which ultimately helps create a positive working relationship in
> much easier. Feel free to find me on IRC if you need specific help. My IRC
> handle is ZZZ
-### Review the code, then review the review
+### Reviewers: Take account of previous reviewer(s) comments
+Sometimes multiple reviewers share reviewing a series. For example,
+reviewer John has reviewed the first 5 iterations of the series. The patch author has
+addressed all of John's comments and Susan comes in and picks up the series after
+iteration 5. In such cases it is possible that John and Susan have different styles, such
+as
+* different preferences on the code layout
+* different preferences on code style
+
+If Susan were to be strict on her own style and highlight her style preferences in
+subsequent reviews, this would cause additional re-work for the code author. In
+addition, it also causes extra work for Susan. The easiest way to avoid such situations,
+would be for Susan to focus on faulty code only and to disregard personal preferences
+when taking over the review of a series.
+
+### Reviewers: Review the code, then review the review
As stated earlier it is often difficult to mentally separate finding issues from articulating
code review comments in a constructive and positive manner. Even as an experienced
code reviewer you can be in a bad mood, which can impact your communication style.
## Issue: Scope creep
One thing which occasionally happens during code review is that a code reviewer
-asks or appears to ask the author of patch to implement additional functionality.
+asks or appears to ask the author of a patch to implement additional functionalities.
This could take for example the form of
> Do you think it would be useful for the code to do XXX?
## Issue: [Bikeshedding](https://en.wiktionary.org/wiki/bikeshedding)
Occasionally discussions about unimportant but easy-to-grasp issues can lead to
-prolonged and unproductive discussion. The best way to approach this is to
+prolonged and unproductive discussions. The best way to approach this is to
try and **anticipate** bikeshedding and highlight it as such upfront. However, the
format of a code review does not always lend itself well to this approach, except
for highlighting it in the cover letter of a patch series.
been performed at your house. Let's say that you have a new bathroom installed.
Before paying your builder the last instalment, you perform an inspection and you find
issues such as
-* The seals around the bathtub are not perfectly event
+* The seals around the bathtub are not perfectly even
* When you open the tap, the plumbing initially makes some loud noise
* The shower mixer has been installed the wrong way around
* An author which always addresses **all** of these issues may end up getting into
difficulties with their employer, as they are too slow getting code upstreamed.
-None of these outcomes are good, so ultimately a balance has been found. At the end
+None of these outcomes are good, so ultimately a balance has to be found. At the end
of the day, the solution should focus on what is best for the community, which may
mean asking for an independent opinion as outlined in the next section.
+## Issue: Multiple ways to solve a problem
+
+Frequently it is possible that a problem can be solved in multiple ways and it is not
+always obvious which one is best. Code reviewers tend to follow their personal
+coding style when reviewing cide and sometimes will suggest that a code author
+makes changes to follow their own style, even when the author's code is correct. In
+such cases, it is easy to disagree and start arguing.
+
+We recommend that the code author tries to follow the code reviewers requests, even
+if they could be considered style issues, trusting the experience of the code reviewer.
+Similarly, we ask code reviewers to let the contributor have the freedom of
+implementation choices, where they do not have a downside.
+
+We do not always succeed in this, as such it is important to **identify** such a
+situation and then call it out as outlined
+[here](communication-practice.md#Misunderstandings).
+
## Resolution: Asking for an independent opinion
Most disagreements can be settled by
projects / people / larsk / code-of-conduct.git / commitdiff