[dpdk-dev] Understanding of Acked-By

[Ferruh Yigit]

On 1/27/2017 10:32 AM, Mcnamara, John wrote:
> 
> 
>> -----Original Message-----
>> From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Shreyansh Jain
>> Sent: Friday, January 27, 2017 10:25 AM
>> To: Richardson, Bruce <bruce.richardson at intel.com>
>> Cc: Thomas Monjalon <thomas.monjalon at 6wind.com>; Van Haaren, Harry
>> <harry.van.haaren at intel.com>; dev at dpdk.org; Yigit, Ferruh
>> <ferruh.yigit at intel.com>; Igor Ryzhov <iryzhov at nfware.com>; Steve Shin
>> <jonshin at cisco.com>
>> Subject: Re: [dpdk-dev] Understanding of Acked-By
>>
>> On Friday 27 January 2017 03:43 PM, Bruce Richardson wrote:
>>> On Fri, Jan 27, 2017 at 12:48:06PM +0530, Shreyansh Jain wrote:
>>>> On Wednesday 25 January 2017 08:28 PM, Thomas Monjalon wrote:
>>>>> 2017-01-25 13:53, Van Haaren, Harry:
>>>>>> There was an idea (from Thomas) to better document the Acked-by and
>> Reviewed-By in the above thread, which I think is worth doing to make the
>> process clearer. I'll kick off a thread*, and offer to submit a patch for
>> the documentation when a consensus is reached.
>>>>>>
>>>>>>
>>>>>> The question that needs to be addressed is "What is the most powerful
>> signoff to add as somebody who checked a patch?"
>>>>>
>>>>> I do not see the benefit of knowing the most powerful.
>>>>> Anyway, the most powerful tags are done by trusted people.
>>>>> And people are trusted after delivering good reviews or patches ;)
>>>>>
>>>>> The question should be "How to use the tags?"
>>>>>
>>>>>> The documentation mentions Acked, Reviewed, and Tested by[1], as
>> signoffs that can be commented on patches. The Review Process[2] section
>> mentions Reviewed and Tested by, but nowhere specifically states what any
>> of these indicate.
>>>>>>
>>>>>> Offered below is my current understanding of the Acked-by; Reviewed-
>> by; and Tested-by tags, in order of least-powerful first:
>>>>>>
>>>>>>
>>>>>> 3) Tested-by: (least powerful)
>>>>>>   - Indicates having passed testing of functionality, and works as
>> expected for Tester
>>>>>>   - Does NOT include full code review (instead use Reviewed by)
>>>>>>   - Does NOT indicate that the Tester understands architecture
>>>>>> (instead use Acked by)
>>>>>>
>>>>>>
>>>>>> 2) Reviewed-by:
>>>>>>   - Indicates having passed code-review, checkpatch and compilation
>>>>>> testing by Reviewer
>>>>>
>>>>> Compilation testing is done by the CI.
>>>>> The reviewer must just check the results.
>>>>>
>>>>>>   - Does NOT include full testing of functionality (instead use
>> Tested-by)
>>>>>>   - Does NOT indicate that the Reviewer understands architecture
>>>>>> (instead use Acked by)
>>>>>
>>>>> I disagree here.
>>>>> The reviewer must understand the impacts of the patch.
>>>>> That's why a Reviewed-by tag is really strong.
>>>>
>>>> From what I understand, 'Reviewed-by' and 'Acked-by' are the other
>>>> way around.
>>>> - Acked-by is intent that 'I agree with change'.
>>>> - Reviewed-by is 'I vouch for the changes' either through review or
>>>>   testing or both.
>>>>
>>>
>>> Other way round in what way - compared to proposed by Harry or by
>>> Thomas? Which do you view as the stronger indication that the patch is
>>> ok?
>>
>> Sorry, I should have posted this against Harry's mail rather than Thomas'.
>> 'Other way round' as compared to Harry's text.
>> Reviewed-by is a strong indication, in my understanding.
> 
> 
> Hi,
> 
> Maybe we should just follow the Kernel guidelines on this:
> 
> https://www.kernel.org/doc/html/latest/process/submitting-patches.html#when-to-use-acked-by-and-cc
> 
> 
> And to, save-you-a-click(tm) here is the relevant sections of the doc:
> 
> 
> 12) When to use Acked-by: and Cc:
> ---------------------------------
> 
> The Signed-off-by: tag indicates that the signer was involved in the
> development of the patch, or that he/she was in the patch's delivery path.
> 
> If a person was not directly involved in the preparation or handling of a
> patch but wishes to signify and record their approval of it then they can
> ask to have an Acked-by: line added to the patch's changelog.
> 
> Acked-by: is often used by the maintainer of the affected code when that
> maintainer neither contributed to nor forwarded the patch.
> 
> 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.  

I want to highlight this part.
Ack means patch _at least_ reviewed, I was expecting a patch not acked
without a review.
That is why I believe Ack is a superset of Reviewed-by. Because it means
patch is reviewed and agreed by a knowledgeable person of that area.

> Hence patch
> mergers will sometimes manually convert an acker's "yep, looks good to me"
> into an Acked-by: (but note that it is usually better to ask for an
> explicit ack).
> 
> Acked-by: does not necessarily indicate acknowledgement of the entire patch.
> For example, if a patch affects multiple subsystems and has an Acked-by: from
> one subsystem maintainer then this usually indicates acknowledgement of just
> the part which affects that maintainer's code.  Judgement should be used here.
> When in doubt people should refer to the original discussion in the mailing
> list archives.
> 
> If a person has had the opportunity to comment on a patch, but has not
> provided such comments, you may optionally add a ``Cc:`` tag to the patch.
> This is the only tag which might be added without an explicit action by the
> person it names - but it should indicate that this person was copied on the
> patch.  This tag documents that potentially interested parties
> have been included in the discussion.
> 
> 
> 13) Using Reported-by:, Tested-by:, Reviewed-by:, Suggested-by: and Fixes:
> --------------------------------------------------------------------------
> 
> The Reported-by tag gives credit to people who find bugs and report them and it
> hopefully inspires them to help us again in the future.  Please note that if
> the bug was reported in private, then ask for permission first before using the
> Reported-by tag.
> 
> A Tested-by: tag indicates that the patch has been successfully tested (in
> some environment) by the person named.  This tag informs maintainers that
> some testing has been performed, provides a means to locate testers for
> future patches, and ensures credit for the testers.
> 
> Reviewed-by:, instead, indicates that the patch has been reviewed and found
> acceptable according to the Reviewer's Statement:
> 
> Reviewer's statement of oversight
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> 
> By offering my Reviewed-by: tag, I state that:
> 
> 	 (a) I have carried out a technical review of this patch to
> 	     evaluate its appropriateness and readiness for inclusion into
> 	     the mainline kernel.
> 
> 	 (b) Any problems, concerns, or questions relating to the patch
> 	     have been communicated back to the submitter.  I am satisfied
> 	     with the submitter's response to my comments.
> 
> 	 (c) While there may be things that could be improved with this
> 	     submission, I believe that it is, at this time, (1) a
> 	     worthwhile modification to the kernel, and (2) free of known
> 	     issues which would argue against its inclusion.
> 
> 	 (d) While I have reviewed the patch and believe it to be sound, I
> 	     do not (unless explicitly stated elsewhere) make any
> 	     warranties or guarantees that it will achieve its stated
> 	     purpose or function properly in any given situation.
> 
> A Reviewed-by tag is a statement of opinion that the patch is an
> appropriate modification of the kernel without any remaining serious
> technical issues.  Any interested reviewer (who has done the work) can
> offer a Reviewed-by tag for a patch.  

And this part, anyone can do the review, and add reviewed-by to state
his/her opinion. It can be strong or not depending who did the review.

> This tag serves to give credit to
> reviewers and to inform maintainers of the degree of review which has been
> done on the patch.  Reviewed-by: tags, when supplied by reviewers known to
> understand the subject area and to perform thorough reviews, will normally
> increase the likelihood of your patch getting into the kernel.

And Reviewed-by helps maintainers to Ack the patch.

> 
> A Suggested-by: tag indicates that the patch idea is suggested by the person
> named and ensures credit to the person for the idea. Please note that this
> tag should not be added without the reporter's permission, especially if the
> idea was not posted in a public forum. That said, if we diligently credit our
> idea reporters, they will, hopefully, be inspired to help us again in the
> future.
> 
> A Fixes: tag indicates that the patch fixes an issue in a previous commit. It
> is used to make it easy to determine where a bug originated, which can help
> review a bug fix. This tag also assists the stable kernel team in determining
> which stable kernel versions should receive your fix. This is the preferred
> method for indicating a bug fixed by the patch. See :ref:`describe_changes`
> for more details.
> 
> 
> 
> 
> 
>