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

[Xen-devel] [PATCH v2 0/6] 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 contains *ALL* the portions I was still going to add.
I spent a bit of time on word-smithing, but I am not a native English speaker
So there is probably time for improvement

The series also reflects the discussion in [2] and some private
discussions on IRC to identify initial members of the Xen
Project’s CoC team.

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

[1] https://www.contributor-covenant.org/version/1/4/code-of-conduct.md
[2] https://xen.markmail.org/thread/56ao2gyhpltqmrew 
[3] http://xenbits.xen.org/gitweb/?p=people/larsk/code-of-conduct.git;a=summary
[4] https://www.slideshare.net/xen_com_mgr/xpdds19-keynote-patch-review-for-nonmaintainers-george-dunlap-citrix-systems-uk-ltd
[5] http://xenbits.xen.org/gitweb/?p=people/larsk/code-of-conduct.git;a=shortlog;h=refs/heads/CoC-v2

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 (6):
  Import v1.4 of Contributor Covenant CoC
  Xen Project Code of Conduct
  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 v2 1/6] 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 v2 2/6] 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 v2 3/6] Add Communication Guide

[Lars Kurth]

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

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..4bcf440
--- /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


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

[Xen-devel] [PATCH v2 4/6] Add Code Review Guide

[Lars Kurth]

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.

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

diff --git a/code-review-guide.md b/code-review-guide.md
new file mode 100644
index 0000000..8639431
--- /dev/null
+++ b/code-review-guide.md
@@ -0,0 +1,125 @@
+# Code Review Guide
+
+This document highlights what reviewers such as maintainers and committers look
+for when reviewing your code. It sets expectations for code authors and provides
+a framework for code reviewers.
+
+This document does **not cover** the following topics:
+* [Communication Best Practice](communication-practice.md)
+* [Resolving Disagreement](resolving-disagreement.md)
+* [Patch Submission Workflow](https://wiki.xenproject.org/wiki/Submitting_Xen_Project_Patches)
+* [Managing Patch Submission with Git](https://wiki.xenproject.org/wiki/Managing_Xen_Patches_with_Git)
+
+## What we look for in Code Reviews
+When performing a code review, reviewers typically look for the following things
+
+### Is the change necessary to accomplish the goals?
+* Is it clear what the goals are?
+* Do we need to make a change, or can the goals be met with existing
+  functionality?
+
+### Architecture / Interface
+* Is this the best way to solve the problem?
+* Is this the right part of the code to modify?
+* Is this the right level of abstraction?
+* Is the interface general enough? Too general? Forward compatible?
+
+### Functionality
+* Does it do what it’s trying to do?
+* Is it doing it in the most efficient way?
+* Does it handle all the corner / error cases correctly?
+
+### Maintainability / Robustness
+* Is the code clear? Appropriately commented?
+* Does it duplicate another piece of code?
+* Does the code make hidden assumptions?
+* Does it introduce sections which need to be kept **in sync** with other sections?
+* Are there other **traps** someone modifying this code might fall into?
+
+**Note:** Sometimes you will work in areas which have identified maintainability
+and/or robustness issues. In such cases, maintainers may ask you to make additional
+changes, such that your submitted code does not make things worse or point you
+to other patches are already being worked on.
+
+### System properties
+In some areas of the code, system properties such as
+* Code size
+* Performance
+* Scalability
+* Latency
+* Complexity
+* &c
+are also important during code reviews.
+
+### Style
+* Comments, carriage returns, **snuggly braces**, &c
+* See [CODING_STYLE](https://xenbits.xenproject.org/gitweb/?p=xen.git;a=blob;f=CODING_STYLE)
+  and [tools/libxl/CODING_STYLE](https://xenbits.xenproject.org/gitweb/?p=xen.git;a=blob;f=tools/libxl/CODING_STYLE)
+* No extraneous whitespace changes
+
+### Documentation and testing
+* If there is pre-existing documentation in the tree, such as man pages, design
+  documents, etc. a contributor may be asked to update the documentation alongside
+  the change. Documentation is typically present in the
+  [docs](https://xenbits.xen.org/gitweb/?p=xen.git;a=tree;f=docs) folder.
+* When adding new features that have an impact on the end-user,
+  a contributor should include an update to the
+  [SUPPORT.md](https://xenbits.xen.org/gitweb/?p=xen.git;a=tree;f=docs) file.
+  Typically, more complex features require several patch series before it is ready to be
+  advertised in SUPPORT.md
+* When adding new features, a contributor may be asked to provide tests or
+  ensure that existing tests pass
+
+#### Testing for the Xen Project Hypervisor
+Tests are typically located in one of the following directories
+* **Unit tests**: [tools/tests](https://xenbits.xenproject.org/gitweb/?p=xen.git;a=tree;f=tools/tests)
+or [xen/test](https://xenbits.xenproject.org/gitweb/?p=xen.git;a=tree;f=xen/test)<br>
+  Unit testing is hard for a system like Xen and typically requires building a subsystem of
+  your tree. If your change can be easily unit tested, you should consider submitting tests
+  with your patch.
+* **Build and smoke test**: see [Xen GitLab CI](https://gitlab.com/xen-project/xen/pipelines)<br>
+  Runs build tests for a combination of various distros and compilers against changes
+  committed to staging. Developers can join as members and test their development
+  branches **before** submitting a patch.
+* **XTF tests** (microkernel-based tests): see [XTF](https://xenbits.xenproject.org/docs/xtf/)<br>
+  XTF has been designed to test interactions between your software and hardware.
+  It is a very useful tool for testing low level functionality and is executed as part of the
+  project's CI system. XTF can be easily executed locally on xen.git trees.
+* **osstest**: see [README](https://xenbits.xenproject.org/gitweb/?p=osstest.git;a=blob;f=README)<br>
+  Osstest is the Xen Projects automated test system, which tests basic Xen use cases on
+  a variety of different hardware. Before changes are committed, but **after** they have
+  been reviewed. A contributor’s changes **cannot be applied to master** unless the
+  tests pass this test suite. Note that XTF and other tests are also executed as part of
+  osstest.
+
+### Patch / Patch series information
+* Informative one-line changelog
+* Full changelog
+* Motivation described
+* All important technical changes mentioned
+* Changes since previous revision listed
+* Reviewed-by’s and Acked-by’s dropped if appropriate
+
+More information related to these items can be found in our
+[Patch submission Guide](https://wiki.xenproject.org/wiki/Submitting_Xen_Project_Patches).
+
+## Reviewing for Patch Authors
+
+The following presentation by George Dunlap, provides an excellent overview on how
+we do code reviews, specifically targeting non-maintainers.
+
+As a community, we would love to have more help reviewing, including from **new
+community members**. But many people
+* do not know where to start, or
+* believe that their review would not contribute much, or
+* may feel intimidated reviewing the code of more established community members
+
+The presentation demonstrates that you do not need to worry about any of these
+concerns. In addition, reviewing other people's patches helps you
+* write better patches and experience the code review process from the other side
+* and build more influence within the community over time
+
+Thus, we recommend strongly that **patch authors** read the watch the recording or
+read the slides:
+* [Patch Review for Non-Maintainers slides](https://www.slideshare.net/xen_com_mgr/xpdds19-keynote-patch-review-for-nonmaintainers-george-dunlap-citrix-systems-uk-ltd)
+* [Patch Review for Non-Maintainers recording - 20"](https://www.youtube.com/watch?v=ehZvBmrLRwg)
-- 
2.13.0


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

[Xen-devel] [PATCH v2 5/6] Add guide on Communication Best Practice

[Lars Kurth]

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

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 | 410 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 410 insertions(+)
 create mode 100644 communication-practice.md

diff --git a/communication-practice.md b/communication-practice.md
new file mode 100644
index 0000000..db9a5ef
--- /dev/null
+++ b/communication-practice.md
@@ -0,0 +1,410 @@
+# 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 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](https://youtu.be/ehZvBmrLRwg?t=834). 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 dictionart 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)[https://en.wikipedia.org/wiki/Foot_binding]
+
+> 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.
+
+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.
+
+### 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 of an issue or disagreement
+By default, every comment which is made **ought to be addressed** by the author.
+However, often reviewers note issues, which would be nice if they were addressed,
+but are not mandatory.
+
+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.
+
+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](https://xenproject.org/developers/governance/#expressingopinion)
+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. This reduces the workload on both the reviewer and patch author. However,
+reviewers should make clear that they have omitted detailed review comments and
+that these will come later.
+
+### 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
+
+### 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](https://www.wired.com/2006/02/the-secret-cause-of-flame-wars/) 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](https://xenproject.org/help/irc/) 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] (resolving-disagreement.md).
+
+### 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](https://en.wikipedia.org/wiki/Erin_Meyer). A short overview
+can be found
+[here](https://www.nsf.gov/attachments/134059/public/15LFW_WorkingWithMulticulturalTeams_LarsonC.pdf)
+[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](resolving-disagreement.md), which lays out situations that
+can lead to dead-lock and shows common patterns on how to avoid and resolve issues.
-- 
2.13.0


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

[Xen-devel] [PATCH v2 6/6] 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

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 | 146 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 146 insertions(+)
 create mode 100644 resolving-disagreement.md

diff --git a/resolving-disagreement.md b/resolving-disagreement.md
new file mode 100644
index 0000000..19aedbe
--- /dev/null
+++ b/resolving-disagreement.md
@@ -0,0 +1,146 @@
+# 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](http://www.paulgraham.com/disagree.html)**, 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 Disagreemen](https://upload.wikimedia.org/wikipedia/commons/a/a3/Graham%27s_Hierarchy_of_Disagreement-en.svg) |
+| *A representation of Graham's hierarchy of disagreement from [Loudacris](http://www.createdebate.com/user/viewprofile/Loudacris) modified by [Rocket000](https://en.wikipedia.org/wiki/User:Rocket000)* |
+
+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 patch to implement additional functionality.
+
+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](https://en.wiktionary.org/wiki/bikeshedding)
+
+Occasionally discussions about unimportant but easy-to-grasp issues can lead to
+prolonged and unproductive discussion. The best way to approach this is to
+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](https://xenproject.org/developers/governance/#informal-votes-or-surveys) or
+[lazy voting](https://xenproject.org/developers/governance/#lazyconsensus) 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 event
+* 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](communication-practice.md#Misunderstandings).
+
+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 been 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.
+
+## 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](communication-guide.md)
+for more information.
+
+## Decision making and conflict resolution in our governance
+
+Our [governance](https://xenproject.org/developers/governance/#decisions) contains
+several proven mechanisms to help with decision making and conflict resolution.
+
+See
+* [Expressing agreement and disagreement](https://xenproject.org/developers/governance/#expressingopinion)
+* [Lazy consensus / Lazy voting](https://xenproject.org/developers/governance/#lazyconsensus)
+* [Informal votes or surveys](https://xenproject.org/developers/governance/#informal-votes-or-surveys)
+* [Leadership team decisions](https://xenproject.org/developers/governance/#leadership)
+* [Conflict resolution](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 v2 5/6] Add guide on Communication Best Practice

[Jan Beulich]

On 26.09.2019 21:39, Lars Kurth wrote:
> +### 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 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.

To all of this I can't resist giving a remark that I've already given
when discussing the matter in person: I'm not sure about English, but
in German the word "Phrase" also has an, at times very, negative
meaning. When I get review feedback starting like suggested above, it
definitely feels to me more like this (the statement was added there
just for it to be there). I realize this may not always (and perhaps
even in a majority of situations) be the case, but that's how it feels
to me nevertheless. As a result I would rather rarely, if ever, start
like this (on the basis of "don't do to others what you dislike
yourself"); a case where I might do so would be when I had asked for
(or offloaded) the putting together of a particular change.

Even worse, there have been (also very recent) examples where replies
come back saying just "Thank you" (e.g. for an ack). Such certainly
get sent with good intentions, but people doing so likely overlook
the fact that there's already way too much email to read for many of
us. (The same applies to other netiquette aspects that I keep
mentioning on e.g. summits, but with apparently little to no effect:
People frequently fail to strip unnecessary context when replying,
requiring _every_ reader to scroll through a perhaps long mail just
to find that there's almost nothing of interest. People also seem to
have difficulty understanding the difference between To and Cc.)

The bottom line of this is - the "being kind to one another" aspect
of asking for this behavior needs to be weighed carefully against its
effects of unduly consuming everybody's time.

Jan

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

Re: [Xen-devel] [PATCH v2 5/6] Add guide on Communication Best Practice

[Jan Beulich]

On 26.09.2019 21:39, Lars Kurth wrote:
> +### 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.

And another remark here: Not being terse in situations like the ones
enumerated as examples above is a double waste of the reviewer's time:
They shouldn't even need to make such comments, especially not many
times for a single patch (see your mention of "overuse"). I realize
we still have no automated mechanism to check style aspects, but
anybody can easily look over their patches before submitting them.
And for an occasional issue I think a terse reply is quite reasonable
to have.

Overall I'm seeing the good intentions of this document, yet I'd still
vote at least -1 on it if it came to a vote. Following even just a
fair part of it is a considerable extra amount of time to invest in
reviews, when we already have a severe reviewing bottleneck. If I have
to judge between doing a bad (stylistically according to this doc, not
technically) review or none at all (because of time constraints), I'd
favor the former. Unless of course I'm asked to stop doing so, in
which case I'd expect whoever asks to arrange for the reviews to be
done by someone else in due course.

I'm sorry for (likely) sounding destructive here.

Jan

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

Re: [Xen-devel] [PATCH v2 5/6] Add guide on Communication Best Practice

[Lars Kurth]

On 27/09/2019, 09:59, "Jan Beulich" <jbeulich@suse.com> wrote:

    On 26.09.2019 21:39, Lars Kurth wrote:
    > +### 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 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.
    
    To all of this I can't resist giving a remark that I've already given
    when discussing the matter in person: I'm not sure about English, but
    in German the word "Phrase" also has an, at times very, negative
    meaning. When I get review feedback starting like suggested above, it
    definitely feels to me more like this (the statement was added there
    just for it to be there). I realize this may not always (and perhaps
    even in a majority of situations) be the case, but that's how it feels
    to me nevertheless. As a result I would rather rarely, if ever, start
    like this (on the basis of "don't do to others what you dislike
    yourself"); a case where I might do so would be when I had asked for
    (or offloaded) the putting together of a particular change.

I think your reply proves almost entirely the point of the article. In the
end all of this depends on communication styles (both personal and
cultural). My take to it is that there is a difference between

a) Someone you know: what ultimately will happen is that 
when you engage with someone you know and had done reviews before
you ultimately become more terse and also drop niceties.
Which is OK

b) Someone you don’t know: in that case, we should start from
a reasonable middle ground and put in a bit more effort

    Even worse, there have been (also very recent) examples where replies
    come back saying just "Thank you" (e.g. for an ack). Such certainly
    get sent with good intentions, but people doing so likely overlook
    the fact that there's already way too much email to read for many of
    us. (The same applies to other netiquette aspects that I keep
    mentioning on e.g. summits, but with apparently little to no effect:
    People frequently fail to strip unnecessary context when replying,
    requiring _every_ reader to scroll through a perhaps long mail just
    to find that there's almost nothing of interest. People also seem to
    have difficulty understanding the difference between To and Cc.)

That is a good point and I had forgotten about it
Thanks for reminding me

I can add a section on this which looks for balance in the interest
of saving your communication partner's time. Ultimately this is a
also showing a degree of thoughtfulness. 

And we can state in there things like the CC/TO list
And not to thank code reviewers for ACKs or otherwise in a 
stand-alone e-mail
    
    The bottom line of this is - the "being kind to one another" aspect
    of asking for this behavior needs to be weighed carefully against its
    effects of unduly consuming everybody's time.
    
I am fully aware of this, and was trying to approach this from this
viewpoint of trying to achieve a sensible balance

But after your comment, maybe that was not clear enough

Best Regards
Lars    

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

Re: [Xen-devel] [PATCH v2 5/6] Add guide on Communication Best Practice

[Lars Kurth]

On 27/09/2019, 10:14, "Jan Beulich" <jbeulich@suse.com> wrote:

    On 26.09.2019 21:39, Lars Kurth wrote:
    > +### 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.
    
    And another remark here: Not being terse in situations like the ones
    enumerated as examples above is a double waste of the reviewer's time:
    They shouldn't even need to make such comments, especially not many
    times for a single patch (see your mention of "overuse"). I realize
    we still have no automated mechanism to check style aspects, but
    anybody can easily look over their patches before submitting them.
    And for an occasional issue I think a terse reply is quite reasonable
    to have.

At the end of the day, none if this is mandatory. The document also
has two audiences
* Authors which get review feedback : for example by just having
this section in there it helps 

I added this section primarily because we do see the occasional
very terse review style and even I think sometimes: wow, that comes
across as harsh. But I also know, that it isn't intentional and that
I have a fairly thick skin. And it is not exclusive to typos and minor issues.

What I was trying to do in this document is to provide
a guide which shows the different patterns from both perspectives.
I hope I succeeded in this, but I believe that you primarily
reviewed the document from the view point of a code reviewer.
    
    Overall I'm seeing the good intentions of this document, yet I'd still
    vote at least -1 on it if it came to a vote. Following even just a
    fair part of it is a considerable extra amount of time to invest in
    reviews, when we already have a severe reviewing bottleneck. If I have
    to judge between doing a bad (stylistically according to this doc, not
    technically) review or none at all (because of time constraints), I'd
    favor the former. Unless of course I'm asked to stop doing so, in
    which case I'd expect whoever asks to arrange for the reviews to be
    done by someone else in due course.

First of all: this would be our gold standard and as pointed out earlier
So it is intended to provide the tools to do better: for example, from 
my point of view if you followed some of it for example for newcomers
and sparingly when you feel it is right, that would already be a 
win-win. Also, consider that a more positive tone should also have the
effect that there may be less unnecessary discussion. I think this
is particularly true when it comes to the sections on fact-based 
responses vs. some which are unclear. Unfortunately, I don't have data
on this to prove it.
    
Can I maybe get you to reconsider and re-review the next version from the
view point of an author and maybe make suggestions on how to create more
balance

    I'm sorry for (likely) sounding destructive here.

I don't see this your feedback as destructive and do hope that I
can convince you that documenting some of the patterns which
happen on the list are in fact a net-positive

Regards
Lars 

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

Re: [Xen-devel] [PATCH v2 5/6] Add guide on Communication Best Practice

[Lars Kurth]

On 27/09/2019, 11:17, "Lars Kurth" <lars.kurth@citrix.com> wrote:

    
    
    On 27/09/2019, 10:14, "Jan Beulich" <jbeulich@suse.com> wrote:
    
        On 26.09.2019 21:39, Lars Kurth wrote:
        > +### 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.
        
        And another remark here: Not being terse in situations like the ones
        enumerated as examples above is a double waste of the reviewer's time:
        They shouldn't even need to make such comments, especially not many
        times for a single patch (see your mention of "overuse"). I realize
        we still have no automated mechanism to check style aspects, but
        anybody can easily look over their patches before submitting them.
        And for an occasional issue I think a terse reply is quite reasonable
        to have.
    
    At the end of the day, none if this is mandatory. The document also
    has two audiences
    * Authors which get review feedback : for example by just having
    this section in there it helps 

This was meant to read: it helps set expectations and promotes 
understanding for some of the patterns used
    
    I added this section primarily because we do see the occasional
    very terse review style and even I think sometimes: wow, that comes
    across as harsh. But I also know, that it isn't intentional and that
    I have a fairly thick skin. And it is not exclusive to typos and minor issues.
    
 Lars

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

Re: [Xen-devel] [PATCH v2 5/6] Add guide on Communication Best Practice

[Jan Beulich]

On 27.09.2019 12:17, Lars Kurth wrote:
> Can I maybe get you to reconsider and re-review the next version from the
> view point of an author and maybe make suggestions on how to create more
> balance

I'll certainly make an attempt.

Jan

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

Re: [Xen-devel] [PATCH v2 1/6] Import v1.4 of Contributor Covenant CoC

[George Dunlap]

On 9/26/19 8:39 PM, Lars Kurth wrote:
> 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.

This is relatively minor, but I don't feel quite comfortable with the
wording.  "pledge to make it a harassment-free experience" to me implies
that we pledge that *nobody will ever experience harassment*.  I don't
think that's something we can deliver, any more than a government can
promise there will be zero crime.  I think we could promise to
*maintain* a harassment-free experience, which implies things to a
restoring harassment-free state after it's been broken.

Everything else looks good.

 -George

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

Re: [Xen-devel] [PATCH v2 1/6] Import v1.4 of Contributor Covenant CoC

[Lars Kurth]

On 07/10/2019, 12:06, "George Dunlap" <george.dunlap@citrix.com> wrote:

    On 9/26/19 8:39 PM, Lars Kurth wrote:
    > 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.
    
    This is relatively minor, but I don't feel quite comfortable with the
    wording.  "pledge to make it a harassment-free experience" to me implies
    that we pledge that *nobody will ever experience harassment*.  I don't
    think that's something we can deliver, any more than a government can
    promise there will be zero crime.  I think we could promise to
    *maintain* a harassment-free experience, which implies things to a
    restoring harassment-free state after it's been broken.
    
    Everything else looks good.
    
Could you come up with an alternative concrete text proposal? 

My take-away is that you say we should use
* "pledge to strive to make ..." or 
* "pledge to try their best to make ..." or
* "strive to make ..." 
in which case we may also need to change "The Pledge".

On the other hand: the rest of the document does clearly lay out that
what we promise is to deal with incidents in an effective manner.
And by the mere inclusion of mechanism to do this, it is actually clear
that we can't guarantee that *nobody will ever experience harassment*.

I guess it comes down to subtleties of how pledge, promise, strive, ...
differ

Regards
Lars
 

    

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

Re: [Xen-devel] [PATCH v2 5/6] Add guide on Communication Best Practice

[George Dunlap]

On 9/26/19 8:39 PM, Lars Kurth wrote:
> +investigate the practice foot-binding, it is hard to disagree with the dictionart entry.

Typo: dictionary

 -George

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

Re: [Xen-devel] [PATCH v2 5/6] Add guide on Communication Best Practice

[George Dunlap]

On 9/27/19 10:14 AM, Jan Beulich wrote:
> On 26.09.2019 21:39, Lars Kurth wrote:
>> +### 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.
> 
> And another remark here: Not being terse in situations like the ones
> enumerated as examples above is a double waste of the reviewer's time:

FWIW I don't think this document prohibits terse replies.  It points out
that they can come across as unfriendly, and they can be perceived as
rude in some cultures; both of which are true.  It then *recommends*
that reviewers compensate for it in a review opening (i.e., once per
patch / series) which expresses appreciation; which is both helpful and
relatively low cost.

The point of the opening is to set the tone.  If you start out with
something positive, and ends with "thanks", then a long series of terse
comments is more likely to be read as simply being efficient.  If the
entire review consists of nothing but criticism or terse comments, it's
more likely to be read as annoyance on the part of the reviewer.

> They shouldn't even need to make such comments, especially not many
> times for a single patch (see your mention of "overuse").  I realize
> we still have no automated mechanism to check style aspects, but
> anybody can easily look over their patches before submitting them.
> And for an occasional issue I think a terse reply is quite reasonable
> to have.

This sort of sounds like you are *intending* to express annoyance?

If so, that's a slightly different question than what this section is
addressing. :-)

 -George

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

Re: [Xen-devel] [PATCH v2 5/6] Add guide on Communication Best Practice

[Jan Beulich]

On 07.10.2019 18:13, George Dunlap wrote:
> On 9/27/19 10:14 AM, Jan Beulich wrote:
>> On 26.09.2019 21:39, Lars Kurth wrote:
>>> +### 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.
>>
>> And another remark here: Not being terse in situations like the ones
>> enumerated as examples above is a double waste of the reviewer's time:
> 
> FWIW I don't think this document prohibits terse replies.  It points out
> that they can come across as unfriendly, and they can be perceived as
> rude in some cultures; both of which are true.  It then *recommends*
> that reviewers compensate for it in a review opening (i.e., once per
> patch / series) which expresses appreciation; which is both helpful and
> relatively low cost.
> 
> The point of the opening is to set the tone.  If you start out with
> something positive, and ends with "thanks", then a long series of terse
> comments is more likely to be read as simply being efficient.  If the
> entire review consists of nothing but criticism or terse comments, it's
> more likely to be read as annoyance on the part of the reviewer.
> 
>> They shouldn't even need to make such comments, especially not many
>> times for a single patch (see your mention of "overuse").  I realize
>> we still have no automated mechanism to check style aspects, but
>> anybody can easily look over their patches before submitting them.
>> And for an occasional issue I think a terse reply is quite reasonable
>> to have.
> 
> This sort of sounds like you are *intending* to express annoyance?

Implicitly by being terse, yes. I've been trying to avoid expressing
such explicitly, but I have to admit there are (luckily only few)
cases where I find it pretty hard to stay away.

Jan

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

Re: [Xen-devel] [PATCH v2 0/6] Code of Conduct + Extra Guides and Best Practices

[Felipe Huici]

Hi Lars,

Sorry for the late response, the Unikraft team is certainly happy to support this code of conduct.

Thanks,

-- Felipe

On 27.09.19, 06:22, "Xen-devel on behalf of Lars Kurth" <xen-devel-bounces@lists.xenproject.org on behalf of lars.kurth@xenproject.org> wrote:

    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 contains *ALL* the portions I was still going to add.
    I spent a bit of time on word-smithing, but I am not a native English speaker
    So there is probably time for improvement
    
    The series also reflects the discussion in [2] and some private
    discussions on IRC to identify initial members of the Xen
    Project’s CoC team.
    
    For convenience of review and in line with other policy documents
    I created a git repository at [3]. This series can be found at [5].
    
    [1] https://www.contributor-covenant.org/version/1/4/code-of-conduct.md
    [2] https://xen.markmail.org/thread/56ao2gyhpltqmrew 
    [3] http://xenbits.xen.org/gitweb/?p=people/larsk/code-of-conduct.git;a=summary
    [4] https://www.slideshare.net/xen_com_mgr/xpdds19-keynote-patch-review-for-nonmaintainers-george-dunlap-citrix-systems-uk-ltd
    [5] http://xenbits.xen.org/gitweb/?p=people/larsk/code-of-conduct.git;a=shortlog;h=refs/heads/CoC-v2
    
    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 (6):
      Import v1.4 of Contributor Covenant CoC
      Xen Project Code of Conduct
      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 mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v2 4/6] Add Code Review Guide

[Stefano Stabellini]

[-- Attachment #1: Type: text/plain, Size: 7965 bytes --]

On Thu, 26 Sep 2019, Lars Kurth wrote:
> 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.

I think the document is missing a couple of things:

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

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


> Signed-off-by: Lars Kurth <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 | 125 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 125 insertions(+)
>  create mode 100644 code-review-guide.md
> 
> diff --git a/code-review-guide.md b/code-review-guide.md
> new file mode 100644
> index 0000000..8639431
> --- /dev/null
> +++ b/code-review-guide.md
> @@ -0,0 +1,125 @@
> +# Code Review Guide
> +
> +This document highlights what reviewers such as maintainers and committers look
> +for when reviewing your code. It sets expectations for code authors and provides
> +a framework for code reviewers.
> +
> +This document does **not cover** the following topics:
> +* [Communication Best Practice](communication-practice.md)
> +* [Resolving Disagreement](resolving-disagreement.md)
> +* [Patch Submission Workflow](https://wiki.xenproject.org/wiki/Submitting_Xen_Project_Patches)
> +* [Managing Patch Submission with Git](https://wiki.xenproject.org/wiki/Managing_Xen_Patches_with_Git)
> +
> +## What we look for in Code Reviews
> +When performing a code review, reviewers typically look for the following things
> +
> +### Is the change necessary to accomplish the goals?
> +* Is it clear what the goals are?
> +* Do we need to make a change, or can the goals be met with existing
> +  functionality?
> +
> +### Architecture / Interface
> +* Is this the best way to solve the problem?
> +* Is this the right part of the code to modify?
> +* Is this the right level of abstraction?
> +* Is the interface general enough? Too general? Forward compatible?
> +
> +### Functionality
> +* Does it do what it’s trying to do?
> +* Is it doing it in the most efficient way?
> +* Does it handle all the corner / error cases correctly?
> +
> +### Maintainability / Robustness
> +* Is the code clear? Appropriately commented?
> +* Does it duplicate another piece of code?
> +* Does the code make hidden assumptions?
> +* Does it introduce sections which need to be kept **in sync** with other sections?
> +* Are there other **traps** someone modifying this code might fall into?
> +
> +**Note:** Sometimes you will work in areas which have identified maintainability
> +and/or robustness issues. In such cases, maintainers may ask you to make additional
> +changes, such that your submitted code does not make things worse or point you
> +to other patches are already being worked on.
> +
> +### System properties
> +In some areas of the code, system properties such as
> +* Code size
> +* Performance
> +* Scalability
> +* Latency
> +* Complexity
> +* &c
> +are also important during code reviews.
> +
> +### Style
> +* Comments, carriage returns, **snuggly braces**, &c
> +* See [CODING_STYLE](https://xenbits.xenproject.org/gitweb/?p=xen.git;a=blob;f=CODING_STYLE)
> +  and [tools/libxl/CODING_STYLE](https://xenbits.xenproject.org/gitweb/?p=xen.git;a=blob;f=tools/libxl/CODING_STYLE)
> +* No extraneous whitespace changes
> +
> +### Documentation and testing
> +* If there is pre-existing documentation in the tree, such as man pages, design
> +  documents, etc. a contributor may be asked to update the documentation alongside
> +  the change. Documentation is typically present in the
> +  [docs](https://xenbits.xen.org/gitweb/?p=xen.git;a=tree;f=docs) folder.
> +* When adding new features that have an impact on the end-user,
> +  a contributor should include an update to the
> +  [SUPPORT.md](https://xenbits.xen.org/gitweb/?p=xen.git;a=tree;f=docs) file.
> +  Typically, more complex features require several patch series before it is ready to be
> +  advertised in SUPPORT.md
> +* When adding new features, a contributor may be asked to provide tests or
> +  ensure that existing tests pass
> +
> +#### Testing for the Xen Project Hypervisor
> +Tests are typically located in one of the following directories
> +* **Unit tests**: [tools/tests](https://xenbits.xenproject.org/gitweb/?p=xen.git;a=tree;f=tools/tests)
> +or [xen/test](https://xenbits.xenproject.org/gitweb/?p=xen.git;a=tree;f=xen/test)<br>
> +  Unit testing is hard for a system like Xen and typically requires building a subsystem of
> +  your tree. If your change can be easily unit tested, you should consider submitting tests
> +  with your patch.
> +* **Build and smoke test**: see [Xen GitLab CI](https://gitlab.com/xen-project/xen/pipelines)<br>
> +  Runs build tests for a combination of various distros and compilers against changes
> +  committed to staging. Developers can join as members and test their development
> +  branches **before** submitting a patch.
> +* **XTF tests** (microkernel-based tests): see [XTF](https://xenbits.xenproject.org/docs/xtf/)<br>
> +  XTF has been designed to test interactions between your software and hardware.
> +  It is a very useful tool for testing low level functionality and is executed as part of the
> +  project's CI system. XTF can be easily executed locally on xen.git trees.
> +* **osstest**: see [README](https://xenbits.xenproject.org/gitweb/?p=osstest.git;a=blob;f=README)<br>
> +  Osstest is the Xen Projects automated test system, which tests basic Xen use cases on
> +  a variety of different hardware. Before changes are committed, but **after** they have
> +  been reviewed. A contributor’s changes **cannot be applied to master** unless the
> +  tests pass this test suite. Note that XTF and other tests are also executed as part of
> +  osstest.
> +
> +### Patch / Patch series information
> +* Informative one-line changelog
> +* Full changelog
> +* Motivation described
> +* All important technical changes mentioned
> +* Changes since previous revision listed
> +* Reviewed-by’s and Acked-by’s dropped if appropriate
> +
> +More information related to these items can be found in our
> +[Patch submission Guide](https://wiki.xenproject.org/wiki/Submitting_Xen_Project_Patches).
> +
> +## Reviewing for Patch Authors
> +
> +The following presentation by George Dunlap, provides an excellent overview on how
> +we do code reviews, specifically targeting non-maintainers.
> +
> +As a community, we would love to have more help reviewing, including from **new
> +community members**. But many people
> +* do not know where to start, or
> +* believe that their review would not contribute much, or
> +* may feel intimidated reviewing the code of more established community members
> +
> +The presentation demonstrates that you do not need to worry about any of these
> +concerns. In addition, reviewing other people's patches helps you
> +* write better patches and experience the code review process from the other side
> +* and build more influence within the community over time
> +
> +Thus, we recommend strongly that **patch authors** read the watch the recording or
> +read the slides:
> +* [Patch Review for Non-Maintainers slides](https://www.slideshare.net/xen_com_mgr/xpdds19-keynote-patch-review-for-nonmaintainers-george-dunlap-citrix-systems-uk-ltd)
> +* [Patch Review for Non-Maintainers recording - 20"](https://www.youtube.com/watch?v=ehZvBmrLRwg)
> -- 
> 2.13.0
> 

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

Re: [Xen-devel] [PATCH v2 6/6] Added Resolving Disagreement

[Stefano Stabellini]

On Thu, 26 Sep 2019, Lars Kurth wrote:
> From: Lars Kurth <lars.kurth@citrix.com>
> 
> This guide provides Best Practice on identifying and resolving
> common classes of disagreement
> 
> 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 | 146 ++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 146 insertions(+)
>  create mode 100644 resolving-disagreement.md
> 
> diff --git a/resolving-disagreement.md b/resolving-disagreement.md
> new file mode 100644
> index 0000000..19aedbe
> --- /dev/null
> +++ b/resolving-disagreement.md
> @@ -0,0 +1,146 @@
> +# 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](http://www.paulgraham.com/disagree.html)**, 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 Disagreemen](https://upload.wikimedia.org/wikipedia/commons/a/a3/Graham%27s_Hierarchy_of_Disagreement-en.svg) |
                             ^ Disagreement

This is a NIT but in a few places in this series you go over the
original line length.


> +| *A representation of Graham's hierarchy of disagreement from [Loudacris](http://www.createdebate.com/user/viewprofile/Loudacris) modified by [Rocket000](https://en.wikipedia.org/wiki/User:Rocket000)* |
> +
> +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 patch to implement additional functionality.
                                       ^ a patch                      ^ 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.

I think we should mention somewhere here that it is recommended for
reviewers to be explicit about whether a request is optional or whether
it is a requirement.

For instance: "I think it would be good if X also did Y" doesn't say if
it is optional (future work) or it is actually required as part of this
series. More explicit word choices are preferable, such as:

"I think it would be good if X also did Y, not a requirement but good to
have."

"I think it would be good if X also did Y and it should be part of this
series."

It helps make the communication with the author more effective,
especially in this kind of situations.


> +## Issue: [Bikeshedding](https://en.wiktionary.org/wiki/bikeshedding)
> +
> +Occasionally discussions about unimportant but easy-to-grasp issues can lead to
> +prolonged and unproductive discussion. The best way to approach this is to
                              ^ discussions


> +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](https://xenproject.org/developers/governance/#informal-votes-or-surveys) or
> +[lazy voting](https://xenproject.org/developers/governance/#lazyconsensus) 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 event
                                                    ^ 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](communication-practice.md#Misunderstandings).
> +
> +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 been found. At the end
                                                            ^ you mean "has to be found?"


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

I think there is something else we should say on this topic. There is a
category of things which could be done in multiple ways and it is not
overtly obvious which one is best. It is done to the maintainer and the
author personal styles. It is easy to disagree on that.

I think a good recommendation would be for the contributor to try to
follow the maintainers requests, even if they could be considered
"style", trusting their experience on the matter. And a good
recommendation for the maintainer would be to try to let the contributor
have freedom of implementation choice on things that don't make a
significant difference.


> +## 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](communication-guide.md)
> +for more information.
> +
> +## Decision making and conflict resolution in our governance
> +
> +Our [governance](https://xenproject.org/developers/governance/#decisions) contains
> +several proven mechanisms to help with decision making and conflict resolution.
> +
> +See
> +* [Expressing agreement and disagreement](https://xenproject.org/developers/governance/#expressingopinion)
> +* [Lazy consensus / Lazy voting](https://xenproject.org/developers/governance/#lazyconsensus)
> +* [Informal votes or surveys](https://xenproject.org/developers/governance/#informal-votes-or-surveys)
> +* [Leadership team decisions](https://xenproject.org/developers/governance/#leadership)
> +* [Conflict resolution](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 v2 5/6] Add guide on Communication Best Practice

[Stefano Stabellini]

[-- Attachment #1: Type: text/plain, Size: 24556 bytes --]

On Thu, 26 Sep 2019, Lars Kurth wrote:
> 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
> 
> 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 | 410 ++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 410 insertions(+)
>  create mode 100644 communication-practice.md
> 
> diff --git a/communication-practice.md b/communication-practice.md
> new file mode 100644
> index 0000000..db9a5ef
> --- /dev/null
> +++ b/communication-practice.md
> @@ -0,0 +1,410 @@
> +# 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 top of a patch, as addressing code review comments typically requires
                 ^ the top


> +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](https://youtu.be/ehZvBmrLRwg?t=834). 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 dictionart 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.
                       ^ remove ,

> +
> +Compare this to the (Wikipedia entry)[https://en.wikipedia.org/wiki/Foot_binding]
> +
> +> 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.
> +
> +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.
> +
> +### 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 of an issue or disagreement
> +By default, every comment which is made **ought to be addressed** by the author.
> +However, often reviewers note issues, which would be nice if they were addressed,
> +but are not mandatory.
> +
> +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.
>
> +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](https://xenproject.org/developers/governance/#expressingopinion)
> +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

It is not just about the willingness of somebody to argue a point, which
is the important thing when voting. During code reviews it is perfectly
fine to make suggestions which are just optional for multiple reasons,
including that they might be too taxing for the contributor.

So, I think we should add that it would be best to use words that make it
clear whether something is optional or whether it is required, see my
reply to patch #6, I wrote an example there.



> +### 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. This reduces the workload on both the reviewer and patch author. However,
> +reviewers should make clear that they have omitted detailed review comments and
> +that these will come later.

Maybe we want to expand on this a bit. Not all series are based on
flawed assumptions, but all series have different class of changes that
are required for acceptance, from major code modifications to minor code
style fixes.

I think we should say that it is good practice to ask for any major
changes early on, during the first or second iteration of the series.
It would be best to avoid asking for major changes at v9 if possible.


Something else which is missing in this document, and it is purely for
reviewers, is to be careful doing reviews late in the cycle when another
maintainer/reviewer has already provided feedback on the series multiple
times previously. For instance, if reviewer R1 has been doing reviews
from the first version of the series and contributor C has been
addressing all comments, it would be best if reviewer R2 didn't come in
providing detailed feedback months later at v5, unless their requests
are actually strictly necessary (i.e. they spotted a bug). The main
reason is that it is difficult not to let your own personal style (code
style, the way to lay out the code) sip through review comments, and it
can cause double-effort for the author if he/she already made changes
according R1's personal style. However, in general, it would be best to
limit "personal style" requests for changes anyway, see my comment to
patch #6.



> +### 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
> +
> +### 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](https://www.wired.com/2006/02/the-secret-cause-of-flame-wars/) 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](https://xenproject.org/help/irc/) 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] (resolving-disagreement.md).
> +
> +### 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](https://en.wikipedia.org/wiki/Erin_Meyer). A short overview
> +can be found
> +[here](https://www.nsf.gov/attachments/134059/public/15LFW_WorkingWithMulticulturalTeams_LarsonC.pdf)
> +[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](resolving-disagreement.md), which lays out 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

Re: [Xen-devel] [PATCH v2 5/6] Add guide on Communication Best Practice

[Stefano Stabellini]

On Fri, 27 Sep 2019, Jan Beulich wrote:
> On 26.09.2019 21:39, Lars Kurth wrote:
> > +### 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.
> 
> And another remark here: Not being terse in situations like the ones
> enumerated as examples above is a double waste of the reviewer's time:
> They shouldn't even need to make such comments, especially not many
> times for a single patch (see your mention of "overuse"). I realize
> we still have no automated mechanism to check style aspects, but
> anybody can easily look over their patches before submitting them.
> And for an occasional issue I think a terse reply is quite reasonable
> to have.
> 
> Overall I'm seeing the good intentions of this document, yet I'd still
> vote at least -1 on it if it came to a vote. Following even just a
> fair part of it is a considerable extra amount of time to invest in
> reviews, when we already have a severe reviewing bottleneck. If I have
> to judge between doing a bad (stylistically according to this doc, not
> technically) review or none at all (because of time constraints), I'd
> favor the former. Unless of course I'm asked to stop doing so, in
> which case I'd expect whoever asks to arrange for the reviews to be
> done by someone else in due course.

Reading the document, I think Jan has a point that it gives the
impression that following the suggestions would take significant
efforts, while actually I don't think Lars meant it that way at all, and
I don't think it should be the case either.

Maybe we should highlight and encourage "clarity" instead of "verbosity"
of the communication, and encourage "expressing appreciation" to
newcomers, not necessarily to seasoned contributors.

The ultimate goal of this document is actually to *reduce* our overall
efforts by making our communication more efficient, not to increase
efforts. Maybe it is worth saying this too.

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

Re: [Xen-devel] [PATCH v2 4/6] Add Code Review Guide

[Jan Beulich]

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

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

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

Jan

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

Re: [Xen-devel] [PATCH v2 6/6] Added Resolving Disagreement

[Jan Beulich]

On 28.11.2019 01:56, Stefano Stabellini wrote:
> On Thu, 26 Sep 2019, Lars Kurth wrote:
>> +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.
> 
> I think we should mention somewhere here that it is recommended for
> reviewers to be explicit about whether a request is optional or whether
> it is a requirement.
> 
> For instance: "I think it would be good if X also did Y" doesn't say if
> it is optional (future work) or it is actually required as part of this
> series. More explicit word choices are preferable, such as:
> 
> "I think it would be good if X also did Y, not a requirement but good to
> have."
> 
> "I think it would be good if X also did Y and it should be part of this
> series."

I think without it being made explicit that something is optional,
the assumption should be that it isn't. I.e. in the first example
I agree with the idea to have something after the comma, but in
the second example I think the extra wording is a waste of effort.

> I think there is something else we should say on this topic. There is a
> category of things which could be done in multiple ways and it is not
> overtly obvious which one is best. It is done to the maintainer and the
> author personal styles. It is easy to disagree on that.
> 
> I think a good recommendation would be for the contributor to try to
> follow the maintainers requests, even if they could be considered
> "style", trusting their experience on the matter. And a good
> recommendation for the maintainer would be to try to let the contributor
> have freedom of implementation choice on things that don't make a
> significant difference.

I think we try to, but I also think we suffer from too little
clear documentation on e.g. style aspects. Attempts on my part
to address this have mostly (not entirely) lead no-where (lack of
feedback on proposed patches to ./CODING_STYLE). So for the time
being there are (many) aspects where we have de-facto expectations
that aren't written down anywhere, with the result of (in a subset
of cases) disagreement on what the perceived de-facto standard
actually is.

Jan

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

Re: [Xen-devel] [PATCH v2 4/6] Add Code Review Guide

[Lars Kurth]

On 28/11/2019, 04:09, "Jan Beulich" <jbeulich@suse.com> wrote:

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

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

Lars
    

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

Re: [Xen-devel] [PATCH v2 4/6] Add Code Review Guide

[Jan Beulich]

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

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

Jan

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

Re: [Xen-devel] [PATCH v2 4/6] Add Code Review Guide

[Lars Kurth]

On 28/11/2019, 07:37, "Jan Beulich" <jbeulich@suse.com> wrote:

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

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

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

Lars

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

Re: [Xen-devel] [PATCH v2 4/6] Add Code Review Guide

[Rich Persaud]

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

To make this actionable, we could have:

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

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

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

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

Re: [Xen-devel] [PATCH v2 4/6] Add Code Review Guide

[Stefano Stabellini]

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

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

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



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

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

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

Re: [Xen-devel] [MirageOS-devel] [PATCH v2 4/6] Add Code Review Guide

[Rich Persaud]

[-- Attachment #1.1: Type: text/plain, Size: 2990 bytes --]

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

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

  https://devopedia.org/shift-left

Rich


[-- Attachment #1.2: Type: text/html, Size: 4292 bytes --]

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

Re: [Xen-devel] [PATCH v2 6/6] Added Resolving Disagreement

[Stefano Stabellini]

On Thu, 28 Nov 2019, Jan Beulich wrote:
> On 28.11.2019 01:56, Stefano Stabellini wrote:
> > On Thu, 26 Sep 2019, Lars Kurth wrote:
> >> +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.
> > 
> > I think we should mention somewhere here that it is recommended for
> > reviewers to be explicit about whether a request is optional or whether
> > it is a requirement.
> > 
> > For instance: "I think it would be good if X also did Y" doesn't say if
> > it is optional (future work) or it is actually required as part of this
> > series. More explicit word choices are preferable, such as:
> > 
> > "I think it would be good if X also did Y, not a requirement but good to
> > have."
> > 
> > "I think it would be good if X also did Y and it should be part of this
> > series."
> 
> I think without it being made explicit that something is optional,
> the assumption should be that it isn't. I.e. in the first example
> I agree with the idea to have something after the comma, but in
> the second example I think the extra wording is a waste of effort.

If you are concerned about brevity, then a better example would be:

  "X should also do Y" -> required

  "It would be good if X also did Y, just optional." -> optional

I didn't want to go that far but we could even have an actually tag,
like:

  "X should also do Y [REQ]"
  "X should also do Y [OPT]"


On the default, let me premise that at the end of the day I agree that
the default should be that "it is required", because it is probably what
it makes more sense.

That said, the issue is that as human beings we tend to forget about
these things. As an example, I have been trying to apply this simple
optional/reply communication style to my own reviews in the last few
months and I still forget often to mark them appropriately.

If you forget to mark an optional suggestion as such, it might annoy the
contributor, thinking that you are requiring something that she doesn't
want to do. If we switched the default to optional, then the risk would
be that the contributor could ignore it, which is problematic for the
reviewer, although we recommend in these docs for the contributor to say
what they intend to do about optional suggestions explicitly, and also
as a reviewer I always manually check if all my comments from a previous
version of the series were addressed anyway.

I don't have a good solution to this, I am just sharing my experience.


> > I think there is something else we should say on this topic. There is a
> > category of things which could be done in multiple ways and it is not
> > overtly obvious which one is best. It is done to the maintainer and the
> > author personal styles. It is easy to disagree on that.
> > 
> > I think a good recommendation would be for the contributor to try to
> > follow the maintainers requests, even if they could be considered
> > "style", trusting their experience on the matter. And a good
> > recommendation for the maintainer would be to try to let the contributor
> > have freedom of implementation choice on things that don't make a
> > significant difference.
> 
> I think we try to, but I also think we suffer from too little
> clear documentation on e.g. style aspects. Attempts on my part
> to address this have mostly (not entirely) lead no-where (lack of
> feedback on proposed patches to ./CODING_STYLE). So for the time
> being there are (many) aspects where we have de-facto expectations
> that aren't written down anywhere, with the result of (in a subset
> of cases) disagreement on what the perceived de-facto standard
> actually is.

I recognize that it could be challenging finding a consensus to update
CODING_STYLE but it might be worth doing to reduce frictions with both
contributors and other reviewers.

But to be clear, I was also referring to things that might be actually
hard to add to CODING_STYLE, such as macro vs. static inlines, when to
split a single function into multiple smaller functions, etc.

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

Re: [Xen-devel] [PATCH v2 5/6] Add guide on Communication Best Practice

[Lars Kurth]

On 27/11/2019, 18:57, "Stefano Stabellini" <sstabellini@kernel.org> wrote:

    On Thu, 26 Sep 2019, Lars Kurth wrote:
    > 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
    > 
    > 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 | 410 ++++++++++++++++++++++++++++++++++++++++++++++
    >  1 file changed, 410 insertions(+)
    >  create mode 100644 communication-practice.md
    > 
    > diff --git a/communication-practice.md b/communication-practice.md
    > new file mode 100644
    > index 0000000..db9a5ef
    > --- /dev/null
    > +++ b/communication-practice.md
    > @@ -0,0 +1,410 @@
    > +# 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 top of a patch, as addressing code review comments typically requires
                     ^ the top
    
    
    > +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](https://youtu.be/ehZvBmrLRwg?t=834). 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 dictionart 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.
                           ^ remove ,
    
    > +
    > +Compare this to the (Wikipedia entry)[https://en.wikipedia.org/wiki/Foot_binding]
    > +
    > +> 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.
    > +
    > +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.
    > +
    > +### 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 of an issue or disagreement
    > +By default, every comment which is made **ought to be addressed** by the author.
    > +However, often reviewers note issues, which would be nice if they were addressed,
    > +but are not mandatory.
    > +
    > +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.
    >
    > +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](https://xenproject.org/developers/governance/#expressingopinion)
    > +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
    
    It is not just about the willingness of somebody to argue a point, which
    is the important thing when voting. During code reviews it is perfectly
    fine to make suggestions which are just optional for multiple reasons,
    including that they might be too taxing for the contributor.
    
    So, I think we should add that it would be best to use words that make it
    clear whether something is optional or whether it is required, see my
    reply to patch #6, I wrote an example there.
    
    
    
    > +### 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. This reduces the workload on both the reviewer and patch author. However,
    > +reviewers should make clear that they have omitted detailed review comments and
    > +that these will come later.
    
    Maybe we want to expand on this a bit. Not all series are based on
    flawed assumptions, but all series have different class of changes that
    are required for acceptance, from major code modifications to minor code
    style fixes.
    
    I think we should say that it is good practice to ask for any major
    changes early on, during the first or second iteration of the series.
    It would be best to avoid asking for major changes at v9 if possible.
    
    
    Something else which is missing in this document, and it is purely for
    reviewers, is to be careful doing reviews late in the cycle when another
    maintainer/reviewer has already provided feedback on the series multiple
    times previously. For instance, if reviewer R1 has been doing reviews
    from the first version of the series and contributor C has been
    addressing all comments, it would be best if reviewer R2 didn't come in
    providing detailed feedback months later at v5, unless their requests
    are actually strictly necessary (i.e. they spotted a bug). The main
    reason is that it is difficult not to let your own personal style (code
    style, the way to lay out the code) sip through review comments, and it
    can cause double-effort for the author if he/she already made changes
    according R1's personal style. However, in general, it would be best to
    limit "personal style" requests for changes anyway, see my comment to
    patch #6.
      
I see whether I can add something. I do like Rich's suggestion to use the Shift Left" terminology (https://devopedia.org/shift-left) of which this is kind of an instance. The same applies 

I can put together a v2, with typos addressed and expand some sections.

Regards
Lars


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

Re: [Xen-devel] [PATCH v2 5/6] Add guide on Communication Best Practice

[Lars Kurth]

On 27/11/2019, 19:06, "Stefano Stabellini" <sstabellini@kernel.org> wrote:

    On Fri, 27 Sep 2019, Jan Beulich wrote:
    > On 26.09.2019 21:39, Lars Kurth wrote:
    > > +### 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.
    > 
    > And another remark here: Not being terse in situations like the ones
    > enumerated as examples above is a double waste of the reviewer's time:
    > They shouldn't even need to make such comments, especially not many
    > times for a single patch (see your mention of "overuse"). I realize
    > we still have no automated mechanism to check style aspects, but
    > anybody can easily look over their patches before submitting them.
    > And for an occasional issue I think a terse reply is quite reasonable
    > to have.
    > 
    > Overall I'm seeing the good intentions of this document, yet I'd still
    > vote at least -1 on it if it came to a vote. Following even just a
    > fair part of it is a considerable extra amount of time to invest in
    > reviews, when we already have a severe reviewing bottleneck. If I have
    > to judge between doing a bad (stylistically according to this doc, not
    > technically) review or none at all (because of time constraints), I'd
    > favor the former. Unless of course I'm asked to stop doing so, in
    > which case I'd expect whoever asks to arrange for the reviews to be
    > done by someone else in due course.
    
    Reading the document, I think Jan has a point that it gives the
    impression that following the suggestions would take significant
    efforts, while actually I don't think Lars meant it that way at all, and
    I don't think it should be the case either.

Yes. Ultimately the effect of a better communication should overall be a net-positive in terms of effort. 
    
    Maybe we should highlight and encourage "clarity" instead of "verbosity"
    of the communication, and encourage "expressing appreciation" to
    newcomers, not necessarily to seasoned contributors.

Good idea
    
    The ultimate goal of this document is actually to *reduce* our overall
    efforts by making our communication more efficient, not to increase
    efforts. Maybe it is worth saying this too.
    
It is worth saying this. 

Regards
Lars



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

Re: [Xen-devel] [MirageOS-devel] [PATCH v2 4/6] Add Code Review Guide

[Lars Kurth]

[-- Attachment #1.1: Type: text/plain, Size: 3918 bytes --]



From: Rich Persaud <persaur@gmail.com>
Date: Thursday, 28 November 2019 at 12:21
To: Lars Kurth <lars.kurth@citrix.com>
Cc: 'Jan Beulich' <JBeulich@suse.com>, "lars.kurth@xenproject.org" <lars.kurth@xenproject.org>, Stefano Stabellini <sstabellini@kernel.org>, "xen-api@lists.xenproject.org" <xen-api@lists.xenproject.org>, "minios-devel@lists.xenproject.org" <minios-devel@lists.xenproject.org>, "committers@xenproject.org" <committers@xenproject.org>, "mirageos-devel@lists.xenproject.org" <mirageos-devel@lists.xenproject.org>, xen-devel <xen-devel@lists.xenproject.org>, "win-pv-devel@lists.xenproject.org" <win-pv-devel@lists.xenproject.org>
Subject: Re: [MirageOS-devel] [PATCH v2 4/6] Add Code Review Guide

On Nov 28, 2019, at 09:05, Lars Kurth <lars.kurth@citrix.com> wrote:

On 28/11/2019, 07:37, "Jan Beulich" <jbeulich@suse.com> wrote:

   On 28.11.2019 14:06, Lars Kurth wrote:

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

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

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

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

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

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

    https://devopedia.org/shift-left

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

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

Lars


[-- Attachment #1.2: Type: text/html, Size: 8352 bytes --]

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

Re: [Xen-devel] [PATCH v2 6/6] Added Resolving Disagreement

[Lars Kurth]

On 27/11/2019, 18:56, "Stefano Stabellini" <sstabellini@kernel.org> wrote:

    On Thu, 26 Sep 2019, Lars Kurth wrote:
    > From: Lars Kurth <lars.kurth@citrix.com>
    > 
    > This guide provides Best Practice on identifying and resolving
    > common classes of disagreement
    > 
    > 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 | 146 ++++++++++++++++++++++++++++++++++++++++++++++
    >  1 file changed, 146 insertions(+)
    >  create mode 100644 resolving-disagreement.md
    > 
    > diff --git a/resolving-disagreement.md b/resolving-disagreement.md
    > new file mode 100644
    > index 0000000..19aedbe
    > --- /dev/null
    > +++ b/resolving-disagreement.md
    > @@ -0,0 +1,146 @@
    > +# 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](http://www.paulgraham.com/disagree.html)**, 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 Disagreemen](https://upload.wikimedia.org/wikipedia/commons/a/a3/Graham%27s_Hierarchy_of_Disagreement-en.svg) |
                                 ^ Disagreement
    
    This is a NIT but in a few places in this series you go over the
    original line length.

True: typically for URLs. Primarily because I don't know whether they are rendered correctly when split
    
    > +| *A representation of Graham's hierarchy of disagreement from [Loudacris](http://www.createdebate.com/user/viewprofile/Loudacris) modified by [Rocket000](https://en.wikipedia.org/wiki/User:Rocket000)* |
    > +
    > +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 patch to implement additional functionality.
                                           ^ a patch                      ^ 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.
    
    I think we should mention somewhere here that it is recommended for
    reviewers to be explicit about whether a request is optional or whether
    it is a requirement.
    
    For instance: "I think it would be good if X also did Y" doesn't say if
    it is optional (future work) or it is actually required as part of this
    series. More explicit word choices are preferable, such as:
    
    "I think it would be good if X also did Y, not a requirement but good to
    have."
    
    "I think it would be good if X also did Y and it should be part of this
    series."
    
    It helps make the communication with the author more effective,
    especially in this kind of situations.
    
Agreed    

    > +## Issue: [Bikeshedding](https://en.wiktionary.org/wiki/bikeshedding)
    > +
    > +Occasionally discussions about unimportant but easy-to-grasp issues can lead to
    > +prolonged and unproductive discussion. The best way to approach this is to
                                  ^ discussions
    
    
    > +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](https://xenproject.org/developers/governance/#informal-votes-or-surveys) or
    > +[lazy voting](https://xenproject.org/developers/governance/#lazyconsensus) 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 event
                                                        ^ 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](communication-practice.md#Misunderstandings).
    > +
    > +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 been found. At the end
                                                                ^ you mean "has to be found?"
    
    
    > +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.
    
    I think there is something else we should say on this topic. There is a
    category of things which could be done in multiple ways and it is not
    overtly obvious which one is best. It is done to the maintainer and the
    author personal styles. It is easy to disagree on that.
    
    I think a good recommendation would be for the contributor to try to
    follow the maintainers requests, even if they could be considered
    "style", trusting their experience on the matter. And a good
    recommendation for the maintainer would be to try to let the contributor
    have freedom of implementation choice on things that don't make a
    significant difference.
    
Agreed.
    
    > +## 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](communication-guide.md)
    > +for more information.
    > +
    > +## Decision making and conflict resolution in our governance
    > +
    > +Our [governance](https://xenproject.org/developers/governance/#decisions) contains
    > +several proven mechanisms to help with decision making and conflict resolution.
    > +
    > +See
    > +* [Expressing agreement and disagreement](https://xenproject.org/developers/governance/#expressingopinion)
    > +* [Lazy consensus / Lazy voting](https://xenproject.org/developers/governance/#lazyconsensus)
    > +* [Informal votes or surveys](https://xenproject.org/developers/governance/#informal-votes-or-surveys)
    > +* [Leadership team decisions](https://xenproject.org/developers/governance/#leadership)
    > +* [Conflict resolution](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 v2 4/6] Add Code Review Guide

[Lars Kurth]

On 28/11/2019, 12:12, "Rich Persaud" <persaur@gmail.com> wrote:

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

Regards
Lars

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

Re: [Xen-devel] [PATCH v2 6/6] Added Resolving Disagreement

[Lars Kurth]

On 28/11/2019, 12:50, "Stefano Stabellini" <sstabellini@kernel.org> wrote:

    On Thu, 28 Nov 2019, Jan Beulich wrote:
    > On 28.11.2019 01:56, Stefano Stabellini wrote:
    > > On Thu, 26 Sep 2019, Lars Kurth wrote:

    > > I think a good recommendation would be for the contributor to try to
    > > follow the maintainers requests, even if they could be considered
    > > "style", trusting their experience on the matter. And a good
    > > recommendation for the maintainer would be to try to let the contributor
    > > have freedom of implementation choice on things that don't make a
    > > significant difference.
    > 
    > I think we try to, but I also think we suffer from too little
    > clear documentation on e.g. style aspects. Attempts on my part
    > to address this have mostly (not entirely) lead no-where (lack of
    > feedback on proposed patches to ./CODING_STYLE). So for the time
    > being there are (many) aspects where we have de-facto expectations
    > that aren't written down anywhere, with the result of (in a subset
    > of cases) disagreement on what the perceived de-facto standard
    > actually is.
    
    I recognize that it could be challenging finding a consensus to update
    CODING_STYLE but it might be worth doing to reduce frictions with both
    contributors and other reviewers.
    
    But to be clear, I was also referring to things that might be actually
    hard to add to CODING_STYLE, such as macro vs. static inlines, when to
    split a single function into multiple smaller functions, etc.
    
I think this is definitely something we ought to do. I am volunteering to
pick this up, but changing/clarifying the CODING_STYLE needs to be 
considered in conjunction with checking tools

I have parked this for now, as
a) I did not want to disrupt 4.13 
b) and until recently I also didn’t fully understand what kind of coding
standards would help with safety certification

And of course, having a bot do the checking would remove the friction
entirely. 

Lars


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

Re: [Xen-devel] [MirageOS-devel] [PATCH v2 4/6] Add Code Review Guide

[Lars Kurth]

From: Lars Kurth <lars.kurth@citrix.com>
Date: Thursday, 28 November 2019 at 19:39
To: Rich Persaud <persaur@gmail.com>
Cc: 'Jan Beulich' <JBeulich@suse.com>, "lars.kurth@xenproject.org" <lars.kurth@xenproject.org>, Stefano Stabellini <sstabellini@kernel.org>, "xen-api@lists.xenproject.org" <xen-api@lists.xenproject.org>, "minios-devel@lists.xenproject.org" <minios-devel@lists.xenproject.org>, "committers@xenproject.org" <committers@xenproject.org>, "mirageos-devel@lists.xenproject.org" <mirageos-devel@lists.xenproject.org>, xen-devel <xen-devel@lists.xenproject.org>, "win-pv-devel@lists.xenproject.org" <win-pv-devel@lists.xenproject.org>
Subject: Re: [MirageOS-devel] [PATCH v2 4/6] Add Code Review Guide

 
 
From: Rich Persaud <persaur@gmail.com>
Date: Thursday, 28 November 2019 at 12:21
To: Lars Kurth <lars.kurth@citrix.com>
Cc: 'Jan Beulich' <JBeulich@suse.com>, "lars.kurth@xenproject.org" <lars.kurth@xenproject.org>, Stefano Stabellini <sstabellini@kernel.org>, "xen-api@lists.xenproject.org" <xen-api@lists.xenproject.org>, "minios-devel@lists.xenproject.org" <minios-devel@lists.xenproject.org>, "committers@xenproject.org" <committers@xenproject.org>, "mirageos-devel@lists.xenproject.org" <mirageos-devel@lists.xenproject.org>, xen-devel <xen-devel@lists.xenproject.org>, "win-pv-devel@lists.xenproject.org" <win-pv-devel@lists.xenproject.org>
Subject: Re: [MirageOS-devel] [PATCH v2 4/6] Add Code Review Guide
 
On Nov 28, 2019, at 09:05, Lars Kurth <lars.kurth@citrix.com> wrote:
 
On 28/11/2019, 07:37, "Jan Beulich" <jbeulich@suse.com> wrote:

   On 28.11.2019 14:06, Lars Kurth wrote:


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

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

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

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

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

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

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

----
## <a name="problems"></a>Problematic Patch Reviews

A typical waterfall software development process is sequential with the following 
steps: define requirements, analyse, design, code, test and deploy. Problems 
uncovered by code review or testing at such a late stage can cause costly redesign 
and delays. The principle of **[Shift Left](https://devopedia.org/shift-left)** is to take a 
task that is traditionally performed at a late stage in the process and perform that task 
at earlier stages. The goal is to save time by avoiding refactoring.

Typically, problematic patch reviews uncover issues such as wrong or missed 
assumptions, a problematic architecture or design, or other bugs that require 
significant re-implementation of a patch series to fix the issue.

The principle of **Shift Left** also applies in code reviews. Let's assume a series has
a major flaw: ideally, this flaw would be picked up in the **first or second iteration** of 
the code review. As significant parts of the code may have to be re-written, it does not 
make sense for reviewers to highlight minor issues (such as style issues) until major 
flaws have been addressed. By providing feedback on minor issues reviewers cause 
the code author and themselves extra work by asking for changes to code, which 
ultimately may be changed later.

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

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

it does not make sense to do a detailed code review. In such cases, it is best to
focus on the major issues first and deal with style and minor issues in a subsequent
review. Not all series have significant flaws, but most series have different classes of 
changes that are required for acceptance: covering a range of major code 
modifications to minor code style fixes. To avoid misunderstandings between 
reviewers and contributors, it is important to establish and agree whether a series or 
part of a series has a significant flaw and agree a course of action. 

A pragmatic approach would be to
* Highlight problematic portions of a series in the cover letter 
* For the patch author and reviewer(s) to agree that for problematic to omit style and
minor issues in the review, until the significant flaw is addressed

This saves both the patch author and reviewer(s) time. Note that some background
is covered in detail in [Problematic Patch Reviews](resolving-disagreement.md#problems).
----
I think is a pragmatic approach that addresses some of Jan's concerns

Best Regards
Lars


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

Re: [Xen-devel] [PATCH v2 4/6] Add Code Review Guide

[Jan Beulich]

On 06.12.2019 00:41, Lars Kurth wrote:
> I propose to add the following section to code-review-guide.md
> 
> ----
> ## <a name="problems"></a>Problematic Patch Reviews
> 
> A typical waterfall software development process is sequential with the following 
> steps: define requirements, analyse, design, code, test and deploy. Problems 
> uncovered by code review or testing at such a late stage can cause costly redesign 
> and delays. The principle of **[Shift Left](https://devopedia.org/shift-left)** is to take a 
> task that is traditionally performed at a late stage in the process and perform that task 
> at earlier stages. The goal is to save time by avoiding refactoring.
> 
> Typically, problematic patch reviews uncover issues such as wrong or missed 
> assumptions, a problematic architecture or design, or other bugs that require 
> significant re-implementation of a patch series to fix the issue.
> 
> The principle of **Shift Left** also applies in code reviews. Let's assume a series has
> a major flaw: ideally, this flaw would be picked up in the **first or second iteration** of 
> the code review. As significant parts of the code may have to be re-written, it does not 
> make sense for reviewers to highlight minor issues (such as style issues) until major 
> flaws have been addressed. By providing feedback on minor issues reviewers cause 
> the code author and themselves extra work by asking for changes to code, which 
> ultimately may be changed later.
> 
> The question then becomes, how do code reviewers identify major issues early? 
> ----
> This is where I really need help. Are there any tips and recommendations that we could give?
> I can clearly highlight that we have RFC series, but in practice that does not solve the problem as RFCs don’t get prioritized
> How do reviewers normally approach a series: do you a) take a big picture view first, or b) do most of you work through a series sequentially

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

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

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

> it does not make sense to do a detailed code review. In such cases, it is best to
> focus on the major issues first and deal with style and minor issues in a subsequent
> review. Not all series have significant flaws, but most series have different classes of 
> changes that are required for acceptance: covering a range of major code 
> modifications to minor code style fixes. To avoid misunderstandings between 
> reviewers and contributors, it is important to establish and agree whether a series or 
> part of a series has a significant flaw and agree a course of action. 
> 
> A pragmatic approach would be to
> * Highlight problematic portions of a series in the cover letter 
> * For the patch author and reviewer(s) to agree that for problematic to omit style and
> minor issues in the review, until the significant flaw is addressed
> 
> This saves both the patch author and reviewer(s) time. Note that some background
> is covered in detail in [Problematic Patch Reviews](resolving-disagreement.md#problems).

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

Jan

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

Re: [Xen-devel] [PATCH v2 4/6] Add Code Review Guide

[Lars Kurth]

On 06/12/2019, 09:51, "Jan Beulich" <jbeulich@suse.com> wrote:

    On 06.12.2019 00:41, Lars Kurth wrote:
    > I propose to add the following section to code-review-guide.md
    > 
    > ----
    > ## <a name="problems"></a>Problematic Patch Reviews
    > 
    > A typical waterfall software development process is sequential with the following 
    > steps: define requirements, analyse, design, code, test and deploy. Problems 
    > uncovered by code review or testing at such a late stage can cause costly redesign 
    > and delays. The principle of **[Shift Left](https://devopedia.org/shift-left)** is to take a 
    > task that is traditionally performed at a late stage in the process and perform that task 
    > at earlier stages. The goal is to save time by avoiding refactoring.
    > 
    > Typically, problematic patch reviews uncover issues such as wrong or missed 
    > assumptions, a problematic architecture or design, or other bugs that require 
    > significant re-implementation of a patch series to fix the issue.
    > 
    > The principle of **Shift Left** also applies in code reviews. Let's assume a series has
    > a major flaw: ideally, this flaw would be picked up in the **first or second iteration** of 
    > the code review. As significant parts of the code may have to be re-written, it does not 
    > make sense for reviewers to highlight minor issues (such as style issues) until major 
    > flaws have been addressed. By providing feedback on minor issues reviewers cause 
    > the code author and themselves extra work by asking for changes to code, which 
    > ultimately may be changed later.
    > 
    > The question then becomes, how do code reviewers identify major issues early? 
    > ----
    > This is where I really need help. Are there any tips and recommendations that we could give?
    > I can clearly highlight that we have RFC series, but in practice that does not solve the problem as RFCs don’t get prioritized
    > How do reviewers normally approach a series: do you a) take a big picture view first, or b) do most of you work through a series sequentially
    
    Afaic - depends heavily on the patch / series. I wouldn't typically
    peek ahead in a series, but it has happened. But as you say
    (elsewhere) the cover letter should put in place the "big picture".
    A series should generally be reviewable going from patch to patch,
    having the cover letter in mind.

I am wondering what others do. 

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

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

I am wondering whether we should introduce some informal convention to mark _part_ of a series as problematic. A simple example of how to do this in the cover letter would do
    
    > it does not make sense to do a detailed code review. In such cases, it is best to
    > focus on the major issues first and deal with style and minor issues in a subsequent
    > review. Not all series have significant flaws, but most series have different classes of 
    > changes that are required for acceptance: covering a range of major code 
    > modifications to minor code style fixes. To avoid misunderstandings between 
    > reviewers and contributors, it is important to establish and agree whether a series or 
    > part of a series has a significant flaw and agree a course of action. 
    > 
    > A pragmatic approach would be to
    > * Highlight problematic portions of a series in the cover letter 
    > * For the patch author and reviewer(s) to agree that for problematic to omit style and
    > minor issues in the review, until the significant flaw is addressed
    > 
    > This saves both the patch author and reviewer(s) time. Note that some background
    > is covered in detail in [Problematic Patch Reviews](resolving-disagreement.md#problems).
    
    I have no issues with the suggested text in general, but I also don't
    think it makes much of a difference wrt what I had mentioned before.
    I guess part of the problem here is that there are things which imo
    you can't really give recipes for how to approach, if the expectation
    is that it would fit at least the vast majority of cases. 

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

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

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

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

Regards
Lars

    

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

Re: [Xen-devel] [PATCH v2 4/6] Add Code Review Guide

[Lars Kurth]

[-- Attachment #1.1: Type: text/plain, Size: 13366 bytes --]



> On 9 Dec 2019, at 11:02, Lars Kurth <lars.kurth@citrix.com> wrote:
> 
> 
> 
> On 06/12/2019, 09:51, "Jan Beulich" <jbeulich@suse.com <mailto:jbeulich@suse.com>> wrote:
> 
>    On 06.12.2019 00:41, Lars Kurth wrote:
>> I propose to add the following section to code-review-guide.md
>> 
>> ----
>> ## <a name="problems"></a>Problematic Patch Reviews
>> 
>> A typical waterfall software development process is sequential with the following 
>> steps: define requirements, analyse, design, code, test and deploy. Problems 
>> uncovered by code review or testing at such a late stage can cause costly redesign 
>> and delays. The principle of **[Shift Left](https://devopedia.org/shift-left)** is to take a 
>> task that is traditionally performed at a late stage in the process and perform that task 
>> at earlier stages. The goal is to save time by avoiding refactoring.
>> 
>> Typically, problematic patch reviews uncover issues such as wrong or missed 
>> assumptions, a problematic architecture or design, or other bugs that require 
>> significant re-implementation of a patch series to fix the issue.
>> 
>> The principle of **Shift Left** also applies in code reviews. Let's assume a series has
>> a major flaw: ideally, this flaw would be picked up in the **first or second iteration** of 
>> the code review. As significant parts of the code may have to be re-written, it does not 
>> make sense for reviewers to highlight minor issues (such as style issues) until major 
>> flaws have been addressed. By providing feedback on minor issues reviewers cause 
>> the code author and themselves extra work by asking for changes to code, which 
>> ultimately may be changed later.
>> 
>> The question then becomes, how do code reviewers identify major issues early? 
>> ----
>> This is where I really need help. Are there any tips and recommendations that we could give?
>> I can clearly highlight that we have RFC series, but in practice that does not solve the problem as RFCs don’t get prioritized
>> How do reviewers normally approach a series: do you a) take a big picture view first, or b) do most of you work through a series sequentially
> 
>    Afaic - depends heavily on the patch / series. I wouldn't typically
>    peek ahead in a series, but it has happened. But as you say
>    (elsewhere) the cover letter should put in place the "big picture".
>    A series should generally be reviewable going from patch to patch,
>    having the cover letter in mind.
> 
> I am wondering what others do. 
> 
> I think explaining the basic work-flow from the viewpoint of a reviewer and code author maybe in a separate section, which is not tied to the problem case would make sense. More input from other maintainers would be valuable. My gut-feel is that most reviewers "read and review" series sequentially, which has implications for the author. E.g.
> - docs/design docs should be at the beginning of a series
> - key header files or changes to them should be at the beginning of a series
> - Etc
> 
>> I then propose to change the following section in communication-practice.md
>> ----
>> ### Prioritize significant flaws
>> If a patch or patch series has significant flaws, such as
>> * It is built on wrong assumptions
>> * There are issues with the architecture or the design
> 
>    In such a case a full review of course doesn't make much sense. But
>    this is far from the typical situation. Way more often you have some
>    _part_ of a patch or series which has a bigger issue, but other
>    parts are in need of no or just minor changes.
> 
> I know that this is an unusual situation. But it has happened in clusters frequently in the past.
> 
> I am wondering whether we should introduce some informal convention to mark _part_ of a series as problematic. A simple example of how to do this in the cover letter would do
> 
>> it does not make sense to do a detailed code review. In such cases, it is best to
>> focus on the major issues first and deal with style and minor issues in a subsequent
>> review. Not all series have significant flaws, but most series have different classes of 
>> changes that are required for acceptance: covering a range of major code 
>> modifications to minor code style fixes. To avoid misunderstandings between 
>> reviewers and contributors, it is important to establish and agree whether a series or 
>> part of a series has a significant flaw and agree a course of action. 
>> 
>> A pragmatic approach would be to
>> * Highlight problematic portions of a series in the cover letter 
>> * For the patch author and reviewer(s) to agree that for problematic to omit style and
>> minor issues in the review, until the significant flaw is addressed
>> 
>> This saves both the patch author and reviewer(s) time. Note that some background
>> is covered in detail in [Problematic Patch Reviews](resolving-disagreement.md#problems).
> 
>    I have no issues with the suggested text in general, but I also don't
>    think it makes much of a difference wrt what I had mentioned before.
>    I guess part of the problem here is that there are things which imo
>    you can't really give recipes for how to approach, if the expectation
>    is that it would fit at least the vast majority of cases. 
> 
> I think the document covers most of the common cases, plus some areas which are problematic
> * From a people-interaction point-of-view - in other words there could be unnecessary conflict, which is bad for the community but also wastes time
> * From an efficient usage of time point-of-view
> 
> For example: the whole thing about thanking, appreciation, ... is something targeted at newcomers and a desire to treat them with more thought and awareness. 
> Granted it takes more time to do a review with a newcomer, but it should make subsequent reviews easier 
> 
> It happens regularly, but not that frequently
> 
>    For code
>    reviews this means that I don't think there should be any wording
>    suggesting they should be done in a certain form; there may be wording
>    suggesting they _could_ be done in a certain form (e.g. to help
>    people not knowing at all how to get started).
> 
> That was definitely my intention. Maybe I have not succeeded in making this clear enough

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

‹gwd›		I think every situation if different.

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

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

‹gwd›		Sometimes in a long series, {1,2}/10 will be clean-ups; {3-6}/10 will be general reorganisations which don't really seem to do anything; and then 7/10 will be the "meat", which helps you understand what {3-6}/10 were about.

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

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

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

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

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

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

‹Diziet› 		gwd: ^

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

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

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

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

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

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

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

‹Diziet› 		Subject:  New signature key arrangements

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

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

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

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

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

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

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

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

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

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

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

 




[-- Attachment #1.2: Type: text/html, Size: 89042 bytes --]

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