differing opinions between maintainers vs patch acks

differing opinions between maintainers vs patch acks

[Jan Beulich]

All,

it's been a (not very often, but anyway) recurring situation that in
order to get an ack on some patch I had to make adjustments which
I didn't agree with. Since all maintainers opinions are supposed to be
equal, it is not really clear to me whether in such cases it should
really be the reviewing maintainer's rather than the submitting
maintainer's opinion which controls what actually goes into the tree.
When there's an odd number of maintainers for a given piece of
code, it may be acceptable to pull in a 3rd maintainer to break ties,
but pulling in a non-maintainer (e.g. some [other] committer) to
help out seems not really appropriate to me.

And just to clarify - such discussions aren't normally about aspects
that affect how the resulting code would work, but just how the
code should look like (see e.g. the thread rooted at
https://lists.xenproject.org/archives/html/xen-devel/2017-05/msg00068.html
for the most recent example, where the question is how to
express numbers and how to name labels), i.e. things in the end
often called "bike shedding".

My proposal is for the submitting maintainer's taste to take
preference over the reviewing maintainer's one in such cases.
And just to avoid any doubt - I don't mean this to extend to
cases where correctness of the code would be affected (albeit
I admit there may still be cases left sitting in a gray area in the
middle).

Jan


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

Re: differing opinions between maintainers vs patch acks

[Ian Jackson]

Jan Beulich writes ("differing opinions between maintainers vs patch acks"):
> When there's an odd number of maintainers for a given piece of
> code, it may be acceptable to pull in a 3rd maintainer to break ties,
> but pulling in a non-maintainer (e.g. some [other] committer) to
> help out seems not really appropriate to me.

I'm afraid I disagree.  Someone with a fresh perspective is often
helpful, even if it involves a bit more explanation.

And, the use of committers as referees in inter-maintainer disputes is
explicitly laid out in the project governance document:
  https://xenproject.org/governance.html#roles-local
  | Committers
  ...
  | committers can also act as referees should disagreements amongst
  | maintainers arise

> My proposal is for the submitting maintainer's taste to take
> preference over the reviewing maintainer's one in such cases.
> And just to avoid any doubt - I don't mean this to extend to
> cases where correctness of the code would be affected (albeit
> I admit there may still be cases left sitting in a gray area in the
> middle).

I definitely agree that there is room for giving the author of some
code (whether they are a maintainer or not) some leeway on matters of
taste.  I think, though, that while this ought to be a principle
applied by maintainers, committers and anyone else making relevant
decisions, it is not a rule of governance to be applied in contested
cases.

Ian.

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

Re: differing opinions between maintainers vs patch acks

[Jan Beulich]

>>> On 04.05.17 at 11:21, <ian.jackson@eu.citrix.com> wrote:
> Jan Beulich writes ("differing opinions between maintainers vs patch acks"):
>> When there's an odd number of maintainers for a given piece of
>> code, it may be acceptable to pull in a 3rd maintainer to break ties,
>> but pulling in a non-maintainer (e.g. some [other] committer) to
>> help out seems not really appropriate to me.
> 
> I'm afraid I disagree.  Someone with a fresh perspective is often
> helpful, even if it involves a bit more explanation.
> 
> And, the use of committers as referees in inter-maintainer disputes is
> explicitly laid out in the project governance document:
>   https://xenproject.org/governance.html#roles-local 
>   | Committers
>   ...
>   | committers can also act as referees should disagreements amongst
>   | maintainers arise

I understand this, and I agree as far as actual technical issues
go. I just don't think this is suitable / appropriate when it comes
to cosmetic questions.

Jan


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

Re: differing opinions between maintainers vs patch acks

[Ian Jackson]

Jan Beulich writes ("Re: differing opinions between maintainers vs patch acks"):
> On 04.05.17 at 11:21, <ian.jackson@eu.citrix.com> wrote:
> > I'm afraid I disagree.  Someone with a fresh perspective is often
> > helpful, even if it involves a bit more explanation.
> > 
> > And, the use of committers as referees in inter-maintainer disputes is
> > explicitly laid out in the project governance document:
> >   https://xenproject.org/governance.html#roles-local 
> >   | Committers
> >   ...
> >   | committers can also act as referees should disagreements amongst
> >   | maintainers arise
> 
> I understand this, and I agree as far as actual technical issues
> go. I just don't think this is suitable / appropriate when it comes
> to cosmetic questions.

Well, if the cosmetic question is not that important then presumably
someone would have given ground and it wouldn't need escalation.

If the cosmetic question _is_ important enough to have a big debate
over then I don't see any reason why the same escalation path is not
appropriate.  Actually, it seems to me that a relative outsider is
more likely to bring a useful sense of proportion.

(And, de jure, the governance doc makes no such distinction.)

Thanks,
Ian.

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

Re: differing opinions between maintainers vs patch acks

[Lars Kurth]

On 04/05/2017 10:55, "Ian Jackson" <ian.jackson@eu.citrix.com> wrote:

>Jan Beulich writes ("Re: differing opinions between maintainers vs patch
>acks"):
>> On 04.05.17 at 11:21, <ian.jackson@eu.citrix.com> wrote:
>> > I'm afraid I disagree.  Someone with a fresh perspective is often
>> > helpful, even if it involves a bit more explanation.
>> > 
>> > And, the use of committers as referees in inter-maintainer disputes is
>> > explicitly laid out in the project governance document:
>> >   https://xenproject.org/governance.html#roles-local
>> >   | Committers
>> >   ...
>> >   | committers can also act as referees should disagreements amongst
>> >   | maintainers arise
>> 
>> I understand this, and I agree as far as actual technical issues
>> go. I just don't think this is suitable / appropriate when it comes
>> to cosmetic questions.
>
>Well, if the cosmetic question is not that important then presumably
>someone would have given ground and it wouldn't need escalation.

In this specific case, I saw a disagreement, but it is not clear whether
this would need an escalation. It could easily be solved on IRC by a quick
chat.

>If the cosmetic question _is_ important enough to have a big debate
>over then I don't see any reason why the same escalation path is not
>appropriate.  Actually, it seems to me that a relative outsider is
>more likely to bring a useful sense of proportion.

However in general, I agree with Ian that "cosmetic" issues which are
contentious and where emotions are attached to it should be covered by the
governance. 


>(And, de jure, the governance doc makes no such distinction.)

As I said, these are intentionally not excluded. Partly, because cosmetic
issues are often more divisive than actual technical issues, and in many
FOSS communities these are exactly the sort of issues people fight over.
Of course, one would hope that we are all mature enough, that we don't
often need to invoke refereeing for it.

Lars

 


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

Re: differing opinions between maintainers vs patch acks

[Andrew Cooper]

On 04/05/17 08:59, Jan Beulich wrote:
> All,
>
> it's been a (not very often, but anyway) recurring situation that in
> order to get an ack on some patch I had to make adjustments which
> I didn't agree with. Since all maintainers opinions are supposed to be
> equal, it is not really clear to me whether in such cases it should
> really be the reviewing maintainer's rather than the submitting
> maintainer's opinion which controls what actually goes into the tree.
> When there's an odd number of maintainers for a given piece of
> code, it may be acceptable to pull in a 3rd maintainer to break ties,
> but pulling in a non-maintainer (e.g. some [other] committer) to
> help out seems not really appropriate to me.
>
> And just to clarify - such discussions aren't normally about aspects
> that affect how the resulting code would work, but just how the
> code should look like (see e.g. the thread rooted at
> https://lists.xenproject.org/archives/html/xen-devel/2017-05/msg00068.html
> for the most recent example, where the question is how to
> express numbers and how to name labels), i.e. things in the end
> often called "bike shedding".
>
> My proposal is for the submitting maintainer's taste to take
> preference over the reviewing maintainer's one in such cases.
> And just to avoid any doubt - I don't mean this to extend to
> cases where correctness of the code would be affected (albeit
> I admit there may still be cases left sitting in a gray area in the
> middle).

Taking this example, as you have called it out, but without going into
the details.

I accept that the issues under debate do not have any impact on the
technical correctness of the fix.  Once compiled/assembled, the binary
will function correctly.

However, the bikeshedding makes a very real material impact on the
understandability and reviewability of the code.

In my mind, all other things being equal, making the code easier to
understand and review is of paramount importance, and particularly in
this case, not making an already complicated bit of code harder to review.

~Andrew

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

Re: differing opinions between maintainers vs patch acks

[Ian Jackson]

Andrew Cooper writes ("Re: differing opinions between maintainers vs patch acks"):
> Taking this example, as you have called it out, but without going into
> the details.
> 
> I accept that the issues under debate do not have any impact on the
> technical correctness of the fix.  Once compiled/assembled, the binary
> will function correctly.
> 
> However, the bikeshedding makes a very real material impact on the
> understandability and reviewability of the code.
> 
> In my mind, all other things being equal, making the code easier to
> understand and review is of paramount importance, and particularly in
> this case, not making an already complicated bit of code harder to review.

Well, at one level I agree with Andrew on at least the 1*1 and 0*8
question.  These seem clearer to me as they state the programmer's
intent as well as merely the effect.  I found Jan's response hard to
understand; there doesn't actually seem to be a counterargument.  I
suspect if I thought about it enough I would agree with Andrew about
the labels too.

But, earlier I said:

   I definitely agree that there is room for giving the author of some
   code (whether they are a maintainer or not) some leeway on matters of
   taste.  I think, though, that while this ought to be a principle
   applied by maintainers, committers and anyone else making relevant
   decisions, it is not a rule of governance to be applied in contested
   cases.

I think this case falls clearly into the category of things where we
could give the original contributor some leeway.  In this case that
means Jan.

IOW if I were in Andrew's position I would probably make the same
requests he has done, but if Jan maintained his position I would
certainly not block the patch over this.


Stepping back a bit: It is indeed important that our code is easy to
understand and modify, expresses its intent clearly, and helps future
programmers avoid writing bugs.  But it is also important that
contributors feel valued, and feel a sense of ownership.

The amount of emotional discouragement to a contributor does not scale
linearly with the size and apparent importance of the disagreement.
Indeed, turning a tiny issue into a blocker or a big argument can be
especially demotivating.

I think this case is an example of a situation where we should pay a
small price in code readability to keep a contributor happy.  (That
the contributor is also a maintainer doesn't seem to change this
analysis for me.)


I doubt either side will be particularly happy with this analysis.
Sorry about that.

Regards,
Ian.

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

Re: differing opinions between maintainers vs patch acks

[Lars Kurth]

On 04/05/2017 13:44, "Ian Jackson" <ian.jackson@eu.citrix.com> wrote:

>Andrew Cooper writes ("Re: differing opinions between maintainers vs
>patch acks"):
>> Taking this example, as you have called it out, but without going into
>> the details.
>> 
>> I accept that the issues under debate do not have any impact on the
>> technical correctness of the fix.  Once compiled/assembled, the binary
>> will function correctly.
>> 
>> However, the bikeshedding makes a very real material impact on the
>> understandability and reviewability of the code.
>> 
>> In my mind, all other things being equal, making the code easier to
>> understand and review is of paramount importance, and particularly in
>> this case, not making an already complicated bit of code harder to
>>review.
>
>Well, at one level I agree with Andrew on at least the 1*1 and 0*8
>question.  These seem clearer to me as they state the programmer's
>intent as well as merely the effect.  I found Jan's response hard to
>understand; there doesn't actually seem to be a counterargument.  I
>suspect if I thought about it enough I would agree with Andrew about
>the labels too.
>
>But, earlier I said:
>
>   I definitely agree that there is room for giving the author of some
>   code (whether they are a maintainer or not) some leeway on matters of
>   taste.  I think, though, that while this ought to be a principle
>   applied by maintainers, committers and anyone else making relevant
>   decisions, it is not a rule of governance to be applied in contested
>   cases.
>
>I think this case falls clearly into the category of things where we
>could give the original contributor some leeway.  In this case that
>means Jan.
>
>IOW if I were in Andrew's position I would probably make the same
>requests he has done, but if Jan maintained his position I would
>certainly not block the patch over this.
>
>Stepping back a bit: It is indeed important that our code is easy to
>understand and modify, expresses its intent clearly, and helps future
>programmers avoid writing bugs.  But it is also important that
>contributors feel valued, and feel a sense of ownership.

Agreed.

>The amount of emotional discouragement to a contributor does not scale
>linearly with the size and apparent importance of the disagreement.
>Indeed, turning a tiny issue into a blocker or a big argument can be
>especially demotivating.

Agreed.

Lars

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

Re: differing opinions between maintainers vs patch acks

[Jan Beulich]

>>> On 04.05.17 at 14:44, <ian.jackson@eu.citrix.com> wrote:
> Andrew Cooper writes ("Re: differing opinions between maintainers vs patch 
> acks"):
>> Taking this example, as you have called it out, but without going into
>> the details.
>> 
>> I accept that the issues under debate do not have any impact on the
>> technical correctness of the fix.  Once compiled/assembled, the binary
>> will function correctly.
>> 
>> However, the bikeshedding makes a very real material impact on the
>> understandability and reviewability of the code.
>> 
>> In my mind, all other things being equal, making the code easier to
>> understand and review is of paramount importance, and particularly in
>> this case, not making an already complicated bit of code harder to review.
> 
> Well, at one level I agree with Andrew on at least the 1*1 and 0*8
> question.  These seem clearer to me as they state the programmer's
> intent as well as merely the effect.  I found Jan's response hard to
> understand; there doesn't actually seem to be a counterargument.

My counterargument was that 0*8 clearly equals 0 for anyone
knowledgeable enough to read this code, as does 1*8 = 8.
Anyway, seeing that you agree with Andrew, I'll go make the
change, no matter that I think it doesn't belong here (besides
being pointless).

>  I
> suspect if I thought about it enough I would agree with Andrew about
> the labels too.

Along those lines I'd then also go make the change here, if only
there was an alternative naming of the label tags that I can at
least live with; the suggestion to simply divide the numbers by 8
is, as expressed, not something I consider reasonable. So I'll
make my changing of those label tags dependent one someone
coming forward with a naming scheme which is both better than
what is there and better then using simple stack slot numbers
without it being clear that stack slots are being meant.

> But, earlier I said:
> 
>    I definitely agree that there is room for giving the author of some
>    code (whether they are a maintainer or not) some leeway on matters of
>    taste.  I think, though, that while this ought to be a principle
>    applied by maintainers, committers and anyone else making relevant
>    decisions, it is not a rule of governance to be applied in contested
>    cases.
> 
> I think this case falls clearly into the category of things where we
> could give the original contributor some leeway.  In this case that
> means Jan.
> 
> IOW if I were in Andrew's position I would probably make the same
> requests he has done, but if Jan maintained his position I would
> certainly not block the patch over this.
> 
> 
> Stepping back a bit: It is indeed important that our code is easy to
> understand and modify, expresses its intent clearly, and helps future
> programmers avoid writing bugs.  But it is also important that
> contributors feel valued, and feel a sense of ownership.
> 
> The amount of emotional discouragement to a contributor does not scale
> linearly with the size and apparent importance of the disagreement.
> Indeed, turning a tiny issue into a blocker or a big argument can be
> especially demotivating.
> 
> I think this case is an example of a situation where we should pay a
> small price in code readability to keep a contributor happy.  (That
> the contributor is also a maintainer doesn't seem to change this
> analysis for me.)
> 
> 
> I doubt either side will be particularly happy with this analysis.
> Sorry about that.

No reason to be sorry - happy or not, your reply at least gives me
an understanding of how others think here.

Jan


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

Re: differing opinions between maintainers vs patch acks

[Andrew Cooper]

On 04/05/17 13:54, Jan Beulich wrote:
>>>> On 04.05.17 at 14:44, <ian.jackson@eu.citrix.com> wrote:
>> Andrew Cooper writes ("Re: differing opinions between maintainers vs patch 
>> acks"):
>>> Taking this example, as you have called it out, but without going into
>>> the details.
>>>
>>> I accept that the issues under debate do not have any impact on the
>>> technical correctness of the fix.  Once compiled/assembled, the binary
>>> will function correctly.
>>>
>>> However, the bikeshedding makes a very real material impact on the
>>> understandability and reviewability of the code.
>>>
>>> In my mind, all other things being equal, making the code easier to
>>> understand and review is of paramount importance, and particularly in
>>> this case, not making an already complicated bit of code harder to review.
>> Well, at one level I agree with Andrew on at least the 1*1 and 0*8
>> question.  These seem clearer to me as they state the programmer's
>> intent as well as merely the effect.  I found Jan's response hard to
>> understand; there doesn't actually seem to be a counterargument.
> My counterargument was that 0*8 clearly equals 0 for anyone
> knowledgeable enough to read this code, as does 1*8 = 8.
> Anyway, seeing that you agree with Andrew, I'll go make the
> change, no matter that I think it doesn't belong here (besides
> being pointless).

Right, so the underlying issue here is the subjective nature of whether
this change is pointless or not.

You have made and argument for the changes being pointless, and I have
made an argument for the changes not being pointless.

For this type of problem, would it help if everyone made a more
conscious effort to work out when a subjective deadlock has been
reached, and try to actively involve a 3rd party to tie break?

>
>>  I
>> suspect if I thought about it enough I would agree with Andrew about
>> the labels too.
> Along those lines I'd then also go make the change here, if only
> there was an alternative naming of the label tags that I can at
> least live with; the suggestion to simply divide the numbers by 8
> is, as expressed, not something I consider reasonable. So I'll
> make my changing of those label tags dependent one someone
> coming forward with a naming scheme which is both better than
> what is there and better then using simple stack slot numbers
> without it being clear that stack slots are being meant.

I am afraid I cannot offer a helpful naming alternative beyond what has
already been discussed.

However, a comment explaining the meaning of the number suffixes would
go a very long way towards aiding the understandability of the code, at
which point leaving them as they are would be ok.

There are a lot of cases where I am happy with leaving the code "no
worse than it was before" (and I do try to identify these cases as they
appear during review), but the root of my objection in this case is my
(subjective) view that changes take an already-hard-to-understand piece
of code and make it worse by introducing inconsistencies in the way that
important pieces of information are expressed.

I am sorry this has flown so much out of proportion, especially as my
XSA followup which reimplements this in C was almost ready to be posted
to xen-devel already.  I am not deliberately trying to be awkward, but I
do care about improving the quality of the codebase, and it is clear
that we have different opinions on what qualifies as "obvious".

~Andrew

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

Re: differing opinions between maintainers vs patch acks

[Ian Jackson]

Jan Beulich writes ("Re: differing opinions between maintainers vs patch acks"):
> On 04.05.17 at 14:44, <ian.jackson@eu.citrix.com> wrote:
> > Well, at one level I agree with Andrew on at least the 1*1 and 0*8
> > question.  These seem clearer to me as they state the programmer's
> > intent as well as merely the effect.  I found Jan's response hard to
> > understand; there doesn't actually seem to be a counterargument.
> 
> My counterargument was that 0*8 clearly equals 0 for anyone
> knowledgeable enough to read this code, as does 1*8 = 8.
> Anyway, seeing that you agree with Andrew, I'll go make the
> change, no matter that I think it doesn't belong here (besides
> being pointless).

Thanks for being flexible.  I continue to think that in this case
Andrew ought to be showing flexibility.  Although of course if you
have been convinced (perhaps about the readability to others), that is
good to acknowledge even if implicitly.


I feel motivated to explain why I don't find your counterargument
convincing.  The reason why `0*8' is better than `0' (or complete
absence of an offset), and `1*8' is better than `8', is that it better
explains _why_ the value of zero was chosen.  Ie, where the value
comes from.

In particular `0*8' mentions 8 (the stride), whereas `0' doesn't
mention 8 at all and so could be some other kind of magic number; and
complete lack of an offset doesn't mention that in some underlying
sense is an offset which happens to be zero slots of size 8.

Ie, while `0*8' clearly implies `0', since everyone knows that 0*8 is
zero, `0' does not necessarily imply `0*8'.

A reader who sees this has to look slightly further to the surrounding
context etc., and expend slightly more cognitive effort, to see that
this code is equivalent to the `2*8' etc. earlier.

I don't know if this interjection helps at all.  Maybe I should just
let the conversation end now...

> >  I
> > suspect if I thought about it enough I would agree with Andrew about
> > the labels too.
> 
> Along those lines I'd then also go make the change here, if only
> there was an alternative naming of the label tags that I can at
> least live with; the suggestion to simply divide the numbers by 8
> is, as expressed, not something I consider reasonable. So I'll
> make my changing of those label tags dependent one someone
> coming forward with a naming scheme which is both better than
> what is there and better then using simple stack slot numbers
> without it being clear that stack slots are being meant.

I'm not sure why `blah_blah_stackslot3' etc. is not suitable.  But I
don't feel I have thought about this particular bikeshed enough to
have an opinion about the right shade of green.

Ian.

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

Re: differing opinions between maintainers vs patch acks

[Stefano Stabellini]

On Thu, 4 May 2017, Ian Jackson wrote:
> Stepping back a bit: It is indeed important that our code is easy to
> understand and modify, expresses its intent clearly, and helps future
> programmers avoid writing bugs.  But it is also important that
> contributors feel valued, and feel a sense of ownership.
> 
> The amount of emotional discouragement to a contributor does not scale
> linearly with the size and apparent importance of the disagreement.
> Indeed, turning a tiny issue into a blocker or a big argument can be
> especially demotivating.
> 
> I think this case is an example of a situation where we should pay a
> small price in code readability to keep a contributor happy.  (That
> the contributor is also a maintainer doesn't seem to change this
> analysis for me.)

Ian, this is very well written, and I completely agree with you. Worthy
of a short blog post :-)

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