Re: [Xen-devel] [PATCH v2 6/6] Added Resolving Disagreement
On 28/11/2019, 12:50, "Stefano Stabellini" 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] [PATCH v2 6/6] Added Resolving Disagreement
On 27/11/2019, 18:56, "Stefano Stabellini" wrote: On Thu, 26 Sep 2019, Lars Kurth wrote: > From: Lars Kurth > > This guide provides Best Practice on identifying and resolving > common classes of disagreement > > Signed-off-by: Lars Kurth > -- > Cc: minios-de...@lists.xenproject.org > Cc: xen-...@lists.xenproject.org > Cc: win-pv-de...@lists.xenproject.org > Cc: mirageos-de...@lists.xenproject.org > Cc: committ...@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 000..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. > + > +|  | ^ 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
Re: [Xen-devel] [PATCH v2 6/6] Added Resolving Disagreement
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
Re: [Xen-devel] [PATCH v2 6/6] Added Resolving Disagreement
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 6/6] Added Resolving Disagreement
On Thu, 26 Sep 2019, Lars Kurth wrote: > From: Lars Kurth > > This guide provides Best Practice on identifying and resolving > common classes of disagreement > > Signed-off-by: Lars Kurth > -- > Cc: minios-de...@lists.xenproject.org > Cc: xen-...@lists.xenproject.org > Cc: win-pv-de...@lists.xenproject.org > Cc: mirageos-de...@lists.xenproject.org > Cc: committ...@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 000..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. > + > +|  > | ^ 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
[Xen-devel] [PATCH v2 6/6] Added Resolving Disagreement
From: Lars Kurth This guide provides Best Practice on identifying and resolving common classes of disagreement Signed-off-by: Lars Kurth -- Cc: minios-de...@lists.xenproject.org Cc: xen-...@lists.xenproject.org Cc: win-pv-de...@lists.xenproject.org Cc: mirageos-de...@lists.xenproject.org Cc: committ...@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 000..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. + +|  | +| *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
