Documentation/patch-tags v3
Jonathan Corbet
maintainerPATCH 1/1 Documentation/patch-tags v3
Here's the current version of the patch-tags document; I've made changes in response to comments from a Randy Dunlap and Scott Preece. The only substantive change, though, is the removal of the privacy term, since it really does seem to be overkill. It can come back if it turns out to be truly needed.
Andrew, you've been quiet on this. Does it correspond to your notion of what Reviewed-by should mean?
jon
Documentation/00-INDEX +2 -0
@@ -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
Documentation/patch-tags new file +76 -0
76 lines not on this page · raw message
Quoted 60 lines · Jonathan Corbet
Here's the current version of the patch-tags document; I've made changes in response to comments from a Randy Dunlap and Scott Preece. The only substantive change, though, is the removal of the privacy term, since it really does seem to be overkill. It can come back if it turns out to be truly needed.
Andrew, you've been quiet on this. Does it correspond to your notion of what Reviewed-by should mean?
jon
Documentation/00-INDEX +2
-0@@ -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.txtDocumentation/patch-tags new file +30
-0@@ -0,0 +1,76 @@ +Patches headed for the mainline may contain a variety of tags documenting +who played a hand in (or was at least aware of) their 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.
Documentation/patch-tags line 40
Quoted 10 lines
Documentation/patch-tags new file +10
-0+ +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.
Does 'Reviewed-by' also imply 'Signed-off-by'? In other words, who is actually supposed to add this tag?
Is it the reviewer who passes on an officially 'reviewed' patch to the maintainer, or is it the patch author him/herself who is responsible for soliciting reviews and adding the tag?
Documentation/patch-tags line 52
Quoted 12 lines
Documentation/patch-tags new file +12
-0+ +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, provides + a means to locate testers for future patches, and ensures + credit for the testers.
Again. Who is supposed to add this?
Quoted 12 lines
Documentation/patch-tags new file +12
-0+---- + +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.
Documentation/patch-tags line 76
Quoted 10 lines
Documentation/patch-tags new file +10
-0+ + (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.
I'm confused about how to reconcile (c) and (d) here. If you are not sure about whether or not the patch will achieve its stated purpose, why would you be arguing that it is a worthwhile modification?
Trond
Quoted 20 lines · On Thu, 11 Oct 2007 16:50:11 -0400 Trond Myklebust wrote:
Quoted 18 lines · Jonathan Corbet
+----
Documentation/patch-tags new file +17
-0+ +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. +
Quoted 8 lines
Documentation/patch-tags line 76
Quoted 4 lines
+ (d) While I have reviewed the patch and believe it to be sound, I do not
Documentation/patch-tags new file +3
-0+ (unless explicitly stated elsewhere) make any warranties or guarantees + that it will achieve its stated purpose or function properly in any + given situation.I'm confused about how to reconcile (c) and (d) here. If you are not sure about whether or not the patch will achieve its stated purpose, why would you be arguing that it is a worthwhile modification?
Well, any non-trivial patch could have a lurking bug in it, even if most code paths are tested.
I thought that I once saw (read) something like this:
Someone who gives a Reviewed-by: tag also is willing to take ownership (or at least help debug) any problems that arise from the patch, including but not limited to patch-author hit-by-bus conditions.
Did I dream that?
---
~Randy
Quoted 1 line · Trond Myklebust
Does 'Reviewed-by' also imply 'Signed-off-by'?
Does a technical review include a review of licensing and copyright issues? (It doesn't seem to be a big issue though if the submitter signed off on it, like he should.)
Quoted 5 lines
In other words, who is actually supposed to add this tag?
Is it the reviewer who passes on an officially 'reviewed' patch to the maintainer, or is it the patch author him/herself who is responsible for soliciting reviews and adding the tag?
Anybody in the patch forwarding chain (author, maintainers... usually the latter) can add Acked-by and Tested-by, based on incoming feedback. The feedback may have explicitly stated an Acked-by or Tested-by or may have said something equivalent. (In case of Tested-by, an appropriate description of how was tested should have been sent. An explicit Tested-by from the tester himself is moot then.)
Reviewed-by is a different beast. If Jon's definition of Reviewed-by (or another definition) is "officially" adopted, people in the patch forwarding chain should only add this tag if the reviewer sent it explicitly in his response. Unlike with Acked-by and Tested-by, we must not guess whether a reviewer wants to have his Reviewed-by added.
[...]
Quoted 10 lines
Documentation/patch-tags line 76
Quoted 9 lines
+ (c) While there may be things that could be improved with this submission,
Documentation/patch-tags new file +8
-0+ 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.
Quoted 3 lines
I'm confused about how to reconcile (c) and (d) here. If you are not sure about whether or not the patch will achieve its stated purpose, why would you be arguing that it is a worthwhile modification?
Being sure of something and making guarantees are different things.
Signature · 3 lines
Stefan Richter -=====-=-=== =-=- -=-== http://arcgraph.de/sr/
Quoted 20 lines · Stefan Richter
Quoted 1 line · Trond Myklebust
Does 'Reviewed-by' also imply 'Signed-off-by'?
Does a technical review include a review of licensing and copyright issues? (It doesn't seem to be a big issue though if the submitter signed off on it, like he should.)
Quoted 5 lines
In other words, who is actually supposed to add this tag?
Is it the reviewer who passes on an officially 'reviewed' patch to the maintainer, or is it the patch author him/herself who is responsible for soliciting reviews and adding the tag?
Anybody in the patch forwarding chain (author, maintainers... usually the latter) can add Acked-by and Tested-by, based on incoming feedback. The feedback may have explicitly stated an Acked-by or Tested-by or may have said something equivalent. (In case of Tested-by, an appropriate description of how was tested should have been sent. An explicit Tested-by from the tester himself is moot then.)
Quoted 5 lines
Reviewed-by is a different beast. If Jon's definition of Reviewed-by (or another definition) is "officially" adopted, people in the patch forwarding chain should only add this tag if the reviewer sent it explicitly in his response. Unlike with Acked-by and Tested-by, we must not guess whether a reviewer wants to have his Reviewed-by added.
In that case the reviewer should be made part of the forwarding chain, and it should be made clear to whoever is upstream that this is a patch that has not been modified since it was reviewed.
Quoted 11 lines
[...]
Quoted 10 lines
Quoted 9 lines
+ (c) While there may be things that could be improved with this submission,
Documentation/patch-tags new file +8
-0+ 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.
Quoted 5 lines
Quoted 3 lines
I'm confused about how to reconcile (c) and (d) here. If you are not sure about whether or not the patch will achieve its stated purpose, why would you be arguing that it is a worthwhile modification?
Being sure of something and making guarantees are different things.
To a lawyer, yes. To everyone else, no, and the GPL already tells you that you are given no warranties.
The reviewed-by tag has, as far as I understand, no legal standing:
unlike the DCE, we're not expecting anyone to be able to sue over this. So we should at least be trying to ensure that reviewers are 'reasonably sure' that the patch works. Otherwise, reviews will turn into yet another coding standards witch hunt of zero value.
Trond
Quoted 9 lines · Trond Myklebust
Quoted 4 lines · Stefan Richter
people in the patch
forwarding chain should only add this tag if the reviewer sent it explicitly in his response. Unlike with Acked-by and Tested-by, we must not guess whether a reviewer wants to have his Reviewed-by added.In that case the reviewer should be made part of the forwarding chain, and it should be made clear to whoever is upstream that this is a patch that has not been modified since it was reviewed.
It's more comfortable for the reviewer to send a mail reply with the tag.
But modifications after review are a problem either way. (If the modifications are minor, add a description below the Reviewed-by and sign off below that additional description. If they are major, drop the Reviewed-by. However, a follow-up patch instead of modifying the reviewed patch should be considered and may be suitable in many cases, since a patch which passed review should already be fine for commit on its own.)
Quoted 4 lines
Quoted 1 line
Quoted 1 line
Being sure of something and making guarantees are different things.
To a lawyer, yes. To everyone else, no, and the GPL already tells you that you are given no warranties.
OK.
Signature · 3 lines
Stefan Richter -=====-=-=== =-=- -=-== http://arcgraph.de/sr/