[PATCH] Documentation/patch-tags v3

[PATCH] Documentation/patch-tags v3

[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

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..6acde5e
--- /dev/null
+++ b/Documentation/patch-tags
@@ -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. 
+
+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, provides
+		a means to locate testers for future patches, and ensures
+		credit for the testers.
+
+
+----
+
+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.

Re: [PATCH] Documentation/patch-tags v3

[Trond Myklebust]

On Thu, 2007-10-11 at 14:16 -0600, Jonathan Corbet wrote:
> 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
> 
> 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..6acde5e
> --- /dev/null
> +++ b/Documentation/patch-tags
> @@ -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. 
> +
> +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?

> +
> +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?

> +----
> +
> +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.

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

Re: [PATCH] Documentation/patch-tags v3

[Randy Dunlap]

On Thu, 11 Oct 2007 16:50:11 -0400 Trond Myklebust wrote:

> 
> On Thu, 2007-10-11 at 14:16 -0600, Jonathan Corbet wrote:
> > +----
> > +
> > +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.
> 
> 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

Re: [PATCH] Documentation/patch-tags v3

[Stefan Richter]

Trond Myklebust wrote:
> 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.)

> 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.

[...]
>> + (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?

Being sure of something and making guarantees are different things.
-- 
Stefan Richter
-=====-=-=== =-=- -=-==
http://arcgraph.de/sr/

Re: [PATCH] Documentation/patch-tags v3

[Trond Myklebust]

On Thu, 2007-10-11 at 23:21 +0200, Stefan Richter wrote:
> Trond Myklebust wrote:
> > 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.)
> 
> > 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.

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.

> [...]
> >> + (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?
> 
> 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

Re: [PATCH] Documentation/patch-tags v3

[Stefan Richter]

Trond Myklebust wrote:
> On Thu, 2007-10-11 at 23:21 +0200, Stefan Richter wrote:
>> 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.)

>> > 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.
-- 
Stefan Richter
-=====-=-=== =-=- -=-==
http://arcgraph.de/sr/