From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga06.intel.com (mga06.intel.com [134.134.136.31]) by dpdk.org (Postfix) with ESMTP id 7B273DE3 for ; Fri, 27 Jan 2017 11:53:03 +0100 (CET) Received: from orsmga003.jf.intel.com ([10.7.209.27]) by orsmga104.jf.intel.com with ESMTP; 27 Jan 2017 02:53:02 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.33,294,1477983600"; d="scan'208";a="927268835" Received: from fyigit-mobl1.ger.corp.intel.com (HELO [10.237.220.38]) ([10.237.220.38]) by orsmga003.jf.intel.com with ESMTP; 27 Jan 2017 02:52:59 -0800 To: "Mcnamara, John" , Shreyansh Jain , "Richardson, Bruce" References: <2358632.GCFl4gnRC2@xps13> <7fb35576-ba10-019a-a6ba-e38418e03848@nxp.com> <20170127101304.GA69896@bricha3-MOBL3.ger.corp.intel.com> Cc: Thomas Monjalon , "Van Haaren, Harry" , "dev@dpdk.org" , Igor Ryzhov , Steve Shin From: Ferruh Yigit Message-ID: Date: Fri, 27 Jan 2017 10:52:58 +0000 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.6.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 8bit Subject: Re: [dpdk-dev] Understanding of Acked-By X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 27 Jan 2017 10:53:04 -0000 On 1/27/2017 10:32 AM, Mcnamara, John wrote: > > >> -----Original Message----- >> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Shreyansh Jain >> Sent: Friday, January 27, 2017 10:25 AM >> To: Richardson, Bruce >> Cc: Thomas Monjalon ; Van Haaren, Harry >> ; dev@dpdk.org; Yigit, Ferruh >> ; Igor Ryzhov ; Steve Shin >> >> 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. > > > > > >