addressed
* A combination of the above
+.. _code-review-problematic-patch-review:
.. _problems:
Problematic Patch Reviews
style and minor issues in the review, until the significant flaw is addressed
This saves both the patch author and reviewer(s) time. Note that some
-background is covered in detail in `Problematic Patch Reviews <4_>`_.
+background is covered in detail in
+:ref:`Problematic Patch Reviews <code-review-problematic-patch-review>`.
Reviewers: Welcome newcomers
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.
+.. _communication-practice-misunderstandings:
+
Misunderstandings
-----------------
.. _1: https://youtu.be/ehZvBmrLRwg?t=834
.. _2: https://en.wikipedia.org/wiki/Foot_binding
.. _3: https://xenproject.org/developers/governance/#expressingopinion
-.. _4: resolving-disagreement.md#problems
.. _5: https://www.wired.com/2006/02/the-secret-cause-of-flame-wars/
.. _6: https://xenproject.org/help/irc/
.. _8: https://en.wikipedia.org/wiki/Erin_Meyer
communication-guide
code-review-guide
communication-practice
+ resolving-disagreement
Indices and tables
==================
+++ /dev/null
-# Resolving Disagreement
-
-This guide provides Best Practice on resolving disagreement, such as
-* Gracefully accept constructive criticism
-* Focus on what is best for the community
-* Resolve differences in opinion effectively
-
-## Theory: Paul Graham's hierarchy of disagreement
-
-Paul Graham proposed a **disagreement hierarchy** in a 2008 essay
-**[How to Disagree][1]**, putting types of arguments into a seven-point
-hierarchy and observing that *moving up the disagreement hierarchy makes people
-less mean, and will make most of them happier*. Graham also suggested that the
-hierarchy can be thought of as a pyramid, as the highest forms of disagreement
-are rarer.
-
-| ![Graham's Hierarchy of Disagreement][2] |
-| *A representation of Graham's hierarchy of disagreement from [Loudacris][3]
- modified by [Rocket000][4]* |
-
-In the context of the Xen Project we strive to **only use the top half** of the
-hierarchy. **Name-calling** and **Ad hominem** arguments are not acceptable
-within the Xen Project.
-
-## Issue: Scope creep
-
-One thing which occasionally happens during code review is that a code reviewer
-asks or appears to ask the author of a patch to implement additional
-functionalities.
-
-This could take for example the form of
-> Do you think it would be useful for the code to do XXX?
-> I can imagine a user wanting to do YYY (and XXX would enable this)
-
-That potentially adds additional work for the code author, which they may not
-have the time to perform. It is good practice for authors to consider such a
-request in terms of
-* Usefulness to the user
-* Code churn, complexity or impact on other system properties
-* Extra time to implement and report back to the reviewer
-
-If you believe that the impact/cost is too high, report back to the reviewer.
-To resolve this, it is advisable to
-* Report your findings
-* And then check whether this was merely an interesting suggestion, or something
- the reviewer feels more strongly about
-
-In the latter case, there are typically several common outcomes
-* The **author and reviewer agree** that the suggestion should be implemented
-* The **author and reviewer agree** that it may make sense to defer
- implementation
-* The **author and reviewer agree** that it makes no sense to implement the
- suggestion
-
-The author of a patch would typically suggest their preferred outcome, for
-example
-> I am not sure it is worth to implement XXX
-> Do you think this could be done as a separate patch in future?
-
-In cases, where no agreement can be found, the best approach would be to get an
-independent opinion from another maintainer or the project's leadership team.
-
-## Issue: [Bikeshedding][5]
-
-Occasionally discussions about unimportant but easy-to-grasp issues can lead to
-prolonged and unproductive discussions. The best way to approach this is to
-try and **anticipate** bikeshedding and highlight it as such upfront. However,
-the format of a code review does not always lend itself well to this approach,
-except for highlighting it in the cover letter of a patch series.
-
-However, typically Bikeshedding issues are fairly easy to recognize in a code
-review, as you will very quickly get different reviewers providing differing
-opinions. In this case it is best for the author or a reviewer to call out the
-potential bikeshedding issue using something like
-
-> Looks we have a bikeshedding issue here
-> I think we should call a quick vote to settle the issue
-
-Our governance provides the mechanisms of [informal votes][6] or
-[lazy voting][7] which lend themselves well to resolve such issues.
-
-## Issue: Small functional issues
-
-The most common area of disagreements which happen in code reviews, are
-differing opinions on whether small functional issues in a patch series have to
-be resolved or not before the code is ready to be submitted. Such disagreements
-are typically caused by different expectations related to the level of
-perfection a patch series needs to fulfill 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 installment, you perform an
-inspection and you find issues such as
-* The seals around the bathtub are not perfectly even
-* When you open the tap, the plumbing initially makes some loud noise
-* The shower mixer has been installed the wrong way around
-
-In all these cases, the bathroom is perfectly functional, but not perfect. At
-this point you have the choice to try and get all the issues addressed, which in
-the example of the shower mixer may require significant re-work and potentially
-push-back from your builder. You may have to refer to the initial statement of
-work, but it turns out it does not contain sufficient information to ascertain
-whether your builder had committed to the level of quality you were expecting.
-
-Similar situations happen in code reviews very frequently and can lead to a long
-discussion before it can be resolved. The most important thing is to
-**identify** a disagreement as such early and then call it out. Tips on how to
-do this, can be found [here][8].
-
-At this point, you will understand why you have the disagreement, but not
-necessarily agreement on how to move forward. An easy fix would be to agree to
-submit the change as it is and fix it in future. In a corporate software
-engineering environment this is the most likely outcome, but in open source
-communities additional concerns have to be considered.
-* Code reviewers frequently have been in this situation before with the most
- common outcome that the issue is then never fixed. By accepting the change,
- the reviewers have no leverage to fix the issue and may have to spend effort
- fixing the issue themselves in future as it may impact the product they built
- on top of the code.
-* Conversely, a reviewer may be asking the author to make too many changes of
- this type which ultimately may lead the author to not contribute to the
- project again.
-* An author, which consistently does not address **any** of these issues may
- end up getting a bad reputation and may find future code reviews more
- difficult.
-* An author which always addresses **all** of these issues may end up getting
- into difficulties with their employer, as they are too slow getting code
- upstreamed.
-
-None of these outcomes are good, so ultimately a balance has to be found. At
-the end of the day, the solution should focus on what is best for the community,
-which may mean asking for an independent opinion as outlined in the next
-section.
-
-## Issue: Multiple ways to solve a problem
-
-Frequently it is possible that a problem can be solved in multiple ways and it
-is not always obvious which one is best. Code reviewers tend to follow their
-personal coding style when reviewing code and sometimes will suggest that a
-code author makes changes to follow their own style, even when the author's
-code is correct. In such cases, it is easy to disagree and start arguing.
-
-We recommend that the code author tries to follow the code reviewers requests,
-even if they could be considered style issues, trusting the experience of the
-code reviewer. Similarly, we ask code reviewers to let the contributor have the
-freedom of implementation choices, where they do not have a downside.
-
-We do not always succeed in this, as such it is important to **identify** such a
-situation and then call it out as outlined [here][8].
-
-## Resolution: Asking for an independent opinion
-
-Most disagreements can be settled by
-* Asking another maintainer or committer to provide an independent opinion on
- the specific issue in public to help resolve it
-* Failing this an issue can be escalated to the project leadership team, which
- is expected to act as referee and make a decision on behalf of the community
-
-If you feel uncomfortable with this approach, you may also contact
-mediation@xenproject.org to get advice. See our [Communication Guide][9]
-for more information.
-
-## Decision making and conflict resolution in our governance
-
-Our [governance][A] contains several proven mechanisms to help with decision
-making and conflict resolution.
-
-See
-* [Expressing agreement and disagreement][B]
-* [Lazy consensus / Lazy voting][7]
-* [Informal votes or surveys][6]
-* [Leadership team decisions][C]
-* [Conflict resolution][D]
-
-[1]: http://www.paulgraham.com/disagree.html
-[2]: https://upload.wikimedia.org/wikipedia/commons/a/a3/Graham%27s_Hierarchy_of_Disagreement-en.svg
-[3]: https://www.createdebate.com/user/viewprofile/Loudacris
-[4]: https://en.wikipedia.org/wiki/User:Rocket000
-[5]: https://en.wiktionary.org/wiki/bikeshedding
-[6]: https://xenproject.org/developers/governance/#informal-votes-or-surveys
-[7]: https://xenproject.org/developers/governance/#lazyconsensus
-[8]: communication-practice.md#Misunderstandings
-[9]: communication-guide.md
-[A]: https://xenproject.org/developers/governance/#decisions
-[B]: https://xenproject.org/developers/governance/#expressingopinion
-[C]: https://xenproject.org/developers/governance/#leadership
-[D]: https://xenproject.org/developers/governance/#conflict
--- /dev/null
+Resolving Disagreement
+**********************
+
+This guide provides Best Practice on resolving disagreement, such as
+
+* Gracefully accept constructive criticism
+* Focus on what is best for the community
+* Resolve differences in opinion effectively
+
+Theory: Paul Graham's hierarchy of disagreement
+===============================================
+
+Paul Graham proposed a **disagreement hierarchy** in a 2008 essay
+`How to Disagree <1_>`_, putting types of arguments into a seven-point
+hierarchy and observing that *moving up the disagreement hierarchy makes people
+less mean, and will make most of them happier*. Graham also suggested that the
+hierarchy can be thought of as a pyramid, as the highest forms of disagreement
+are rarer.
+
+.. figure:: https://upload.wikimedia.org/wikipedia/commons/a/a3/Graham%27s_Hierarchy_of_Disagreement-en.svg
+ :alt: Graham's Hierarchy of Disagreement
+
+ A representation of Graham's hierarchy of disagreement from `Loudacris <3_>`_
+ modified by `Rocket000 <4_>`_
+
+In the context of the Xen Project we strive to **only use the top half** of the
+hierarchy. **Name-calling** and **Ad hominem** arguments are not acceptable
+within the Xen Project.
+
+Issue: Scope creep
+==================
+
+One thing which occasionally happens during code review is that a code reviewer
+asks or appears to ask the author of a patch to implement additional
+functionalities.
+
+This could take for example the form of
+
+ *Do you think it would be useful for the code to do XXX?*
+
+ *I can imagine a user wanting to do YYY (and XXX would enable this)*
+
+That potentially adds additional work for the code author, which they may not
+have the time to perform. It is good practice for authors to consider such a
+request in terms of
+
+* Usefulness to the user
+* Code churn, complexity or impact on other system properties
+* Extra time to implement and report back to the reviewer
+
+If you believe that the impact/cost is too high, report back to the reviewer.
+To resolve this, it is advisable to
+
+* Report your findings
+* And then check whether this was merely an interesting suggestion, or something
+ the reviewer feels more strongly about
+
+In the latter case, there are typically several common outcomes
+
+* The **author and reviewer agree** that the suggestion should be implemented
+* The **author and reviewer agree** that it may make sense to defer
+ implementation
+* The **author and reviewer agree** that it makes no sense to implement the
+ suggestion
+
+The author of a patch would typically suggest their preferred outcome, for
+example
+
+ *I am not sure it is worth to implement XXX*
+
+ *Do you think this could be done as a separate patch in future?*
+
+In cases, where no agreement can be found, the best approach would be to get an
+independent opinion from another maintainer or the project's leadership team.
+
+Issue: `Bikeshedding <5_>`_
+===========================
+
+Occasionally discussions about unimportant but easy-to-grasp issues can lead to
+prolonged and unproductive discussions. The best way to approach this is to
+try and **anticipate** bikeshedding and highlight it as such upfront. However,
+the format of a code review does not always lend itself well to this approach,
+except for highlighting it in the cover letter of a patch series.
+
+However, typically Bikeshedding issues are fairly easy to recognize in a code
+review, as you will very quickly get different reviewers providing differing
+opinions. In this case it is best for the author or a reviewer to call out the
+potential bikeshedding issue using something like
+
+ *Looks we have a bikeshedding issue here*
+
+ *I think we should call a quick vote to settle the issue*
+
+Our governance provides the mechanisms of `informal votes <6_>`_ or
+`lazy voting <7_>`_ which lend themselves well to resolve such issues.
+
+Issue: Small functional issues
+==============================
+
+The most common area of disagreements which happen in code reviews, are
+differing opinions on whether small functional issues in a patch series have to
+be resolved or not before the code is ready to be submitted. Such disagreements
+are typically caused by different expectations related to the level of
+perfection a patch series needs to fulfill 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 installment, you perform an
+inspection and you find issues such as
+
+* The seals around the bathtub are not perfectly even
+* When you open the tap, the plumbing initially makes some loud noise
+* The shower mixer has been installed the wrong way around
+
+In all these cases, the bathroom is perfectly functional, but not perfect. At
+this point you have the choice to try and get all the issues addressed, which in
+the example of the shower mixer may require significant re-work and potentially
+push-back from your builder. You may have to refer to the initial statement of
+work, but it turns out it does not contain sufficient information to ascertain
+whether your builder had committed to the level of quality you were expecting.
+
+Similar situations happen in code reviews very frequently and can lead to a long
+discussion before it can be resolved. The most important thing is to
+**identify** a disagreement as such early and then call it out. Tips on how to
+do this, can be found :doc:`here <communication-practice>`.
+
+At this point, you will understand why you have the disagreement, but not
+necessarily agreement on how to move forward. An easy fix would be to agree to
+submit the change as it is and fix it in future. In a corporate software
+engineering environment this is the most likely outcome, but in open source
+communities additional concerns have to be considered.
+
+* Code reviewers frequently have been in this situation before with the most
+ common outcome that the issue is then never fixed. By accepting the change,
+ the reviewers have no leverage to fix the issue and may have to spend effort
+ fixing the issue themselves in future as it may impact the product they built
+ on top of the code.
+* Conversely, a reviewer may be asking the author to make too many changes of
+ this type which ultimately may lead the author to not contribute to the
+ project again.
+* An author, which consistently does not address **any** of these issues may
+ end up getting a bad reputation and may find future code reviews more
+ difficult.
+* An author which always addresses **all** of these issues may end up getting
+ into difficulties with their employer, as they are too slow getting code
+ upstreamed.
+
+None of these outcomes are good, so ultimately a balance has to be found. At
+the end of the day, the solution should focus on what is best for the community,
+which may mean asking for an independent opinion as outlined in the next
+section.
+
+Issue: Multiple ways to solve a problem
+=======================================
+
+Frequently it is possible that a problem can be solved in multiple ways and it
+is not always obvious which one is best. Code reviewers tend to follow their
+personal coding style when reviewing code and sometimes will suggest that a
+code author makes changes to follow their own style, even when the author's
+code is correct. In such cases, it is easy to disagree and start arguing.
+
+We recommend that the code author tries to follow the code reviewers requests,
+even if they could be considered style issues, trusting the experience of the
+code reviewer. Similarly, we ask code reviewers to let the contributor have the
+freedom of implementation choices, where they do not have a downside.
+
+We do not always succeed in this, as such it is important to
+**identify** such a situation and then call it out as outlined
+:ref:`here <communication-practice-misunderstandings>`.
+
+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 :doc:`Communication Guide <communication-guide>`
+for more information.
+
+Decision making and conflict resolution in our governance
+=========================================================
+
+Our `governance <A_>`_ contains several proven mechanisms to help with decision
+making and conflict resolution.
+
+See
+
+* `Expressing agreement and disagreement <B_>`_
+* `Lazy consensus / Lazy voting <7_>`_
+* `Informal votes or surveys <6_>`_
+* `Leadership team decisions <C_>`_
+* `Conflict resolution <D_>`_
+
+.. _1: http://www.paulgraham.com/disagree.html
+.. _3: https://www.createdebate.com/user/viewprofile/Loudacris
+.. _4: https://en.wikipedia.org/wiki/User:Rocket000
+.. _5: https://en.wiktionary.org/wiki/bikeshedding
+.. _6: https://xenproject.org/developers/governance/#informal-votes-or-surveys
+.. _7: https://xenproject.org/developers/governance/#lazyconsensus
+.. _A: https://xenproject.org/developers/governance/#decisions
+.. _B: https://xenproject.org/developers/governance/#expressingopinion
+.. _C: https://xenproject.org/developers/governance/#leadership
+.. _D: https://xenproject.org/developers/governance/#conflict
projects / people / gdunlap / governance.git / commitdiff