login
Search
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.
-
Hi Jonathan,
[snip]
Looks good but how is this different from the Acked-by tag we are already using?
Pekka
-
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 -
* 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 -
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 -
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
-
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. -
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/ -
--- 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/ -
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/ -
> >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. -
--- 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 -
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 -
...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 ...
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> -
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. -
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/
-
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 -
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 -
