Spurious Acks (was Re: PVH domU patches....)

Spurious Acks (was Re: PVH domU patches....)

[Tim Deegan]

Hi,

At 14:43 -0700 on 31 Jul (1375281803), Mukesh Rathor wrote:
> The latest tree with Tim's acks are at:
> 
>    git clone git://oss.oracle.com/git/mrathor/xen.git .
>    git checkout pvh.v10.acked-1

This branch has my Reviewed-by: on 1f2087845751569fc55c202ac3265e18c974b0bf
(PVH xen: vmcs related changes), which I don't remember giving. 
Please be careful about that sort of thing.  

There have been a few instances in the past of patches that went in on
someone else's ack that seem to have sprouted mine (not from Mukesh, I
should add, and AFAICT through misunderstanding rather than malice).
In future I am going to revert such patches when I notice them.

Cheers,

Tim.

Re: Spurious Acks (was Re: PVH domU patches....)

[Mukesh Rathor]

On Thu, 1 Aug 2013 11:21:33 +0100
Tim Deegan <tim@xen.org> wrote:

> Hi,
> 
> At 14:43 -0700 on 31 Jul (1375281803), Mukesh Rathor wrote:
> > The latest tree with Tim's acks are at:
> > 
> >    git clone git://oss.oracle.com/git/mrathor/xen.git .
> >    git checkout pvh.v10.acked-1
> 
> This branch has my Reviewed-by: on
> 1f2087845751569fc55c202ac3265e18c974b0bf (PVH xen: vmcs related
> changes), which I don't remember giving. Please be careful about that
> sort of thing.  
> 
> There have been a few instances in the past of patches that went in on
> someone else's ack that seem to have sprouted mine (not from Mukesh, I
> should add, and AFAICT through misunderstanding rather than malice).
> In future I am going to revert such patches when I notice them.
> 
> Cheers,
> 
> Tim.


Ok, I misunderstood whey you said the code looked OK, I assumed it was
an implicit ack, as I've seen here in the past. I'll make a note, Tim
doesn't give implicit acks... :)...

I'll remove your ack.

Mukesh

Re: Spurious Acks (was Re: PVH domU patches....)

[George Dunlap]

On Mon, Aug 5, 2013 at 10:18 PM, Mukesh Rathor <mukesh.rathor@oracle.com> wrote:
> On Thu, 1 Aug 2013 11:21:33 +0100
> Tim Deegan <tim@xen.org> wrote:
>
>> Hi,
>>
>> At 14:43 -0700 on 31 Jul (1375281803), Mukesh Rathor wrote:
>> > The latest tree with Tim's acks are at:
>> >
>> >    git clone git://oss.oracle.com/git/mrathor/xen.git .
>> >    git checkout pvh.v10.acked-1
>>
>> This branch has my Reviewed-by: on
>> 1f2087845751569fc55c202ac3265e18c974b0bf (PVH xen: vmcs related
>> changes), which I don't remember giving. Please be careful about that
>> sort of thing.
>>
>> There have been a few instances in the past of patches that went in on
>> someone else's ack that seem to have sprouted mine (not from Mukesh, I
>> should add, and AFAICT through misunderstanding rather than malice).
>> In future I am going to revert such patches when I notice them.
>>
>> Cheers,
>>
>> Tim.
>
>
> Ok, I misunderstood whey you said the code looked OK, I assumed it was
> an implicit ack, as I've seen here in the past. I'll make a note, Tim
> doesn't give implicit acks... :)...
>
> I'll remove your ack.

I'm pretty sure no one gives implicit Acks.  Saying the code looks OK
is just that -- it says the code looks OK, not that the person is OK
with the code going in, and absolutely not everything that
"Reviewed-by" means (which is a lot more than an Ack).

 -George

Re: Spurious Acks (was Re: PVH domU patches....)

[Ian Campbell]

On Tue, 2013-08-06 at 11:00 +0100, George Dunlap wrote:
> On Mon, Aug 5, 2013 at 10:18 PM, Mukesh Rathor <mukesh.rathor@oracle.com> wrote:
> > On Thu, 1 Aug 2013 11:21:33 +0100
> > Tim Deegan <tim@xen.org> wrote:
> >
> >> Hi,
> >>
> >> At 14:43 -0700 on 31 Jul (1375281803), Mukesh Rathor wrote:
> >> > The latest tree with Tim's acks are at:
> >> >
> >> >    git clone git://oss.oracle.com/git/mrathor/xen.git .
> >> >    git checkout pvh.v10.acked-1
> >>
> >> This branch has my Reviewed-by: on
> >> 1f2087845751569fc55c202ac3265e18c974b0bf (PVH xen: vmcs related
> >> changes), which I don't remember giving. Please be careful about that
> >> sort of thing.
> >>
> >> There have been a few instances in the past of patches that went in on
> >> someone else's ack that seem to have sprouted mine (not from Mukesh, I
> >> should add, and AFAICT through misunderstanding rather than malice).
> >> In future I am going to revert such patches when I notice them.
> >>
> >> Cheers,
> >>
> >> Tim.
> >
> >
> > Ok, I misunderstood whey you said the code looked OK, I assumed it was
> > an implicit ack, as I've seen here in the past. I'll make a note, Tim
> > doesn't give implicit acks... :)...
> >
> > I'll remove your ack.
> 
> I'm pretty sure no one gives implicit Acks.  Saying the code looks OK
> is just that -- it says the code looks OK, not that the person is OK
> with the code going in, and absolutely not everything that
> "Reviewed-by" means (which is a lot more than an Ack).

There are some maintainers who say something informal but when asked
"can I take that as an ack" say yes, which eventually leads to laziness
on the part of submitters (and indeed committers) who get bored of
asking the clarify question. It doesn't seem to be happening as much as
it once was though.

I think people, and maintainers in particular, need to get in the habit
of spelling the ack or review-by out explicitly, because ambiguity does
lead to confusion here.

Ian

Re: Spurious Acks (was Re: PVH domU patches....)

[Konrad Rzeszutek Wilk]

On Tue, Aug 06, 2013 at 11:00:37AM +0100, George Dunlap wrote:
> On Mon, Aug 5, 2013 at 10:18 PM, Mukesh Rathor <mukesh.rathor@oracle.com> wrote:
> > On Thu, 1 Aug 2013 11:21:33 +0100
> > Tim Deegan <tim@xen.org> wrote:
> >
> >> Hi,
> >>
> >> At 14:43 -0700 on 31 Jul (1375281803), Mukesh Rathor wrote:
> >> > The latest tree with Tim's acks are at:
> >> >
> >> >    git clone git://oss.oracle.com/git/mrathor/xen.git .
> >> >    git checkout pvh.v10.acked-1
> >>
> >> This branch has my Reviewed-by: on
> >> 1f2087845751569fc55c202ac3265e18c974b0bf (PVH xen: vmcs related
> >> changes), which I don't remember giving. Please be careful about that
> >> sort of thing.
> >>
> >> There have been a few instances in the past of patches that went in on
> >> someone else's ack that seem to have sprouted mine (not from Mukesh, I
> >> should add, and AFAICT through misunderstanding rather than malice).
> >> In future I am going to revert such patches when I notice them.
> >>
> >> Cheers,
> >>
> >> Tim.
> >
> >
> > Ok, I misunderstood whey you said the code looked OK, I assumed it was
> > an implicit ack, as I've seen here in the past. I'll make a note, Tim
> > doesn't give implicit acks... :)...
> >
> > I'll remove your ack.
> 
> I'm pretty sure no one gives implicit Acks.  Saying the code looks OK

I do. If I say 'code looks OK to me' that implies to me 'Acked-by'.

That is similar to how Linux works (from Documentation/SubmittingPatches):

"                                                                      
Acked-by: is not as formal as Signed-off-by:.  It is a record that the acker       
has at least reviewed the patch and has indicated acceptance.  Hence patch         
mergers will sometimes manually convert an acker's "yep, looks good to me"         
into an Acked-by:."


> is just that -- it says the code looks OK, not that the person is OK
> with the code going in, and absolutely not everything that
> "Reviewed-by" means (which is a lot more than an Ack).
> 
>  -George
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: Spurious Acks (was Re: PVH domU patches....)

[Tim Deegan]

At 09:00 -0400 on 06 Aug (1375779658), Konrad Rzeszutek Wilk wrote:
> On Tue, Aug 06, 2013 at 11:00:37AM +0100, George Dunlap wrote:
> > On Mon, Aug 5, 2013 at 10:18 PM, Mukesh Rathor <mukesh.rathor@oracle.com> wrote:
> > > On Thu, 1 Aug 2013 11:21:33 +0100
> > > Tim Deegan <tim@xen.org> wrote:
> > >
> > >> Hi,
> > >>
> > >> At 14:43 -0700 on 31 Jul (1375281803), Mukesh Rathor wrote:
> > >> > The latest tree with Tim's acks are at:
> > >> >
> > >> >    git clone git://oss.oracle.com/git/mrathor/xen.git .
> > >> >    git checkout pvh.v10.acked-1
> > >>
> > >> This branch has my Reviewed-by: on
> > >> 1f2087845751569fc55c202ac3265e18c974b0bf (PVH xen: vmcs related
> > >> changes), which I don't remember giving. Please be careful about that
> > >> sort of thing.
> > >>
> > >> There have been a few instances in the past of patches that went in on
> > >> someone else's ack that seem to have sprouted mine (not from Mukesh, I
> > >> should add, and AFAICT through misunderstanding rather than malice).
> > >> In future I am going to revert such patches when I notice them.
> > >>
> > >> Cheers,
> > >>
> > >> Tim.
> > >
> > >
> > > Ok, I misunderstood whey you said the code looked OK, I assumed it was
> > > an implicit ack, as I've seen here in the past. I'll make a note, Tim
> > > doesn't give implicit acks... :)...

That made me go back and look again -- what I said was: 

| If these aren't already committed, you can add my Reviewed-by to all
| except 9, 10, 19 and 23.
[...]
| #19 is probably OK for correctness, though I haven't reviewed the VMCS
| settings in enough detail to be sure they're complete.  My main
| reservation is that it seems to duplicate a bunch of code from the
| HVM VMCS setup.

which doesn't look to me anything like an implicit ack, much less
a Reviewed-by -- in fact I explicitly called out #19 as _not_ being
Reviewed-by.

But yes, I don't give implicit acks.  And I'll try to be clearer in
future when I'm commenting on code that already has a relevant
maintainer's ack, as that seems to be where the confusion arises.
I think I need a way of saying "I found no bugs in this patch but I
still don't like it".

Cheers,

Tim.

Re: Spurious Acks (was Re: PVH domU patches....)

[Wei Liu]

On Tue, Aug 06, 2013 at 09:00:58AM -0400, Konrad Rzeszutek Wilk wrote:
> On Tue, Aug 06, 2013 at 11:00:37AM +0100, George Dunlap wrote:
> > On Mon, Aug 5, 2013 at 10:18 PM, Mukesh Rathor <mukesh.rathor@oracle.com> wrote:
> > > On Thu, 1 Aug 2013 11:21:33 +0100
> > > Tim Deegan <tim@xen.org> wrote:
> > >
> > >> Hi,
> > >>
> > >> At 14:43 -0700 on 31 Jul (1375281803), Mukesh Rathor wrote:
> > >> > The latest tree with Tim's acks are at:
> > >> >
> > >> >    git clone git://oss.oracle.com/git/mrathor/xen.git .
> > >> >    git checkout pvh.v10.acked-1
> > >>
> > >> This branch has my Reviewed-by: on
> > >> 1f2087845751569fc55c202ac3265e18c974b0bf (PVH xen: vmcs related
> > >> changes), which I don't remember giving. Please be careful about that
> > >> sort of thing.
> > >>
> > >> There have been a few instances in the past of patches that went in on
> > >> someone else's ack that seem to have sprouted mine (not from Mukesh, I
> > >> should add, and AFAICT through misunderstanding rather than malice).
> > >> In future I am going to revert such patches when I notice them.
> > >>
> > >> Cheers,
> > >>
> > >> Tim.
> > >
> > >
> > > Ok, I misunderstood whey you said the code looked OK, I assumed it was
> > > an implicit ack, as I've seen here in the past. I'll make a note, Tim
> > > doesn't give implicit acks... :)...
> > >
> > > I'll remove your ack.
> > 
> > I'm pretty sure no one gives implicit Acks.  Saying the code looks OK
> 
> I do. If I say 'code looks OK to me' that implies to me 'Acked-by'.
> 
> That is similar to how Linux works (from Documentation/SubmittingPatches):
> 
> "                                                                      
> Acked-by: is not as formal as Signed-off-by:.  It is a record that the acker       
> has at least reviewed the patch and has indicated acceptance.  Hence patch         

"at least reviewed the patch"? That's news to me. Geroge once told me
that Acked-by only means "I'm OK with this idea, I don't even look at the
patch at all". I'm quite confused here. :-(


Wei.

> mergers will sometimes manually convert an acker's "yep, looks good to me"         
> into an Acked-by:."
> 
> 
> > is just that -- it says the code looks OK, not that the person is OK
> > with the code going in, and absolutely not everything that
> > "Reviewed-by" means (which is a lot more than an Ack).
> > 
> >  -George
> > 
> > _______________________________________________
> > Xen-devel mailing list
> > Xen-devel@lists.xen.org
> > http://lists.xen.org/xen-devel
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: Spurious Acks (was Re: PVH domU patches....)

[Konrad Rzeszutek Wilk]

On Tue, Aug 06, 2013 at 02:43:19PM +0100, Wei Liu wrote:
> On Tue, Aug 06, 2013 at 09:00:58AM -0400, Konrad Rzeszutek Wilk wrote:
> > On Tue, Aug 06, 2013 at 11:00:37AM +0100, George Dunlap wrote:
> > > On Mon, Aug 5, 2013 at 10:18 PM, Mukesh Rathor <mukesh.rathor@oracle.com> wrote:
> > > > On Thu, 1 Aug 2013 11:21:33 +0100
> > > > Tim Deegan <tim@xen.org> wrote:
> > > >
> > > >> Hi,
> > > >>
> > > >> At 14:43 -0700 on 31 Jul (1375281803), Mukesh Rathor wrote:
> > > >> > The latest tree with Tim's acks are at:
> > > >> >
> > > >> >    git clone git://oss.oracle.com/git/mrathor/xen.git .
> > > >> >    git checkout pvh.v10.acked-1
> > > >>
> > > >> This branch has my Reviewed-by: on
> > > >> 1f2087845751569fc55c202ac3265e18c974b0bf (PVH xen: vmcs related
> > > >> changes), which I don't remember giving. Please be careful about that
> > > >> sort of thing.
> > > >>
> > > >> There have been a few instances in the past of patches that went in on
> > > >> someone else's ack that seem to have sprouted mine (not from Mukesh, I
> > > >> should add, and AFAICT through misunderstanding rather than malice).
> > > >> In future I am going to revert such patches when I notice them.
> > > >>
> > > >> Cheers,
> > > >>
> > > >> Tim.
> > > >
> > > >
> > > > Ok, I misunderstood whey you said the code looked OK, I assumed it was
> > > > an implicit ack, as I've seen here in the past. I'll make a note, Tim
> > > > doesn't give implicit acks... :)...
> > > >
> > > > I'll remove your ack.
> > > 
> > > I'm pretty sure no one gives implicit Acks.  Saying the code looks OK
> > 
> > I do. If I say 'code looks OK to me' that implies to me 'Acked-by'.
> > 
> > That is similar to how Linux works (from Documentation/SubmittingPatches):
> > 
> > "                                                                      
> > Acked-by: is not as formal as Signed-off-by:.  It is a record that the acker       
> > has at least reviewed the patch and has indicated acceptance.  Hence patch         
> 
> "at least reviewed the patch"? That's news to me. Geroge once told me
> that Acked-by only means "I'm OK with this idea, I don't even look at the
> patch at all". I'm quite confused here. :-(

Maybe we should copy the SubmittingPatches from Linux in the file so we
have it in there and just base it on that?

> 
> 
> Wei.
> 
> > mergers will sometimes manually convert an acker's "yep, looks good to me"         
> > into an Acked-by:."
> > 
> > 
> > > is just that -- it says the code looks OK, not that the person is OK
> > > with the code going in, and absolutely not everything that
> > > "Reviewed-by" means (which is a lot more than an Ack).
> > > 
> > >  -George
> > > 
> > > _______________________________________________
> > > Xen-devel mailing list
> > > Xen-devel@lists.xen.org
> > > http://lists.xen.org/xen-devel
> > 
> > _______________________________________________
> > Xen-devel mailing list
> > Xen-devel@lists.xen.org
> > http://lists.xen.org/xen-devel

Re: Spurious Acks (was Re: PVH domU patches....)

[Jan Beulich]

>>> On 08.08.13 at 02:46, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:
> Maybe we should copy the SubmittingPatches from Linux in the file so we
> have it in there and just base it on that?

Our flow is a bit different from theirs, so doing so without some
editing might cause more confusion than clarification.

Jan

Re: Spurious Acks (was Re: PVH domU patches....)

[Konrad Rzeszutek Wilk]

On Thu, Aug 08, 2013 at 08:24:06AM +0100, Jan Beulich wrote:
> >>> On 08.08.13 at 02:46, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:
> > Maybe we should copy the SubmittingPatches from Linux in the file so we
> > have it in there and just base it on that?
> 
> Our flow is a bit different from theirs, so doing so without some
> editing might cause more confusion than clarification.

Sounds a like a perfect candidate then - otherwise we are bound to hit
issues when folks are used to Linux try to develop in Xen and hit the
process problem.

I will post a patch with the initial copy of SubmittingPatches and then
some on follow patches with some modifications of what I think had been
discussed in the past.

> 
> Jan
>