Re: [Qemu-devel] [PATCH v5] Xen PV Device

["Andreas Färber" <afaerber@suse.de>]

Am 08.07.2013 16:10, schrieb Peter Maydell:
> On 8 July 2013 15:04, Anthony Liguori <anthony@codemonkey.ws> wrote:
>> (Just a nit and responding because this happens commonly).
>>
>> You probably mean Reviewed-by.  Acked-by really means, "I am not the
>> maintainer of this area, I have not reviewed this patch, but I am
>> generally okay with the idea as best I can tell."
> 
> Don't you mean "I *am* the maintainer of this area" ? I've always
> assumed it means "as the maintainer I have a potential veto over
> this code change and I am explicitly not exercising it even though
> I may not have done a complete review and/or test"...

I think Anthony was referring to: if I am the maintainer I don't usually
put tags on patches but pick them up and add my Signed-off-by.
(Possible exception: when only part of a series is good and you don't
feel like cherry-picking from it.)

Also we have an increasing number of series (CPUState, PCI, IOMMU) that
touch things across the tree with more than one maintainer involved,
where in my case I'm happy about getting Acked-bys at all. :)

>> It's a very low vote of confidence.  I wouldn't apply a patch that only
>> had Acked-bys.
>>
>> OTOH, Reviewed-by means, "I have reviewed the patch and believe it works
>> as described and meets project guidelines".  Based on your review of V4,
>> pretty sure that's what you mean here.
>>
>> https://github.com/torvalds/linux/blob/master/Documentation/SubmittingPatches#L392
>>
>> The distinction matters in practice because I have scripts to track
>> patches based on whether they've received Reviewed-bys or not.  I'm
>> often running into cases where people are Acked-by'ing instead of
>> Reviewed-by'ing patches and then wondering why they haven't gotten
>> merged...
> 
> I think Andreas is the major exponent of the idea that "acked-by"
> is stronger than "reviewed-by". Regardless, I think we should
> standardise on what we mean by both tags. (Alas the kernel docs
> are not entirely clear about acked-by, though the meaning of
> reviewed-by is certainly clear.)

Exponent? I am not advocating it's "stronger". However I believe that
they do express different things - ack may express that the patch has
been looked at and possibly tested but may contain stylistic or
convention nits, whereas Reviewed-by says the code looks fine but is not
guaranteed to build on the supported platforms. Adding a Reviewed-by on
an obvious spelling fix so that it gets picked up already feels silly to
me, and adding two tags (Reviewed-by+Tested-by or
Acked-by+Reviewed-by+Tested-by) even more so.

We could certainly use some clear guidance on the Wiki. Volunteers?

Cheers,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg