Re: [Xen-devel] Is: Reviewed-by, Acked-by rules. Was:Re: [PATCH v7 10/19] x86/VPMU: Make vpmu not HVM-specific

[George Dunlap <dunlapg@xxxxxxxxx>]

On Wed, Jun 11, 2014 at 9:25 PM, Jan Beulich <jbeulich@xxxxxxxx> wrote:
>>>> Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx> 06/11/14 3:57 PM >>>
>>On Wed, Jun 11, 2014 at 11:07:14AM +0100, Jan Beulich wrote:
>>> I pointed this out on an earlier version of the series, perhaps on
>>> another patch (but comments like this should be taken to apply
>>> globally): An ack by a non-maintainer of any of the code being
>>> modified is bogus/confusing - it should either be Reviewed-by or
>>> left off.
>>
>>That is at odds with what the Linux style has:
>
> I think you said mostly the same when I made a similar statement a couple
> of months ago; nothing has changed since then afaict.
>
>>Now obviously Xen != Linux, but if we want to this 'Acked-by' or
>>'Reviewed-by' policy to be different from Linux (which I think we
>>should not), we should have it written somewhere so folks do not
>>get confused.
>
> Exactly - Xen != Linux. We've been following the maintainer-only-ack
> style for quite a while now, with infrequent acks from non-maintainers
> just ignored (or applied) silently. With this series however, I felt it would
> be better to mention the misuse as it has been on more than one patch.

I'm not sure why you *object* to non-maintainer Acks, Jan -- people
who are either pushing the patch or applying the patch need to know
who the maintainers are anyway, if for no other reason to know when a
"Reviewed-by" counts as a maintainer Ack or not.

It seems to me there are several times when non-maintainer Acks are
useful; for example, when someone has previously objected to a patch,
and is now OK with it, but has not actually reviewed it.

>>And I think George has a different opinion of when Reviewed-by
>>get carried on patches that undergo modifications. Sadly I don't see
>>anything in the Linux Documentation/Submitting* to explain what
>>the rules are for that.
>
> I don't recall having a "different" opinion here - at least I agree: With
> non-benign changes to a patch, acks and reviews should be dropped.
> Fixing bugs in a patch - which I think is the recent case you recall -
> is clearly a non-benign change.
>
> In fact I would go farther and suggest that more than a week or two
> old tags should also get dropped. There have been a number of cases
> not that long ago that new versions of patch series got posted weeks
> if not months after the most recent earlier versions. And among them
> have been cases where I wasn't really certain whether a tag with my
> name was actually validly there, or valid anymore.

FWIW, one example was Dario's soft-affinity series; when he was here
before the hackathon we actually discussed what the protocol should be
for that.  I didn't think it was clear, so I said he should retain
them, and people could complain if that's not what they wanted. :-)

For my part, I tend to appreciate being reminded of what my opinion
was the last time around, so I don't think time itself should expire
my tags.  The tags should expire when either the patch itself, or the
code it depends on, has changed in a functionally significant way.  It
should be the job of the person submitting the code to keep track of
that.

The only risk to doing things this way, I suppose, would be if code
the patch depends on had changed in a way that was incompatible, and
the patch author didn't notice; the "Reviewed-by" might lull the
maintainer into a false sense of security.

 -George

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