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

[Lars Kurth <lars.kurth@xxxxxxxxxx>]

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

    On Thu, 26 Sep 2019, Lars Kurth wrote:
    > From: Lars Kurth <lars.kurth@xxxxxxxxxx>
    > 
    > This guide provides Best Practice on identifying and resolving
    > common classes of disagreement
    > 
    > Signed-off-by: Lars Kurth <lars.kurth@xxxxxxxxxx>
    > --
    > Cc: minios-devel@xxxxxxxxxxxxxxxxxxxx
    > Cc: xen-api@xxxxxxxxxxxxxxxxxxxx
    > Cc: win-pv-devel@xxxxxxxxxxxxxxxxxxxx
    > Cc: mirageos-devel@xxxxxxxxxxxxxxxxxxxx
    > Cc: committers@xxxxxxxxxxxxxx
    > ---
    >  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@xxxxxxxxxxxxxx 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
    > 
    

_______________________________________________
MirageOS-devel mailing list
MirageOS-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/mirageos-devel