[v2,4/6] Add Code Review Guide

Commit Message

Lars Kurth Sept. 26, 2019, 7:39 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.

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 | 125 +++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 125 insertions(+)
 create mode 100644 code-review-guide.md

Comments

Stefano Stabellini Nov. 28, 2019, 12:54 a.m. UTC | #1

On Thu, 26 Sep 2019, 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.

I think the document is missing a couple of things:

- a simple one line statement that possibly the most important thing in
  a code review is to indentify any bugs in the code

- an explanation that requests for major changes to the series should be
  made early on (i.e. let's not change the architecture of a feature at
  v9 if possible) I also made this comment in reply to patch #5. I'll
  let you decide where is the best place for it.


> 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 | 125 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 125 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..8639431
> --- /dev/null
> +++ b/code-review-guide.md
> @@ -0,0 +1,125 @@
> +# 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.
> +
> +This document does **not cover** the following topics:
> +* [Communication Best Practice](communication-practice.md)
> +* [Resolving Disagreement](resolving-disagreement.md)
> +* [Patch Submission Workflow](https://wiki.xenproject.org/wiki/Submitting_Xen_Project_Patches)
> +* [Managing Patch Submission with Git](https://wiki.xenproject.org/wiki/Managing_Xen_Patches_with_Git)
> +
> +## 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](https://xenbits.xenproject.org/gitweb/?p=xen.git;a=blob;f=CODING_STYLE)
> +  and [tools/libxl/CODING_STYLE](https://xenbits.xenproject.org/gitweb/?p=xen.git;a=blob;f=tools/libxl/CODING_STYLE)
> +* 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](https://xenbits.xen.org/gitweb/?p=xen.git;a=tree;f=docs) folder.
> +* When adding new features that have an impact on the end-user,
> +  a contributor should include an update to the
> +  [SUPPORT.md](https://xenbits.xen.org/gitweb/?p=xen.git;a=tree;f=docs) 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](https://xenbits.xenproject.org/gitweb/?p=xen.git;a=tree;f=tools/tests)
> +or [xen/test](https://xenbits.xenproject.org/gitweb/?p=xen.git;a=tree;f=xen/test)<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](https://gitlab.com/xen-project/xen/pipelines)<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](https://xenbits.xenproject.org/docs/xtf/)<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](https://xenbits.xenproject.org/gitweb/?p=osstest.git;a=blob;f=README)<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](https://wiki.xenproject.org/wiki/Submitting_Xen_Project_Patches).
> +
> +## 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](https://www.slideshare.net/xen_com_mgr/xpdds19-keynote-patch-review-for-nonmaintainers-george-dunlap-citrix-systems-uk-ltd)
> +* [Patch Review for Non-Maintainers recording - 20"](https://www.youtube.com/watch?v=ehZvBmrLRwg)
> -- 
> 2.13.0
>

Jan Beulich Nov. 28, 2019, 10:09 a.m. UTC | #2

On 28.11.2019 01:54, Stefano Stabellini wrote:
> On Thu, 26 Sep 2019, 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.
> 
> I think the document is missing a couple of things:
> 
> - a simple one line statement that possibly the most important thing in
>   a code review is to indentify any bugs in the code
> 
> - an explanation that requests for major changes to the series should be
>   made early on (i.e. let's not change the architecture of a feature at
>   v9 if possible) I also made this comment in reply to patch #5. I'll
>   let you decide where is the best place for it.

This needs balancing. People crucial to the evaluation of a new
feature and its implementation simply may not have the time to
reply prior to v9. We've had situations where people posted new
revisions every other day, sometimes even more than one per day.

As indicated in several other contexts before - imo people not
helping to shoulder the review load should also not have the
expectation that their (large) contributions will be looked at
in due course. 

Jan

Lars Kurth Nov. 28, 2019, 1:06 p.m. UTC | #3

On 28/11/2019, 04:09, "Jan Beulich" <[e-mail]> wrote:

    On 28.11.2019 01:54, Stefano Stabellini wrote:
    > On Thu, 26 Sep 2019, 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.
    > 
    > I think the document is missing a couple of things:
    > 
    > - a simple one line statement that possibly the most important thing in
    >   a code review is to indentify any bugs in the code
    > 
    > - an explanation that requests for major changes to the series should be
    >   made early on (i.e. let's not change the architecture of a feature at
    >   v9 if possible) I also made this comment in reply to patch #5. I'll
    >   let you decide where is the best place for it.
    
    This needs balancing. People crucial to the evaluation of a new
    feature and its implementation simply may not have the time to
    reply prior to v9. We've had situations where people posted new
    revisions every other day, sometimes even more than one per day.

I can certainly add something on the timing , along the lines of
* For complex series, consider the time it takes to do reviews (maybe with a guide of LOC per hour) and give reviewers enough time to
* For series with design issues or large questions, try and highlight the key open issues in cover letters clearly and solicit feedback from key maintainers who can comment on the open issue. The idea is to save both the contributor and the reviewers time by focussing on what needs to be resolved 
* Don’t repost a series, unless all review comments are addressed or the reviewers asked you to do so. The problem with this is that this is somewhat in conflict with the "let's focus on the core issues and not get distracted by details early on in a review cycle". In other words, this can only work, if reviewers focus on major issues in early reviews only and do not focus on style, coding standards, etc. As soon as a reviewer comes back with detailed feedback, the contributor will feel obliged to fix these. This creates a motivation to want to please the reviewer send out new versions of series fixing cosmetic issues without addressing the substantial issues, leading to what Jan describes. I am looking for opinions here.  
    
    As indicated in several other contexts before - imo people not
    helping to shoulder the review load should also not have the
    expectation that their (large) contributions will be looked at
    in due course. 
    
I can add something to this effect.  

Lars

Jan Beulich Nov. 28, 2019, 1:37 p.m. UTC | #4

On 28.11.2019 14:06, Lars Kurth wrote:
> I can certainly add something on the timing , along the lines of
> * For complex series, consider the time it takes to do reviews (maybe with a guide of LOC per hour) and give reviewers enough time to
> * For series with design issues or large questions, try and highlight the key open issues in cover letters clearly and solicit feedback from key maintainers who can comment on the open issue. The idea is to save both the contributor and the reviewers time by focussing on what needs to be resolved 
> * Don’t repost a series, unless all review comments are addressed
> or the reviewers asked you to do so. The problem with this is that
> this is somewhat in conflict with the "let's focus on the core
> issues and not get distracted by details early on in a review cycle".
> In other words, this can only work, if reviewers focus on major
> issues in early reviews only and do not focus on style, coding
> standards, etc.

But this doesn't make much sense either, because then full re-reviews
need to happen anyway on later versions, to also deal with the minor
issues. For RFC kind of series omitting style and alike feedback
certainly makes sense, but as soon as a patch is non-RFC, it should
be considered good to go in by the submitter.

Jan

Lars Kurth Nov. 28, 2019, 2:02 p.m. UTC | #5

On 28/11/2019, 07:37, "Jan Beulich" <[e-mail]> wrote:

    On 28.11.2019 14:06, Lars Kurth wrote:
    > I can certainly add something on the timing , along the lines of
    > * For complex series, consider the time it takes to do reviews (maybe with a guide of LOC per hour) and give reviewers enough time to
    > * For series with design issues or large questions, try and highlight the key open issues in cover letters clearly and solicit feedback from key maintainers who can comment on the open issue. The idea is to save both the contributor and the reviewers time by focussing on what needs to be resolved 
    > * Don’t repost a series, unless all review comments are addressed
    > or the reviewers asked you to do so. The problem with this is that
    > this is somewhat in conflict with the "let's focus on the core
    > issues and not get distracted by details early on in a review cycle".
    > In other words, this can only work, if reviewers focus on major
    > issues in early reviews only and do not focus on style, coding
    > standards, etc.
    
    But this doesn't make much sense either, because then full re-reviews
    need to happen anyway on later versions, to also deal with the minor
    issues. For RFC kind of series omitting style and alike feedback
    certainly makes sense, but as soon as a patch is non-RFC, it should
    be considered good to go in by the submitter.
    
OK, I think we have a disconnect between ideal and reality. 

I see two issues today
* Key maintainers don't always review RFC series [they end up at the bottom of the priority list, even though spending time on RFCs will save time elsewhere later]. So the effect is that then the contributor assumes there are no major issues and ends it as a proper series
* In practice what has happened often in the past is that design, architecture, assumption flaws are found in early versions of a series.
   - This usually happens because of an oversight or because there was no design discussion prior to the series being posted and agreed
   - Common sense would dictate that the biggest benefit for both the reviewer, the contributor and the community as a whole would be to try and focus on such flaws and leave everything aside
   - Of course there may be value in doing a detailed reviews of such a series as there may be bits that are unaffected by such a flaw
   - But there will likely be parts which are not: doing a detailed review of such portions wastes everyone's time

So coming back to your point. Ideally, it would be nice if we had the capability to call out parts of a series as "problematic" and treating such parts differently

Lars

Rich Persaud Nov. 28, 2019, 6:12 p.m. UTC | #6

On Nov 28, 2019, at 05:12, Jan Beulich <[e-mail]> wrote:
> 
> On 28.11.2019 01:54, Stefano Stabellini wrote:
>>> On Thu, 26 Sep 2019, 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.
>> 
>> I think the document is missing a couple of things:
>> 
>> - a simple one line statement that possibly the most important thing in
>>  a code review is to indentify any bugs in the code
>> 
>> - an explanation that requests for major changes to the series should be
>>  made early on (i.e. let's not change the architecture of a feature at
>>  v9 if possible) I also made this comment in reply to patch #5. I'll
>>  let you decide where is the best place for it.
> 
> This needs balancing. People crucial to the evaluation of a new
> feature and its implementation simply may not have the time to
> reply prior to v9. We've had situations where people posted new
> revisions every other day, sometimes even more than one per day.
> 
> As indicated in several other contexts before - imo people not
> helping to shoulder the review load should also not have the
> expectation that their (large) contributions will be looked at
> in due course. 

To make this actionable, we could have:

- reviewer demand index:  automated index of open patches still in need of review, sorted by decreasing age

- review flow control:  each new patch submission cites one recent review by the patch submitter, for a patch of comparable size

- reviewer supply growth:  a bootstrapping guide for new reviewers and submitters, with patterns, anti-patterns, and examples to be emulated

Rich

Stefano Stabellini Nov. 28, 2019, 6:19 p.m. UTC | #7

On Thu, 28 Nov 2019, Jan Beulich wrote:
> On 28.11.2019 01:54, Stefano Stabellini wrote:
> > On Thu, 26 Sep 2019, 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.
> > 
> > I think the document is missing a couple of things:
> > 
> > - a simple one line statement that possibly the most important thing in
> >   a code review is to indentify any bugs in the code
> > 
> > - an explanation that requests for major changes to the series should be
> >   made early on (i.e. let's not change the architecture of a feature at
> >   v9 if possible) I also made this comment in reply to patch #5. I'll
> >   let you decide where is the best place for it.
> 
> This needs balancing. People crucial to the evaluation of a new
> feature and its implementation simply may not have the time to
> reply prior to v9. We've had situations where people posted new
> revisions every other day, sometimes even more than one per day.

Yes, you are right, it needs balancing. This is not meant to encourage
contributors to send 9 versions of a series within a week or two :-)

We could say that "contributors should make sure to give enough time to
all the key stakeholders to review the series".



> As indicated in several other contexts before - imo people not
> helping to shoulder the review load should also not have the
> expectation that their (large) contributions will be looked at
> in due course. 

I think you are right on this point, and maybe we could add something to
that effect

Rich Persaud Nov. 28, 2019, 6:20 p.m. UTC | #8

On Nov 28, 2019, at 09:05, Lars Kurth <[e-mail]> wrote:
> 
> On 28/11/2019, 07:37, "Jan Beulich" <[e-mail]> wrote:
> 
>>    On 28.11.2019 14:06, Lars Kurth wrote:
>> I can certainly add something on the timing , along the lines of
>> * For complex series, consider the time it takes to do reviews (maybe with a guide of LOC per hour) and give reviewers enough time to
>> * For series with design issues or large questions, try and highlight the key open issues in cover letters clearly and solicit feedback from key maintainers who can comment on the open issue. The idea is to save both the contributor and the reviewers time by focussing on what needs to be resolved 
>> * Don’t repost a series, unless all review comments are addressed
>> or the reviewers asked you to do so. The problem with this is that
>> this is somewhat in conflict with the "let's focus on the core
>> issues and not get distracted by details early on in a review cycle".
>> In other words, this can only work, if reviewers focus on major
>> issues in early reviews only and do not focus on style, coding
>> standards, etc.
> 
>    But this doesn't make much sense either, because then full re-reviews
>    need to happen anyway on later versions, to also deal with the minor
>    issues. For RFC kind of series omitting style and alike feedback
>    certainly makes sense, but as soon as a patch is non-RFC, it should
>    be considered good to go in by the submitter.
> 
> OK, I think we have a disconnect between ideal and reality. 
> 
> I see two issues today
> * Key maintainers don't always review RFC series [they end up at the bottom of the priority list, even though spending time on RFCs will save time elsewhere later]. So the effect is that then the contributor assumes there are no major issues and ends it as a proper series
> * In practice what has happened often in the past is that design, architecture, assumption flaws are found in early versions of a series.
>   - This usually happens because of an oversight or because there was no design discussion prior to the series being posted and agreed
>   - Common sense would dictate that the biggest benefit for both the reviewer, the contributor and the community as a whole would be to try and focus on such flaws and leave everything aside
>   - Of course there may be value in doing a detailed reviews of such a series as there may be bits that are unaffected by such a flaw
>   - But there will likely be parts which are not: doing a detailed review of such portions wastes everyone's time
> 
> So coming back to your point. Ideally, it would be nice if we had the capability to call out parts of a series as "problematic" and treating such parts differently.

We may be able to reuse some "Shift Left" terminology, including citations of previous Xen code reviews to illustrate categories of design issues that can be shifted left:

  https://devopedia.org/shift-left

Rich

Lars Kurth Nov. 29, 2019, 1:39 a.m. UTC | #9

From: Rich Persaud <[e-mail]>
Date: Thursday, 28 November 2019 at 12:21
To: Lars Kurth <[e-mail]>
Cc: 'Jan Beulich' <[e-mail]>, "[e-mail]" <[e-mail]>, Stefano Stabellini <[e-mail]>, "[e-mail]" <[e-mail]>, "[e-mail]" <[e-mail]>, "[e-mail]" <[e-mail]>, "[e-mail]" <[e-mail]>, xen-devel <[e-mail]>, "[e-mail]" <[e-mail]>
Subject: Re: [MirageOS-devel] [PATCH v2 4/6] Add Code Review Guide

On Nov 28, 2019, at 09:05, Lars Kurth <[e-mail]> wrote:

On 28/11/2019, 07:37, "Jan Beulich" <[e-mail]> wrote:

   On 28.11.2019 14:06, Lars Kurth wrote:

I can certainly add something on the timing , along the lines of
* For complex series, consider the time it takes to do reviews (maybe with a guide of LOC per hour) and give reviewers enough time to
* For series with design issues or large questions, try and highlight the key open issues in cover letters clearly and solicit feedback from key maintainers who can comment on the open issue. The idea is to save both the contributor and the reviewers time by focussing on what needs to be resolved
* Don’t repost a series, unless all review comments are addressed
or the reviewers asked you to do so. The problem with this is that
this is somewhat in conflict with the "let's focus on the core
issues and not get distracted by details early on in a review cycle".
In other words, this can only work, if reviewers focus on major
issues in early reviews only and do not focus on style, coding
standards, etc.

   But this doesn't make much sense either, because then full re-reviews
   need to happen anyway on later versions, to also deal with the minor
   issues. For RFC kind of series omitting style and alike feedback
   certainly makes sense, but as soon as a patch is non-RFC, it should
   be considered good to go in by the submitter.

OK, I think we have a disconnect between ideal and reality.

I see two issues today
* Key maintainers don't always review RFC series [they end up at the bottom of the priority list, even though spending time on RFCs will save time elsewhere later]. So the effect is that then the contributor assumes there are no major issues and ends it as a proper series
* In practice what has happened often in the past is that design, architecture, assumption flaws are found in early versions of a series.
  - This usually happens because of an oversight or because there was no design discussion prior to the series being posted and agreed
  - Common sense would dictate that the biggest benefit for both the reviewer, the contributor and the community as a whole would be to try and focus on such flaws and leave everything aside
  - Of course there may be value in doing a detailed review of parts of such a series as there may be bits that are unaffected by such a flaw
  - But there will likely be parts which are not: doing a detailed review of such portions wastes everyone's time

So coming back to your point. Ideally, it would be nice if we had the capability to call out parts of a series as "problematic" and treating such parts differently.

  We may be able to reuse some "Shift Left" terminology, including citations of previous Xen code reviews to illustrate categories of design issues that can be shifted left:

    https://devopedia.org/shift-left

I like that idea. We seem to not have come to a conclusion on this specific topic, but maybe for now it is sufficient to call this out as a potential issue in the guide.

Before I send out a new version, it would be good to get at least Jan’s view on the issue.

Lars

Lars Kurth Nov. 29, 2019, 1:50 a.m. UTC | #10

On 28/11/2019, 12:12, "Rich Persaud" <[e-mail]> wrote:

    On Nov 28, 2019, at 05:12, Jan Beulich <[e-mail]> wrote:
    > 
    > On 28.11.2019 01:54, Stefano Stabellini wrote:
    >>> On Thu, 26 Sep 2019, 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.
    >> 
    >> I think the document is missing a couple of things:
    >> 
    >> - a simple one line statement that possibly the most important thing in
    >>  a code review is to indentify any bugs in the code
    >> 
    >> - an explanation that requests for major changes to the series should be
    >>  made early on (i.e. let's not change the architecture of a feature at
    >>  v9 if possible) I also made this comment in reply to patch #5. I'll
    >>  let you decide where is the best place for it.
    > 
    > This needs balancing. People crucial to the evaluation of a new
    > feature and its implementation simply may not have the time to
    > reply prior to v9. We've had situations where people posted new
    > revisions every other day, sometimes even more than one per day.
    > 
    > As indicated in several other contexts before - imo people not
    > helping to shoulder the review load should also not have the
    > expectation that their (large) contributions will be looked at
    > in due course. 
    
    To make this actionable, we could have:
    
    - reviewer demand index:  automated index of open patches still in need of review, sorted by decreasing age
    
    - review flow control:  each new patch submission cites one recent review by the patch submitter, for a patch of comparable size
    
    - reviewer supply growth:  a bootstrapping guide for new reviewers and submitters, with patterns, anti-patterns, and examples to be emulated
    
That is a great idea. However, I would not want to hold up the publication of these documents on these suggestions. Some of them would require implementing tools. I was hoping there would be more progress on lore and others tooling/workflow related stuff by now. So I think for now, I think it is sufficient to set expectations better.

Regards
Lars

Lars Kurth Dec. 5, 2019, 11:41 p.m. UTC | #11

From: Lars Kurth <[e-mail]>
Date: Thursday, 28 November 2019 at 19:39
To: Rich Persaud <[e-mail]>
Cc: 'Jan Beulich' <[e-mail]>, "[e-mail]" <[e-mail]>, Stefano Stabellini <[e-mail]>, "[e-mail]" <[e-mail]>, "[e-mail]" <[e-mail]>, "[e-mail]" <[e-mail]>, "[e-mail]" <[e-mail]>, xen-devel <[e-mail]>, "[e-mail]" <[e-mail]>
Subject: Re: [MirageOS-devel] [PATCH v2 4/6] Add Code Review Guide

 
 
From: Rich Persaud <[e-mail]>
Date: Thursday, 28 November 2019 at 12:21
To: Lars Kurth <[e-mail]>
Cc: 'Jan Beulich' <[e-mail]>, "[e-mail]" <[e-mail]>, Stefano Stabellini <[e-mail]>, "[e-mail]" <[e-mail]>, "[e-mail]" <[e-mail]>, "[e-mail]" <[e-mail]>, "[e-mail]" <[e-mail]>, xen-devel <[e-mail]>, "[e-mail]" <[e-mail]>
Subject: Re: [MirageOS-devel] [PATCH v2 4/6] Add Code Review Guide
 
On Nov 28, 2019, at 09:05, Lars Kurth <[e-mail]> wrote:
 
On 28/11/2019, 07:37, "Jan Beulich" <[e-mail]> wrote:

   On 28.11.2019 14:06, Lars Kurth wrote:


I can certainly add something on the timing , along the lines of
* For complex series, consider the time it takes to do reviews (maybe with a guide of LOC per hour) and give reviewers enough time to
* For series with design issues or large questions, try and highlight the key open issues in cover letters clearly and solicit feedback from key maintainers who can comment on the open issue. The idea is to save both the contributor and the reviewers time by focussing on what needs to be resolved 
* Don’t repost a series, unless all review comments are addressed
or the reviewers asked you to do so. The problem with this is that
this is somewhat in conflict with the "let's focus on the core
issues and not get distracted by details early on in a review cycle".
In other words, this can only work, if reviewers focus on major
issues in early reviews only and do not focus on style, coding
standards, etc.

   But this doesn't make much sense either, because then full re-reviews
   need to happen anyway on later versions, to also deal with the minor
   issues. For RFC kind of series omitting style and alike feedback
   certainly makes sense, but as soon as a patch is non-RFC, it should
   be considered good to go in by the submitter.

OK, I think we have a disconnect between ideal and reality. 

I see two issues today
* Key maintainers don't always review RFC series [they end up at the bottom of the priority list, even though spending time on RFCs will save time elsewhere later]. So the effect is that then the contributor assumes there are no major issues and ends it as a proper series
* In practice what has happened often in the past is that design, architecture, assumption flaws are found in early versions of a series.
  - This usually happens because of an oversight or because there was no design discussion prior to the series being posted and agreed
  - Common sense would dictate that the biggest benefit for both the reviewer, the contributor and the community as a whole would be to try and focus on such flaws and leave everything aside
  - Of course there may be value in doing a detailed review of parts of such a series as there may be bits that are unaffected by such a flaw
  - But there will likely be parts which are not: doing a detailed review of such portions wastes everyone's time

So coming back to your point. Ideally, it would be nice if we had the capability to call out parts of a series as "problematic" and treating such parts differently.
 
  We may be able to reuse some "Shift Left" terminology, including citations of previous Xen code reviews to illustrate categories of design issues that can be shifted left:
  
    https://devopedia.org/shift-left
  
I like that idea. We seem to not have come to a conclusion on this specific topic, but maybe for now it is sufficient to call this out as a potential issue in the guide.
 
Before I send out a new version, it would be good to get at least Jan’s view on the issue. 
 
Lars

I have a draft version of  this series ready, but wanted to check how some of it resonates. Also, I do have open questions, where I am looking for input from seasoned reviewers

I propose to add the following section to code-review-guide.md

----
## <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. By 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.

The question then becomes, how do code reviewers identify major issues early? 
----
This is where I really need help. Are there any tips and recommendations that we could give?
I can clearly highlight that we have RFC series, but in practice that does not solve the problem as RFCs don’t get prioritized
How do reviewers normally approach a series: do you a) take a big picture view first, or b) do most of you work through a series sequentially

I then propose to change the following section in communication-practice.md
----
### Prioritize significant flaws
If a patch or patch series has significant flaws, such as
* It is built on wrong assumptions
* There are issues with the architecture or the design

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. 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).
----
I think is a pragmatic approach that addresses some of Jan's concerns

Best Regards
Lars

Jan Beulich Dec. 6, 2019, 9:51 a.m. UTC | #12

On 06.12.2019 00:41, Lars Kurth wrote:
> I propose to add the following section to code-review-guide.md
> 
> ----
> ## <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. By 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.
> 
> The question then becomes, how do code reviewers identify major issues early? 
> ----
> This is where I really need help. Are there any tips and recommendations that we could give?
> I can clearly highlight that we have RFC series, but in practice that does not solve the problem as RFCs don’t get prioritized
> How do reviewers normally approach a series: do you a) take a big picture view first, or b) do most of you work through a series sequentially

Afaic - depends heavily on the patch / series. I wouldn't typically
peek ahead in a series, but it has happened. But as you say
(elsewhere) the cover letter should put in place the "big picture".
A series should generally be reviewable going from patch to patch,
having the cover letter in mind.

> I then propose to change the following section in communication-practice.md
> ----
> ### Prioritize significant flaws
> If a patch or patch series has significant flaws, such as
> * It is built on wrong assumptions
> * There are issues with the architecture or the design

In such a case a full review of course doesn't make much sense. But
this is far from the typical situation. Way more often you have some
_part_ of a patch or series which has a bigger issue, but other
parts are in need of no or just minor changes.

> 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. 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).

I have no issues with the suggested text in general, but I also don't
think it makes much of a difference wrt what I had mentioned before.
I guess part of the problem here is that there are things which imo
you can't really give recipes for how to approach, if the expectation
is that it would fit at least the vast majority of cases. For code
reviews this means that I don't think there should be any wording
suggesting they should be done in a certain form; there may be wording
suggesting they _could_ be done in a certain form (e.g. to help
people not knowing at all how to get started).

Jan

Lars Kurth Dec. 9, 2019, 11:02 a.m. UTC | #13

On 06/12/2019, 09:51, "Jan Beulich" <[e-mail]> wrote:

    On 06.12.2019 00:41, Lars Kurth wrote:
    > I propose to add the following section to code-review-guide.md
    > 
    > ----
    > ## <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. By 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.
    > 
    > The question then becomes, how do code reviewers identify major issues early? 
    > ----
    > This is where I really need help. Are there any tips and recommendations that we could give?
    > I can clearly highlight that we have RFC series, but in practice that does not solve the problem as RFCs don’t get prioritized
    > How do reviewers normally approach a series: do you a) take a big picture view first, or b) do most of you work through a series sequentially
    
    Afaic - depends heavily on the patch / series. I wouldn't typically
    peek ahead in a series, but it has happened. But as you say
    (elsewhere) the cover letter should put in place the "big picture".
    A series should generally be reviewable going from patch to patch,
    having the cover letter in mind.

I am wondering what others do. 

I think explaining the basic work-flow from the viewpoint of a reviewer and code author maybe in a separate section, which is not tied to the problem case would make sense. More input from other maintainers would be valuable. My gut-feel is that most reviewers "read and review" series sequentially, which has implications for the author. E.g.
- docs/design docs should be at the beginning of a series
- key header files or changes to them should be at the beginning of a series
- Etc
   
    > I then propose to change the following section in communication-practice.md
    > ----
    > ### Prioritize significant flaws
    > If a patch or patch series has significant flaws, such as
    > * It is built on wrong assumptions
    > * There are issues with the architecture or the design
    
    In such a case a full review of course doesn't make much sense. But
    this is far from the typical situation. Way more often you have some
    _part_ of a patch or series which has a bigger issue, but other
    parts are in need of no or just minor changes.

I know that this is an unusual situation. But it has happened in clusters frequently in the past.

I am wondering whether we should introduce some informal convention to mark _part_ of a series as problematic. A simple example of how to do this in the cover letter would do
    
    > 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. 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).
    
    I have no issues with the suggested text in general, but I also don't
    think it makes much of a difference wrt what I had mentioned before.
    I guess part of the problem here is that there are things which imo
    you can't really give recipes for how to approach, if the expectation
    is that it would fit at least the vast majority of cases. 

I think the document covers most of the common cases, plus some areas which are problematic
* From a people-interaction point-of-view - in other words there could be unnecessary conflict, which is bad for the community but also wastes time
* From an efficient usage of time point-of-view

For example: the whole thing about thanking, appreciation, ... is something targeted at newcomers and a desire to treat them with more thought and awareness. 
Granted it takes more time to do a review with a newcomer, but it should make subsequent reviews easier 

It happens regularly, but not that frequently
  
    For code
    reviews this means that I don't think there should be any wording
    suggesting they should be done in a certain form; there may be wording
    suggesting they _could_ be done in a certain form (e.g. to help
    people not knowing at all how to get started).

That was definitely my intention. Maybe I have not succeeded in making this clear enough

Regards
Lars

Lars Kurth Dec. 9, 2019, 3:58 p.m. UTC | #14

> On 9 Dec 2019, at 11:02, Lars Kurth <[e-mail]> wrote:
> 
> 
> 
> On 06/12/2019, 09:51, "Jan Beulich" <[e-mail] <mailto:[e-mail]>> wrote:
> 
>    On 06.12.2019 00:41, Lars Kurth wrote:
>> I propose to add the following section to code-review-guide.md
>> 
>> ----
>> ## <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. By 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.
>> 
>> The question then becomes, how do code reviewers identify major issues early? 
>> ----
>> This is where I really need help. Are there any tips and recommendations that we could give?
>> I can clearly highlight that we have RFC series, but in practice that does not solve the problem as RFCs don’t get prioritized
>> How do reviewers normally approach a series: do you a) take a big picture view first, or b) do most of you work through a series sequentially
> 
>    Afaic - depends heavily on the patch / series. I wouldn't typically
>    peek ahead in a series, but it has happened. But as you say
>    (elsewhere) the cover letter should put in place the "big picture".
>    A series should generally be reviewable going from patch to patch,
>    having the cover letter in mind.
> 
> I am wondering what others do. 
> 
> I think explaining the basic work-flow from the viewpoint of a reviewer and code author maybe in a separate section, which is not tied to the problem case would make sense. More input from other maintainers would be valuable. My gut-feel is that most reviewers "read and review" series sequentially, which has implications for the author. E.g.
> - docs/design docs should be at the beginning of a series
> - key header files or changes to them should be at the beginning of a series
> - Etc
> 
>> I then propose to change the following section in communication-practice.md
>> ----
>> ### Prioritize significant flaws
>> If a patch or patch series has significant flaws, such as
>> * It is built on wrong assumptions
>> * There are issues with the architecture or the design
> 
>    In such a case a full review of course doesn't make much sense. But
>    this is far from the typical situation. Way more often you have some
>    _part_ of a patch or series which has a bigger issue, but other
>    parts are in need of no or just minor changes.
> 
> I know that this is an unusual situation. But it has happened in clusters frequently in the past.
> 
> I am wondering whether we should introduce some informal convention to mark _part_ of a series as problematic. A simple example of how to do this in the cover letter would do
> 
>> 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. 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).
> 
>    I have no issues with the suggested text in general, but I also don't
>    think it makes much of a difference wrt what I had mentioned before.
>    I guess part of the problem here is that there are things which imo
>    you can't really give recipes for how to approach, if the expectation
>    is that it would fit at least the vast majority of cases. 
> 
> I think the document covers most of the common cases, plus some areas which are problematic
> * From a people-interaction point-of-view - in other words there could be unnecessary conflict, which is bad for the community but also wastes time
> * From an efficient usage of time point-of-view
> 
> For example: the whole thing about thanking, appreciation, ... is something targeted at newcomers and a desire to treat them with more thought and awareness. 
> Granted it takes more time to do a review with a newcomer, but it should make subsequent reviews easier 
> 
> It happens regularly, but not that frequently
> 
>    For code
>    reviews this means that I don't think there should be any wording
>    suggesting they should be done in a certain form; there may be wording
>    suggesting they _could_ be done in a certain form (e.g. to help
>    people not knowing at all how to get started).
> 
> That was definitely my intention. Maybe I have not succeeded in making this clear enough

Adding the log of a very productive IRC conversation for reference. Sorry for the formatting
Regards
Lars

‹lars_kurth›     It would also be good if some maintainers could have a look at https://lists.xenproject.org/archives/html/xen-devel/2019-12/threads.html#00348

‹gwd›		lars_kurth: What exactly more feedback did you want?

‹lars_kurth›     gwd: primarily about maintainers workflow. How do you approach a review. Is there anything a code author can do. Right now, I have one data point from Jan. I basically want to validate my assumptions before I send out another revision

‹lars_kurth›     gwd: a code author can do to make your life easier => e.g. by putting big picture stuff at the beginning of a series

‹lars_kurth›     e.g by putting potentially controversial elements of it at the beginning, etc

‹lars_kurth›     Obviously, this would only help if most reviewers approach a review sequentially, which is a reasonable assumption, but I am not actually 100% sure this is true

‹royger›           lars_kurth: patch series must be reviewed sequentially, that's why it's numbered

‹gwd›		lars_kurth: Actually it's often the case that you find uncontroversial side clean-up things in the course of doing a series; putting those at the beginning means they can be checked in independently of the whole series, which makes less work for everyone

‹gwd›		They can be checked in after v1 or v2, and then 1) less work for authors to rebase, 2) less cognitive overhead for reviewers to see what's going on.

‹lars_kurth›     royger: That is not a necessarily obvious conclusion - at least for me. But if it is true, we should clearly state that

‹lars_kurth›     gwd: that's interesting. I guess there are differing objectives

‹royger›           I don't think any maintainer/reviewer does review non-sequentially, as it would lead to extreme confusion. It's quite common for patches on a patch series to rely on the previous ones

‹royger›           lars_kurth: I think it can be safely stated

‹lars_kurth›     1) get uncontroversial / easy to do stuff first

‹lars_kurth›     2) then put more complex things, but start with documents/headers/anything that is substantial to understand the rest of the changes - if they fit into logical units maybe repeat that pattern

‹gwd›		I think every situation if different.

‹royger›           the only time you can get non-ordered review is if your patch series touches multiple subsystems and you have properly splitted this into different patches, in that case each subsystem maintainer is likelky to review his ares without looking at other patches

‹gwd›  royger: You're talking about checking in, not review I think?

‹gwd›		Sometimes in a long series, {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 will be the "meat", which helps you understand what {3-6}/10 were about.

‹lars_kurth›     royger, gwd: "It's quite common for patches on a patch series to rely on the previous ones" - is an important point.

‹gwd›		Usually it's not possible to put 7/10 earlier without making it far more complicated.

‹lars_kurth›     I think the way how to best structure patch series is an important one, and there is practically NO information about this. So people learn stuff by trial and error. So I think it is worth exploring that

‹gwd›		It's a bit like saying, "How do you teach someone something", or "How do you make an argument to convince someone of something". There are patterns, but it's so broad a topic you can't really give direct advice.

‹lars_kurth›     gwd: maybe showing some examples will help. I think the problem is that cover letters frequently don't help much

‹Diziet› 		I think what you just said above "Sometimes in a long series," would be a very useful thing to put in as a general rule.

‹Diziet› 		gwd: ^

‹Diziet› 		Sometimes some of these pieces will be empty.

‹Diziet› 		1. cleanups 2. reorgs 3. headers/docs/etc. 4. meat 5. cleaning up any infelicities introduced temporarily 6. deleting old code

‹Diziet› 		If there are multiple subsystems involved, then these are best separated out where possible, so you end up with those 6 categories x N subsystems.

‹gwd›  		Of course, sometimes there are several "meat" patches, which could be ordered in different ways; and then you may want to put reorgs in between the meat patches.

‹gwd›  		The XSA-299 series is an example of that.

‹Diziet› 		In this context I am reminded of https://www.chiark.greenend.org.uk/pipermail/sgo-software-discuss/2019/000616.html My main achievement for the weekend, in a personal project.

‹gwd›  		And lars_kurth could probably go back over the history and see the series develop; the "meat" patches were reorganized several times to try to find out which order created the most comprehensible series of individual patches.

‹Diziet› 		Subject:  New signature key arrangements

‹gwd›  		lars_kurth: And say, if you take a look at the golang xenlight bindings series; v1 would have been useless without the entire thing being checked in. It's likely that v3 I'll be able to check in {1..16}/22, meaning v4 will have a lot fewer patches to rebase / recheck.

‹Diziet› 		I'm not suggesting it as an example in this context because I have done much less squashing than would be usual in a community like Xen where we have many more reviewers so the goal of making review easy and comprehensible is more important relative to the goal of later understanding what the original programmer was thinking when they wrote soemthing.

‹lars_kurth›     This is very useful. I think this is the missing piece

‹Diziet› 		But we should provide some examples. Do we have good example cover letters we could use ?

‹Diziet› 		I'm sure there are some libxl ones but ideally we would have an example touching multiple subsystems. And one which wasn't affected by release constraints.

‹lars_kurth›     That would be good. Maybe I can write something up and put a place-holder in place for good examples, if we can't think of one now

‹jbeulich›         gwd, lars_kurth: The opposite case (cleanups last) would often be preferable for series where the "meat" one(s) are to be backported, but the cleanups aren't.

‹lars_kurth›     jbeulich: interesting point. Do you have an example?

‹jbeulich›         An example of what? A series where we asked for re-ordering because of the above? If so, I don't think I could easily spot one.

‹jbeulich›         I can tell you though that the above is what I would typically do. Most prominently for security fixes *where the cleanup is being held back altogether).

‹lars_kurth›     jbeulich: I will try and put something together. I think I have enough to go on

Patch

diff --git a/code-review-guide.md b/code-review-guide.md
new file mode 100644
index 0000000..8639431
--- /dev/null
+++ b/code-review-guide.md
@@ -0,0 +1,125 @@ 
+# 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.
+
+This document does **not cover** the following topics:
+* [Communication Best Practice](communication-practice.md)
+* [Resolving Disagreement](resolving-disagreement.md)
+* [Patch Submission Workflow](https://wiki.xenproject.org/wiki/Submitting_Xen_Project_Patches)
+* [Managing Patch Submission with Git](https://wiki.xenproject.org/wiki/Managing_Xen_Patches_with_Git)
+
+## 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](https://xenbits.xenproject.org/gitweb/?p=xen.git;a=blob;f=CODING_STYLE)
+  and [tools/libxl/CODING_STYLE](https://xenbits.xenproject.org/gitweb/?p=xen.git;a=blob;f=tools/libxl/CODING_STYLE)
+* 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](https://xenbits.xen.org/gitweb/?p=xen.git;a=tree;f=docs) folder.
+* When adding new features that have an impact on the end-user,
+  a contributor should include an update to the
+  [SUPPORT.md](https://xenbits.xen.org/gitweb/?p=xen.git;a=tree;f=docs) 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](https://xenbits.xenproject.org/gitweb/?p=xen.git;a=tree;f=tools/tests)
+or [xen/test](https://xenbits.xenproject.org/gitweb/?p=xen.git;a=tree;f=xen/test)<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](https://gitlab.com/xen-project/xen/pipelines)<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](https://xenbits.xenproject.org/docs/xtf/)<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](https://xenbits.xenproject.org/gitweb/?p=osstest.git;a=blob;f=README)<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](https://wiki.xenproject.org/wiki/Submitting_Xen_Project_Patches).
+
+## 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](https://www.slideshare.net/xen_com_mgr/xpdds19-keynote-patch-review-for-nonmaintainers-george-dunlap-citrix-systems-uk-ltd)
+* [Patch Review for Non-Maintainers recording - 20"](https://www.youtube.com/watch?v=ehZvBmrLRwg)