"Last month, at the kernel summit, there was discussion of putting a Reviewed-by: tag onto patches to document the oversight they had received on their way into the mainline," began Jonathan Corbet in an effort to define the meaning [1] of the recently introduced reviewed-by tag. He continued, "that tag has made an occasional appearance since then, but there has not yet been a discussion of what it really means. So it has not yet brought a whole lot of value to the process."
In the continued discussion, it was requested that all commit tags be defined, prompting Jonathan to update his documentation to include Signed-off-by, Acked-by, Cc, and Tested-by along with his documentation for Reviewed-by. He offered the following definition for the new Reviewed-by tag:
"The patch has been reviewed and found acceptible according to the Reviewer's Statement as found at the bottom of this file. 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."
From: Jonathan Corbet <corbet@...>
Subject: RFC: reviewer's statement of oversight
[1]Date: Oct 8, 1:24 pm 2007
Last month, at the kernel summit, there was discussion of putting a
Reviewed-by: tag onto patches to document the oversight they had
received on their way into the mainline. That tag has made an
occasional appearance since then, but there has not yet been a
discussion of what it really means. So it has not yet brought a whole
lot of value to the process.
As I was trying to sleep last night, it occurred to me that what we
might need is an equivalent of the DCO for the Reviewed-by tag. To that
end, I dedicated a few minutes of my life to the following bit of text.
It's really just meant to be a starting point for the discussion. Is
the following something close to what we understand Reviewed-by to mean?
jon
Reviewer's statement of oversight v0.01
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 how the
submitter has responded to my comments.
(c) While there may (or may not) be things which could be improved with
this submission, I believe that it is, at this time, (1) a
worthwhile addition to the kernel, and (2) free of serious known
issues which would argue against its inclusion.
(d) While I have reviewed the patch and believe it to be sound, I can not
(unless explicitly stated elsewhere) make any warranties or guarantees
that it will achieve its stated purpose or function properly in any
given situation.
(e) I understand and agree that this project and the contribution are
public and that a record of the contribution (including my Reviewed-by
tag and any associated public communications) is maintained
indefinitely and may be redistributed consistent with this project or
the open source license(s) involved.
-
From: Sam Ravnborg <sam@...>
Subject: Re: RFC: reviewer's statement of oversight
[1]Date: Oct 8, 1:37 pm 2007
On Mon, Oct 08, 2007 at 11:24:45AM -0600, Jonathan Corbet wrote:
> Last month, at the kernel summit, there was discussion of putting a
> Reviewed-by: tag onto patches to document the oversight they had
> received on their way into the mainline. That tag has made an
> occasional appearance since then, but there has not yet been a
> discussion of what it really means. So it has not yet brought a whole
> lot of value to the process.
>
> As I was trying to sleep last night, it occurred to me that what we
> might need is an equivalent of the DCO for the Reviewed-by tag. To that
> end, I dedicated a few minutes of my life to the following bit of text.
> It's really just meant to be a starting point for the discussion. Is
> the following something close to what we understand Reviewed-by to mean?
>
> jon
>
>
> Reviewer's statement of oversight v0.01
>
> By offering my Reviewed-by: tag, I state that:
snip...
Or maybe we need something much less formal that explain the purpose of the
four tags we use:
Signed-of-by:
Acked-by:
Reviewed-by:
Cc:
Tested-by:
OK - make it five then. I continously to see people mixing up especially
Acked-by: and Signed-of-by: both here at lkml but especially at the
arm-kernel list (the only other Linux dev list I follow atm). I do
beleive we see similar pattern in the other linux-dev lists where
people are confused by these tags and need a short two line summery for
each of them.
Sam
-
From: Jonathan Corbet <corbet@...>
Subject: Re: RFC: reviewer's statement of oversight
[1]Date: Oct 8, 6:43 pm 2007
Sam Ravnborg <sam@ravnborg.org> wrote:
> Or maybe we need something much less formal that explain the purpose of the
> four tags we use:
...or maybe a combination? How does the following patch look as a way
to describe how the tags are used and what Reviewed-by, in particular,
means?
Perhaps the DCO should move to this file as well?
jon
---
Add a document on patch tags.
Signed-off-by: Jonathan Corbet <corbet@lwn.net>
diff --git a/Documentation/00-INDEX b/Documentation/00-INDEX
index 43e89b1..fa1518b 100644
--- a/Documentation/00-INDEX
+++ b/Documentation/00-INDEX
@@ -284,6 +284,8 @@ parport.txt
- how to use the parallel-port driver.
parport-lowlevel.txt
- description and usage of the low level parallel port functions.
+patch-tags
+ - description of the tags which can be added to patches
pci-error-recovery.txt
- info on PCI error recovery.
pci.txt
diff --git a/Documentation/patch-tags b/Documentation/patch-tags
new file mode 100644
index 0000000..fb5f8e1
--- /dev/null
+++ b/Documentation/patch-tags
@@ -0,0 +1,66 @@
+Patches headed for the mainline may contain a variety of tags documenting
+who played a hand in (or was at least aware of) its progress. All of these
+tags have the form:
+
+ Something-done-by: Full name <email@address>
+
+These tags are:
+
+Signed-off-by: A person adding a Signed-off-by tag is attesting that the
+ patch is, to the best of his or her knowledge, legally able
+ to be merged into the mainline and distributed under the
+ terms of the GNU General Public License, version 2. See
+ the Developer's Certificate of Origin, found in
+ Documentation/SubmittingPatches, for the precise meaning of
+ Signed-off-by.
+
+Acked-by: The person named (who should be an active developer in the
+ area addressed by the patch) is aware of the patch and has
+ no objection to its inclusion. An Acked-by tag does not
+ imply any involvement in the development of the patch or
+ that a detailed review was done.
+
+Reviewed-by: The patch has been reviewed and found acceptible according
+ to the Reviewer's Statement as found at the bottom of this
+ file. 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.
+
+Cc: The person named was given the opportunity to comment on
+ the patch. This is the only tag which might be added
+ without an explicit action by the person it names.
+
+Tested-by: The patch has been successfully tested (in some
+ environment) by the person named.
+
+
+----
+
+Reviewer's statement of oversight, v0.02
+
+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 how the
+ submitter has responded to my comments.
+
+ (c) While there may (or may not) be things which 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 can not
+ (unless explicitly stated elsewhere) make any warranties or guarantees
+ that it will achieve its stated purpose or function properly in any
+ given situation.
+
+ (e) I understand and agree that this project and the contribution are
+ public and that a record of the contribution (including my Reviewed-by
+ tag and any associated public communications) is maintained
+ indefinitely and may be redistributed consistent with this project or
+ the open source license(s) involved.
-From: Jan Engelhardt <jengelh@...> Subject: Re: RFC: reviewer's statement of oversight [1]Date: Oct 8, 1:45 pm 2007 On Oct 8 2007 19:37, Sam Ravnborg wrote: >snip... > >Or maybe we need something much less formal that explain the purpose of the >four tags we use: At least formal try: >Signed-of-by: * Used by original submitter(s). * Also used by maintainers to track the patch's path (ATM, does not imply "I have look at it in depth") >Acked-by: >Tested-by: * Used by random people to express their (dis)like/experience with the patch. >Reviewed-by: * I am maintaner or an 'important' person and have had a look at it in depth >Cc: * Used by original submitter to denote additional maintainers it goes to * Parties who should be Cced when an a posteriori question comes up My 2¥. Jan -
From: H. Peter Anvin <hpa@...> Subject: Re: RFC: reviewer's statement of oversight [1]Date: Oct 8, 4:33 pm 2007 Jan Engelhardt wrote: > >> Acked-by: >> Tested-by: > > * Used by random people to express their (dis)like/experience with the > patch. > >> Reviewed-by: > > * I am maintaner or an 'important' person and have had a > look at it in depth > Uhm, no. There is no reason an "unimportant" person couldn't review a patch, and therefore perform a potentially highly valuable service to the maintainer. None of these are indicative of the authority of the person acking, reviewing, testing, or nacking. That's only as good as the trust in the person signing. -hpa -
From: Theodore Tso <tytso@...> Subject: Re: RFC: reviewer's statement of oversight [1]Date: Oct 8, 5:38 pm 2007 On Mon, Oct 08, 2007 at 01:33:38PM -0700, H. Peter Anvin wrote: > Uhm, no. There is no reason an "unimportant" person couldn't review a > patch, and therefore perform a potentially highly valuable service to > the maintainer. > > None of these are indicative of the authority of the person acking, > reviewing, testing, or nacking. That's only as good as the trust in the > person signing. I would tend to agree. Right now I think the problem is that we are getting too little reviews, not enough. And someone who reviews patches, even if unknown, could be building up expertise that eventually would make them a valued developer, even while they are doing us a service. The concern that I suspect some people have is what if this gets abused by people who don't really bother to do a full review of a patch before they ack it. We could ask reviewers to include a URL to an LKML archive of their review, to make it easier to find a review of a patch so later on people can judge how effective they their review was. Unfortunately, this would be an added burden for the regular reviewers, so I doubt this would be well accepted as a practice. My suggestion is to not worry about this for now, and see how well it works out in practice. If we start getting half a dozen or more Reviewed-by: where the patch is pretty clearly not getting adequately reviewed, or where someone is obviously abusing the system, and social pressures aren't working, we can try to figure out then how we want to address that problem then. Let's not make the process too complicated unless we know it's necessary. Premature complexity is almost as bad as premature optimization.... - Ted -
From: Jonathan Corbet <corbet@...> Subject: [PATCH] Documentation/patch-tags V2 [1]Date: Oct 9, 12:59 pm 2007 Here's a new version of the patch-tags document. I've incorporated comments from Randy Dunlap, Stefan Richter, and Neil Brown, though I have retained, for now, the more verbose process discussion that Neil didn't like. Comments? jon -- Document the tags used with kernel patches Signed-off-by: Jonathan Corbet <corbet@lwn.net> diff --git a/Documentation/00-INDEX b/Documentation/00-INDEX index 43e89b1..fa1518b 100644 --- a/Documentation/00-INDEX +++ b/Documentation/00-INDEX @@ -284,6 +284,8 @@ parport.txt - how to use the parallel-port driver. parport-lowlevel.txt - description and usage of the low level parallel port functions. +patch-tags + - description of the tags which can be added to patches pci-error-recovery.txt - info on PCI error recovery. pci.txt diff --git a/Documentation/patch-tags b/Documentation/patch-tags new file mode 100644 index 0000000..d955fa2 --- /dev/null +++ b/Documentation/patch-tags @@ -0,0 +1,81 @@ +Patches headed for the mainline may contain a variety of tags documenting +who played a hand in (or was at least aware of) its progress. All of these +tags have the form: + + Something-done-by: Full name <email@address> [optional random stuff] + +These tags are: + +From: The original author of the patch. This tag will ensure + that credit is properly given when somebody other than the + original author submits the patch. + +Signed-off-by: A person adding a Signed-off-by tag is attesting that the + patch is, to the best of his or her knowledge, legally able + to be merged into the mainline and distributed under the + terms of the GNU General Public License, version 2. See + the Developer's Certificate of Origin, found in + Documentation/SubmittingPatches, for the precise meaning of + Signed-off-by. This tag assures upstream maintainers that + the provenance of the patch is known and allows the origin + of the patch to be reviewed should copyright questions + arise. + +Acked-by: The person named (who should be an active developer in the + area addressed by the patch) is aware of the patch and has + no objection to its inclusion; it informs upstream + maintainers that a certain degree of consensus on the patch + as been achieved.. An Acked-by tag does not imply any + involvement in the development of the patch or that a + detailed review was done. + +Reviewed-by: The patch has been reviewed and found acceptable according + to the Reviewer's Statement as found at the bottom of this + file. 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. This tag serves to give credit to + reviewers and to inform maintainers of the degree of review + which has been done on the patch. + +Cc: The person named was given the opportunity to comment on + the patch. This is the only tag which might be added + without an explicit action by the person it names. This + tag documents that potentially interested parties have been + included in the discussion. + +Tested-by: The patch has been successfully tested (in some + environment) by the person named. This tag informs + maintainers that some testing has been performed and + ensures credit for the testers. + + +---- + +Reviewer's statement of oversight, v0.02 + +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 how the + submitter has responded to my comments. + + (c) While there may (or may not) be things which 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 cannot + (unless explicitly stated elsewhere) make any warranties or guarantees + that it will achieve its stated purpose or function properly in any + given situation. + + (e) I understand and agree that this project and the contribution are + public and that a record of the contribution (including my Reviewed-by + tag and any associated public communications) is maintained + indefinitely and may be redistributed consistent with this project or + the open source license(s) involved. -
Related links:
- Archive of above thread [1]
- Archive of above thread [1]