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.

-

C-1 "worthwhile addition..." Probably shouldn't be part of this.  That's

what additional Signed off by ACK's provide.  I think reviewed by should

limit its scope to code correctness leaving the subjective "worthwhile"
I think this is a good thing to have, although recruiting reviews remains

an open issue.
I think it would be easier to recruit patch testers than reviewers

should a Tested-by: tag be considered as well?
--mgross

-

A patch which is not "worthwhile" is also not "appropriate".  Mere

correctness in a mathematical sense is not enough as technical review

criterion.

--

Stefan Richter

-=====-=-=== =-=- -=---

http://arcgraph.de/sr/

-

Yes, but there's also such thing as "worthwhile removal".

-

Good point.  So the text should probably say "worthwhile change" rather

than "worthwhile addition."  I do believe that thinking about whether

the change as a whole is a desirable thing is an important part of the

review process.
jon

-

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

-

...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....
[ message continues ]

On 10/8/07, Jonathan Corbet <corbet@lwn.net> wrote:
---
Replace the last sentence with "I am satisfied with the submitter's

response to my comments." or "The submitter has responded to my

comments in a way that satisfied my concerns."
---
I would suggest dropping the "(or may not)" as unnecessary, and

changing the "which would" to "that would".
---
From a legal standpoint, "I do not" might be preferable to "I cannot",

since it disclaims any intention to make such a statement, regardless

of qualification.
---
(e) seems over-careful, especially since you're applying it only to

the Review-by tag, while all the other tags would also have the same

concern.
--

scott preece

-

To be precise:

    Something-done-by: Full name <email@address> [optional random stuff]
"Some people also put extra tags at the end.  They'll just be ignored

for now, but you can do this to mark internal company procedures or just

point out some special detail about the sign-off.", says

SubmittingPatches.  I actually do so on occasions.

--

Stefan Richter

-=====-=-=== =-=- -=--=

http://arcgraph.de/sr/

-

Is this paragraph really necessary?  (For example, is there some history

of problems that this is addressing?)
--b.

-

On Mon, 8 Oct 2007 19:30:54 -0400
It was added between 1.0 and 1.1 to ensure that there were no awkward

interactions with various privacy and data protection laws by making sure

the submitter understands it is a public record and is contributing on

that basis.

-

---

~Randy

-

On Mon, 8 Oct 2007 16:06:03 -0700

All changes are licensed under the terms of the file modified. 
(Some people seem not to understand that

if the file is dual licensed, then the changes are dual licensed. 
IMHO the other tags actually are a poor substitute for providing a

more complete description of the reviewer's involvement. It would be better

to have more complete responses like "the patch should be merged as is for

2.6.X but the following should be fixed, ..." etc. The certificate of origin

has meaning for legal things that have a more concrete definition, but the

existing process is about people making good (or bad) decisions based on

feedback and other data. Trying to reduce the feedback down to 3 Acks, and 1 Review

seems like noise. The problem is getting good reviews of new code in

a timely manner, not the descriptions of the result.
--

Stephen Hemminger <shemminger@linux-foundation.org>
-

* Used by original submitter(s).

* Also used by maintainers to track the patch's path
* Used by random people to express their (dis)like/experience with the 
* I am maintaner or an 'important' person and have had a
* 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

-

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

-

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

-

Experience of convincing experienced patch author, that some things in

the patch are wrong :)
I vote for more little summaries in the `Subject'(again). Long, boring

threads with whole threading part of screen being empty due to same

subjects isn't fun, when some of thousands of messages can have

interesting stuff inside.
And it's easy not only for mailing list readers now, and for archive

readers also; readers of the www search results (who ever that may be):
google.com/search?q=reviewed+crashkernel
First hit on the review of the patch, i happened to make. And i just

thought "hell, just string parsing, what can be more simply?", yet there

was productive discussion and bug fixing. After i saw convincing

statements about testing, i've placed review mark. Though i'm really

"unimportant" random hacker.

--

-o--=O`C

 #oo'L O

<___=E M

-

I agree.
Greetings,

Rafael

-

---
A couple of months ago there was a thread about Acked-by. At that time

the consensus seemed to be that it was inappropriate for random people

to add Acked-by - that it should only be added by people whose opinion

was expected to a gate for merging. Did the Summit reach a decision

that Acked-by was for anybody and Reviewed-by was for authorities?
I like Randy's point - the assigning of weight to the Reviewer's

opinion is necessarily in the hands of those at the top of the merge

process and there's no need to ask reviewers to decide whether their

opinion matters. In that view, "Acked-by" means "I have no objection

to this patch, but don't claim deep review" and "Reviewed-by" means "I

have no objection to this patch after a thorough review", and either

can be attached by anybody who wants to express an opinion.
scott

--

scott preece

-

> >Reviewed-by:

 >

 > * I am maintaner or an 'important' person and have had a

 >   look at it in depth
I think anyone should be able to supply a Reviewed-by: tag no matter

who they are.  Of course the weight that such a tag carries in the

final decision of whether or not to merge may depend on who supplied

it, but I don't think we should try to formalize that aspect.
In other words, I would define Reviewed-by: as just "I have looked at

this patch in depth."  Jon's text seems like a really good summation

of that idea in more explicit language.
 - R.

-

Signed-off-by (as far as the Developer's Certificate of Origin defines

it) says:  I made sure that this patch complies with all involved licenses.

--

Stefan Richter

-=====-=-=== =-=- -=---

http://arcgraph.de/sr/

-

Tested-by is more valuable than acked-by, because its empirical.

Acked-by generally means "I don't generally object to the idea of the

patch, but may not have read beyond the changelog".  Tested-by implies

"I did something that exercised the patch, and it didn't explode" -

that's on par with an actual review (ideally all patches would be both
Hm.  We have a tension here:
    * there aren't enough reviewers

    * some reviews are more useful than others
While a review by a trustworthy person is invaluable, we don't want to

discourage people from reviewing.  A new reviewer's review may not be

terribly useful, but a meta-review may help improve it.  Or it could be

a great review.
I guess I'm proposing that we also need to expand the reviewer base, and

to do so we need some kind of reviewer-mentoring or metareview process.

Of course that could just be an extra burden on the existing (small)

trusted reviewer base, but the hope is that over time the reviewer pool
Well, any interested parties, really.  I use it for original bug

reporters, people who followed up on the report, people who have patches

in a nearby area, people who are known to be interested in the affected

subsystem, people who have reviewed previous versions of the patch, etc...
    J

-

but Tested-by: doesn't have to involve any "actually looking at/reading

the patch."  Right?
---

~Randy

-

Tested-by translated into German and back into English:  "Works for me,

test methods not specified."
So, putting a Tested-by into the changelog is only useful if the

necessary testing is rather simple (i.e. "fixed the bug which I was

always able to reproduce before") or if the tester is known to have

performed rigorous and sufficiently broad tests.

--

Stefan Richter

-=====-=-=== =-=- -=---

http://arcgraph.de/sr/

-

Well, you can still include those test-method details in the body of the

message in addition to adding the "Tested-by:".
Does "Tested-by" just mean they ran some relevant test on the final

version of the patch?  The really hard part is often the initial work

required to find a good reproduceable test case, capture the right error

report, or bisect to the right commit.  I think that also counts as

"testing".  And it'd be nice to have a tag for those sorts of

contributions, partly just as another way to ackowledge them.
--b.

-

---
Tested-by should, at the very least, have a description of the test

environment in the body (suggesting that the change compiled and ran

in that environment). Preferably it should also have a description of

the test or test suite run and whether that test failed on an

unpatched system.
scott

--

scott preece

-

Tested-by: is sort of trivial for a fix patch, for example, if a bug reporter

confirms that the proposed patch actually fixes the issue.  IMHO it wouldn't

be practical to complicate that.
Greetings,

Rafael

-

I see two types of Tested-by.
1) As you stated, a fixed to a problem that the reporter has seen. So

that someone could state a "fixes issue" in the change log and that

would simple mean that the tester has seen a problem, and the attached

patch fixes it.
2) Someone has a testsuite to the area that the change affects. So if

someone has developed a networking test suite and a patch changes some

networking logic, the Tested-by could be that the tester actually ran

specific tests.  This should require a more detail explaination of what

was done. Or the very least, a pointer to a web page of the tests that

were run.
So for the user that sees an issue, then gets a patch, perhaps all they

need to do is add a "fixed problem" or "works now" in the change log to

denote that the patch has actually (or seems to) fix the problem that

they previously seen. This shouldn't be too hard.
But for those that run test suites, they should be smart enough to put

in more documentation into the change log to state how it was tested.
Perhaps we need to add yet another signed off.
"Verified-by", which could be for the user that saw an issue and the

patch now fixes it. That user could just add the "Verified-by" to the

patch to acknowledge (and record) that the patch did fix the issue.
The "Tested-by" can be used for patches that are run through a test

suite.
Just a thought.
-- Steve
-

I disagree.  The SCM changelog should contain _what_ a patch does and if

necessary _why_ it does so.  The rest (e.g. the sign-off tag to state

that the licensing is alright, and any other tags) should have its

meaning sufficiently defined outside the changelog.
Remember what the SCM changelog is for, i.e. what we do with it after

commit.

--

Stefan Richter

-=====-=-=== =-=- -=--=

http://arcgraph.de/sr/

-

The _why_ part is more important than _what_. The diff should hopefully

explain the _what_ part.
	Sam

-

"What": fix lockup in this and that circumstances

"Why": because lockups are annoying

"How": the diff

(That's what I meant with what and why.)

--

Stefan Richter

-=====-=-=== =-=- -=--=

http://arcgraph.de/sr/

-

PS, example with non-trivial why:

What: add ABI which correlates bus cycle counter and local time

Why: apps need it to sync streams from different buses

How: the diff

--

Stefan Richter

-=====-=-=== =-=- -=--=

http://arcgraph.de/sr/

-

Yes, although the primary purpose of the various tags should be to

document the quality assurance process, or how to call it.
However, what belongs into the SCM changelog, and what can the list

archives provide?

--

Stefan Richter

-=====-=-=== =-=- -=---

http://arcgraph.de/sr/

-

Sure, absolutely.  I never said its a substitute for review.  An ugly

working patch is useful, because its the raw material for a nice working

patch.  A nice non-working patch can be framed and admired from a

distance, but it isn't terribly useful.
    J

-

Hi Jonathan,
[snip]
Looks good but how is this different from the Acked-by tag we are already using?
                                     Pekka

-