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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

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.

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