Re: [Xen-devel] REST MAINTAINERS feedback requested Was:Re: [PATCH v5 01/28] HYPERCALL_version_op. New hypercall mirroring XENVER_ but sane.

[George Dunlap <george.dunlap@xxxxxxxxxx>]

On Thu, Apr 14, 2016 at 6:01 PM, Jan Beulich <jbeulich@xxxxxxxx> wrote:
>>Sure, mistakes happen; but I hope it's not being to controversial to
>>say that in general, the procedure should be arranged such that the
>>person who makes the mistake is the one who has to do deal with the
>>most consequences from the mistake, not the people around him.  I
>>mean, if you asked a bartender for a bottle of beer, and after he'd
>>already opened it you said, "Oh sorry, I actually wanted a cider", it
>>would be fair enough for the bartender to ask you to pay for the beer,
>>rather than eating it*, wouldn't it? :-)
>
> Sure. And I think that's what I've done. I suggested to revert the thing,
> collecting opinions either direction. I just didn't post a revert patch, as I
> think that makes little sense - a revert is a mechanical operation, which
> doesn't need people looking at the actual patch.

You don't need people to review the content of the actual patch, but
it provides a standardized way to have a discussion about the patch,
and it makes it clear what the procedure is for having it applied.
You don't need any technical code review for adding or removing
maintainers either, but we ask people to send patches to the
MAINTAINERS file anyway, because it provides a structured way to have
an open discussion and make a decision.

When Konrad asked for further input on the patch and then didn't get
any in a few days, you responded, "It looks like it will have to be
reverted then." As I've said, I think that's the wrong way round --
it's not that the commit is reverted unless someone else acks it; it's
that the commit stays unless someone acks your reversion.  If you had
posted a patch (probably RFC) requesting to revert the change in favor
of a different one, then it would have been more obvious that the
burden was on you to get the reversion Acked, rather than on Konrad to
get the existing commit re-acked.

>>Well Ian and I have already given our opinions -- Ian thinks moving to
>>a clean interface and deprecating the old one is in general a good
>>thing, and doesn't look too painful in this case.  I don't really see
>>a problem with using a large fixed size, but I don't fundamentally see
>>a problem with moving to a new clean interface either.  Given that
>>Andy has a strong aversion to the way things are, if you had only a
>>mild distaste rather than a  strong objection to the new hypercall, it
>>would probably make sense to go with the new hypercall.
>>
>>If you do have a strong objection, then maybe we could try to convince
>>Andy to accept adding subops with different calling semantics to the
>>existing hypercall.  But I would still think the burden of persuasion
>>is primarily on you.
>
>  I do not have a strong objection, or else I would have converted my ack
> into a nak instead of just withdrawing it. I just dislike the duplication, and
> hence I'm not happy with me now being the one having approved it to go
> in. Hence the request for a replacement ack (or whatever else referee
> decision).

Well this makes it sound like you're saying that you were asking us to
save you from having to appear to have approved of a patch that you
didn't like.  Which doesn't sound very nice. :-)

But I wonder if something slightly different is going on.  Forgive me
for trying to guess at motivations here, but I think it may be
helpful.  I'm often in the situation where my gut objects to a patch
that other coders I respect think is fine; and often a few years down
the road, I look back and agree that it's fine as well.  In other
words, I know that sometimes my own objections turn out to be
unreasonable; and that in any case, working with other people you
sometimes have to compromise.  But on the other hand, I've also had
the experience of giving in and accepting patches that later I regret,
or that other people come back and say, "This was a terrible idea, you
should have stood up for it more."

So in the moment -- particularly, as you say, under time pressure --
how do you determine if objecting to the patch is being unreasonable
and obstructive, or if assenting to the patch is failing your duty as
a maintainer to prevent bad code, and is a decision you'll regret
later?

Was it perhaps actually the case that your internal reasoning was
along the lines of, "Actually, adding a new hypercall seems like a bad
idea.  But since Andrew strongly disagrees, maybe I'm being
unreasonable.  Or, maybe it really is a bad idea and he's being
unreasonable.  Why don't I ask the REST maintainers to look at it; if
they Ack it, then I'm probably being too conservative; if they don't,
then I'm probably justified in objecting to it"?

If so, we should probably find a better way for maintainers to ask for
additional feedback in those situations. :-)  I'd certainly appreciate
that at times.

If that's not the case, and you genuinely only have a mild dislike for
the hypercall, then there are a couple of things to say about that.
The first is that given the circumstances, it seems to me that giving
the Ack was not only reasonable, but the right thing to do.  An Ack
doesn't mean, "I think this is a good idea", but it means "Given all
considerations, I think this should be in the tree."  And given that
Andrew had strong objections to the other solutions, and you only had
a distaste for this one, this is obviously the solution to choose.
Acking a patch on the basis that you don't really like it but it's the
best compromise with the other maintainers is perfectly reasonable.

Secondly,  I can certainly see that you might be a bit embarrased
about having your name on a patch you dislike, but... well, you did in
fact approve it going in. :-)  Maybe that was because you were in
haste, and you regret it, but that's an accurate record of what
happened.  Stand up and take responsibility for your actions. :-)

> And btw., considering that Konrad has already posted a revert patch,
> and I have ack-ed that one, this could now go in right away (and the
> discussion could either be settled or start over).

Yes, unless someone objects to that patch then we have a clear way
forward, and we're just discussing principles for future reference at
this point.

 -George

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