Linux kernel review tags (Re: [PATCH 0/3] Fix Thumb-2 undef handling for mixed-arch kernels)
On Tue, Aug 16, 2011 at 05:43:08PM +0100, Tixy wrote:
On Tue, 2011-08-16 at 17:06 +0100, Dave Martin wrote:
On Tue, Aug 16, 2011 at 3:58 PM, Tixy tixy@yxit.co.uk wrote:
[...]
I've been re-reading Documentation/SubmittingPatches to try and work out when it's appropriate to use Acked-by, but it seems a bit woolly - mostly for maintainers?
Actually I asked the question semi-automatically since you are CC'd on this series -- because I was originally touching kprobes code, which you have in-depth understanding of.
But now the series doesn't touch that code any more, reviewed-by makes more sense.
Here's my own interpretation of the tags, based on how they appear to be used (obviously, this doesn't override SubmittingPatches; it's my additional interpretation)
I could of course be wrong in some aspects.
Any comments on this from the more experienced kernel developers out there?
Signed-off-by = I take full responsibility for this patch, and can maintain it in the author's absence if required. In practice, this implies Acked-by.
Acked-by = This patch is definitely right, or I fully agree with the patch and trust the author's judgement ("I will share responsibility for the correctness and appropriateness of this patch"). This implies Reviewed-by. Normally an ack shouldn't get added unless the acker is confident that the patch is adequately tested (where the level of testing deemed adequate depends on the complexity of the patch) Again, this may rely on judgement of the comptence of the author and the other reviewers.
Reviewed-by = This patch looks correct and appropriate and I judge it ok to merge, but I assume the author knows what they're doing, and I don't necessarily take responsibility for the change.
Tested-by = I have built and tested the patch, and saw reasonable evidence that it achieves the intended result and has no harmful effects. I make no comment about contents of the patch, and I don't necessarily take responsibility for the change.
In all cases, the tag may influence upstreams' judgement about whether to merge the patch: adding a tag implies understanding of this, and takes responsibility for the effect this has (wise or unwise) on the decision to merge.
NA(C)K = Informal indication that this patch is definitely wrong (not listed in SubmittingPatches, because if someone is that convinced the patch is wrong, it isn't normally going to get merged without rework... or the NACKer is themselves known by upstream to be reliably untrustworthy, biased, or wrong. In that case, the upstream maintainter effectively overrides the NACKer.
An Ack is normally not useful unless it comes from someone who is themselves known to and trusted by the maintainer of the tree where the patch will get merged, or unless it comes from the original author or a maintainer of the code that was modified (since they are normally the best people to judge). Acked-by: randomnewbie@hotmail.com isn't likely to carry much weight. In fact, including blatantly frivolous or fraudulent acks from people in your upstream patch submissions may harm their chance of merging (and your reputation).
So for now, I don't often ack patches -- I generally do so when someone touches code I modified/added, to indicate that I understand their change well enough, and am confident enough that it is correct, that I would be happy to share some responsiibility for their change and any resulting impacts on the maintenance of the code I added.
In areas where I know I have and am known to have a good understanding (such as Thumb-2 related issues), I may ack patches touching code that is nothing to do with me, because in such cases the Ack has a useful meaning for the upstream maintainer.
Cheers ---Dave
On Wed, 17 Aug 2011, Dave Martin wrote:
Acked-by = This patch is definitely right, or I fully agree with the patch and trust the author's judgement ("I will share responsibility for the correctness and appropriateness of this patch"). This implies Reviewed-by. Normally an ack shouldn't get added unless the acker is confident that the patch is adequately tested (where the level of testing deemed adequate depends on the complexity of the patch) Again, this may rely on judgement of the comptence of the author and the other reviewers.
Reviewed-by = This patch looks correct and appropriate and I judge it ok to merge, but I assume the author knows what they're doing, and I don't necessarily take responsibility for the change.
I think some aspects of the above two are mixed up.
Normally, ACK == acknowledgement i.e. "I conceptually agree with the patch", but that doesn't necessarily mean that it was reviewed thoroughly. In other words, this quite matches your definition, but does not imply a Reviewed-by, and that assumes the author knows what they're doing.
Reviewed-by means that you did review the patch content in details, whether or not the author knows what they're doing. A Reviewed-by obviously implies an Acked-by.
Nicolas
On Wed, Aug 17, 2011 at 6:24 PM, Nicolas Pitre nicolas.pitre@linaro.org wrote:
On Wed, 17 Aug 2011, Dave Martin wrote:
Acked-by = This patch is definitely right, or I fully agree with the patch and trust the author's judgement ("I will share responsibility for the correctness and appropriateness of this patch"). This implies Reviewed-by. Normally an ack shouldn't get added unless the acker is confident that the patch is adequately tested (where the level of testing deemed adequate depends on the complexity of the patch) Again, this may rely on judgement of the comptence of the author and the other reviewers.
Reviewed-by = This patch looks correct and appropriate and I judge it ok to merge, but I assume the author knows what they're doing, and I don't necessarily take responsibility for the change.
I think some aspects of the above two are mixed up.
Normally, ACK == acknowledgement i.e. "I conceptually agree with the patch", but that doesn't necessarily mean that it was reviewed thoroughly. In other words, this quite matches your definition, but does not imply a Reviewed-by, and that assumes the author knows what they're doing.
Reviewed-by means that you did review the patch content in details, whether or not the author knows what they're doing. A Reviewed-by obviously implies an Acked-by.
Interesting... I thought there was a chance I was getting this wrong.
My impression was that an Ack carries more weight with upstream maintainers when it comes to merging; but does it instead depend on _who_ the tag comes from? (i.e., if an experienced and well-known person takes a cursory glance at the patch and the review that's gone on and Acks it, this may carry more weight than a Reviewed-by by a less well-known person?)
Cheers ---Dave
On Thu, 18 Aug 2011, Dave Martin wrote:
On Wed, Aug 17, 2011 at 6:24 PM, Nicolas Pitre nicolas.pitre@linaro.org wrote:
On Wed, 17 Aug 2011, Dave Martin wrote:
Acked-by = This patch is definitely right, or I fully agree with the patch and trust the author's judgement ("I will share responsibility for the correctness and appropriateness of this patch"). This implies Reviewed-by. Normally an ack shouldn't get added unless the acker is confident that the patch is adequately tested (where the level of testing deemed adequate depends on the complexity of the patch) Again, this may rely on judgement of the comptence of the author and the other reviewers.
Reviewed-by = This patch looks correct and appropriate and I judge it ok to merge, but I assume the author knows what they're doing, and I don't necessarily take responsibility for the change.
I think some aspects of the above two are mixed up.
Normally, ACK == acknowledgement i.e. "I conceptually agree with the patch", but that doesn't necessarily mean that it was reviewed thoroughly. In other words, this quite matches your definition, but does not imply a Reviewed-by, and that assumes the author knows what they're doing.
Reviewed-by means that you did review the patch content in details, whether or not the author knows what they're doing. A Reviewed-by obviously implies an Acked-by.
Interesting... I thought there was a chance I was getting this wrong.
My impression was that an Ack carries more weight with upstream maintainers when it comes to merging; but does it instead depend on _who_ the tag comes from? (i.e., if an experienced and well-known person takes a cursory glance at the patch and the review that's gone on and Acks it, this may carry more weight than a Reviewed-by by a less well-known person?)
Absolutely.
And the more experienced a person might be, the more patches that person might be expected to look at. So it is normal for such person to look at the purpose and general design of a patch only, while trusting the author to get the details right. Hence the acked-by tag.
This is also where the coding style get important as it is possible for a reviewer to look at the patch and get a feel for that general design more easily.
A Reviewed-by is meant to be more thorough. See the definition from Ted Tso here:
http://kerneltrap.org/Linux/Introducing_Reviewed-by_Tags
But it is true that the value of any such tag is pondered by the reputation of the person providing it, and that reputation is usually based on the perceived quality of the code that person provided in the past.
Nicolas
On Thu, Aug 18, 2011 at 10:18:10PM -0400, Nicolas Pitre wrote:
On Thu, 18 Aug 2011, Dave Martin wrote:
On Wed, Aug 17, 2011 at 6:24 PM, Nicolas Pitre nicolas.pitre@linaro.org wrote:
On Wed, 17 Aug 2011, Dave Martin wrote:
Acked-by = This patch is definitely right, or I fully agree with the patch and trust the author's judgement ("I will share responsibility for the correctness and appropriateness of this patch"). This implies Reviewed-by. Normally an ack shouldn't get added unless the acker is confident that the patch is adequately tested (where the level of testing deemed adequate depends on the complexity of the patch) Again, this may rely on judgement of the comptence of the author and the other reviewers.
Reviewed-by = This patch looks correct and appropriate and I judge it ok to merge, but I assume the author knows what they're doing, and I don't necessarily take responsibility for the change.
I think some aspects of the above two are mixed up.
Normally, ACK == acknowledgement i.e. "I conceptually agree with the patch", but that doesn't necessarily mean that it was reviewed thoroughly. In other words, this quite matches your definition, but does not imply a Reviewed-by, and that assumes the author knows what they're doing.
Reviewed-by means that you did review the patch content in details, whether or not the author knows what they're doing. A Reviewed-by obviously implies an Acked-by.
Interesting... I thought there was a chance I was getting this wrong.
My impression was that an Ack carries more weight with upstream maintainers when it comes to merging; but does it instead depend on _who_ the tag comes from? (i.e., if an experienced and well-known person takes a cursory glance at the patch and the review that's gone on and Acks it, this may carry more weight than a Reviewed-by by a less well-known person?)
Absolutely.
And the more experienced a person might be, the more patches that person might be expected to look at. So it is normal for such person to look at the purpose and general design of a patch only, while trusting the author to get the details right. Hence the acked-by tag.
This is also where the coding style get important as it is possible for a reviewer to look at the patch and get a feel for that general design more easily.
A Reviewed-by is meant to be more thorough. See the definition from Ted Tso here:
http://kerneltrap.org/Linux/Introducing_Reviewed-by_Tags
But it is true that the value of any such tag is pondered by the reputation of the person providing it, and that reputation is usually based on the perceived quality of the code that person provided in the past.
Useful advice, thanks
---Dave
-
Dave Martin -
Nicolas Pitre