[Xen-devel] [PATCH v3 0/7] Code of Conduct + Extra Guides and Best Practices

[Xen-devel] [PATCH v3 0/7] Code of Conduct + Extra Guides and Best Practices

[Lars Kurth]

From: Lars Kurth <lars.kurth@citrix.com>

This series proposes a concrete version of the Xen Project
CoC based on v1.4 of the Contributor Covenant. See [1]

It tries to address all elements in the v2 review, which raised
a number of hard questions. One of the main outstanding items
were good examples for cover letters and well structured large
patch series (see TODO in "Add Code Review Guide")

For convenience of review and in line with other policy documents
I created a git repository at [2]. This series can be found at [3].

I also reformatted the series to 80 characters and replaced
inline syile links with reference style links to make it easier
to stick to a character limit. To just see text but not formatting changes,
have a look at [4]. This is why patch 3 "Reformat Xen Project CoC to fit
into 80 character limit" was added.

[1] https://www.contributor-covenant.org/version/1/4/code-of-conduct.md
[2] http://xenbits.xen.org/gitweb/?p=people/larsk/code-of-conduct.git;a=summary
[3] http://xenbits.xen.org/gitweb/?p=people/larsk/code-of-conduct.git;a=shortlog;h=refs/heads/CoC-v3
[4] http://xenbits.xen.org/gitweb/?p=people/larsk/code-of-conduct.git;a=commitdiff;h=ee96578446eb41320b3e8a3cada441bb891ad0df;hp=2e4b36afaa73277d246d7e84037db1532a136ec7

Changes since v2
  * Reformatted all text to 80 characters and replaced link style

  code-review-guide.md
  * Extend introduction
  * Add "Code Review Workflow" covering
    - "Workflow from a Reviewer's Perspective"
    - "Workflow from an Author's Perspective"
    - "Problematic Patch Reviews"

  TODO: find suitable examples on how to structure/describe good patch series

  communication-practice.md
  * Fix typos
  * Extended "Verbose vs. terse"
  * Added "Clarity over Verbosity"
  * Broke "Identify the severity of an issue or disagreement" into two chapters
    - "Identify the severity and optionality of review comments" and made
      clarifications
    - "Identify the severity of a disagreement"
    - Expanded "Prioritize significant flaws"
  * Added "Reviewers: Take account of previous reviewer(s) comments"
  * Added prefixes such as "Reviewers:" where appropriate

  resolving-disagreement.md
  * Fix typos
  * Add section: "Issue: Multiple ways to solve a problem"

Changes since v1
* Code of Conduct
  Only whitespace changes

* Added Communication Guide
  Contains values and a process based on advice and mediation in case of issues
  This is the primary portal for

* Added Code Review Guide
  Which is based on [4] with some additions for completeness
  It primarily sets expectations and anything communication related is removed

* Added guide on Communication Best Practice
  Takes the communication section from [4] and expands on it with more examples
  and cases. This is probably where we may need some discussion

* Added document on Resolving Disagreement
  A tiny bit of theory to set the scene
  It covers some common cases of disagreements and how we may approach them
  Again, this probably needs some discussion

Cc: minios-devel@lists.xenproject.org
Cc: xen-api@lists.xenproject.org
Cc: win-pv-devel@lists.xenproject.org
Cc: mirageos-devel@lists.xenproject.org
Cc: committers@xenproject.org

Lars Kurth (7):
  Import v1.4 of Contributor Covenant CoC
  Xen Project Code of Conduct
  Reformat Xen Project CoC to fit into 80 character limit
  Add Communication Guide
  Add Code Review Guide
  Add guide on Communication Best Practice
  Added Resolving Disagreement

--
2.13.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH v3 1/7] Import v1.4 of Contributor Covenant CoC

[Lars Kurth]

From: Lars Kurth <lars.kurth@citrix.com>

Signed-off-by: Lars Kurth <lars.kurth@citrix.com>
---
Cc: minios-devel@lists.xenproject.org
Cc: xen-api@lists.xenproject.org
Cc: win-pv-devel@lists.xenproject.org
Cc: mirageos-devel@lists.xenproject.org
Cc: committers@xenproject.org
---
 code-of-conduct.md | 76 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 76 insertions(+)
 create mode 100644 code-of-conduct.md

diff --git a/code-of-conduct.md b/code-of-conduct.md
new file mode 100644
index 0000000..81b217c
--- /dev/null
+++ b/code-of-conduct.md
@@ -0,0 +1,76 @@
+# Contributor Covenant Code of Conduct
+
+## Our Pledge
+
+In the interest of fostering an open and welcoming environment, we as
+contributors and maintainers pledge to make participation in our project and
+our community a harassment-free experience for everyone, regardless of age, body
+size, disability, ethnicity, sex characteristics, gender identity and expression,
+level of experience, education, socio-economic status, nationality, personal
+appearance, race, religion, or sexual identity and orientation.
+
+## Our Standards
+
+Examples of behavior that contributes to creating a positive environment
+include:
+
+* Using welcoming and inclusive language
+* Being respectful of differing viewpoints and experiences
+* Gracefully accepting constructive criticism
+* Focusing on what is best for the community
+* Showing empathy towards other community members
+
+Examples of unacceptable behavior by participants include:
+
+* The use of sexualized language or imagery and unwelcome sexual attention or
+  advances
+* Trolling, insulting/derogatory comments, and personal or political attacks
+* Public or private harassment
+* Publishing others' private information, such as a physical or electronic
+  address, without explicit permission
+* Other conduct which could reasonably be considered inappropriate in a
+  professional setting
+
+## Our Responsibilities
+
+Project maintainers are responsible for clarifying the standards of acceptable
+behavior and are expected to take appropriate and fair corrective action in
+response to any instances of unacceptable behavior.
+
+Project maintainers have the right and responsibility to remove, edit, or
+reject comments, commits, code, wiki edits, issues, and other contributions
+that are not aligned to this Code of Conduct, or to ban temporarily or
+permanently any contributor for other behaviors that they deem inappropriate,
+threatening, offensive, or harmful.
+
+## Scope
+
+This Code of Conduct applies within all project spaces, and it also applies when
+an individual is representing the project or its community in public spaces.
+Examples of representing a project or community include using an official
+project e-mail address, posting via an official social media account, or acting
+as an appointed representative at an online or offline event. Representation of
+a project may be further defined and clarified by project maintainers.
+
+## Enforcement
+
+Instances of abusive, harassing, or otherwise unacceptable behavior may be
+reported by contacting the project team at [INSERT EMAIL ADDRESS]. All
+complaints will be reviewed and investigated and will result in a response that
+is deemed necessary and appropriate to the circumstances. The project team is
+obligated to maintain confidentiality with regard to the reporter of an incident.
+Further details of specific enforcement policies may be posted separately.
+
+Project maintainers who do not follow or enforce the Code of Conduct in good
+faith may face temporary or permanent repercussions as determined by other
+members of the project's leadership.
+
+## Attribution
+
+This Code of Conduct is adapted from the [Contributor Covenant][homepage], version 1.4,
+available at https://www.contributor-covenant.org/version/1/4/code-of-conduct.html
+
+[homepage]: https://www.contributor-covenant.org
+
+For answers to common questions about this code of conduct, see
+https://www.contributor-covenant.org/faq
-- 
2.13.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH v3 2/7] Xen Project Code of Conduct

[Lars Kurth]

From: Lars Kurth <lars.kurth@citrix.com>

Specific changes to the baseline:
* Replace list of positive behaviors with link to separate process
* Replace maintainers with project leadership
  (except in our pledge where maintainers is more appropriate)
* Add 'of all sub-projects' to clarify scope of CoC
* Rename Enforcement
* Replace "project team" with "Conduct Team members"
* Add e-mail alias
* Add section on contacting individual Conduct Team members
* Add section on Conduct Team members

Signed-off-by: Lars Kurth <lars.kurth@citrix.com>
---
Chagges since v1:
* Addressed newline changes

Cc: minios-devel@lists.xenproject.org
Cc: xen-api@lists.xenproject.org
Cc: win-pv-devel@lists.xenproject.org
Cc: mirageos-devel@lists.xenproject.org
Cc: committers@xenproject.org
---
 code-of-conduct.md | 45 ++++++++++++++++++++++++++++-----------------
 1 file changed, 28 insertions(+), 17 deletions(-)

diff --git a/code-of-conduct.md b/code-of-conduct.md
index 81b217c..5d6d1d5 100644
--- a/code-of-conduct.md
+++ b/code-of-conduct.md
@@ -1,4 +1,4 @@
-# Contributor Covenant Code of Conduct
+# Xen Project Code of Conduct
 
 ## Our Pledge
 
@@ -11,14 +11,10 @@ appearance, race, religion, or sexual identity and orientation.
 
 ## Our Standards
 
-Examples of behavior that contributes to creating a positive environment
-include:
-
-* Using welcoming and inclusive language
-* Being respectful of differing viewpoints and experiences
-* Gracefully accepting constructive criticism
-* Focusing on what is best for the community
-* Showing empathy towards other community members
+We believe that a Code of Conduct can help create a harassment-free environment,
+but is not sufficient to create a welcoming environment on its own: guidance on
+creating a welcoming environment, how to communicate in an effective and friendly
+way, etc. can be found [here](communication-guide.md).
 
 Examples of unacceptable behavior by participants include:
 
@@ -33,11 +29,11 @@ Examples of unacceptable behavior by participants include:
 
 ## Our Responsibilities
 
-Project maintainers are responsible for clarifying the standards of acceptable
+Project leadership team members are responsible for clarifying the standards of acceptable
 behavior and are expected to take appropriate and fair corrective action in
 response to any instances of unacceptable behavior.
 
-Project maintainers have the right and responsibility to remove, edit, or
+Project leadership team members have the right and responsibility to remove, edit, or
 reject comments, commits, code, wiki edits, issues, and other contributions
 that are not aligned to this Code of Conduct, or to ban temporarily or
 permanently any contributor for other behaviors that they deem inappropriate,
@@ -45,26 +41,41 @@ threatening, offensive, or harmful.
 
 ## Scope
 
-This Code of Conduct applies within all project spaces, and it also applies when
+This Code of Conduct applies within all project spaces of all sub-projects, and it also applies when
 an individual is representing the project or its community in public spaces.
 Examples of representing a project or community include using an official
 project e-mail address, posting via an official social media account, or acting
 as an appointed representative at an online or offline event. Representation of
-a project may be further defined and clarified by project maintainers.
+a project may be further defined and clarified by the project leadership.
 
-## Enforcement
+## What to do if you witness or are subject to unacceptable behavior
 
 Instances of abusive, harassing, or otherwise unacceptable behavior may be
-reported by contacting the project team at [INSERT EMAIL ADDRESS]. All
+reported by contacting Conduct Team members at conduct@xenproject.org. All
 complaints will be reviewed and investigated and will result in a response that
-is deemed necessary and appropriate to the circumstances. The project team is
+is deemed necessary and appropriate to the circumstances. Conduct Team members are
 obligated to maintain confidentiality with regard to the reporter of an incident.
 Further details of specific enforcement policies may be posted separately.
 
-Project maintainers who do not follow or enforce the Code of Conduct in good
+If you have concerns about any of the members of the conduct@ alias,
+you are welcome to contact precisely the Conduct Team member(s) of
+your choice.
+
+Project leadership team members who do not follow or enforce the Code of Conduct in good
 faith may face temporary or permanent repercussions as determined by other
 members of the project's leadership.
 
+## Conduct Team members
+Conduct Team members are project leadership team members from any
+sub-project. The current list of Conduct Team members is:
+* Lars Kurth <lars dot kurth at xenproject dot org>
+* George Dunlap <george dot dunlap at citrix dot com>
+* Ian Jackson <ian dot jackson at citrix dot com>
+
+Conduct Team members are changed by proposing a change to this document,
+posted on all sub-project lists, followed by a formal global vote as outlined
+[here]: https://xenproject.org/developers/governance/#project-decisions
+
 ## Attribution
 
 This Code of Conduct is adapted from the [Contributor Covenant][homepage], version 1.4,
-- 
2.13.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH v3 3/7] Reformat Xen Project CoC to fit into 80 character limit

[Lars Kurth]

From: Lars Kurth <lars.kurth@citrix.com>

No content changes

Signed-off-by: Lars Kurth <lars.kurth@citrix.com>
---
Cc: minios-devel@lists.xenproject.org
Cc: xen-api@lists.xenproject.org
Cc: win-pv-devel@lists.xenproject.org
Cc: mirageos-devel@lists.xenproject.org
Cc: committers@xenproject.org
---
 code-of-conduct.md | 56 ++++++++++++++++++++++++++++--------------------------
 1 file changed, 29 insertions(+), 27 deletions(-)

diff --git a/code-of-conduct.md b/code-of-conduct.md
index 5d6d1d5..4912f47 100644
--- a/code-of-conduct.md
+++ b/code-of-conduct.md
@@ -5,16 +5,16 @@
 In the interest of fostering an open and welcoming environment, we as
 contributors and maintainers pledge to make participation in our project and
 our community a harassment-free experience for everyone, regardless of age, body
-size, disability, ethnicity, sex characteristics, gender identity and expression,
-level of experience, education, socio-economic status, nationality, personal
-appearance, race, religion, or sexual identity and orientation.
+size, disability, ethnicity, sex characteristics, gender identity and
+expression, level of experience, education, socio-economic status, nationality,
+personal appearance, race, religion, or sexual identity and orientation.
 
 ## Our Standards
 
 We believe that a Code of Conduct can help create a harassment-free environment,
 but is not sufficient to create a welcoming environment on its own: guidance on
-creating a welcoming environment, how to communicate in an effective and friendly
-way, etc. can be found [here](communication-guide.md).
+creating a welcoming environment, how to communicate in an effective and
+friendly way, etc. can be found [here](communication-guide.md).
 
 Examples of unacceptable behavior by participants include:
 
@@ -29,41 +29,43 @@ Examples of unacceptable behavior by participants include:
 
 ## Our Responsibilities
 
-Project leadership team members are responsible for clarifying the standards of acceptable
-behavior and are expected to take appropriate and fair corrective action in
-response to any instances of unacceptable behavior.
+Project leadership team members are responsible for clarifying the standards of
+acceptable behavior and are expected to take appropriate and fair corrective
+action in response to any instances of unacceptable behavior.
 
-Project leadership team members have the right and responsibility to remove, edit, or
-reject comments, commits, code, wiki edits, issues, and other contributions
-that are not aligned to this Code of Conduct, or to ban temporarily or
-permanently any contributor for other behaviors that they deem inappropriate,
-threatening, offensive, or harmful.
+Project leadership team members have the right and responsibility to remove,
+edit, or reject comments, commits, code, wiki edits, issues, and other
+contributions that are not aligned to this Code of Conduct, or to ban
+temporarily or permanently any contributor for other behaviors that they deem
+inappropriate, threatening, offensive, or harmful.
 
 ## Scope
 
-This Code of Conduct applies within all project spaces of all sub-projects, and it also applies when
-an individual is representing the project or its community in public spaces.
-Examples of representing a project or community include using an official
-project e-mail address, posting via an official social media account, or acting
-as an appointed representative at an online or offline event. Representation of
-a project may be further defined and clarified by the project leadership.
+This Code of Conduct applies within all project spaces of all sub-projects,
+and it also applies when an individual is representing the project or its
+community in public spaces. Examples of representing a project or community
+include using an official project e-mail address, posting via an official social
+media account, or acting as an appointed representative at an online or offline
+event. Representation of a project may be further defined and clarified by the
+project leadership.
 
 ## What to do if you witness or are subject to unacceptable behavior
 
 Instances of abusive, harassing, or otherwise unacceptable behavior may be
 reported by contacting Conduct Team members at conduct@xenproject.org. All
 complaints will be reviewed and investigated and will result in a response that
-is deemed necessary and appropriate to the circumstances. Conduct Team members are
-obligated to maintain confidentiality with regard to the reporter of an incident.
-Further details of specific enforcement policies may be posted separately.
+is deemed necessary and appropriate to the circumstances. Conduct Team members
+are obligated to maintain confidentiality with regard to the reporter of an
+incident. Further details of specific enforcement policies may be posted
+separately.
 
 If you have concerns about any of the members of the conduct@ alias,
 you are welcome to contact precisely the Conduct Team member(s) of
 your choice.
 
-Project leadership team members who do not follow or enforce the Code of Conduct in good
-faith may face temporary or permanent repercussions as determined by other
-members of the project's leadership.
+Project leadership team members who do not follow or enforce the Code of Conduct
+in good faith may face temporary or permanent repercussions as determined by
+other members of the project's leadership.
 
 ## Conduct Team members
 Conduct Team members are project leadership team members from any
@@ -78,8 +80,8 @@ posted on all sub-project lists, followed by a formal global vote as outlined
 
 ## Attribution
 
-This Code of Conduct is adapted from the [Contributor Covenant][homepage], version 1.4,
-available at https://www.contributor-covenant.org/version/1/4/code-of-conduct.html
+This Code of Conduct is adapted from the [Contributor Covenant][homepage],
+version 1.4, available at https://www.contributor-covenant.org/version/1/4/code-of-conduct.html
 
 [homepage]: https://www.contributor-covenant.org
 
-- 
2.13.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH v3 4/7] Add Communication Guide

[Lars Kurth]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=Y, Size: 4218 bytes --]

From: Lars Kurth <lars.kurth@citrix.com>

This document is a portal page that lays out our gold standard,
best practices for some common situations and mechanisms to help
resolve issues that can have a negative effect on our community.

Detail is covered in subsequent documents

Changes since v2 (introduced in v2)
- Make lines break at 80 characters

Signed-off-by: Lars Kurth <lars.kurth@citrix.com>
---
Cc: minios-devel@lists.xenproject.org
Cc: xen-api@lists.xenproject.org
Cc: win-pv-devel@lists.xenproject.org
Cc: mirageos-devel@lists.xenproject.org
Cc: committers@xenproject.org
---
 communication-guide.md | 67 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 67 insertions(+)
 create mode 100644 communication-guide.md

diff --git a/communication-guide.md b/communication-guide.md
new file mode 100644
index 0000000..3c412d9
--- /dev/null
+++ b/communication-guide.md
@@ -0,0 +1,67 @@
+# Communication Guide
+
+We believe that our [Code of Conduct](code-of-conduct.md) can help create a
+harassment-free environment, but is not sufficient to create a welcoming
+environment on its own. We can all make mistakes: when we do, we take
+responsibility for them and try to improve.
+
+This document lays out our gold standard, best practices for some common
+situations and mechanisms to help resolve issues that can have a
+negative effect on our community.
+
+## Goal
+
+We want a productive, welcoming and agile community that can welcome new
+ideas in a complex technical field which is able to reflect on and improve how
+we work.
+
+## Communication & Handling Differences in Opinions
+
+Examples of behavior that contributes to creating a positive environment
+include:
+* Use welcoming and inclusive language
+* Keep discussions technical and actionable
+* Be respectful of differing viewpoints and experiences
+* Be aware of your own and counterpart’s communication style and culture
+* Gracefully accept constructive criticism
+* Focus on what is best for the community
+* Show empathy towards other community members
+* Resolve differences in opinion effectively
+
+## Getting Help
+
+When developing code collaboratively, technical discussion and disagreements
+are unavoidable. Our contributors come from different countries and cultures,
+are driven by different goals and take pride in their work and in their point
+of view. This invariably can lead to lengthy and unproductive debate,
+followed by indecision, sometimes this can impact working relationships
+or lead to other issues that can have a negative effect on our community.
+
+To minimize such issue, we provide a 3-stage process
+* Self-help as outlined in this document
+* Ability to ask for an independent opinion or help in private
+* Mediation between parties which disagree. In this case a neutral community
+  member assists the disputing parties resolve the issues or will work with the
+  parties such that they can improve future interactions.
+
+If you need and independent opinion or help, feel free to contact
+mediation@xenproject.org. The team behind mediation@ is made up of the
+same community members as those listed in the Conduct Team: see
+[Code of Conduct](code-of-conduct.md). In addition, team members are obligated
+to maintain confidentiality with regard discussions that take place. If you
+have concerns about any of the members of the mediation@ alias, you are
+welcome to contact precisely the team member(s) of your choice. In this case,
+please make certain that you highlight the nature of a request by making sure
+that either help or mediation is mentioned in the e-mail subject or body.
+
+## Specific Topics and Best Practice
+
+* [Code Review Guide](code-review-guide.md):
+  Essential reading for code reviewers and contributors
+* [Communication Best Practice](communication-practice.md):
+  This guide covers communication guidelines for code reviewers and reviewees.
+  It should help you create self-awareness, anticipate, avoid  and help resolve
+  communication issues.
+* [Resolving Disagreement](resolving-disagreement.md):
+  This guide lays out common situations that can lead to dead-lock and shows
+  common patterns on how to avoid and resolve issues.
-- 
2.13.0



[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH v3 5/7] Add Code Review Guide

[Lars Kurth]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=Y, Size: 15821 bytes --]

From: Lars Kurth <lars.kurth@citrix.com>

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

TODO: find suitable examples on how to structure/describe good patch series

Signed-off-by: Lars Kurth <lars.kurth@citrix.com>
---
Cc: minios-devel@lists.xenproject.org
Cc: xen-api@lists.xenproject.org
Cc: win-pv-devel@lists.xenproject.org
Cc: mirageos-devel@lists.xenproject.org
Cc: committers@xenproject.org
---
 code-review-guide.md | 309 +++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 309 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..5ffbbac
--- /dev/null
+++ b/code-review-guide.md
@@ -0,0 +1,309 @@
+# 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 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][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 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][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 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
+howwe 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]: http:s//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
-- 
2.13.0



[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH v3 6/7] Add guide on Communication Best Practice

[Lars Kurth]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=Y, Size: 25367 bytes --]

From: Lars Kurth <lars.kurth@citrix.com>

This guide covers the bulk on Best Practice related to code review
It primarily focusses on code review interactions
It also covers how to deal with Misunderstandings and Cultural
Differences

Changes since v2 (added in v2)
* Fix typos
* Extended "Verbose vs. terse"
* Added "Clarity over Verbosity"
* Broke "Identify the severity of an issue or disagreement" into two chapters
  - "Identify the severity and optionality of review comments" and made
    clarifications
  - "Identify the severity of a disagreement"
  - Expanded "Prioritize significant flaws"
* Added "Reviewers: Take account of previous reviewer(s) comments"
* Added prefixes such as "Reviewers:" where appropriate
* Fixed lien wrapping to 80 characters
* Replaced inline links with reference links

Signed-off-by: Lars Kurth <lars.kurth@citrix.com>
---
Cc: minios-devel@lists.xenproject.org
Cc: xen-api@lists.xenproject.org
Cc: win-pv-devel@lists.xenproject.org
Cc: mirageos-devel@lists.xenproject.org
Cc: committers@xenproject.org
---
 communication-practice.md | 504 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 504 insertions(+)
 create mode 100644 communication-practice.md

diff --git a/communication-practice.md b/communication-practice.md
new file mode 100644
index 0000000..0ae2426
--- /dev/null
+++ b/communication-practice.md
@@ -0,0 +1,504 @@
+# Communication Best Practice
+
+This guide provides communication Best Practice that helps you in
+* Using welcoming and inclusive language
+* Keeping discussions technical and actionable
+* Being respectful of differing viewpoints and experiences
+* Being aware of your own and counterpart’s communication style and culture
+* Show empathy towards other community members
+
+## Code reviews for **reviewers** and **patch authors**
+
+Before embarking on a code review, it is important to remember that
+* A poorly executed code review can hurt the contributors feeling, even when a
+  reviewer did not intend to do so. Feeling defensive is a normal reaction to
+  a critique or feedback. A reviewer should be aware of how the pitch, tone,
+  or sentiment of their comments could be interpreted by the contributor. The
+  same applies to responses of an author to the reviewer.
+* When reviewing someone's code, you are ultimately looking for issues. A good
+  code reviewer is able to mentally separate finding issues from articulating
+  code review comments in a constructive and positive manner: depending on your
+  personality this can be **difficult** and you may need to develop a technique
+  that works for you.
+* As software engineers we like to be proud of the solutions we came up with.
+  This can make it easy to take another people’s criticism personally. Always
+  remember that it is the code that is being reviewed, not you as a person.
+* When you receive code review feedback, please be aware that we have reviewers
+  from different backgrounds, communication styles and cultures. Although we
+  all trying to create a productive, welcoming and agile environment, we do
+  not always succeed.
+
+### Express appreciation
+
+As the nature of code review to find bugs and possible issues, it is very easy
+for reviewers to get into a mode of operation where the patch review ends up
+being a list of issues, not mentioning what is right and well done. This can
+lead to the code submitter interpreting your feedback in a negative way.
+
+The opening of a code review provides an opportunity to address this and also
+sets the tone for the rest of the code review. Starting **every** review on a
+positive note, helps set the tone for the rest of the review.
+
+For an initial patch, you can use phrases such as
+> Thanks for the patch
+> Thanks for doing this
+
+For further revisions within a review, phrases such as
+> Thank you for addressing the last set of changes
+
+If you believe the code was good, it is good practice to highlight this by
+using phrases such as
+> Looks good, just a few comments
+> The changes you have made since the last version look good
+
+If you think there were issues too many with the code to use one of the
+phrases, you can still start on a positive note, by for example saying
+> I think this is a good change
+> 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 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.
+
+You should also consider, that if you review a patch of an experienced
+contributor phrases such as *Thanks for the patch* could come across as
+patronizing, while using *Thanks for doing this* is less likely to be
+interpreted as such.
+
+Appreciation should also be expressed by patch authors when asking for
+clarifications to a review or responding to questions. A simple
+> Thank you for your feedback
+> Thank you for your reply
+> Thank you XXX!
+
+is normally sufficient.
+
+### Avoid opinion: stick to the facts
+
+The way how a reviewer expresses feedback, has a big impact on how the author
+perceives the feedback. Key to this is what we call **stick to the facts**.
+The same is true when a patch author is responding to a comment from a
+reviewer.
+
+One of our maintainers has been studying Mandarin for several years and has
+come across the most strongly-worded dictionary entry [he has ever seen][1].
+This example illustrates the problem of using opinion in code reviews vs.
+using facts extremely well.
+
+> 裹脚 (guo3 jiao3): foot-binding (a vile feudal practice which crippled women
+> both 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
+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.
+
+Compare this to the [Wikipedia entry][2]
+
+> Foot binding was the custom of applying tight binding to the feet of young
+> girls to modify the shape and size of their feet. ... foot binding was a
+> painful practice and significantly limited the mobility of women, resulting
+> in lifelong disabilities for most of its subjects. ... Binding usually
+> started during the winter months since the feet were more likely to be numb,
+> and therefore the pain would not be as extreme. …The toes on each foot
+> were curled under, then pressed with great force downwards and squeezed
+> into the sole of the foot until the toes broke…
+
+Without going into the details of foot-binding, it is noticeable that none of
+what is written above uses opinion which could be interpreted as inflammatory
+language. It is a list of simple facts that are laid out in a way that make it
+obvious what the correct conclusion is.
+
+Because the Wikipedia entry is entirely fact based it is more powerful and
+persuasive then the dictionary entry. The same applies to code reviews.
+
+Making statements in code reviews such as
+> Your code is garbage
+> This idea is stupid
+
+besides being an opinion is rude and counter productive
+* It will make the patch author angry: instead of finding a solution to the
+  problem the author will spend time and mental energy wrestling with their
+  feelings
+* It does not contain any information
+* Facts are both more powerful and more persuasive
+
+Consider the following two pieces of feedback on a piece of code
+> This piece of code is confusing
+> It took me a long time to figure out what was going on here
+
+The first example expresses an opinion, whereas the second re-phrases the
+statement in terms of what you experienced, which is a fact.
+
+Other examples:
+> BAD: This is fragile
+> SOMEWHAT BETTER: This seems fragile to me
+> BEST: If X happens, Y will happen.
+
+A certain piece of code can be written in many different ways: this can lead to
+disagreements on the best architecture, design or coding pattern. As already
+pointed out in this section: avoid feedback that is opinion-based and thus
+does not add any value. Back your criticism (or idea on how to solve a
+problem) with a sensible rationale.
+
+### Review the code, not the person
+
+Without realizing it, it is easy to overlook the difference between insightful
+critique of code and personal criticism. Let's look at a theoretical function
+where there is an opportunity to return out of the function early. In this
+case, you could say
+
+> You should return from this function early, because of XXX
+
+On its own, there is nothing wrong with this statement. However, a code review
+is made up of multiple comments and using **You should** consistently can
+start to feel negative and can be mis-interpreted as a personal attack. Using
+something like avoids this issue:
+
+> Returning from this function early is better, because of XXX
+
+Without personal reference, a code review will communicate the problem, idea
+or issue without risking mis-interpretation.
+
+### Verbose vs. terse
+
+Due to the time it takes to review and compose code reviewer, reviewers often
+adopt a terse style. It is not unusual to see review comments such as
+> typo
+> s/resions/regions/
+> coding style
+> coding style: brackets not needed
+etc.
+
+Terse code review style has its place and can be productive for both the
+reviewer and the author. However, overuse can come across as unfriendly,
+lacking empathy and can thus create a negative impression with the author of a
+patch. This is in particular true, when you do not know the author or the
+author is a newcomer. Terse communication styles can also be perceived as rude
+in some cultures.
+
+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.
+
+Statements such as
+> BAD: This is wrong
+> BAD: This does not work
+> BETTER, BUT NOT GOOD: This does not work, because of XXX
+
+do not normally provide the author of a patch with enough information to send
+out a new patch version. By doing this, you essentially force the patch author
+to **find** and **implement** an alternative, which then may also not be
+acceptable to you as the **reviewer** of the patch.
+
+A better way to approach this is to say
+
+> This does not work, because of XXX
+> You may want to investigate YYY and ZZZ as alternatives
+
+In some cases, it may not be clear whether YYY or ZZZ are the better solution.
+As a reviewer you should be as up-front and possible in such a case and say
+something like
+
+> I am not sure whether YYY and ZZZ are better, so you may want to outline your
+> thoughts about both solutions by e-mail first, such that we can decide what
+> works best
+
+### Identify the severity and optionality of review comments
+
+By default, every comment which is made **ought to be addressed** by the
+author. 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
+> This is not a blocker
+
+Some maintainers use
+> NIT: XXX
+
+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 strongly about a specific issue.
+
+Within the Xen Project, we have [a way][3] to highlight one's position on
+proposals, formal or informal votes using the following notation:
+> +2 : I am happy with this proposal, and I will argue for it
+> +1 : I am happy with this proposal, but will not argue for it
+> 0 : I have no opinion
+> -1 : I am not happy with this proposal, but will not argue against it
+> -2 : I am not happy with this proposal, and I will argue against it
+
+You can use a phrase such as
+> I am not happy with this suggestion, but will not argue against it
+
+to make clear where you stand, while recording your position. Conversely, a
+reviewer may do something similar
+> I am not happy with XYZ, but will not argue against it [anymore]
+> What we have now is good enough, but could be better
+
+### Authors: responding to review comments
+
+Typically patch authors are expected to **address all** review comments in the
+next version of a patch or patch series. In a smooth-running code review where
+you do not have further questions it is not at all necessary to acknowledge
+the changes you are going to make:
+* Simply send the next version with the changes addressed and record it in the
+  change-log
+
+When there is discussion, the normal practice is to remove the portion of the
+e-mail thread where there is agreement. Otherwise, the thread can become
+exceptionally long.
+
+In cases where there was discussion and maybe disagreement, it does however
+make sense to close the discussion by saying something like
+
+> ACK
+> Seems we are agreed, I am going to do this
+
+Other situations when you may want to do this are cases where the reviewer made
+optional suggestions, to make clear whether the suggestion will be followed or
+not.
+
+### Avoid uncommon words: not everyone is a native English speaker
+
+Avoid uncommon words both when reviewing code or responding to a review. Not
+everyone is a native English speaker. The use of such words can come across
+badly and can lead to misunderstandings.
+
+### 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][4].
+
+
+### 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 the long term.
+
+When someone does their first code submission, they will not be familiar with
+**all** conventions in the project. A good approach is to
+* Welcome the newcomer
+* Offer to help with specific questions, for example on IRC
+* Point to existing documentation: in particular if mistakes with the
+  submission itself were made. In most situations, following the submission
+  process makes the process more seamless for the contributor. So, you could
+  say something like
+
+> Hi XXX. Welcome to the community and thank you for the patch
+>
+> I noticed that the submission you made seems to not follow our process.
+> Are you aware of this document at YYY? If you follow the instructions the
+> entire code submission process and dealing with review comments becomes
+> much easier. Feel free to find me on IRC if you need specific help. My IRC
+> handle is ZZZ
+
+### 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.
+
+A good trick to avoid this, is to start and complete the code review and then
+**not send it immediately**. You can then have a final go over the code review
+at some later point in time and review your comments from the other author's
+point of view. This minimizes the risk of being misunderstood. The same
+applies when replying to a code review: draft your reply and give it a final
+scan before pressing the send button.
+
+Generally, it is a good idea for code reviewers to do this regularly, purely
+from the viewpoint of self-improvement and self-awareness.
+
+## Common Communication Pitfalls
+
+This section contains common communication issues and provides suggestions on
+how to avoid them and resolve them. These are **general** issues which affect
+**all** online communication. As such, we can only try and do our best.
+
+### Misunderstandings
+
+When you meet face to face, you can read a person’s emotions. Even with a
+phone call, someone’s tone of voice can convey a lot of information. Using
+on-line communication channels you are flying blind, which often leads to
+misunderstandings. [Research][5] shows that in up to 50% of email
+conversations, the tone of voice is misinterpreted.
+
+In code reviews and technical discussions in general we tend to see two things
+* The reviewer or author interprets an exchange as too critical, passive
+  aggressive, or other: this usually comes down to different cultures and
+  communication styles, which are covered in the next section
+* There is an actual misunderstanding of a subject under discussion
+
+In the latter case, the key to resolution is to **identify the
+misunderstanding** as quickly as possible and call it out and de-escalate
+rather than let the misunderstanding linger. This is inherently difficult and
+requires more care than normal communication. Typically you would start with
+* Showing appreciation
+* Highlighting the potential misunderstanding and verifying whether the other
+  person also feels that maybe there was a misunderstanding
+* Proposing a way forward: for example, it may make sense to move the
+  conversation from the mailing list to [IRC][6] either in private or public,
+  a community call or a private phone/video call.
+
+It is entirely acceptable to do this in a direct reply to your communication
+partner, rather than on a public e-mail list on or an otherwise public forum.
+
+A good approach is to use something like the following:
+> Hi XXX! Thank you for the insights you have given me in this code review
+> I feel that we are misunderstanding each other on the topic of YYY
+> Would you mind trying to resolve this on IRC. I am available at ZZZ
+
+Usually, technical misunderstandings come down two either
+1. Misinterpreting what the other person meant
+2. Different - usually unstated - assumptions on how something works or what
+   is to be achieved
+3. Different - usually unstated - objectives and goals, which may be
+   conflicting
+4. Real differences in opinion
+
+The goal of calling out a possible misunderstanding is to establish what
+caused the misunderstanding, such that all parties can move forward.
+Typically, 1 and 2 are easily resolved and will lead back to a constructive
+discussion. Whereas 3 and 4 may highlight an inherent disagreement, which may
+need to be resolved through techniques as
+outlined in [Resolving Disagreement][7].
+
+### Cultural differences and different communication styles
+
+The Xen Project is a global community with contributors from many different
+backgrounds. Typically, when we communicate with a person we know, we factor
+in past interactions. The less we know a person, the more we rely on cultural
+norms.
+
+However, different norms and value systems come into play when people from
+diverse cultural backgrounds interact. That can lead to misunderstandings,
+especially in sensitive situations such as conflict resolution, giving and
+receiving feedback, and consensus building.
+
+For example, giving direct feedback such as
+> [Please] replace XXX with YYY, as XXX does not do ZZZ
+
+is acceptable and normal in some cultures, whereas in cultures which value
+indirect feedback it would be considered rude. In the latter case, something
+like the following would be used
+> This looks very good to me, but I believe you should use YYY here,
+> because XXX would....
+
+The key to working and communicating well with people from different cultural
+backgrounds is **self-awareness**, which can then be used to either
+* Adapt your own communication style depending on who you talk to
+* Or to find a middle-ground that covers most bases
+
+A number of different theories in the field of working effectively are
+currently popular, with the most well-known one being
+[Erin Meyer's Culture Map][8]. A short overview can be found [here][9]
+(33 slides).
+
+### Code reviews and discussions are not competitions
+
+Code reviews on our mailing lists are not competitions on who can come up with
+the smartest solution or who is the real coding genius.
+
+In a code review - as well as in general - we expect that all stake-holders
+* Gracefully accept constructive criticism
+* Focus on what is best for the community
+* Resolve differences in opinion effectively
+
+The next section provides pointers on how to do this effectively.
+
+### Resolving Disagreement Effectively
+
+Common scenarios are covered our guide on [Resolving Disagreement][7], which
+lays out situations that can lead to dead-lock and shows common patterns on
+how to avoid and resolve issues.
+
+[1]: https://youtu.be/ehZvBmrLRwg?t=834
+[2]: https://en.wikipedia.org/wiki/Foot_binding
+[3]: https://xenproject.org/developers/governance/#expressingopinion
+[4]: resolving-disagreement.md#problems
+[5]: https://www.wired.com/2006/02/the-secret-cause-of-flame-wars/
+[6]: https://xenproject.org/help/irc/
+[7]: resolving-disagreement.md
+[8]: https://en.wikipedia.org/wiki/Erin_Meyer
+[9]: https://www.nsf.gov/attachments/134059/public/15LFW_WorkingWithMulticulturalTeams_LarsonC.pdf
\ No newline at end of file
-- 
2.13.0



[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH v3 7/7] Added Resolving Disagreement

[Lars Kurth]

From: Lars Kurth <lars.kurth@citrix.com>

This guide provides Best Practice on identifying and resolving
common classes of disagreement

Changes since v2 (added in v2)
* Fix typos
* Add section: "Issue: Multiple ways to solve a problem"
* Changed line wrapping to 80 characters
* Replaced inline style links with reference style links

Signed-off-by: Lars Kurth <lars.kurth@citrix.com>
--
Cc: minios-devel@lists.xenproject.org
Cc: xen-api@lists.xenproject.org
Cc: win-pv-devel@lists.xenproject.org
Cc: mirageos-devel@lists.xenproject.org
Cc: committers@xenproject.org
---
 resolving-disagreement.md | 188 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 188 insertions(+)
 create mode 100644 resolving-disagreement.md

diff --git a/resolving-disagreement.md b/resolving-disagreement.md
new file mode 100644
index 0000000..97bca7b
--- /dev/null
+++ b/resolving-disagreement.md
@@ -0,0 +1,188 @@
+# Resolving Disagreement
+
+This guide provides Best Practice on resolving disagreement, such as
+* Gracefully accept constructive criticism
+* Focus on what is best for the community
+* Resolve differences in opinion effectively
+
+## Theory: Paul Graham's hierarchy of disagreement
+
+Paul Graham proposed a **disagreement hierarchy** in a 2008 essay
+**[How to Disagree][1]**, putting types of arguments into a seven-point
+hierarchy and observing that *moving up the disagreement hierarchy makes people
+less mean, and will make most of them happier*. Graham also suggested that the
+hierarchy can be thought of as a pyramid, as the highest forms of disagreement
+are rarer.
+
+| ![Graham's Hierarchy of Disagreement][2]                                    |
+| *A representation of Graham's hierarchy of disagreement from [Loudacris][3]
+  modified by [Rocket000][4]*                                                 |
+
+In the context of the Xen Project we strive to **only use the top half** of the
+hierarchy. **Name-calling** and **Ad hominem** arguments are not acceptable
+within the Xen Project.
+
+## Issue: Scope creep
+
+One thing which occasionally happens during code review is that a code reviewer
+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?
+> I can imagine a user wanting to do YYY (and XXX would enable this)
+
+That potentially adds additional work for the code author, which they may not
+have the time to perform. It is good practice for authors to consider such a
+request in terms of
+* Usefulness to the user
+* Code churn, complexity or impact on other system properties
+* Extra time to implement and report back to the reviewer
+
+If you believe that the impact/cost is too high, report back to the reviewer.
+To resolve this, it is advisable to
+* Report your findings
+* And then check whether this was merely an interesting suggestion, or something
+  the reviewer feels more strongly about
+
+In the latter case, there are typically several common outcomes
+* The **author and reviewer agree** that the suggestion should be implemented
+* The **author and reviewer agree** that it may make sense to defer
+  implementation
+* The **author and reviewer agree** that it makes no sense to implement the
+  suggestion
+
+The author of a patch would typically suggest their preferred outcome, for
+example
+> I am not sure it is worth to implement XXX
+> Do you think this could be done as a separate patch in future?
+
+In cases, where no agreement can be found, the best approach would be to get an
+independent opinion from another maintainer or the project's leadership team.
+
+## Issue: [Bikeshedding][5]
+
+Occasionally discussions about unimportant but easy-to-grasp issues can lead 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.
+
+However, typically Bikeshedding issues are fairly easy to recognize in a code
+review, as you will very quickly get different reviewers providing differing
+opinions. In this case it is best for the author or a reviewer to call out the
+potential bikeshedding issue using something like
+
+> Looks we have a bikeshedding issue here
+> I think we should call a quick vote to settle the issue
+
+Our governance provides the mechanisms of [informal votes][6] or
+[lazy voting][7] which lend themselves well to resolve such issues.
+
+## Issue: Small functional issues
+
+The most common area of disagreements which happen in code reviews, are
+differing opinions on whether small functional issues in a patch series have to
+be resolved or not before the code is ready to be submitted. Such disagreements
+are typically caused by different expectations related to the level of
+perfection a patch series needs to fulfil before it can be considered ready to
+be committed.
+
+To explain this better, I am going to use the analogy of some building work that
+has 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 even
+* When you open the tap, the plumbing initially makes some loud noise
+* The shower mixer has been installed the wrong way around
+
+In all these cases, the bathroom is perfectly functional, but not perfect. At
+this point you have the choice to try and get all the issues addressed, which in
+the example of the shower mixer may require significant re-work and potentially
+push-back from your builder. You may have to refer to the initial statement of
+work, but it turns out it does not contain sufficient information to ascertain
+whether your builder had committed to the level of quality you were expecting.
+
+Similar situations happen in code reviews very frequently and can lead to a long
+discussion before it can be resolved. The most important thing is to
+**identify** a disagreement as such early and then call it out. Tips on how to
+do this, can be found [here][8].
+
+At this point, you will understand why you have the disagreement, but not
+necessarily agreement on how to move forward. An easy fix would be to agree to
+submit the change as it is and fix it in future. In a corporate software
+engineering environment this is the most likely outcome, but in open source
+communities additional concerns have to be considered.
+* Code reviewers frequently have been in this situation before with the most
+  common outcome that the issue is then never fixed. By accepting the change,
+  the reviewers have no leverage to fix the issue and may have to spend effort
+  fixing the issue themselves in future as it may impact the product they built
+  on top of the code.
+* Conversely, a reviewer may be asking the author to make too many changes of
+  this type which ultimately may lead the author to not contribute to the
+  project again.
+* An author, which consistently does not address **any** of these issues may
+  end up getting a bad reputation and may find future code reviews more
+  difficult.
+* 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 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][8].
+
+## Resolution: Asking for an independent opinion
+
+Most disagreements can be settled by
+* Asking another maintainer or committer to provide an independent opinion on the
+  specific issue in public to help resolve it
+* Failing this an issue can be escalated to the project leadership team, which is
+  expected to act as referee and make a decision on behalf of the community
+
+If you feel uncomfortable with this approach, you may also contact
+mediation@xenproject.org to get advice. See our [Communication Guide][9]
+for more information.
+
+## Decision making and conflict resolution in our governance
+
+Our [governance][A] contains several proven mechanisms to help with decision
+making and conflict resolution.
+
+See
+* [Expressing agreement and disagreement][B]
+* [Lazy consensus / Lazy voting][7]
+* [Informal votes or surveys][6]
+* [Leadership team decisions][C]
+* [Conflict resolution][D]
+
+[1]: http://www.paulgraham.com/disagree.html
+[2]: https://upload.wikimedia.org/wikipedia/commons/a/a3/Graham%27s_Hierarchy_of_Disagreement-en.svg
+[3]: http://www.createdebate.com/user/viewprofile/Loudacris
+[4]: https://en.wikipedia.org/wiki/User:Rocket000
+[5]: https://en.wiktionary.org/wiki/bikeshedding
+[6]: https://xenproject.org/developers/governance/#informal-votes-or-surveys
+[7]: https://xenproject.org/developers/governance/#lazyconsensus
+[8]: communication-practice.md#Misunderstandings
+[9]: communication-guide.md
+[A]: https://xenproject.org/developers/governance/#decisions
+[B]: https://xenproject.org/developers/governance/#expressingopinion
+[C]: https://xenproject.org/developers/governance/#leadership
+[D]: https://xenproject.org/developers/governance/#conflict
-- 
2.13.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3 5/7] Add Code Review Guide

[Julien Grall]

Hi Lars,

On 12/12/2019 21:14, Lars Kurth wrote:
> +### 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

Just a small typo: I think you meant "helps" rather than "he;ps".

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3 5/7] Add Code Review Guide

[Lars Kurth]

On 18/12/2019, 14:29, "Julien Grall" <julien@xen.org> wrote:

    Hi Lars,
    
    On 12/12/2019 21:14, Lars Kurth wrote:
    > +### 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
    
    Just a small typo: I think you meant "helps" rather than "he;ps".
    
    Cheers,
    
Thank you: fixed in my working copy.

One thing which occurred to me for reviews like these, where there is no ACK's or Reviewed-by's is that I don't actually know whether you as reviewer is otherwise happy with the remainder of patch.
Normally the ACKed-by or Reviewed-by is a signal that it is

I am assuming it is, but I think it may be worthwhile pointing this out in the document, that unless stated otherwise, the reviewer is happy with the patch

Regards
Lars 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3 5/7] Add Code Review Guide

[Jan Beulich]

On 18.12.2019 18:09, Lars Kurth wrote:
> 
> 
> On 18/12/2019, 14:29, "Julien Grall" <julien@xen.org> wrote:
> 
>     Hi Lars,
>     
>     On 12/12/2019 21:14, Lars Kurth wrote:
>     > +### 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
>     
>     Just a small typo: I think you meant "helps" rather than "he;ps".
>     
>     Cheers,
>     
> Thank you: fixed in my working copy.
> 
> One thing which occurred to me for reviews like these, where there is no ACK's or Reviewed-by's is that I don't actually know whether you as reviewer is otherwise happy with the remainder of patch.
> Normally the ACKed-by or Reviewed-by is a signal that it is
> 
> I am assuming it is, but I think it may be worthwhile pointing this out in the document, that unless stated otherwise, the reviewer is happy with the patch

I don't think there should ever be such an implication. Afaic there
are patches I comment upon, but that I either don't feel qualified
to give an ack/R-b on, or that I simply don't want to for whatever
reason. At best, no other comment (as in the above example) may be
taken as "I don't object".

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3 5/7] Add Code Review Guide

[Lars Kurth]

> On 19 Dec 2019, at 09:56, Jan Beulich <jbeulich@suse.com> wrote:
> 
> On 18.12.2019 18:09, Lars Kurth wrote:
>> 
>> 
>> On 18/12/2019, 14:29, "Julien Grall" <julien@xen.org> wrote:
>> 
>>    Hi Lars,
>> 
>>    On 12/12/2019 21:14, Lars Kurth wrote:
>>> +### 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
>> 
>>    Just a small typo: I think you meant "helps" rather than "he;ps".
>> 
>>    Cheers,
>> 
>> Thank you: fixed in my working copy.
>> 
>> One thing which occurred to me for reviews like these, where there is no ACK's or Reviewed-by's is that I don't actually know whether you as reviewer is otherwise happy with the remainder of the patch.
>> Normally the ACKed-by or Reviewed-by is a signal that it is
>> 
>> I am assuming it is, but I think it may be worthwhile pointing this out in the document, that unless stated otherwise, the reviewer is happy with the patch
> 
> I don't think there should ever be such an implication. Afaic there
> are patches I comment upon, but that I either don't feel qualified
> to give an ack/R-b on, or that I simply don't want to for whatever
> reason. At best, no other comment (as in the above example) may be
> taken as "I don't object".


If that is the case, would there be a reasonable convention to make this clear? 

In a nutshell, in such a review the possible interpretations are
* I reviewed, but didn't feel qualified to do the rest
* I reviewed, but did not get round to give it full attention
* I reviewed, but before I make a final decision want to look at the next version
...
* I reviewed and don't object the rest
* I reviewed and agreed with the rest 
The latter two are equivalent to Ack/R-b

That is quite a large range of possibilities and kind of leaves the author guessing what state the review is in

Regards
Lars




_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3 5/7] Add Code Review Guide

[Jan Beulich]

On 19.12.2019 11:03, Lars Kurth wrote:
> 
> 
>> On 19 Dec 2019, at 09:56, Jan Beulich <jbeulich@suse.com> wrote:
>>
>> On 18.12.2019 18:09, Lars Kurth wrote:
>>>
>>>
>>> On 18/12/2019, 14:29, "Julien Grall" <julien@xen.org> wrote:
>>>
>>>    Hi Lars,
>>>
>>>    On 12/12/2019 21:14, Lars Kurth wrote:
>>>> +### 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
>>>
>>>    Just a small typo: I think you meant "helps" rather than "he;ps".
>>>
>>>    Cheers,
>>>
>>> Thank you: fixed in my working copy.
>>>
>>> One thing which occurred to me for reviews like these, where there is no ACK's or Reviewed-by's is that I don't actually know whether you as reviewer is otherwise happy with the remainder of the patch.
>>> Normally the ACKed-by or Reviewed-by is a signal that it is
>>>
>>> I am assuming it is, but I think it may be worthwhile pointing this out in the document, that unless stated otherwise, the reviewer is happy with the patch
>>
>> I don't think there should ever be such an implication. Afaic there
>> are patches I comment upon, but that I either don't feel qualified
>> to give an ack/R-b on, or that I simply don't want to for whatever
>> reason. At best, no other comment (as in the above example) may be
>> taken as "I don't object".
> 
> 
> If that is the case, would there be a reasonable convention to make this clear? 
> 
> In a nutshell, in such a review the possible interpretations are
> * I reviewed, but didn't feel qualified to do the rest
> * I reviewed, but did not get round to give it full attention
> * I reviewed, but before I make a final decision want to look at the next version
> ...
> * I reviewed and don't object the rest
> * I reviewed and agreed with the rest 
> The latter two are equivalent to Ack/R-b
> 
> That is quite a large range of possibilities and kind of leaves the author guessing what state the review is in

Well, I though the convention is to give A-b / R-b explicitly. In
a few overly ambiguous cases we tend to simply ask back whether a
given reply can be transformed into a tag. I don't see any need
for further formalization here.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3 5/7] Add Code Review Guide

[Ian Jackson]

Lars Kurth writes ("Re: [PATCH v3 5/7] Add Code Review Guide"):
> In a nutshell, in such a review the possible interpretations are
> * I reviewed, but didn't feel qualified to do the rest
> * I reviewed, but did not get round to give it full attention
> * I reviewed, but before I make a final decision want to look at the next version
> ...
> * I reviewed and don't object the rest
> * I reviewed and agreed with the rest 
> The latter two are equivalent to Ack/R-b
> 
> That is quite a large range of possibilities and kind of leaves the author guessing what state the review is in

There are only three possibilities:

Acked-by:
  I hereby bless this with my maintainer powers.
  I may or may not have reviewed it.  The body text may contain
  more information about that.

Reviewed-by:
  I have reviewed this to my own satisfaction and this mail contains
  all the comments I have on it.  If I am a maintainer, I hereby
  bless it with my maintainer powers.  (This is a weird use of the
  word "Reviewed" since in usual usage one can review something and
  end up disapproving of it; nevertheless this is the convention.)

In both of the above cases, additionally
  If my approval is conditional (eg on changes) this will be
  stated explicitly in the body text of my message.

Neither of the above:
  I have read at least some of this and felt motivated to make some
  observations.  If I have reviewed it properly this would usually be
  stated in the body text.  If I am taking enough of an interest in
  this patch, the body text may also say what I think the current
  state and/or next steps should be.  In any case, I do *not* bless
  this patch (in its current form) with any maintainer powers I may
  have.

IOW: if you do not get an A-b or R-b, then the person writing is not
necessarily making any of the statements you posit which start "I
reviewed".

Having said that it is a good idea for people commenting on patches to
make clear what they have and haven't done.  I often start a message
with "Thanks for this patch which I have reviewed.  I have some
comments" or words to that effect.

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3 5/7] Add Code Review Guide

[Ian Jackson]

Ian Jackson writes ("Re: [PATCH v3 5/7] Add Code Review Guide"):
> Lars Kurth writes ("Re: [PATCH v3 5/7] Add Code Review Guide"):
> > In a nutshell, in such a review the possible interpretations are
> > * I reviewed, but didn't feel qualified to do the rest
> > * I reviewed, but did not get round to give it full attention
> > * I reviewed, but before I make a final decision want to look at the next version
> > ...
> > * I reviewed and don't object the rest
> > * I reviewed and agreed with the rest 
> > The latter two are equivalent to Ack/R-b
> > 
> > That is quite a large range of possibilities and kind of leaves the author guessing what state the review is in
...
> IOW: if you do not get an A-b or R-b, then the person writing is not
> necessarily making any of the statements you posit which start "I
> reviewed".

The other point it occurs to me to make here is that whether someone
reviewed something is not of formal importance to the process,
although it is of course very relevant information.

With my maintainer hat on I frequently approve patches which I have
not reviewed.

Likewise I sometimes read and even review in detail patches where I
have no formal authority.  My comments are then intended (in a formal
sense) as input to the maintainers' decisionmaking.

(In practice, since we are all working together to make the best
software, maintainers generally expect a submitter to address issues
raised by a review from a non-maintainer, and strong objections from
non-maintainer stakeholders usually lead to a discussion about how to
resolve matters.  This is all cooperation and common courtesy I
think.)

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3 5/7] Add Code Review Guide

[Julien Grall]

Hi Lars,

On 18/12/2019 17:09, Lars Kurth wrote:
> 
> 
> On 18/12/2019, 14:29, "Julien Grall" <julien@xen.org> wrote:
> 
>      Hi Lars,
>      
>      On 12/12/2019 21:14, Lars Kurth wrote:
>      > +### 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
>      
>      Just a small typo: I think you meant "helps" rather than "he;ps".
>      
>      Cheers,
>      
> Thank you: fixed in my working copy.
> 
> One thing which occurred to me for reviews like these, where there is no ACK's or Reviewed-by's is that I don't actually know whether you as reviewer is otherwise happy with the remainder of patch.
> Normally the ACKed-by or Reviewed-by is a signal that it is
>  > I am assuming it is, but I think it may be worthwhile pointing this 
out in the document, that unless stated otherwise, the reviewer is happy 
with the patch
I don't think you can always assume this. There are case where I don't 
give a reviewed-by yet because I want to understand the follow-up 
patches first.

I think what Ian described correspond the best to my view here. And I 
agree that we probably want to be more explicit in the review to avoid 
confusion.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel