RFC: reviewer's statement of oversight

RFC: reviewer's statement of oversight

[Jonathan Corbet]

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.

Re: RFC: reviewer's statement of oversight

[Pekka Enberg]

Hi Jonathan,

On 10/8/07, Jonathan Corbet <corbet@lwn.net> wrote:
> 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?

[snip]

Looks good but how is this different from the Acked-by tag we are already using?

                                     Pekka

Re: RFC: reviewer's statement of oversight

[Sam Ravnborg]

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

Re: RFC: reviewer's statement of oversight

[Jan Engelhardt]

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

Re: RFC: reviewer's statement of oversight

[Jeremy Fitzhardinge]

Jan Engelhardt wrote:
>> Acked-by:
>> Tested-by:
>>     
>
> * Used by random people to express their (dis)like/experience with the 
> patch.
>   

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
tested and reviewed).

>> Reviewed-by:
>>     
>
> * I am maintaner or an 'important' person and have had a
>   look at it in depth
>   

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
size grows enough to make the effort worthwhile...


>> Cc:
>>     
>
> * Used by original submitter to denote additional maintainers it goes to
> * Parties who should be Cced when an a posteriori question comes up
>   

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

Re: RFC: reviewer's statement of oversight

[Randy Dunlap]

On Mon, 08 Oct 2007 11:01:49 -0700 Jeremy Fitzhardinge wrote:

> Jan Engelhardt wrote:
> >> Acked-by:
> >> Tested-by:
> >>     
> >
> > * Used by random people to express their (dis)like/experience with the 
> > patch.
> >   
> 
> 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
> tested and reviewed).

but Tested-by: doesn't have to involve any "actually looking at/reading
the patch."  Right?

IOW, the patch could be ugly as sin but it works...


> >> Reviewed-by:
> >>     
> >
> > * I am maintaner or an 'important' person and have had a
> >   look at it in depth
> >   
> 
> 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
> size grows enough to make the effort worthwhile...
> 
> 
> >> Cc:
> >>     
> >
> > * Used by original submitter to denote additional maintainers it goes to
> > * Parties who should be Cced when an a posteriori question comes up
> >   
> 
> 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...

---
~Randy

Re: RFC: reviewer's statement of oversight

[Jeremy Fitzhardinge]

Randy Dunlap wrote:
> but Tested-by: doesn't have to involve any "actually looking at/reading
> the patch."  Right?
>
> IOW, the patch could be ugly as sin but it works...
>   

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

Re: RFC: reviewer's statement of oversight

[Stefan Richter]

Randy Dunlap wrote:
> On Mon, 08 Oct 2007 11:01:49 -0700 Jeremy Fitzhardinge wrote:
>> 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
>> tested and reviewed).
> 
> but Tested-by: doesn't have to involve any "actually looking at/reading
> the patch."  Right?
> 
> IOW, the patch could be ugly as sin but it works...

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/

Re: RFC: reviewer's statement of oversight

[J. Bruce Fields]

On Mon, Oct 08, 2007 at 08:34:47PM +0200, Stefan Richter wrote:
> Randy Dunlap wrote:
> > On Mon, 08 Oct 2007 11:01:49 -0700 Jeremy Fitzhardinge wrote:
> >> 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
> >> tested and reviewed).
> > 
> > but Tested-by: doesn't have to involve any "actually looking at/reading
> > the patch."  Right?
> > 
> > IOW, the patch could be ugly as sin but it works...
> 
> 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.

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.

Re: RFC: reviewer's statement of oversight

[Stefan Richter]

J. Bruce Fields wrote:
> And it'd be nice to have a tag for those sorts of
> contributions, partly just as another way to ackowledge them.

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/

Re: RFC: reviewer's statement of oversight

[Scott Preece]

On 10/8/07, J. Bruce Fields <bfields@fieldses.org> wrote:
> On Mon, Oct 08, 2007 at 08:34:47PM +0200, Stefan Richter wrote:
...
> > 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.
>
> 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.
---

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

Re: RFC: reviewer's statement of oversight

[Rafael J. Wysocki]

On Monday, 8 October 2007 21:26, Scott Preece wrote:
> On 10/8/07, J. Bruce Fields <bfields@fieldses.org> wrote:
> > On Mon, Oct 08, 2007 at 08:34:47PM +0200, Stefan Richter wrote:
> ...
> > > 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.
> >
> > 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.
> ---
> 
> 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.

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

Re: RFC: reviewer's statement of oversight

[Steven Rostedt]

On Mon, Oct 08, 2007 at 10:16:26PM +0200, Rafael J. Wysocki wrote:
> 
> 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.
>

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

Re: RFC: reviewer's statement of oversight

[Stefan Richter]

Steven Rostedt wrote:
> 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.

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/

Re: RFC: reviewer's statement of oversight

[Sam Ravnborg]

On Tue, Oct 09, 2007 at 08:11:53AM +0200, Stefan Richter wrote:
> Steven Rostedt wrote:
> > 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.
> 
> I disagree.  The SCM changelog should contain _what_ a patch does and if
> necessary _why_ it does so.
The _why_ part is more important than _what_. The diff should hopefully
explain the _what_ part.

	Sam

Re: RFC: reviewer's statement of oversight

[Stefan Richter]

Sam Ravnborg wrote:
> On Tue, Oct 09, 2007 at 08:11:53AM +0200, Stefan Richter wrote:
>> The SCM changelog should contain _what_ a patch does and if
>> necessary _why_ it does so.
> The _why_ part is more important than _what_. The diff should hopefully
> explain the _what_ part.

"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/

Re: RFC: reviewer's statement of oversight

[Stefan Richter]

> Sam Ravnborg wrote:
>> On Tue, Oct 09, 2007 at 08:11:53AM +0200, Stefan Richter wrote:
>>> The SCM changelog should contain _what_ a patch does and if
>>> necessary _why_ it does so.
>> The _why_ part is more important than _what_. The diff should hopefully
>> explain the _what_ part.
> 
> "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.)

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/

Re: RFC: reviewer's statement of oversight

[Stefan Richter]

Jan Engelhardt wrote:
> 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")

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/

Re: RFC: reviewer's statement of oversight

[Roland Dreier]

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

Re: RFC: reviewer's statement of oversight

[Scott Preece]

On 10/8/07, Jan Engelhardt <jengelh@computergmbh.de> wrote:
>
> On Oct 8 2007 19:37, Sam Ravnborg wrote:
> >snip...
> >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
---

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

Re: RFC: reviewer's statement of oversight

[H. Peter Anvin]

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

Re: RFC: reviewer's statement of oversight

[Theodore Tso]

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

Re: RFC: reviewer's statement of oversight

[Rafael J. Wysocki]

On Monday, 8 October 2007 23:38, Theodore Tso wrote:
> 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....

I agree.

Greetings,
Rafael

Re: RFC: reviewer's statement of oversight

[Oleg Verych]

* Mon, 8 Oct 2007 17:38:52 -0400
>
> 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.   

Experience of convincing experienced patch author, that some things in
the patch are wrong :)

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

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

Re: RFC: reviewer's statement of oversight

[Jonathan Corbet]

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.

Re: RFC: reviewer's statement of oversight

[Randy Dunlap]

On Mon, 08 Oct 2007 16:43:10 -0600 Jonathan Corbet wrote:

> 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

Just typos noted below...

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

                                                      acceptable

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

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


---
~Randy

Re: RFC: reviewer's statement of oversight

[Stephen Hemminger]

On Mon, 8 Oct 2007 16:06:03 -0700
Randy Dunlap <randy.dunlap@oracle.com> wrote:

> On Mon, 08 Oct 2007 16:43:10 -0600 Jonathan Corbet wrote:
> 
> > 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
> 
> Just typos noted below...
> 
> > ---
> > 
> > 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.
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. 
If file is GPL v2 only, then the changes are GPL v2 only, ...)

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

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>

Re: RFC: reviewer's statement of oversight

[J. Bruce Fields]

On Mon, Oct 08, 2007 at 04:43:10PM -0600, Jonathan Corbet wrote:
> + (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.

Is this paragraph really necessary?  (For example, is there some history
of problems that this is addressing?)

--b.

Re: RFC: reviewer's statement of oversight

[Alan Cox]

On Mon, 8 Oct 2007 19:30:54 -0400
"J. Bruce Fields" <bfields@fieldses.org> wrote:

> On Mon, Oct 08, 2007 at 04:43:10PM -0600, Jonathan Corbet wrote:
> > + (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.
> 
> Is this paragraph really necessary?  (For example, is there some history
> of problems that this is addressing?)

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.

Re: RFC: reviewer's statement of oversight

[Stefan Richter]

Jonathan Corbet wrote:
> All of these
> +tags have the form:
> +
> +	Something-done-by: Full name <email@address>

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/

Re: RFC: reviewer's statement of oversight

[Neil Brown]

On Monday October 8, corbet@lwn.net wrote:

I find it is always good to know *why* we have the tags.  That
information is a useful complement to what they mean, and can guide
people in adding them.

So below I present some "Purposes", YetAnotherTag, and a comment on
the RSO.

(And I'd like to add a vote for "Blame-Shared-By:" rather than
"Reviewed-by:", however I don't I'll get much support...)

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

   From:        The Author, Primary Author, or Authors of the patch.
                Authors should also provide a Signed-off-by: tag.

                Purpose: to give credit to authors
> +
> +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.

                Purpose: to allow subsequent review of the originality of 
                the contribution 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.  An Acked-by tag does not
> +		imply any involvement in the development of the patch or
> +		that a detailed review was done.

                Purpose:  to inform upstream aggregators that
		consensus was achieved for the change.  This is
		particularly relevant for changes that affect multiple
		Maintenance Domains.

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

		Purpose: to inform upstream aggregators that due
		diligence has been performed to ensure correctness of
		the change.  Also to give credit to reviewers.

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

		Purpose: to ensure that interested parties are
		included in subsequent discussions of the change.

> +
> +Tested-by:	The patch has been successfully tested (in some
> +		environment) by the person named.

		Purpose: to give credit to 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.

This seems more detailed that necessary.  The process (communicated
back / responded) is not really relevant.  I would go for something
like:

    (b) I have no outstanding problems, concerns, or questions about
        this patch (except as noted in the above comments).

and in fact, given (c2), (b) might not be needed at all.

NeilBrown


> +
> + (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.
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: RFC: reviewer's statement of oversight

[Jonathan Corbet]

Neil Brown <neilb@suse.de> wrote:

> I find it is always good to know *why* we have the tags.  That
> information is a useful complement to what they mean, and can guide
> people in adding them.

Hmm...I was just going to go with the "because I told you so" approach
that I use with my kids.  It works so well with them after all.  

<pauses to go scream at his kids who have never understood why playing
"Dance Dance Revolution" directly above the office is hard on
productivity> 

I agree with just about everything you've said, and am tweaking things
accordingly.  But...

> > + (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.
> 
> This seems more detailed that necessary.  The process (communicated
> back / responded) is not really relevant.

Instead, it seems to me that the process is crucially important.
Reviewed-by shouldn't be a rubber stamp that somebody applies to a
patch; I think it should really imply that issues of interest have been
communicated to the developers.  If we are setting expectations for what
Reviewed-by means, I would prefer to leave an explicit mention of
communication in there.  If I'm in the minority here, though, it can
certainly come out.

Thanks,

jon

Re: RFC: reviewer's statement of oversight

[Roland Dreier]

 > > > + (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.
 > > 
 > > This seems more detailed that necessary.  The process (communicated
 > > back / responded) is not really relevant.
 > 
 > Instead, it seems to me that the process is crucially important.

I agree with you Jon.  In fact my first reaction to your initial post
was that this section (b) was the most succinct distillation of the
most important part of reviewing that I've seen.

 - R.

Re: RFC: reviewer's statement of oversight

[David Chinner]

On Tue, Oct 09, 2007 at 10:49:20AM -0600, Jonathan Corbet wrote:
> Neil Brown <neilb@suse.de> wrote:
> > > + (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.
> > 
> > This seems more detailed that necessary.  The process (communicated
> > back / responded) is not really relevant.
> 
> Instead, it seems to me that the process is crucially important.
> Reviewed-by shouldn't be a rubber stamp that somebody applies to a
> patch; I think it should really imply that issues of interest have been
> communicated to the developers.  If we are setting expectations for what
> Reviewed-by means, I would prefer to leave an explicit mention of
> communication in there. 

I couldn't agree more, Jon.

If we are to have a meaningful reviewed-by tag, it has to be clearly
documented as to what responsibilities it places on the reviewer. If
someone doesn't want to perform a well conducted review, then they
haven't earned the right to issue a Reviewed-by tag - they can use
the Acked-by rubber stamp instead.

FWIW, w.r.t. XFS patches, we already follow both the letter and
intent of your proposed reviewed-by tag for all changes to XFS code
and reviewers are currently listed as Signed-off-by in git-commits
(our internal SCM records the reviewer(s) and the git export script
converts that to s-o-b).  It would be much more meaningful if they
were exported as Reviewed-by under your definition....

IOWs, I fully support your definition of the Reviewed-by tag.

Cheers,

Dave.
-- 
Dave Chinner
Principal Engineer
SGI Australian Software Group

Re: RFC: reviewer's statement of oversight

[Neil Brown]

On Wednesday October 10, dgc@sgi.com wrote:
> On Tue, Oct 09, 2007 at 10:49:20AM -0600, Jonathan Corbet wrote:
> > Neil Brown <neilb@suse.de> wrote:
> > > > + (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.
> > > 
> > > This seems more detailed that necessary.  The process (communicated
> > > back / responded) is not really relevant.
> > 
> > Instead, it seems to me that the process is crucially important.
> > Reviewed-by shouldn't be a rubber stamp that somebody applies to a
> > patch; I think it should really imply that issues of interest have been
> > communicated to the developers.  If we are setting expectations for what
> > Reviewed-by means, I would prefer to leave an explicit mention of
> > communication in there. 
> 
> I couldn't agree more, Jon.
> 
> If we are to have a meaningful reviewed-by tag, it has to be clearly
> documented as to what responsibilities it places on the reviewer. If
> someone doesn't want to perform a well conducted review, then they
> haven't earned the right to issue a Reviewed-by tag - they can use
> the Acked-by rubber stamp instead.

Maybe I'm making a mountain out of a molehill but...

Clearly documented responsibilities?  Yes.
Prescribed process?  No.

If someone sends me a patch, and I review it, and I find a couple of
problems, do I need to negotiate with the submitter before correcting
them and putting a "Reviewed-by" tag on it (along with my
Signed-off-by before sending it upstream)?

The above clause (b) seems to say that I do.  Is that something we
want to mandate?

My take on the responsibilities implied by Reviewed-by: is that the
code has been inspected, comprehended, considered, and found to be
both appropriate and without discernible error.  The process by which
the code got to that state is not relevant to the tag (though it
probably is relevant to the general health of the community).

NeilBrown

Re: RFC: reviewer's statement of oversight

[Sam Ravnborg]

Hi Neil.
> 
>    From:        The Author, Primary Author, or Authors of the patch.
>                 Authors should also provide a Signed-off-by: tag.
> 
>                 Purpose: to give credit to authors
The SCM should include this info and we should not duplicate this
in the changelog's.
I know some tools require this format but that's something else.

> > +
> > +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.
> 
>                 Purpose: to allow subsequent review of the originality of 
>                 the contribution should copyright questions arise.

We often use s-o-b to docuemnt the path a patch took from origin (the
top-most s-o-b) to tree apply (lowest s-o-b).
This is IIUC part of the intended behaviour of s-o-b but it is not
clear from the above text.


> > +
> > +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.
> 
>                 Purpose:  to inform upstream aggregators that
> 		consensus was achieved for the change.  This is
> 		particularly relevant for changes that affect multiple
> 		Maintenance Domains.
> 
consensus seems too strong a wording here. consensus imply more than one
that agree on the patch where I often see people give their "Acked-by:" by
simple changelog reading.
So Acked-by: is not used like documented today.

	Sam

Re: RFC: reviewer's statement of oversight

[Neil Brown]

On Tuesday October 9, sam@ravnborg.org wrote:
> Hi Neil.
> > 
> >    From:        The Author, Primary Author, or Authors of the patch.
> >                 Authors should also provide a Signed-off-by: tag.
> > 
> >                 Purpose: to give credit to authors
> The SCM should include this info and we should not duplicate this
> in the changelog's.
> I know some tools require this format but that's something else.

If the SCM stores some tags in special places, that is fine with me.
The remove the need for the tag and an understanding of why it exists.
Can 'git' store a list of Authors?  Do we want to allow a list?

> 
> > > +
> > > +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.
> > 
> >                 Purpose: to allow subsequent review of the originality of 
> >                 the contribution should copyright questions arise.
> 
> We often use s-o-b to docuemnt the path a patch took from origin (the
> top-most s-o-b) to tree apply (lowest s-o-b).
> This is IIUC part of the intended behaviour of s-o-b but it is not
> clear from the above text.

My understanding of Andrew Morton's position on s-o-b is that it is an
unordered set.  I know this because when I have sent him patches with
a proper From: line, he has complained and begrudingly took the first
s-o-b, but said he didn't like to.
So there seems to be disagreement on this (I think it looks like a
path to - but apparently not to everyone).

> 
> 
> > > +
> > > +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.
> > 
> >                 Purpose:  to inform upstream aggregators that
> > 		consensus was achieved for the change.  This is
> > 		particularly relevant for changes that affect multiple
> > 		Maintenance Domains.
> > 
> consensus seems too strong a wording here. consensus imply more than one
> that agree on the patch where I often see people give their "Acked-by:" by
> simple changelog reading.

I'm failing to follow your logic.
You seem to be contrasting:
  "consensus imply more than one that agree"
     which I agree with:  "From" plus all "Acked-By" will be more than
     one in all cases that "Acked-By" is used
with
  "people give their "Acked-by:" by simple changlog reading"
     which I also agree with but this just highlights that "Acked-by"
     is different from "Reviewed-by" 

Confused.

Thanks,
NeilBrown

Re: RFC: reviewer's statement of oversight

[Sam Ravnborg]

On Mon, Oct 15, 2007 at 10:35:39AM +1000, Neil Brown wrote:
> On Tuesday October 9, sam@ravnborg.org wrote:
> > Hi Neil.
> > > 
> > >    From:        The Author, Primary Author, or Authors of the patch.
> > >                 Authors should also provide a Signed-off-by: tag.
> > > 
> > >                 Purpose: to give credit to authors
> > The SCM should include this info and we should not duplicate this
> > in the changelog's.
> > I know some tools require this format but that's something else.
> 
> If the SCM stores some tags in special places, that is fine with me.
> The remove the need for the tag and an understanding of why it exists.
> Can 'git' store a list of Authors?  Do we want to allow a list?
git stores to my best knowledge only a single author.
Infrequently we need a list but then people have solved it putting
relevant people in s-o-b and by give credit in changelog.
This is IMHO good enugh.

> 
> > 
> > > > +
> > > > +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.
> > > 
> > >                 Purpose: to allow subsequent review of the originality of 
> > >                 the contribution should copyright questions arise.
> > 
> > We often use s-o-b to docuemnt the path a patch took from origin (the
> > top-most s-o-b) to tree apply (lowest s-o-b).
> > This is IIUC part of the intended behaviour of s-o-b but it is not
> > clear from the above text.
> 
> My understanding of Andrew Morton's position on s-o-b is that it is an
> unordered set.  I know this because when I have sent him patches with
> a proper From: line, he has complained and begrudingly took the first
> s-o-b, but said he didn't like to.
> So there seems to be disagreement on this (I think it looks like a
> path to - but apparently not to everyone).
With the current definition you need to supply BOTH a from: and a s-o-b.
I usually request a s-o-b when it is missing no matter what other content 
is present in the changelog.

> 
> > 
> > 
> > > > +
> > > > +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.
> > > 
> > >                 Purpose:  to inform upstream aggregators that
> > > 		consensus was achieved for the change.  This is
> > > 		particularly relevant for changes that affect multiple
> > > 		Maintenance Domains.
> > > 
> > consensus seems too strong a wording here. consensus imply more than one
> > that agree on the patch where I often see people give their "Acked-by:" by
> > simple changelog reading.
> 
> I'm failing to follow your logic.
> You seem to be contrasting:
>   "consensus imply more than one that agree"
>      which I agree with:  "From" plus all "Acked-By" will be more than
>      one in all cases that "Acked-By" is used
I did not realise that "concensus" in this context refered to both the
one that give the "Acked-by" and the author.
Viewing it this way I agree with the intent and the text.

	Sam

Re: RFC: reviewer's statement of oversight

[Scott Preece]

On 10/8/07, Jonathan Corbet <corbet@lwn.net> wrote:

Some minor rewording suggestions:

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

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

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

I would suggest dropping the "(or may not)" as unnecessary, and
changing the "which would" to "that would".

---
> +
> + (d) While I have reviewed the patch and believe it to be sound, I can not
---

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

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

(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

Re: RFC: reviewer's statement of oversight

[Mark Gross]

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

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"
statements are better expressed with other tags.

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

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

Re: RFC: reviewer's statement of oversight

[Stefan Richter]

Mark Gross wrote:
> On Mon, Oct 08, 2007 at 11:24:45AM -0600, Jonathan Corbet wrote:
>>  (a) I have carried out a technical review of this patch to evaluate its
>>      appropriateness and readiness for inclusion into the mainline kernel. 
[...]
>>  (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.
> 
> 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"
> statements are better expressed with other tags.

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/

Re: RFC: reviewer's statement of oversight

[Al Viro]

On Mon, Oct 08, 2007 at 08:53:05PM +0200, Stefan Richter wrote:
> Mark Gross wrote:
> > On Mon, Oct 08, 2007 at 11:24:45AM -0600, Jonathan Corbet wrote:
> >>  (a) I have carried out a technical review of this patch to evaluate its
> >>      appropriateness and readiness for inclusion into the mainline kernel. 
> [...]
> >>  (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.
> > 
> > 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"
> > statements are better expressed with other tags.
> 
> A patch which is not "worthwhile" is also not "appropriate".  Mere
> correctness in a mathematical sense is not enough as technical review
> criterion.

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

Re: RFC: reviewer's statement of oversight

[Jonathan Corbet]

Al Viro <viro@ftp.linux.org.uk> wrote:

> > A patch which is not "worthwhile" is also not "appropriate".  Mere
> > correctness in a mathematical sense is not enough as technical review
> > criterion.
> 
> 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