Re: [PATCH RFC 1/2] MAINTAINERS: Add "R:" designated-reviewers tag
On 6/4/2014 9:01 PM, Dave Chinner wrote: > On Tue, Jun 03, 2014 at 01:43:47PM -0400, Steven Rostedt wrote: >> On Tue, 3 Jun 2014 17:16:54 +1000 >> Dave Chinner wrote: >> >> >>> If you take it to an extremes. Think about what you can test in 15 >>> minutes. Or for larger patchsets, how long it takes you to read the >>> patchset? >> >> Yeah, what about that? > > That testing a patch for obvious, common regressions takes no longer > than it does to read and review the logic. > >>> IMO, every reviewer has their own developement environment and they >>> should be at least testing that the change they are reviewing >>> doesn't cause problems in that environment, just like they do for >>> their own code before they post it for review. >> >> Let me ask you this. In the scientific community, when someone posts a >> research project and asks their peers to review their work. Are all >> those reviewers required to test out that paper? >> Or are they to review it, check the math, look for cases that are >> missed, see common errors, and other checks? I'm sure some >> reviewers may do various tests, but others will just check the >> logic. I'm having a very hard time seeing where Reviewed-by means >> tested-by. I see those as two completely different tags. > > We are not conducting a scientific research experiment here. We are > conduting a very large software *engineering* project here. Yes, software engineering. Where software review is a manual process of *reading* and understanding code, in all of the processes I have been involved in at big corporations that love big process. (Not to claim I know of all the processes everyone else uses...) Why can't you just let reviewed-by and tested-by mean different things instead of one being a super-set of the other? If you force reviewed-by to also mean tested-by then you just shrank your available pool of reviewers. > > So perhaps we should be using robust software engineering processes > rather than academic peer review as the model for our code review > process? < snip > Cheers, Frank -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@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: [PATCH RFC 1/2] MAINTAINERS: Add R: designated-reviewers tag
On 6/4/2014 9:01 PM, Dave Chinner wrote: On Tue, Jun 03, 2014 at 01:43:47PM -0400, Steven Rostedt wrote: On Tue, 3 Jun 2014 17:16:54 +1000 Dave Chinner da...@fromorbit.com wrote: If you take it to an extremes. Think about what you can test in 15 minutes. Or for larger patchsets, how long it takes you to read the patchset? Yeah, what about that? That testing a patch for obvious, common regressions takes no longer than it does to read and review the logic. IMO, every reviewer has their own developement environment and they should be at least testing that the change they are reviewing doesn't cause problems in that environment, just like they do for their own code before they post it for review. Let me ask you this. In the scientific community, when someone posts a research project and asks their peers to review their work. Are all those reviewers required to test out that paper? Or are they to review it, check the math, look for cases that are missed, see common errors, and other checks? I'm sure some reviewers may do various tests, but others will just check the logic. I'm having a very hard time seeing where Reviewed-by means tested-by. I see those as two completely different tags. We are not conducting a scientific research experiment here. We are conduting a very large software *engineering* project here. Yes, software engineering. Where software review is a manual process of *reading* and understanding code, in all of the processes I have been involved in at big corporations that love big process. (Not to claim I know of all the processes everyone else uses...) Why can't you just let reviewed-by and tested-by mean different things instead of one being a super-set of the other? If you force reviewed-by to also mean tested-by then you just shrank your available pool of reviewers. /dead-horse beating So perhaps we should be using robust software engineering processes rather than academic peer review as the model for our code review process? snip Cheers, Frank -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@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: [PATCH RFC 1/2] MAINTAINERS: Add "R:" designated-reviewers tag
On Tue, Jun 03, 2014 at 01:43:47PM -0400, Steven Rostedt wrote: > On Tue, 3 Jun 2014 17:16:54 +1000 > Dave Chinner wrote: > > > > If you take it to an extremes. Think about what you can test in 15 > > minutes. Or for larger patchsets, how long it takes you to read the > > patchset? > > Yeah, what about that? That testing a patch for obvious, common regressions takes no longer than it does to read and review the logic. > > IMO, every reviewer has their own developement environment and they > > should be at least testing that the change they are reviewing > > doesn't cause problems in that environment, just like they do for > > their own code before they post it for review. > > Let me ask you this. In the scientific community, when someone posts a > research project and asks their peers to review their work. Are all > those reviewers required to test out that paper? > Or are they to review it, check the math, look for cases that are > missed, see common errors, and other checks? I'm sure some > reviewers may do various tests, but others will just check the > logic. I'm having a very hard time seeing where Reviewed-by means > tested-by. I see those as two completely different tags. We are not conducting a scientific research experiment here. We are conduting a very large software *engineering* project here. So perhaps we should be using robust software engineering processes rather than academic peer review as the model for our code review process? > > > I find the reviewers of my code to be the worse testers. > > > That's because those that I ask to review my code know what > > > it's suppose to do, and those are the people that are not > > > going to stumble over bugs. It's the people that have no idea > > > how your code works that will trigger the most bugs in testing > > > it. My best testers would be my worse reviewers. > > > > "Reviewers are the worst testers". > > > > Yet you accept the code they write as sufficiently well tested > > for merging? :/ > > Heh, that's why I have a full test suite. Yes, I run my tests on > every single patch (to make sure it's fully bisectable, this is > why I wrote ktest.pl). > > And yes, I find bugs and then send the patches back to be fixed. I > never blindly accept anyone's patches and just merge it. IOWs, part of your patch acceptance requirement is that patches are fully bisectable and mostly pass your own tests. Isn't that exactly what I've been saying Reviewed-by is supposed to mean? Anyway, let's not split hairs over the testing levels anymore, we can agree to disagree there. Why? Because that disagreement is the most important point here: reviewed-by means different things to different people, and that's a big problem from a software engineering process point of view. > > > patches, but since I don't have an XFS filesystem, I doubt > > > that would be much use for you. > > > > I don't want you to review XFS code - you have no domain > > specific knowledge nor a test environment for XFS changes. IOWs, > > you meet none of the requirements to give a "reviewed-by" for an > > XFS change, and so to say you reviewed a change makes a mockery > > of the expectations outlines in the SubmittingPatches > > guidelines. > > Actually, I would be able to review tracing changes in XFS. I've > given tags like "Revieved-by: Steven Rostedt > # for the tracing part". before. But in that case you are reviewing tracing code - not XFS code - which is within your field of expertise. However, I wouldn't expect your review to tell me that the tracepoints are accessing some internal XFS state unsafely or not providing the information that the patch submitter expects to provide. That side of things still needs XFS expertise to determine > > Reviewed-by only has value when it is backed by process and > > domain specific knowledge. If the person giving that tag doesn't > > have either of these, then it's worthless and they need to be > > taught what the correct thing to do is. Most people (even > > projects) don't learn proper software engineering processes > > until after they have been at the pointy end of a pile of crap > > at least once. :/ > > This last paragraph I totally agree with. There's a few people > that I trust very much so for a review. I don't expect them to > test my code, but they are usually good enough to see something > that may break under certain conditions. I don't add any review-by > tags from anyone I don't already have a working relationship with > and trust their judgment. Again, you're demonstrating exactly the problem I see: Reviewed-by has no meaning unless the maintainer trusts the developer and knows they have the necessary processes in place to perform a reliable review. > What I'm saying is that to most, Reviewed-by means just that. The > patch was reviewed. I think the person adding their reviewed-by > tag is placing their reputation on the line. I know I am every > time I add it. That's why I give a lot more "Acked-by" than
Re: [PATCH RFC 1/2] MAINTAINERS: Add R: designated-reviewers tag
On Tue, Jun 03, 2014 at 01:43:47PM -0400, Steven Rostedt wrote: On Tue, 3 Jun 2014 17:16:54 +1000 Dave Chinner da...@fromorbit.com wrote: If you take it to an extremes. Think about what you can test in 15 minutes. Or for larger patchsets, how long it takes you to read the patchset? Yeah, what about that? That testing a patch for obvious, common regressions takes no longer than it does to read and review the logic. IMO, every reviewer has their own developement environment and they should be at least testing that the change they are reviewing doesn't cause problems in that environment, just like they do for their own code before they post it for review. Let me ask you this. In the scientific community, when someone posts a research project and asks their peers to review their work. Are all those reviewers required to test out that paper? Or are they to review it, check the math, look for cases that are missed, see common errors, and other checks? I'm sure some reviewers may do various tests, but others will just check the logic. I'm having a very hard time seeing where Reviewed-by means tested-by. I see those as two completely different tags. We are not conducting a scientific research experiment here. We are conduting a very large software *engineering* project here. So perhaps we should be using robust software engineering processes rather than academic peer review as the model for our code review process? I find the reviewers of my code to be the worse testers. That's because those that I ask to review my code know what it's suppose to do, and those are the people that are not going to stumble over bugs. It's the people that have no idea how your code works that will trigger the most bugs in testing it. My best testers would be my worse reviewers. Reviewers are the worst testers. Yet you accept the code they write as sufficiently well tested for merging? :/ Heh, that's why I have a full test suite. Yes, I run my tests on every single patch (to make sure it's fully bisectable, this is why I wrote ktest.pl). And yes, I find bugs and then send the patches back to be fixed. I never blindly accept anyone's patches and just merge it. IOWs, part of your patch acceptance requirement is that patches are fully bisectable and mostly pass your own tests. Isn't that exactly what I've been saying Reviewed-by is supposed to mean? Anyway, let's not split hairs over the testing levels anymore, we can agree to disagree there. Why? Because that disagreement is the most important point here: reviewed-by means different things to different people, and that's a big problem from a software engineering process point of view. patches, but since I don't have an XFS filesystem, I doubt that would be much use for you. I don't want you to review XFS code - you have no domain specific knowledge nor a test environment for XFS changes. IOWs, you meet none of the requirements to give a reviewed-by for an XFS change, and so to say you reviewed a change makes a mockery of the expectations outlines in the SubmittingPatches guidelines. Actually, I would be able to review tracing changes in XFS. I've given tags like Revieved-by: Steven Rostedt rost...@goodmis.org # for the tracing part. before. But in that case you are reviewing tracing code - not XFS code - which is within your field of expertise. However, I wouldn't expect your review to tell me that the tracepoints are accessing some internal XFS state unsafely or not providing the information that the patch submitter expects to provide. That side of things still needs XFS expertise to determine Reviewed-by only has value when it is backed by process and domain specific knowledge. If the person giving that tag doesn't have either of these, then it's worthless and they need to be taught what the correct thing to do is. Most people (even projects) don't learn proper software engineering processes until after they have been at the pointy end of a pile of crap at least once. :/ This last paragraph I totally agree with. There's a few people that I trust very much so for a review. I don't expect them to test my code, but they are usually good enough to see something that may break under certain conditions. I don't add any review-by tags from anyone I don't already have a working relationship with and trust their judgment. Again, you're demonstrating exactly the problem I see: Reviewed-by has no meaning unless the maintainer trusts the developer and knows they have the necessary processes in place to perform a reliable review. What I'm saying is that to most, Reviewed-by means just that. The patch was reviewed. I think the person adding their reviewed-by tag is placing their reputation on the line. I know I am every time I add it. That's why I give a lot more Acked-by than Reviewed-by. Those acks are me saying I skimmed the patch, and there's nothing in
Re: [PATCH RFC 1/2] MAINTAINERS: Add "R:" designated-reviewers tag
On 06/03/2014 03:27 AM, Paul E. McKenney wrote: > On Mon, Jun 02, 2014 at 12:11:55PM -0700, j...@joshtriplett.org wrote: >> On Mon, Jun 02, 2014 at 12:08:21PM -0700, Paul E. McKenney wrote: >>> On Mon, Jun 02, 2014 at 11:56:35AM -0700, j...@joshtriplett.org wrote: On Mon, Jun 02, 2014 at 11:16:58AM -0700, Paul E. McKenney wrote: > On Mon, Jun 02, 2014 at 10:59:28AM -0700, Joe Perches wrote: >> On Mon, 2014-06-02 at 10:48 -0700, j...@joshtriplett.org wrote: >>> On Mon, Jun 02, 2014 at 10:22:58AM -0700, Joe Perches wrote: On Mon, 2014-06-02 at 10:00 -0700, Paul E. McKenney wrote: > A ksummit-discuss email thread looked at the difficulty recruiting > and retaining reviewers. [] > Paul Walmsley also noted the need for patch > submitters to know who the key reviewers are and suggested adding an > "R:" tag to the MAINTAINERS file to record this information on a > per-subsystem basis. I'm not sure of the value of this. Why not just mark the actual reviewers as maintainers? >>> >>> As discussed in the kernel summit discussion, being a regular patch >>> reviewer isn't the same thing as being *the* maintainer. >> >> I think it's not particularly important or valuable >> here to make that distinction. >> >> What real difference does it make? > > In the particular case of Josh, none, at least from my viewpoint. He of > course might or might not want to take on additional maintainership > responsibility at this particular point in time, in which case, I would > be more than happy to have him as a designated maintainer. For the record, I'd be happy to be listed as a co-maintainer for RCU. :) >>> >>> I would be happy to put you down as maintainer and Steven down as >>> official reviewer. ;-) >> >> I'd suggest adding Mathieu Desnoyers, Oleg Nesterov, and Lai Jiangshan >> as reviewers as well, with their consent. > > Mathieu, Oleg, Lai, any objections? > > I agree to add me. Thanks, Lai -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@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: [PATCH RFC 1/2] MAINTAINERS: Add "R:" designated-reviewers tag
On Wed, 2014-06-04 at 00:48 +0100, Ken Moffat wrote: > On Mon, Jun 02, 2014 at 05:12:05PM -0700, Joe Perches wrote: > > "Tested-by:" tags would be more helpful if the test > > cases that were used were somehow sent along with the > > signature. [] > Tested-by is usually used for a fix of some problem, often a > regression. A good commit message will explain the problem. It seems about half of the commits with tested-by are for regressions. The latest commit message with your tested-by is great. commit 18ee37a485653aa635cfab9a3710e9bcf5fbca01 Author: Daniel Vetter Date: Fri May 30 16:41:23 2014 +0200 drm/radeon: Resume fbcon last [detailed explanation elided] This commit log with your tested-by seems a bit mysterious though: commit 74ad54f249de39bc040cce7237b1b854a9c6f0ad Author: Christian König Date: Tue May 13 12:50:54 2014 +0200 drm/radeon: fix typo in finding PLL params Otherwise the limit is raised to high. As the commit that introduces this error is: commit 3b333c55485fef0089ae7398906599d000df195e Author: Christian König Date: Thu Apr 24 18:39:59 2014 +0200 drm/radeon: avoid high jitter with small frac divs This is the entirety of the commit log. The calculation that was done here: max(fb_div_min, (9 - (fb_div % 10)) * 20 + 60) Doesn't give any indication why 50 is better than 60. > This is _often_ not like userspace programs where you can write a > testsuite to exercise the corner cases. Kernel problems can be > tied up with intricate details of the hardware, or equally they > might happen only for certain usage, and for those it might not be > at all obvious what is "special" about the affected usage. Shrug. Mostly where that wonderful test robot that Fengguang Wu put together can aid in finding regressions, it'd be nice if the tested-by: tests done could be added somehow. And not in the commit log itself. It's certainly not possible for test cases to be mandatory. cheers, Joe -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@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: [PATCH RFC 1/2] MAINTAINERS: Add "R:" designated-reviewers tag
On Wed, 4 Jun 2014 00:48:16 +0100 Ken Moffat wrote: > This is _often_ not like userspace programs where you can write a > testsuite to exercise the corner cases. Kernel problems can be > tied up with intricate details of the hardware, or equally they > might happen only for certain usage, and for those it might not be > at all obvious what is "special" about the affected usage. Very true. Most of the Tested-by: tags I give is for one of my older boxes that triggers bugs that none of my other boxes do. Can't really add a test case for that. If only I could insert my test box into the git repository :-/ -- Steve -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@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: [PATCH RFC 1/2] MAINTAINERS: Add "R:" designated-reviewers tag
On Mon, Jun 02, 2014 at 05:12:05PM -0700, Joe Perches wrote: > > "Tested-by:" tags would be more helpful if the test > cases that were used were somehow sent along with the > signature. > To me, that seems either a perverse, or else a bureaucratic, interpretation of what should go where. Tested-by is usually used for a fix of some problem, often a regression. A good commit message will explain the problem. I have recently offered this tag in two cases - in the first case it did not boot without the fix, in the second it did not wake up from suspend. In each case, only one of my boxes was affected. Do you think I should have insisted that some of my lspci -V information was appended to the commit (they both affected the radeon code) if my tag was going to be added ? This is _often_ not like userspace programs where you can write a testsuite to exercise the corner cases. Kernel problems can be tied up with intricate details of the hardware, or equally they might happen only for certain usage, and for those it might not be at all obvious what is "special" about the affected usage. ĸen -- Nanny Ogg usually went to bed early. After all, she was an old lady. Sometimes she went to bed as early as 6 a.m. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@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: [PATCH RFC 1/2] MAINTAINERS: Add "R:" designated-reviewers tag
On Tue, Jun 03, 2014 at 05:46:31PM -0400, Steven Rostedt wrote: > On Tue, 3 Jun 2014 16:52:53 -0400 > Theodore Ts'o wrote: > > > > The important thing to note here is that we do not have consensus > > across all subsystems what Reviewed-by: means, and I think that's OK. > > The Reviewed-by: is mostly of interest to the maintainer before the > > patch is submitted to mainline. The value of keeping it in the git > > commit logs after the fact seems to be a bit variable, although if > > there are companies blindly using it as a performance metric and this > > is driving down the quality of reviews, perhaps one could even argue > > that including these tags in the git commit logs is actually adding > > negative value. But I don't really care about that issue, because > > like most maintainers, I know the reviewers by reputation, and whether > > someone actually says "you can add my Reviewed-by" is actually not so > > important as their comments on the patch, one way or another. > > I prefer the Reviewed-by tags in the git commit. If something is > screwed up in a commit, I can blame the reviewers just as much as I can > blame the author :-) I like it there for the same reason: if there turns out to be an issue in a commit I reviewed, I'd like to get CCed on the resulting discussion and fix, so that I can take that into account in future reviews. - Josh Triplett -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@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: [PATCH RFC 1/2] MAINTAINERS: Add "R:" designated-reviewers tag
On Tue, 3 Jun 2014 16:52:53 -0400 Theodore Ts'o wrote: > The important thing to note here is that we do not have consensus > across all subsystems what Reviewed-by: means, and I think that's OK. > The Reviewed-by: is mostly of interest to the maintainer before the > patch is submitted to mainline. The value of keeping it in the git > commit logs after the fact seems to be a bit variable, although if > there are companies blindly using it as a performance metric and this > is driving down the quality of reviews, perhaps one could even argue > that including these tags in the git commit logs is actually adding > negative value. But I don't really care about that issue, because > like most maintainers, I know the reviewers by reputation, and whether > someone actually says "you can add my Reviewed-by" is actually not so > important as their comments on the patch, one way or another. I prefer the Reviewed-by tags in the git commit. If something is screwed up in a commit, I can blame the reviewers just as much as I can blame the author :-) -- Steve -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@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: [PATCH RFC 1/2] MAINTAINERS: Add "R:" designated-reviewers tag
On Tue, Jun 03, 2014 at 01:43:47PM -0400, Steven Rostedt wrote: > > I know from experience that a "quick" 15 minute run on xfstests on a > > ramdisk will catch 99% of typical problems a filesystem patch might > > introduce. Code coverage reporting (done recently by RH QE > > engineers) tells me that this covers between 50-70% of the > > filesystem, VFS and MM subsystems (numbers vary with fs being > > tested), and so that's a pretty good, fast smoke test that I can run > > on any patch or patchset that is sent for review. > > Testing is fine, but I think you are undervaluing true review. Seems to > me, all you ask for others to do is to run their test suite on the code > to give a reviewed-by. I ask for people to look at the logic and spend > a lot more time on seeing if something strange is there. I found people > find things that tests usually don't. That's because before I post code > for review, I usually run it under a lot of tests myself that weeds out > most of those bugs. I'm not sure why you guys are arguing so much about this ditinction between testing versus review as if it were an either/or sort of situation. Both are important; there are problems that will be caught by the review that won't get caught using smoke tests, and often smoke tests (assuming you have a good collection of tests for a particular subsystem, which is not something which is a given for all subsystems) can find problems that a desk check of the code can miss. As far as who should run the tests, the way we do things in the ext4 world is that ideally the developer who is submitting the patch as well as the maintainer should be running the tests, and I don't worry so much about whether the reviewer is running the tests. If I find problems in my testing, I'll often point out this fact out to the developer, and to try to gently push them to do tests before pushing code out for review. The fact that I can point them at kvm-xfstests which is a turnkey smoke test system which requires nothing running a script and waiting 30 minutes (or 16 hours or so for a full test run with the full set of configurations, which I will ask developers who are making substantive changes to do instead of just the quick smoke test). The way I see it, if the developer and the maintainer is running tests, it's not so clear to me that making the reviewer run the tests as well adds enough value that it's worth requiring it. The important thing to note here is that we do not have consensus across all subsystems what Reviewed-by: means, and I think that's OK. The Reviewed-by: is mostly of interest to the maintainer before the patch is submitted to mainline. The value of keeping it in the git commit logs after the fact seems to be a bit variable, although if there are companies blindly using it as a performance metric and this is driving down the quality of reviews, perhaps one could even argue that including these tags in the git commit logs is actually adding negative value. But I don't really care about that issue, because like most maintainers, I know the reviewers by reputation, and whether someone actually says "you can add my Reviewed-by" is actually not so important as their comments on the patch, one way or another. Regards, - Ted -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@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: [PATCH RFC 1/2] MAINTAINERS: Add "R:" designated-reviewers tag
On 06/03/2014 10:43 AM, Steven Rostedt wrote: > On Tue, 3 Jun 2014 17:16:54 +1000 > Dave Chinner wrote: > > But you can't say that is it both free of techical and known issues without both reading the code and testing it (Reviewed-by). >>> >>> I disagree. Testing only tests what you run. It's useless otherwise. >>> Most code I review, and find bugs for in that review, will not be >>> caught by tests unless you ran it on a 1024 CPU box for a week. >>> >>> I value looking hard at code much more than booting it and running some >>> insignificant micro test. >> >> Running "insignficant micro tests" is exactly that - insignificant - >> and it's not a robust code review if that is all that is done. >> Runing *regression tests*, OTOH > > I really think you are bending the definition of Reviewed-by. Perhaps > Regression-tested-by would be more appropriate? If the XFS community wants to have a stricter or stronger meaning for Reviewed-by:, I think that's OK, but for the kernel in general, it just means what SubmittingPatches says, and that does not imply Tested-by:. >> >> I know from experience that a "quick" 15 minute run on xfstests on a >> ramdisk will catch 99% of typical problems a filesystem patch might >> introduce. Code coverage reporting (done recently by RH QE >> engineers) tells me that this covers between 50-70% of the >> filesystem, VFS and MM subsystems (numbers vary with fs being >> tested), and so that's a pretty good, fast smoke test that I can run >> on any patch or patchset that is sent for review. > > Testing is fine, but I think you are undervaluing true review. Seems to > me, all you ask for others to do is to run their test suite on the code > to give a reviewed-by. I ask for people to look at the logic and spend > a lot more time on seeing if something strange is there. I found people > find things that tests usually don't. That's because before I post code > for review, I usually run it under a lot of tests myself that weeds out > most of those bugs. Ack. Review is lots of cogitation. >> Any significant patch set requires longer to read and digest than a >> full xfstests run across all my test machines (about 80 minutes for >> a single mkfs/mount option configuration). So by the time I've >> actually read the code and commented on it, it's been through a full >> test cycle and it's pretty clear if there are problems or not.. >> >> And so when I say "reviewed-by" I'm pretty certain that there aren't >> any known issues. Sure, it's not going to catch the "ran it on a >> 1024 CPU box for a week" type of bug, but that's the repsonsibility >> of the bug reporter to give you a "tested-by" for that specific >> problem. > > But a good review *can* catch a bug that would trigger on a 1024 CPU > box! Really, I've had people point things out that I said, oh! but that > would be a really hard race to get to. And then go and fix it. > Sometimes, people will find things that have been broken for years in a > review of a patch that touches code around the broken part. > > >> >> And really, that points out *exactly* the how "reviewed-by" is a far >> more onerous than "tested-by". Tested-by only means the patch fixes >> the *specific problem* that was reported. Reviewed-by means that, >> as far as the reviewer can determine, it doesn't cause regressions >> or other problems. It may or may not imply "tested-by" depending on >> the nature of the bug being fixed, but it certainly implies "no >> obvious regressions". They are two very different meanings, and >> reviewed-by has a much, much wider scope than "tested-by". > > I don't see reviewed-by as a wider scope than tested-by, I seem them > under two completely different scopes. > > To me, a reviewed-by is someone spent time agonizing over every single > change, and how that change affects the code. I remember spending a > full week on a couple of patches for RCU that Paul submitted a while > ago. I may have run the code, but there's really no test I have that > would trigger the changes. I went back and forth with Paul and we found > a few minor issues and when it was done, I gave my Reviewed-by for it. > I was exhausted, but I learned a lot. I really did understand how that > code worked. Unfortunately, I forgot everything I learned since then ;-) > > >> And, yes, this is the definition we've been using for "reviewed-by" for XFS code since, well, years before the "reviewed-by" tag even existed... >>> >>> Fine, just like all else. That is up to the maintainer to decide. You >>> may require people to run and test it as their review, but I require >>> that people understand the code I write and look for those flaws that >>> 99% of tests wont catch. >>> >>> I run lots of specific tests on the code I write, I don't expect those >>> that review my code to do the same. In fact that's never what I even >>> ask for when I ask someone to review my code. Note, I do ask for >>> testers when I want people to test it, but
Re: [PATCH RFC 1/2] MAINTAINERS: Add "R:" designated-reviewers tag
On Tue, 3 Jun 2014 17:16:54 +1000 Dave Chinner wrote: > If you take it to an extremes. Think about what you can test in 15 > minutes. Or for larger patchsets, how long it takes you to read the > patchset? Yeah, what about that? > > IMO, every reviewer has their own developement environment and they > should be at least testing that the change they are reviewing > doesn't cause problems in that environment, just like they do for > their own code before they post it for review. Let me ask you this. In the scientific community, when someone posts a research project and asks their peers to review their work. Are all those reviewers required to test out that paper? Or are they to review it, check the math, look for cases that are missed, see common errors, and other checks? I'm sure some reviewers may do various tests, but others will just check the logic. I'm having a very hard time seeing where Reviewed-by means tested-by. I see those as two completely different tags. > > Code being reviewed should pass the same testing bar that the > developer uses for code they write and send for review. A maintainer > quickly learns whose test environments are up to scratch or not. :/ This very much limits your review base. Maybe you're this paranoid because for filesystems, if something goes wrong, people's data is gone. If something goes wrong with tracing or even the scheduler, people may get pissed but seldom do they lose a lot of work. > > > > But you can't say that is it both free of techical and known > > > issues without both reading the code and testing it (Reviewed-by). > > > > I disagree. Testing only tests what you run. It's useless otherwise. > > Most code I review, and find bugs for in that review, will not be > > caught by tests unless you ran it on a 1024 CPU box for a week. > > > > I value looking hard at code much more than booting it and running some > > insignificant micro test. > > Running "insignficant micro tests" is exactly that - insignificant - > and it's not a robust code review if that is all that is done. > Runing *regression tests*, OTOH I really think you are bending the definition of Reviewed-by. Perhaps Regression-tested-by would be more appropriate? > > I know from experience that a "quick" 15 minute run on xfstests on a > ramdisk will catch 99% of typical problems a filesystem patch might > introduce. Code coverage reporting (done recently by RH QE > engineers) tells me that this covers between 50-70% of the > filesystem, VFS and MM subsystems (numbers vary with fs being > tested), and so that's a pretty good, fast smoke test that I can run > on any patch or patchset that is sent for review. Testing is fine, but I think you are undervaluing true review. Seems to me, all you ask for others to do is to run their test suite on the code to give a reviewed-by. I ask for people to look at the logic and spend a lot more time on seeing if something strange is there. I found people find things that tests usually don't. That's because before I post code for review, I usually run it under a lot of tests myself that weeds out most of those bugs. > > Any significant patch set requires longer to read and digest than a > full xfstests run across all my test machines (about 80 minutes for > a single mkfs/mount option configuration). So by the time I've > actually read the code and commented on it, it's been through a full > test cycle and it's pretty clear if there are problems or not.. > > And so when I say "reviewed-by" I'm pretty certain that there aren't > any known issues. Sure, it's not going to catch the "ran it on a > 1024 CPU box for a week" type of bug, but that's the repsonsibility > of the bug reporter to give you a "tested-by" for that specific > problem. But a good review *can* catch a bug that would trigger on a 1024 CPU box! Really, I've had people point things out that I said, oh! but that would be a really hard race to get to. And then go and fix it. Sometimes, people will find things that have been broken for years in a review of a patch that touches code around the broken part. > > And really, that points out *exactly* the how "reviewed-by" is a far > more onerous than "tested-by". Tested-by only means the patch fixes > the *specific problem* that was reported. Reviewed-by means that, > as far as the reviewer can determine, it doesn't cause regressions > or other problems. It may or may not imply "tested-by" depending on > the nature of the bug being fixed, but it certainly implies "no > obvious regressions". They are two very different meanings, and > reviewed-by has a much, much wider scope than "tested-by". I don't see reviewed-by as a wider scope than tested-by, I seem them under two completely different scopes. To me, a reviewed-by is someone spent time agonizing over every single change, and how that change affects the code. I remember spending a full week on a couple of patches for RCU that Paul submitted a while ago. I may have run the code,
Re: [PATCH RFC 1/2] MAINTAINERS: Add "R:" designated-reviewers tag
On Tue, Jun 03, 2014 at 12:16:42PM -0400, Steven Rostedt wrote: > On Tue, 3 Jun 2014 08:37:55 -0700 > "Paul E. McKenney" wrote: > > > Very good! I resent the series, replacing Dipankar with Josh as > > co-maintainer, and adding Steven (perhaps prematurely) and Mathieu > > as designated reviewers. > > I'm fine with being added. Although I seldom have time to review your > patches when you send out those 40 patch patch bombs! ;-) I could try sending them out in smaller batches. ;-) Thanx, Paul -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@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: [PATCH RFC 1/2] MAINTAINERS: Add "R:" designated-reviewers tag
On Tue, 3 Jun 2014 08:37:55 -0700 "Paul E. McKenney" wrote: > Very good! I resent the series, replacing Dipankar with Josh as > co-maintainer, and adding Steven (perhaps prematurely) and Mathieu > as designated reviewers. I'm fine with being added. Although I seldom have time to review your patches when you send out those 40 patch patch bombs! ;-) -- Steve -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@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: [PATCH RFC 1/2] MAINTAINERS: Add "R:" designated-reviewers tag
On Tue, Jun 03, 2014 at 01:24:23PM +, Mathieu Desnoyers wrote: > - Original Message - > > From: "Dave Chinner" > > To: "Steven Rostedt" > > Cc: j...@joshtriplett.org, "Joe Perches" , > > paul...@linux.vnet.ibm.com, linux-kernel@vger.kernel.org, > > mi...@kernel.org, la...@cn.fujitsu.com, dipan...@in.ibm.com, > > a...@linux-foundation.org, "mathieu desnoyers" > > , n...@us.ibm.com, t...@linutronix.de, > > pet...@infradead.org, dhowe...@redhat.com, > > eduma...@google.com, dvh...@linux.intel.com, fweis...@gmail.com, > > o...@redhat.com, s...@mit.edu > > Sent: Tuesday, June 3, 2014 3:16:54 AM > > Subject: Re: [PATCH RFC 1/2] MAINTAINERS: Add "R:" designated-reviewers tag > > > > On Mon, Jun 02, 2014 at 09:30:45PM -0400, Steven Rostedt wrote: > > > On Tue, 3 Jun 2014 11:11:25 +1000 > > > Dave Chinner wrote: > > > > > > > > > > You've ignored the (c).(2) "free of known issues" criteria there. > > > > You cannot say a patch is free of issues if you haven't applied, > > > > compiled and tested it. > > > > > > > > > We should not, for instance, prevent someone from providing a > > > > > Reviewed-by (as opposed to an Acked-by) for a driver whose hardware > > > > > few > > > > > people actually have. There's significant value in code review even > > > > > without the ability to test. > > > > > > > > I don't disagree with you that there's value in code review, but > > > > that's not the only part of what "reviewed-by" means. > > > > > > > > You can test that the code is free of known issues without reviewing > > > > it (i.e. tested-by). You can read the code and note that you can't > > > > see any technical issues without testing it (Acked-by). > > > > > > Unless you run every test imaginable on all existing hardware, you are > > > not stating that it is free of known issues. I say your logic is flawed > > > right there. > > > > If you take it to an extremes. Think about what you can test in 15 > > minutes. Or for larger patchsets, how long it takes you to read the > > patchset? > > > > IMO, every reviewer has their own developement environment and they > > should be at least testing that the change they are reviewing > > doesn't cause problems in that environment, just like they do for > > their own code before they post it for review. > > > > Code being reviewed should pass the same testing bar that the > > developer uses for code they write and send for review. A maintainer > > quickly learns whose test environments are up to scratch or not. :/ > > > > > > But you can't say that is it both free of techical and known > > > > issues without both reading the code and testing it (Reviewed-by). > > > > > > I disagree. Testing only tests what you run. It's useless otherwise. > > > Most code I review, and find bugs for in that review, will not be > > > caught by tests unless you ran it on a 1024 CPU box for a week. > > > > > > I value looking hard at code much more than booting it and running some > > > insignificant micro test. > > > > Running "insignficant micro tests" is exactly that - insignificant - > > and it's not a robust code review if that is all that is done. > > Runing *regression tests*, OTOH > > > > I know from experience that a "quick" 15 minute run on xfstests on a > > ramdisk will catch 99% of typical problems a filesystem patch might > > introduce. Code coverage reporting (done recently by RH QE > > engineers) tells me that this covers between 50-70% of the > > filesystem, VFS and MM subsystems (numbers vary with fs being > > tested), and so that's a pretty good, fast smoke test that I can run > > on any patch or patchset that is sent for review. > > > > Any significant patch set requires longer to read and digest than a > > full xfstests run across all my test machines (about 80 minutes for > > a single mkfs/mount option configuration). So by the time I've > > actually read the code and commented on it, it's been through a full > > test cycle and it's pretty clear if there are problems or not.. > > > > And so when I say "reviewed-by" I'm pretty certain that there aren't > > any known issues. Sure, it's not going to catch the "ran it on a > > 1024 CPU box for a week" type of bug, but that's the r
Re: [PATCH RFC 1/2] MAINTAINERS: Add "R:" designated-reviewers tag
On Mon, Jun 02, 2014 at 08:25:51PM +, Mathieu Desnoyers wrote: > > From: "Paul E. McKenney" > > On Mon, Jun 02, 2014 at 12:11:55PM -0700, j...@joshtriplett.org wrote: > > > On Mon, Jun 02, 2014 at 12:08:21PM -0700, Paul E. McKenney wrote: > > > > On Mon, Jun 02, 2014 at 11:56:35AM -0700, j...@joshtriplett.org wrote: > > > > > On Mon, Jun 02, 2014 at 11:16:58AM -0700, Paul E. McKenney wrote: > > > > > > On Mon, Jun 02, 2014 at 10:59:28AM -0700, Joe Perches wrote: > > > > > > > On Mon, 2014-06-02 at 10:48 -0700, j...@joshtriplett.org wrote: [ . . . ] > > > > > For the record, I'd be happy to be listed as a co-maintainer for RCU. > > > > > :) > > > > > > > > I would be happy to put you down as maintainer and Steven down as > > > > official reviewer. ;-) > > > > > > I'd suggest adding Mathieu Desnoyers, Oleg Nesterov, and Lai Jiangshan > > > as reviewers as well, with their consent. > > > > Mathieu, Oleg, Lai, any objections? > > No objection from me. I'm always glad to help out > reviewing RCU patches whenever I have some cycles > available. Very good! I resent the series, replacing Dipankar with Josh as co-maintainer, and adding Steven (perhaps prematurely) and Mathieu as designated reviewers. Thanx, Paul -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@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: [PATCH RFC 1/2] MAINTAINERS: Add "R:" designated-reviewers tag
- Original Message - > From: "Dave Chinner" > To: "Steven Rostedt" > Cc: j...@joshtriplett.org, "Joe Perches" , > paul...@linux.vnet.ibm.com, linux-kernel@vger.kernel.org, > mi...@kernel.org, la...@cn.fujitsu.com, dipan...@in.ibm.com, > a...@linux-foundation.org, "mathieu desnoyers" > , n...@us.ibm.com, t...@linutronix.de, > pet...@infradead.org, dhowe...@redhat.com, > eduma...@google.com, dvh...@linux.intel.com, fweis...@gmail.com, > o...@redhat.com, s...@mit.edu > Sent: Tuesday, June 3, 2014 3:16:54 AM > Subject: Re: [PATCH RFC 1/2] MAINTAINERS: Add "R:" designated-reviewers tag > > On Mon, Jun 02, 2014 at 09:30:45PM -0400, Steven Rostedt wrote: > > On Tue, 3 Jun 2014 11:11:25 +1000 > > Dave Chinner wrote: > > > > > > > You've ignored the (c).(2) "free of known issues" criteria there. > > > You cannot say a patch is free of issues if you haven't applied, > > > compiled and tested it. > > > > > > > We should not, for instance, prevent someone from providing a > > > > Reviewed-by (as opposed to an Acked-by) for a driver whose hardware few > > > > people actually have. There's significant value in code review even > > > > without the ability to test. > > > > > > I don't disagree with you that there's value in code review, but > > > that's not the only part of what "reviewed-by" means. > > > > > > You can test that the code is free of known issues without reviewing > > > it (i.e. tested-by). You can read the code and note that you can't > > > see any technical issues without testing it (Acked-by). > > > > Unless you run every test imaginable on all existing hardware, you are > > not stating that it is free of known issues. I say your logic is flawed > > right there. > > If you take it to an extremes. Think about what you can test in 15 > minutes. Or for larger patchsets, how long it takes you to read the > patchset? > > IMO, every reviewer has their own developement environment and they > should be at least testing that the change they are reviewing > doesn't cause problems in that environment, just like they do for > their own code before they post it for review. > > Code being reviewed should pass the same testing bar that the > developer uses for code they write and send for review. A maintainer > quickly learns whose test environments are up to scratch or not. :/ > > > > But you can't say that is it both free of techical and known > > > issues without both reading the code and testing it (Reviewed-by). > > > > I disagree. Testing only tests what you run. It's useless otherwise. > > Most code I review, and find bugs for in that review, will not be > > caught by tests unless you ran it on a 1024 CPU box for a week. > > > > I value looking hard at code much more than booting it and running some > > insignificant micro test. > > Running "insignficant micro tests" is exactly that - insignificant - > and it's not a robust code review if that is all that is done. > Runing *regression tests*, OTOH > > I know from experience that a "quick" 15 minute run on xfstests on a > ramdisk will catch 99% of typical problems a filesystem patch might > introduce. Code coverage reporting (done recently by RH QE > engineers) tells me that this covers between 50-70% of the > filesystem, VFS and MM subsystems (numbers vary with fs being > tested), and so that's a pretty good, fast smoke test that I can run > on any patch or patchset that is sent for review. > > Any significant patch set requires longer to read and digest than a > full xfstests run across all my test machines (about 80 minutes for > a single mkfs/mount option configuration). So by the time I've > actually read the code and commented on it, it's been through a full > test cycle and it's pretty clear if there are problems or not.. > > And so when I say "reviewed-by" I'm pretty certain that there aren't > any known issues. Sure, it's not going to catch the "ran it on a > 1024 CPU box for a week" type of bug, but that's the repsonsibility > of the bug reporter to give you a "tested-by" for that specific > problem. > > And really, that points out *exactly* the how "reviewed-by" is a far > more onerous than "tested-by". Tested-by only means the patch fixes > the *specific problem* that was reported. Reviewed-by means that, > as far as the reviewer can determine, it doesn't cause regressions > or other problems. It may or may not imply "tested-by" depending on &
Re: [PATCH RFC 1/2] MAINTAINERS: Add "R:" designated-reviewers tag
On Mon, Jun 02, 2014 at 09:30:45PM -0400, Steven Rostedt wrote: > On Tue, 3 Jun 2014 11:11:25 +1000 > Dave Chinner wrote: > > > > You've ignored the (c).(2) "free of known issues" criteria there. > > You cannot say a patch is free of issues if you haven't applied, > > compiled and tested it. > > > > > We should not, for instance, prevent someone from providing a > > > Reviewed-by (as opposed to an Acked-by) for a driver whose hardware few > > > people actually have. There's significant value in code review even > > > without the ability to test. > > > > I don't disagree with you that there's value in code review, but > > that's not the only part of what "reviewed-by" means. > > > > You can test that the code is free of known issues without reviewing > > it (i.e. tested-by). You can read the code and note that you can't > > see any technical issues without testing it (Acked-by). > > Unless you run every test imaginable on all existing hardware, you are > not stating that it is free of known issues. I say your logic is flawed > right there. If you take it to an extremes. Think about what you can test in 15 minutes. Or for larger patchsets, how long it takes you to read the patchset? IMO, every reviewer has their own developement environment and they should be at least testing that the change they are reviewing doesn't cause problems in that environment, just like they do for their own code before they post it for review. Code being reviewed should pass the same testing bar that the developer uses for code they write and send for review. A maintainer quickly learns whose test environments are up to scratch or not. :/ > > But you can't say that is it both free of techical and known > > issues without both reading the code and testing it (Reviewed-by). > > I disagree. Testing only tests what you run. It's useless otherwise. > Most code I review, and find bugs for in that review, will not be > caught by tests unless you ran it on a 1024 CPU box for a week. > > I value looking hard at code much more than booting it and running some > insignificant micro test. Running "insignficant micro tests" is exactly that - insignificant - and it's not a robust code review if that is all that is done. Runing *regression tests*, OTOH I know from experience that a "quick" 15 minute run on xfstests on a ramdisk will catch 99% of typical problems a filesystem patch might introduce. Code coverage reporting (done recently by RH QE engineers) tells me that this covers between 50-70% of the filesystem, VFS and MM subsystems (numbers vary with fs being tested), and so that's a pretty good, fast smoke test that I can run on any patch or patchset that is sent for review. Any significant patch set requires longer to read and digest than a full xfstests run across all my test machines (about 80 minutes for a single mkfs/mount option configuration). So by the time I've actually read the code and commented on it, it's been through a full test cycle and it's pretty clear if there are problems or not.. And so when I say "reviewed-by" I'm pretty certain that there aren't any known issues. Sure, it's not going to catch the "ran it on a 1024 CPU box for a week" type of bug, but that's the repsonsibility of the bug reporter to give you a "tested-by" for that specific problem. And really, that points out *exactly* the how "reviewed-by" is a far more onerous than "tested-by". Tested-by only means the patch fixes the *specific problem* that was reported. Reviewed-by means that, as far as the reviewer can determine, it doesn't cause regressions or other problems. It may or may not imply "tested-by" depending on the nature of the bug being fixed, but it certainly implies "no obvious regressions". They are two very different meanings, and reviewed-by has a much, much wider scope than "tested-by". > > And, yes, this is the definition we've been using for "reviewed-by" > > for XFS code since, well, years before the "reviewed-by" tag even > > existed... > > Fine, just like all else. That is up to the maintainer to decide. You > may require people to run and test it as their review, but I require > that people understand the code I write and look for those flaws that > 99% of tests wont catch. > > I run lots of specific tests on the code I write, I don't expect those > that review my code to do the same. In fact that's never what I even > ask for when I ask someone to review my code. Note, I do ask for > testers when I want people to test it, but those are not the same > people that review my code. Very few subsystems have dedicated testers and hence rely on the test environments that the subsystem developers use every day to test their own code. IOWs, in most cases "tester" and the "reviewer" roles are performed by the same people. > I find the reviewers of my code to be the worse testers. That's because > those that I ask to review my code know what it's suppose to do, and > those are the people that are not going to
Re: [PATCH RFC 1/2] MAINTAINERS: Add R: designated-reviewers tag
On Mon, Jun 02, 2014 at 09:30:45PM -0400, Steven Rostedt wrote: On Tue, 3 Jun 2014 11:11:25 +1000 Dave Chinner da...@fromorbit.com wrote: You've ignored the (c).(2) free of known issues criteria there. You cannot say a patch is free of issues if you haven't applied, compiled and tested it. We should not, for instance, prevent someone from providing a Reviewed-by (as opposed to an Acked-by) for a driver whose hardware few people actually have. There's significant value in code review even without the ability to test. I don't disagree with you that there's value in code review, but that's not the only part of what reviewed-by means. You can test that the code is free of known issues without reviewing it (i.e. tested-by). You can read the code and note that you can't see any technical issues without testing it (Acked-by). Unless you run every test imaginable on all existing hardware, you are not stating that it is free of known issues. I say your logic is flawed right there. If you take it to an extremes. Think about what you can test in 15 minutes. Or for larger patchsets, how long it takes you to read the patchset? IMO, every reviewer has their own developement environment and they should be at least testing that the change they are reviewing doesn't cause problems in that environment, just like they do for their own code before they post it for review. Code being reviewed should pass the same testing bar that the developer uses for code they write and send for review. A maintainer quickly learns whose test environments are up to scratch or not. :/ But you can't say that is it both free of techical and known issues without both reading the code and testing it (Reviewed-by). I disagree. Testing only tests what you run. It's useless otherwise. Most code I review, and find bugs for in that review, will not be caught by tests unless you ran it on a 1024 CPU box for a week. I value looking hard at code much more than booting it and running some insignificant micro test. Running insignficant micro tests is exactly that - insignificant - and it's not a robust code review if that is all that is done. Runing *regression tests*, OTOH I know from experience that a quick 15 minute run on xfstests on a ramdisk will catch 99% of typical problems a filesystem patch might introduce. Code coverage reporting (done recently by RH QE engineers) tells me that this covers between 50-70% of the filesystem, VFS and MM subsystems (numbers vary with fs being tested), and so that's a pretty good, fast smoke test that I can run on any patch or patchset that is sent for review. Any significant patch set requires longer to read and digest than a full xfstests run across all my test machines (about 80 minutes for a single mkfs/mount option configuration). So by the time I've actually read the code and commented on it, it's been through a full test cycle and it's pretty clear if there are problems or not.. And so when I say reviewed-by I'm pretty certain that there aren't any known issues. Sure, it's not going to catch the ran it on a 1024 CPU box for a week type of bug, but that's the repsonsibility of the bug reporter to give you a tested-by for that specific problem. And really, that points out *exactly* the how reviewed-by is a far more onerous than tested-by. Tested-by only means the patch fixes the *specific problem* that was reported. Reviewed-by means that, as far as the reviewer can determine, it doesn't cause regressions or other problems. It may or may not imply tested-by depending on the nature of the bug being fixed, but it certainly implies no obvious regressions. They are two very different meanings, and reviewed-by has a much, much wider scope than tested-by. And, yes, this is the definition we've been using for reviewed-by for XFS code since, well, years before the reviewed-by tag even existed... Fine, just like all else. That is up to the maintainer to decide. You may require people to run and test it as their review, but I require that people understand the code I write and look for those flaws that 99% of tests wont catch. I run lots of specific tests on the code I write, I don't expect those that review my code to do the same. In fact that's never what I even ask for when I ask someone to review my code. Note, I do ask for testers when I want people to test it, but those are not the same people that review my code. Very few subsystems have dedicated testers and hence rely on the test environments that the subsystem developers use every day to test their own code. IOWs, in most cases tester and the reviewer roles are performed by the same people. I find the reviewers of my code to be the worse testers. That's because those that I ask to review my code know what it's suppose to do, and those are the people that are not going to stumble over bugs. It's the people that have no idea how your code works that will trigger
Re: [PATCH RFC 1/2] MAINTAINERS: Add R: designated-reviewers tag
- Original Message - From: Dave Chinner da...@fromorbit.com To: Steven Rostedt rost...@goodmis.org Cc: j...@joshtriplett.org, Joe Perches j...@perches.com, paul...@linux.vnet.ibm.com, linux-kernel@vger.kernel.org, mi...@kernel.org, la...@cn.fujitsu.com, dipan...@in.ibm.com, a...@linux-foundation.org, mathieu desnoyers mathieu.desnoy...@efficios.com, n...@us.ibm.com, t...@linutronix.de, pet...@infradead.org, dhowe...@redhat.com, eduma...@google.com, dvh...@linux.intel.com, fweis...@gmail.com, o...@redhat.com, s...@mit.edu Sent: Tuesday, June 3, 2014 3:16:54 AM Subject: Re: [PATCH RFC 1/2] MAINTAINERS: Add R: designated-reviewers tag On Mon, Jun 02, 2014 at 09:30:45PM -0400, Steven Rostedt wrote: On Tue, 3 Jun 2014 11:11:25 +1000 Dave Chinner da...@fromorbit.com wrote: You've ignored the (c).(2) free of known issues criteria there. You cannot say a patch is free of issues if you haven't applied, compiled and tested it. We should not, for instance, prevent someone from providing a Reviewed-by (as opposed to an Acked-by) for a driver whose hardware few people actually have. There's significant value in code review even without the ability to test. I don't disagree with you that there's value in code review, but that's not the only part of what reviewed-by means. You can test that the code is free of known issues without reviewing it (i.e. tested-by). You can read the code and note that you can't see any technical issues without testing it (Acked-by). Unless you run every test imaginable on all existing hardware, you are not stating that it is free of known issues. I say your logic is flawed right there. If you take it to an extremes. Think about what you can test in 15 minutes. Or for larger patchsets, how long it takes you to read the patchset? IMO, every reviewer has their own developement environment and they should be at least testing that the change they are reviewing doesn't cause problems in that environment, just like they do for their own code before they post it for review. Code being reviewed should pass the same testing bar that the developer uses for code they write and send for review. A maintainer quickly learns whose test environments are up to scratch or not. :/ But you can't say that is it both free of techical and known issues without both reading the code and testing it (Reviewed-by). I disagree. Testing only tests what you run. It's useless otherwise. Most code I review, and find bugs for in that review, will not be caught by tests unless you ran it on a 1024 CPU box for a week. I value looking hard at code much more than booting it and running some insignificant micro test. Running insignficant micro tests is exactly that - insignificant - and it's not a robust code review if that is all that is done. Runing *regression tests*, OTOH I know from experience that a quick 15 minute run on xfstests on a ramdisk will catch 99% of typical problems a filesystem patch might introduce. Code coverage reporting (done recently by RH QE engineers) tells me that this covers between 50-70% of the filesystem, VFS and MM subsystems (numbers vary with fs being tested), and so that's a pretty good, fast smoke test that I can run on any patch or patchset that is sent for review. Any significant patch set requires longer to read and digest than a full xfstests run across all my test machines (about 80 minutes for a single mkfs/mount option configuration). So by the time I've actually read the code and commented on it, it's been through a full test cycle and it's pretty clear if there are problems or not.. And so when I say reviewed-by I'm pretty certain that there aren't any known issues. Sure, it's not going to catch the ran it on a 1024 CPU box for a week type of bug, but that's the repsonsibility of the bug reporter to give you a tested-by for that specific problem. And really, that points out *exactly* the how reviewed-by is a far more onerous than tested-by. Tested-by only means the patch fixes the *specific problem* that was reported. Reviewed-by means that, as far as the reviewer can determine, it doesn't cause regressions or other problems. It may or may not imply tested-by depending on the nature of the bug being fixed, but it certainly implies no obvious regressions. They are two very different meanings, and reviewed-by has a much, much wider scope than tested-by. And, yes, this is the definition we've been using for reviewed-by for XFS code since, well, years before the reviewed-by tag even existed... Fine, just like all else. That is up to the maintainer to decide. You may require people to run and test it as their review, but I require that people understand the code I write and look for those flaws that 99% of tests wont catch. I run lots of specific tests on the code I write, I don't
Re: [PATCH RFC 1/2] MAINTAINERS: Add R: designated-reviewers tag
On Mon, Jun 02, 2014 at 08:25:51PM +, Mathieu Desnoyers wrote: From: Paul E. McKenney paul...@linux.vnet.ibm.com On Mon, Jun 02, 2014 at 12:11:55PM -0700, j...@joshtriplett.org wrote: On Mon, Jun 02, 2014 at 12:08:21PM -0700, Paul E. McKenney wrote: On Mon, Jun 02, 2014 at 11:56:35AM -0700, j...@joshtriplett.org wrote: On Mon, Jun 02, 2014 at 11:16:58AM -0700, Paul E. McKenney wrote: On Mon, Jun 02, 2014 at 10:59:28AM -0700, Joe Perches wrote: On Mon, 2014-06-02 at 10:48 -0700, j...@joshtriplett.org wrote: [ . . . ] For the record, I'd be happy to be listed as a co-maintainer for RCU. :) I would be happy to put you down as maintainer and Steven down as official reviewer. ;-) I'd suggest adding Mathieu Desnoyers, Oleg Nesterov, and Lai Jiangshan as reviewers as well, with their consent. Mathieu, Oleg, Lai, any objections? No objection from me. I'm always glad to help out reviewing RCU patches whenever I have some cycles available. Very good! I resent the series, replacing Dipankar with Josh as co-maintainer, and adding Steven (perhaps prematurely) and Mathieu as designated reviewers. Thanx, Paul -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@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: [PATCH RFC 1/2] MAINTAINERS: Add R: designated-reviewers tag
On Tue, Jun 03, 2014 at 01:24:23PM +, Mathieu Desnoyers wrote: - Original Message - From: Dave Chinner da...@fromorbit.com To: Steven Rostedt rost...@goodmis.org Cc: j...@joshtriplett.org, Joe Perches j...@perches.com, paul...@linux.vnet.ibm.com, linux-kernel@vger.kernel.org, mi...@kernel.org, la...@cn.fujitsu.com, dipan...@in.ibm.com, a...@linux-foundation.org, mathieu desnoyers mathieu.desnoy...@efficios.com, n...@us.ibm.com, t...@linutronix.de, pet...@infradead.org, dhowe...@redhat.com, eduma...@google.com, dvh...@linux.intel.com, fweis...@gmail.com, o...@redhat.com, s...@mit.edu Sent: Tuesday, June 3, 2014 3:16:54 AM Subject: Re: [PATCH RFC 1/2] MAINTAINERS: Add R: designated-reviewers tag On Mon, Jun 02, 2014 at 09:30:45PM -0400, Steven Rostedt wrote: On Tue, 3 Jun 2014 11:11:25 +1000 Dave Chinner da...@fromorbit.com wrote: You've ignored the (c).(2) free of known issues criteria there. You cannot say a patch is free of issues if you haven't applied, compiled and tested it. We should not, for instance, prevent someone from providing a Reviewed-by (as opposed to an Acked-by) for a driver whose hardware few people actually have. There's significant value in code review even without the ability to test. I don't disagree with you that there's value in code review, but that's not the only part of what reviewed-by means. You can test that the code is free of known issues without reviewing it (i.e. tested-by). You can read the code and note that you can't see any technical issues without testing it (Acked-by). Unless you run every test imaginable on all existing hardware, you are not stating that it is free of known issues. I say your logic is flawed right there. If you take it to an extremes. Think about what you can test in 15 minutes. Or for larger patchsets, how long it takes you to read the patchset? IMO, every reviewer has their own developement environment and they should be at least testing that the change they are reviewing doesn't cause problems in that environment, just like they do for their own code before they post it for review. Code being reviewed should pass the same testing bar that the developer uses for code they write and send for review. A maintainer quickly learns whose test environments are up to scratch or not. :/ But you can't say that is it both free of techical and known issues without both reading the code and testing it (Reviewed-by). I disagree. Testing only tests what you run. It's useless otherwise. Most code I review, and find bugs for in that review, will not be caught by tests unless you ran it on a 1024 CPU box for a week. I value looking hard at code much more than booting it and running some insignificant micro test. Running insignficant micro tests is exactly that - insignificant - and it's not a robust code review if that is all that is done. Runing *regression tests*, OTOH I know from experience that a quick 15 minute run on xfstests on a ramdisk will catch 99% of typical problems a filesystem patch might introduce. Code coverage reporting (done recently by RH QE engineers) tells me that this covers between 50-70% of the filesystem, VFS and MM subsystems (numbers vary with fs being tested), and so that's a pretty good, fast smoke test that I can run on any patch or patchset that is sent for review. Any significant patch set requires longer to read and digest than a full xfstests run across all my test machines (about 80 minutes for a single mkfs/mount option configuration). So by the time I've actually read the code and commented on it, it's been through a full test cycle and it's pretty clear if there are problems or not.. And so when I say reviewed-by I'm pretty certain that there aren't any known issues. Sure, it's not going to catch the ran it on a 1024 CPU box for a week type of bug, but that's the repsonsibility of the bug reporter to give you a tested-by for that specific problem. And really, that points out *exactly* the how reviewed-by is a far more onerous than tested-by. Tested-by only means the patch fixes the *specific problem* that was reported. Reviewed-by means that, as far as the reviewer can determine, it doesn't cause regressions or other problems. It may or may not imply tested-by depending on the nature of the bug being fixed, but it certainly implies no obvious regressions. They are two very different meanings, and reviewed-by has a much, much wider scope than tested-by. And, yes, this is the definition we've been using for reviewed-by for XFS code since, well, years before the reviewed-by tag even existed... Fine, just like all else. That is up to the maintainer to decide. You may require people to run and test it as their review, but I
Re: [PATCH RFC 1/2] MAINTAINERS: Add R: designated-reviewers tag
On Tue, 3 Jun 2014 08:37:55 -0700 Paul E. McKenney paul...@linux.vnet.ibm.com wrote: Very good! I resent the series, replacing Dipankar with Josh as co-maintainer, and adding Steven (perhaps prematurely) and Mathieu as designated reviewers. I'm fine with being added. Although I seldom have time to review your patches when you send out those 40 patch patch bombs! ;-) -- Steve -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@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: [PATCH RFC 1/2] MAINTAINERS: Add R: designated-reviewers tag
On Tue, Jun 03, 2014 at 12:16:42PM -0400, Steven Rostedt wrote: On Tue, 3 Jun 2014 08:37:55 -0700 Paul E. McKenney paul...@linux.vnet.ibm.com wrote: Very good! I resent the series, replacing Dipankar with Josh as co-maintainer, and adding Steven (perhaps prematurely) and Mathieu as designated reviewers. I'm fine with being added. Although I seldom have time to review your patches when you send out those 40 patch patch bombs! ;-) I could try sending them out in smaller batches. ;-) Thanx, Paul -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@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: [PATCH RFC 1/2] MAINTAINERS: Add R: designated-reviewers tag
On Tue, 3 Jun 2014 17:16:54 +1000 Dave Chinner da...@fromorbit.com wrote: If you take it to an extremes. Think about what you can test in 15 minutes. Or for larger patchsets, how long it takes you to read the patchset? Yeah, what about that? IMO, every reviewer has their own developement environment and they should be at least testing that the change they are reviewing doesn't cause problems in that environment, just like they do for their own code before they post it for review. Let me ask you this. In the scientific community, when someone posts a research project and asks their peers to review their work. Are all those reviewers required to test out that paper? Or are they to review it, check the math, look for cases that are missed, see common errors, and other checks? I'm sure some reviewers may do various tests, but others will just check the logic. I'm having a very hard time seeing where Reviewed-by means tested-by. I see those as two completely different tags. Code being reviewed should pass the same testing bar that the developer uses for code they write and send for review. A maintainer quickly learns whose test environments are up to scratch or not. :/ This very much limits your review base. Maybe you're this paranoid because for filesystems, if something goes wrong, people's data is gone. If something goes wrong with tracing or even the scheduler, people may get pissed but seldom do they lose a lot of work. But you can't say that is it both free of techical and known issues without both reading the code and testing it (Reviewed-by). I disagree. Testing only tests what you run. It's useless otherwise. Most code I review, and find bugs for in that review, will not be caught by tests unless you ran it on a 1024 CPU box for a week. I value looking hard at code much more than booting it and running some insignificant micro test. Running insignficant micro tests is exactly that - insignificant - and it's not a robust code review if that is all that is done. Runing *regression tests*, OTOH I really think you are bending the definition of Reviewed-by. Perhaps Regression-tested-by would be more appropriate? I know from experience that a quick 15 minute run on xfstests on a ramdisk will catch 99% of typical problems a filesystem patch might introduce. Code coverage reporting (done recently by RH QE engineers) tells me that this covers between 50-70% of the filesystem, VFS and MM subsystems (numbers vary with fs being tested), and so that's a pretty good, fast smoke test that I can run on any patch or patchset that is sent for review. Testing is fine, but I think you are undervaluing true review. Seems to me, all you ask for others to do is to run their test suite on the code to give a reviewed-by. I ask for people to look at the logic and spend a lot more time on seeing if something strange is there. I found people find things that tests usually don't. That's because before I post code for review, I usually run it under a lot of tests myself that weeds out most of those bugs. Any significant patch set requires longer to read and digest than a full xfstests run across all my test machines (about 80 minutes for a single mkfs/mount option configuration). So by the time I've actually read the code and commented on it, it's been through a full test cycle and it's pretty clear if there are problems or not.. And so when I say reviewed-by I'm pretty certain that there aren't any known issues. Sure, it's not going to catch the ran it on a 1024 CPU box for a week type of bug, but that's the repsonsibility of the bug reporter to give you a tested-by for that specific problem. But a good review *can* catch a bug that would trigger on a 1024 CPU box! Really, I've had people point things out that I said, oh! but that would be a really hard race to get to. And then go and fix it. Sometimes, people will find things that have been broken for years in a review of a patch that touches code around the broken part. And really, that points out *exactly* the how reviewed-by is a far more onerous than tested-by. Tested-by only means the patch fixes the *specific problem* that was reported. Reviewed-by means that, as far as the reviewer can determine, it doesn't cause regressions or other problems. It may or may not imply tested-by depending on the nature of the bug being fixed, but it certainly implies no obvious regressions. They are two very different meanings, and reviewed-by has a much, much wider scope than tested-by. I don't see reviewed-by as a wider scope than tested-by, I seem them under two completely different scopes. To me, a reviewed-by is someone spent time agonizing over every single change, and how that change affects the code. I remember spending a full week on a couple of patches for RCU that Paul submitted a while ago. I may have run the code, but there's really no test I have that would trigger the changes.
Re: [PATCH RFC 1/2] MAINTAINERS: Add R: designated-reviewers tag
On 06/03/2014 10:43 AM, Steven Rostedt wrote: On Tue, 3 Jun 2014 17:16:54 +1000 Dave Chinner da...@fromorbit.com wrote: But you can't say that is it both free of techical and known issues without both reading the code and testing it (Reviewed-by). I disagree. Testing only tests what you run. It's useless otherwise. Most code I review, and find bugs for in that review, will not be caught by tests unless you ran it on a 1024 CPU box for a week. I value looking hard at code much more than booting it and running some insignificant micro test. Running insignficant micro tests is exactly that - insignificant - and it's not a robust code review if that is all that is done. Runing *regression tests*, OTOH I really think you are bending the definition of Reviewed-by. Perhaps Regression-tested-by would be more appropriate? If the XFS community wants to have a stricter or stronger meaning for Reviewed-by:, I think that's OK, but for the kernel in general, it just means what SubmittingPatches says, and that does not imply Tested-by:. I know from experience that a quick 15 minute run on xfstests on a ramdisk will catch 99% of typical problems a filesystem patch might introduce. Code coverage reporting (done recently by RH QE engineers) tells me that this covers between 50-70% of the filesystem, VFS and MM subsystems (numbers vary with fs being tested), and so that's a pretty good, fast smoke test that I can run on any patch or patchset that is sent for review. Testing is fine, but I think you are undervaluing true review. Seems to me, all you ask for others to do is to run their test suite on the code to give a reviewed-by. I ask for people to look at the logic and spend a lot more time on seeing if something strange is there. I found people find things that tests usually don't. That's because before I post code for review, I usually run it under a lot of tests myself that weeds out most of those bugs. Ack. Review is lots of cogitation. Any significant patch set requires longer to read and digest than a full xfstests run across all my test machines (about 80 minutes for a single mkfs/mount option configuration). So by the time I've actually read the code and commented on it, it's been through a full test cycle and it's pretty clear if there are problems or not.. And so when I say reviewed-by I'm pretty certain that there aren't any known issues. Sure, it's not going to catch the ran it on a 1024 CPU box for a week type of bug, but that's the repsonsibility of the bug reporter to give you a tested-by for that specific problem. But a good review *can* catch a bug that would trigger on a 1024 CPU box! Really, I've had people point things out that I said, oh! but that would be a really hard race to get to. And then go and fix it. Sometimes, people will find things that have been broken for years in a review of a patch that touches code around the broken part. And really, that points out *exactly* the how reviewed-by is a far more onerous than tested-by. Tested-by only means the patch fixes the *specific problem* that was reported. Reviewed-by means that, as far as the reviewer can determine, it doesn't cause regressions or other problems. It may or may not imply tested-by depending on the nature of the bug being fixed, but it certainly implies no obvious regressions. They are two very different meanings, and reviewed-by has a much, much wider scope than tested-by. I don't see reviewed-by as a wider scope than tested-by, I seem them under two completely different scopes. To me, a reviewed-by is someone spent time agonizing over every single change, and how that change affects the code. I remember spending a full week on a couple of patches for RCU that Paul submitted a while ago. I may have run the code, but there's really no test I have that would trigger the changes. I went back and forth with Paul and we found a few minor issues and when it was done, I gave my Reviewed-by for it. I was exhausted, but I learned a lot. I really did understand how that code worked. Unfortunately, I forgot everything I learned since then ;-) And, yes, this is the definition we've been using for reviewed-by for XFS code since, well, years before the reviewed-by tag even existed... Fine, just like all else. That is up to the maintainer to decide. You may require people to run and test it as their review, but I require that people understand the code I write and look for those flaws that 99% of tests wont catch. I run lots of specific tests on the code I write, I don't expect those that review my code to do the same. In fact that's never what I even ask for when I ask someone to review my code. Note, I do ask for testers when I want people to test it, but those are not the same people that review my code. Very few subsystems have dedicated testers and hence rely on the test environments that the subsystem developers use every
Re: [PATCH RFC 1/2] MAINTAINERS: Add R: designated-reviewers tag
On Tue, Jun 03, 2014 at 01:43:47PM -0400, Steven Rostedt wrote: I know from experience that a quick 15 minute run on xfstests on a ramdisk will catch 99% of typical problems a filesystem patch might introduce. Code coverage reporting (done recently by RH QE engineers) tells me that this covers between 50-70% of the filesystem, VFS and MM subsystems (numbers vary with fs being tested), and so that's a pretty good, fast smoke test that I can run on any patch or patchset that is sent for review. Testing is fine, but I think you are undervaluing true review. Seems to me, all you ask for others to do is to run their test suite on the code to give a reviewed-by. I ask for people to look at the logic and spend a lot more time on seeing if something strange is there. I found people find things that tests usually don't. That's because before I post code for review, I usually run it under a lot of tests myself that weeds out most of those bugs. I'm not sure why you guys are arguing so much about this ditinction between testing versus review as if it were an either/or sort of situation. Both are important; there are problems that will be caught by the review that won't get caught using smoke tests, and often smoke tests (assuming you have a good collection of tests for a particular subsystem, which is not something which is a given for all subsystems) can find problems that a desk check of the code can miss. As far as who should run the tests, the way we do things in the ext4 world is that ideally the developer who is submitting the patch as well as the maintainer should be running the tests, and I don't worry so much about whether the reviewer is running the tests. If I find problems in my testing, I'll often point out this fact out to the developer, and to try to gently push them to do tests before pushing code out for review. The fact that I can point them at kvm-xfstests which is a turnkey smoke test system which requires nothing running a script and waiting 30 minutes (or 16 hours or so for a full test run with the full set of configurations, which I will ask developers who are making substantive changes to do instead of just the quick smoke test). The way I see it, if the developer and the maintainer is running tests, it's not so clear to me that making the reviewer run the tests as well adds enough value that it's worth requiring it. The important thing to note here is that we do not have consensus across all subsystems what Reviewed-by: means, and I think that's OK. The Reviewed-by: is mostly of interest to the maintainer before the patch is submitted to mainline. The value of keeping it in the git commit logs after the fact seems to be a bit variable, although if there are companies blindly using it as a performance metric and this is driving down the quality of reviews, perhaps one could even argue that including these tags in the git commit logs is actually adding negative value. But I don't really care about that issue, because like most maintainers, I know the reviewers by reputation, and whether someone actually says you can add my Reviewed-by is actually not so important as their comments on the patch, one way or another. Regards, - Ted -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@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: [PATCH RFC 1/2] MAINTAINERS: Add R: designated-reviewers tag
On Tue, 3 Jun 2014 16:52:53 -0400 Theodore Ts'o ty...@mit.edu wrote: The important thing to note here is that we do not have consensus across all subsystems what Reviewed-by: means, and I think that's OK. The Reviewed-by: is mostly of interest to the maintainer before the patch is submitted to mainline. The value of keeping it in the git commit logs after the fact seems to be a bit variable, although if there are companies blindly using it as a performance metric and this is driving down the quality of reviews, perhaps one could even argue that including these tags in the git commit logs is actually adding negative value. But I don't really care about that issue, because like most maintainers, I know the reviewers by reputation, and whether someone actually says you can add my Reviewed-by is actually not so important as their comments on the patch, one way or another. I prefer the Reviewed-by tags in the git commit. If something is screwed up in a commit, I can blame the reviewers just as much as I can blame the author :-) -- Steve -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@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: [PATCH RFC 1/2] MAINTAINERS: Add R: designated-reviewers tag
On Tue, Jun 03, 2014 at 05:46:31PM -0400, Steven Rostedt wrote: On Tue, 3 Jun 2014 16:52:53 -0400 Theodore Ts'o ty...@mit.edu wrote: The important thing to note here is that we do not have consensus across all subsystems what Reviewed-by: means, and I think that's OK. The Reviewed-by: is mostly of interest to the maintainer before the patch is submitted to mainline. The value of keeping it in the git commit logs after the fact seems to be a bit variable, although if there are companies blindly using it as a performance metric and this is driving down the quality of reviews, perhaps one could even argue that including these tags in the git commit logs is actually adding negative value. But I don't really care about that issue, because like most maintainers, I know the reviewers by reputation, and whether someone actually says you can add my Reviewed-by is actually not so important as their comments on the patch, one way or another. I prefer the Reviewed-by tags in the git commit. If something is screwed up in a commit, I can blame the reviewers just as much as I can blame the author :-) I like it there for the same reason: if there turns out to be an issue in a commit I reviewed, I'd like to get CCed on the resulting discussion and fix, so that I can take that into account in future reviews. - Josh Triplett -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@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: [PATCH RFC 1/2] MAINTAINERS: Add R: designated-reviewers tag
On Mon, Jun 02, 2014 at 05:12:05PM -0700, Joe Perches wrote: Tested-by: tags would be more helpful if the test cases that were used were somehow sent along with the signature. To me, that seems either a perverse, or else a bureaucratic, interpretation of what should go where. Tested-by is usually used for a fix of some problem, often a regression. A good commit message will explain the problem. I have recently offered this tag in two cases - in the first case it did not boot without the fix, in the second it did not wake up from suspend. In each case, only one of my boxes was affected. Do you think I should have insisted that some of my lspci -V information was appended to the commit (they both affected the radeon code) if my tag was going to be added ? This is _often_ not like userspace programs where you can write a testsuite to exercise the corner cases. Kernel problems can be tied up with intricate details of the hardware, or equally they might happen only for certain usage, and for those it might not be at all obvious what is special about the affected usage. ĸen -- Nanny Ogg usually went to bed early. After all, she was an old lady. Sometimes she went to bed as early as 6 a.m. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@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: [PATCH RFC 1/2] MAINTAINERS: Add R: designated-reviewers tag
On Wed, 4 Jun 2014 00:48:16 +0100 Ken Moffat zarniwh...@ntlworld.com wrote: This is _often_ not like userspace programs where you can write a testsuite to exercise the corner cases. Kernel problems can be tied up with intricate details of the hardware, or equally they might happen only for certain usage, and for those it might not be at all obvious what is special about the affected usage. Very true. Most of the Tested-by: tags I give is for one of my older boxes that triggers bugs that none of my other boxes do. Can't really add a test case for that. If only I could insert my test box into the git repository :-/ -- Steve -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@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: [PATCH RFC 1/2] MAINTAINERS: Add R: designated-reviewers tag
On Wed, 2014-06-04 at 00:48 +0100, Ken Moffat wrote: On Mon, Jun 02, 2014 at 05:12:05PM -0700, Joe Perches wrote: Tested-by: tags would be more helpful if the test cases that were used were somehow sent along with the signature. [] Tested-by is usually used for a fix of some problem, often a regression. A good commit message will explain the problem. It seems about half of the commits with tested-by are for regressions. The latest commit message with your tested-by is great. commit 18ee37a485653aa635cfab9a3710e9bcf5fbca01 Author: Daniel Vetter daniel.vet...@ffwll.ch Date: Fri May 30 16:41:23 2014 +0200 drm/radeon: Resume fbcon last [detailed explanation elided] This commit log with your tested-by seems a bit mysterious though: commit 74ad54f249de39bc040cce7237b1b854a9c6f0ad Author: Christian König christian.koe...@amd.com Date: Tue May 13 12:50:54 2014 +0200 drm/radeon: fix typo in finding PLL params Otherwise the limit is raised to high. As the commit that introduces this error is: commit 3b333c55485fef0089ae7398906599d000df195e Author: Christian König christian.koe...@amd.com Date: Thu Apr 24 18:39:59 2014 +0200 drm/radeon: avoid high jitter with small frac divs This is the entirety of the commit log. The calculation that was done here: max(fb_div_min, (9 - (fb_div % 10)) * 20 + 60) Doesn't give any indication why 50 is better than 60. This is _often_ not like userspace programs where you can write a testsuite to exercise the corner cases. Kernel problems can be tied up with intricate details of the hardware, or equally they might happen only for certain usage, and for those it might not be at all obvious what is special about the affected usage. Shrug. Mostly where that wonderful test robot that Fengguang Wu put together can aid in finding regressions, it'd be nice if the tested-by: tests done could be added somehow. And not in the commit log itself. It's certainly not possible for test cases to be mandatory. cheers, Joe -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@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: [PATCH RFC 1/2] MAINTAINERS: Add R: designated-reviewers tag
On 06/03/2014 03:27 AM, Paul E. McKenney wrote: On Mon, Jun 02, 2014 at 12:11:55PM -0700, j...@joshtriplett.org wrote: On Mon, Jun 02, 2014 at 12:08:21PM -0700, Paul E. McKenney wrote: On Mon, Jun 02, 2014 at 11:56:35AM -0700, j...@joshtriplett.org wrote: On Mon, Jun 02, 2014 at 11:16:58AM -0700, Paul E. McKenney wrote: On Mon, Jun 02, 2014 at 10:59:28AM -0700, Joe Perches wrote: On Mon, 2014-06-02 at 10:48 -0700, j...@joshtriplett.org wrote: On Mon, Jun 02, 2014 at 10:22:58AM -0700, Joe Perches wrote: On Mon, 2014-06-02 at 10:00 -0700, Paul E. McKenney wrote: A ksummit-discuss email thread looked at the difficulty recruiting and retaining reviewers. [] Paul Walmsley also noted the need for patch submitters to know who the key reviewers are and suggested adding an R: tag to the MAINTAINERS file to record this information on a per-subsystem basis. I'm not sure of the value of this. Why not just mark the actual reviewers as maintainers? As discussed in the kernel summit discussion, being a regular patch reviewer isn't the same thing as being *the* maintainer. I think it's not particularly important or valuable here to make that distinction. What real difference does it make? In the particular case of Josh, none, at least from my viewpoint. He of course might or might not want to take on additional maintainership responsibility at this particular point in time, in which case, I would be more than happy to have him as a designated maintainer. For the record, I'd be happy to be listed as a co-maintainer for RCU. :) I would be happy to put you down as maintainer and Steven down as official reviewer. ;-) I'd suggest adding Mathieu Desnoyers, Oleg Nesterov, and Lai Jiangshan as reviewers as well, with their consent. Mathieu, Oleg, Lai, any objections? I agree to add me. Thanks, Lai -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@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: [PATCH RFC 1/2] MAINTAINERS: Add "R:" designated-reviewers tag
On Tue, 3 Jun 2014 11:11:25 +1000 Dave Chinner wrote: > You've ignored the (c).(2) "free of known issues" criteria there. > You cannot say a patch is free of issues if you haven't applied, > compiled and tested it. > > > We should not, for instance, prevent someone from providing a > > Reviewed-by (as opposed to an Acked-by) for a driver whose hardware few > > people actually have. There's significant value in code review even > > without the ability to test. > > I don't disagree with you that there's value in code review, but > that's not the only part of what "reviewed-by" means. > > You can test that the code is free of known issues without reviewing > it (i.e. tested-by). You can read the code and note that you can't > see any technical issues without testing it (Acked-by). Unless you run every test imaginable on all existing hardware, you are not stating that it is free of known issues. I say your logic is flawed right there. I find that review finds more bugs than testing does. > > But you can't say that is it both free of techical and known > issues without both reading the code and testing it (Reviewed-by). I disagree. Testing only tests what you run. It's useless otherwise. Most code I review, and find bugs for in that review, will not be caught by tests unless you ran it on a 1024 CPU box for a week. I value looking hard at code much more than booting it and running some insignificant micro test. > > > > Anyone using Reviewed-by without having actually applied and tested > > > the patch is mis-using the tag - they should be using Acked-by: if > > > all they have done is read the code in their mail program > > > > Acked-by and Reviewed-by mean two different things (Reviewed-by being a > > superset of Acked-by), and the difference is not "I've applied and > > tested this"; that's Tested-by. > > Right, the difference is more than that - Reviewed-by is a > superset of both Acked-by and Tested-by. I disagree. > > And, yes, this is the definition we've been using for "reviewed-by" > for XFS code since, well, years before the "reviewed-by" tag even > existed... Fine, just like all else. That is up to the maintainer to decide. You may require people to run and test it as their review, but I require that people understand the code I write and look for those flaws that 99% of tests wont catch. I run lots of specific tests on the code I write, I don't expect those that review my code to do the same. In fact that's never what I even ask for when I ask someone to review my code. Note, I do ask for testers when I want people to test it, but those are not the same people that review my code. I find the reviewers of my code to be the worse testers. That's because those that I ask to review my code know what it's suppose to do, and those are the people that are not going to stumble over bugs. It's the people that have no idea how your code works that will trigger the most bugs in testing it. My best testers would be my worse reviewers. What do you require as a test anyway? I could boot your patches, but since I don't have an XFS filesystem, I doubt that would be much use for you. -- Steve -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@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: [PATCH RFC 1/2] MAINTAINERS: Add "R:" designated-reviewers tag
On Mon, Jun 02, 2014 at 04:59:15PM -0700, j...@joshtriplett.org wrote: > On Tue, Jun 03, 2014 at 09:19:49AM +1000, Dave Chinner wrote: > > On Mon, Jun 02, 2014 at 12:17:46PM -0700, Joe Perches wrote: > > > What it needs is testing, not reviewing. > > > > > > I tested it for all of 10 seconds. > > > > From Documentation/SubmittingPatches: > > > > " (c) While there may be things that could be improved with this > > submission, I believe that it is, at this time, (1) a > > worthwhile modification to the kernel, and (2) free of known > > issues which would argue against its inclusion. > > . > > > > 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." > > > > So, for someone to say they have reviewed the code and are able to > > say it is free of known issues and has no remaining technical > > issues, they would have had to apply, compile and test the patch, > > yes? > > > > i.e. Reviewed-by implies both Acked-by, Tested-by and that the code > > is technically sound. > > No, not at all. It implies Acked-by, and that the code is technically > sound (both at the micro-level and in overall architecture/approach), > but does not imply Tested-by; that's a separate tag for a reason. You've ignored the (c).(2) "free of known issues" criteria there. You cannot say a patch is free of issues if you haven't applied, compiled and tested it. > We should not, for instance, prevent someone from providing a > Reviewed-by (as opposed to an Acked-by) for a driver whose hardware few > people actually have. There's significant value in code review even > without the ability to test. I don't disagree with you that there's value in code review, but that's not the only part of what "reviewed-by" means. You can test that the code is free of known issues without reviewing it (i.e. tested-by). You can read the code and note that you can't see any technical issues without testing it (Acked-by). But you can't say that is it both free of techical and known issues without both reading the code and testing it (Reviewed-by). > > Anyone using Reviewed-by without having actually applied and tested > > the patch is mis-using the tag - they should be using Acked-by: if > > all they have done is read the code in their mail program > > Acked-by and Reviewed-by mean two different things (Reviewed-by being a > superset of Acked-by), and the difference is not "I've applied and > tested this"; that's Tested-by. Right, the difference is more than that - Reviewed-by is a superset of both Acked-by and Tested-by. And, yes, this is the definition we've been using for "reviewed-by" for XFS code since, well, years before the "reviewed-by" tag even existed... Cheers, Dave. -- Dave Chinner da...@fromorbit.com -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@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: [PATCH RFC 1/2] MAINTAINERS: Add "R:" designated-reviewers tag
On Mon, 2 Jun 2014 16:24:24 -0700 Andrew Morton wrote: > On Tue, 3 Jun 2014 09:19:49 +1000 Dave Chinner wrote: > > > Anyone using Reviewed-by without having actually applied and tested > > the patch is mis-using the tag > > I think you just described 94.7% of Reviewed-by:s. 94.8% /me me raises his hand in shame! Yeah, to me Reviewed-by means that I agonized over the patch to understand it as much as if I wrote it myself. But I honestly don't always test it, or even compile it for that matter. -- Steve -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@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: [PATCH RFC 1/2] MAINTAINERS: Add "R:" designated-reviewers tag
On Mon, 2014-06-02 at 16:59 -0700, j...@joshtriplett.org wrote: > Acked-by and Reviewed-by mean two different things (Reviewed-by being a > superset of Acked-by), and the difference is not "I've applied and > tested this"; that's Tested-by. I think the standards for every signature tag other than Signed-off-by: should be _much_ higher than they are and that most all uses of "Acked-by:" and "Reviewed-by:" tags are for vanity. "Tested-by:" tags would be more helpful if the test cases that were used were somehow sent along with the signature. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@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: [PATCH RFC 1/2] MAINTAINERS: Add "R:" designated-reviewers tag
On Tue, Jun 03, 2014 at 09:19:49AM +1000, Dave Chinner wrote: > On Mon, Jun 02, 2014 at 12:17:46PM -0700, Joe Perches wrote: > > On Mon, 2014-06-02 at 12:09 -0700, j...@joshtriplett.org wrote: > > > On Mon, Jun 02, 2014 at 12:05:17PM -0700, Joe Perches wrote: > > > > On Mon, 2014-06-02 at 11:55 -0700, j...@joshtriplett.org wrote: > > > > > this should go along with a change to > > > > > get_maintainer.pl to add those folks to the CC list. > > > > > > > > Something like this: > > > > > > Yes, exactly. Given an appropriate commit message, > > > Reviewed-by: Josh Triplett > > > > That's the sort of patch where reviewing is > > pretty useless. > > > > What it needs is testing, not reviewing. > > > > I tested it for all of 10 seconds. > > From Documentation/SubmittingPatches: > > " (c) While there may be things that could be improved with this > submission, I believe that it is, at this time, (1) a > worthwhile modification to the kernel, and (2) free of known > issues which would argue against its inclusion. > . > > 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." > > So, for someone to say they have reviewed the code and are able to > say it is free of known issues and has no remaining technical > issues, they would have had to apply, compile and test the patch, > yes? > > i.e. Reviewed-by implies both Acked-by, Tested-by and that the code > is technically sound. No, not at all. It implies Acked-by, and that the code is technically sound (both at the micro-level and in overall architecture/approach), but does not imply Tested-by; that's a separate tag for a reason. We should not, for instance, prevent someone from providing a Reviewed-by (as opposed to an Acked-by) for a driver whose hardware few people actually have. There's significant value in code review even without the ability to test. > Anyone using Reviewed-by without having actually applied and tested > the patch is mis-using the tag - they should be using Acked-by: if > all they have done is read the code in their mail program Acked-by and Reviewed-by mean two different things (Reviewed-by being a superset of Acked-by), and the difference is not "I've applied and tested this"; that's Tested-by. - Josh Triplett -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@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: [PATCH RFC 1/2] MAINTAINERS: Add "R:" designated-reviewers tag
On Tue, 3 Jun 2014 09:19:49 +1000 Dave Chinner wrote: > Anyone using Reviewed-by without having actually applied and tested > the patch is mis-using the tag I think you just described 94.7% of Reviewed-by:s. > - they should be using Acked-by: if > all they have done is read the code in their mail program -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@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: [PATCH RFC 1/2] MAINTAINERS: Add "R:" designated-reviewers tag
On Mon, Jun 02, 2014 at 12:17:46PM -0700, Joe Perches wrote: > On Mon, 2014-06-02 at 12:09 -0700, j...@joshtriplett.org wrote: > > On Mon, Jun 02, 2014 at 12:05:17PM -0700, Joe Perches wrote: > > > On Mon, 2014-06-02 at 11:55 -0700, j...@joshtriplett.org wrote: > > > > this should go along with a change to > > > > get_maintainer.pl to add those folks to the CC list. > > > > > > Something like this: > > > > Yes, exactly. Given an appropriate commit message, > > Reviewed-by: Josh Triplett > > That's the sort of patch where reviewing is > pretty useless. > > What it needs is testing, not reviewing. > > I tested it for all of 10 seconds. >From Documentation/SubmittingPatches: " (c) While there may be things that could be improved with this submission, I believe that it is, at this time, (1) a worthwhile modification to the kernel, and (2) free of known issues which would argue against its inclusion. . 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." So, for someone to say they have reviewed the code and are able to say it is free of known issues and has no remaining technical issues, they would have had to apply, compile and test the patch, yes? i.e. Reviewed-by implies both Acked-by, Tested-by and that the code is technically sound. Anyone using Reviewed-by without having actually applied and tested the patch is mis-using the tag - they should be using Acked-by: if all they have done is read the code in their mail program Cheers, Dave. -- Dave Chinner da...@fromorbit.com -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@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: [PATCH RFC 1/2] MAINTAINERS: Add "R:" designated-reviewers tag
On Mon, Jun 02, 2014 at 12:26:31PM -0700, Paul E. McKenney wrote: > On Mon, Jun 02, 2014 at 12:05:17PM -0700, Joe Perches wrote: > > On Mon, 2014-06-02 at 11:55 -0700, j...@joshtriplett.org wrote: > > > this should go along with a change to > > > get_maintainer.pl to add those folks to the CC list. > > > > Something like this: > > To test this, I added a comment to kernel/rcu/srcu.c, then ran > scripts/get_maintainer.pl on the resulting diffs. Without the > below change to scripts/get_maintainer.pl, I get the following: > > Lai Jiangshan (supporter:SLEEPABLE READ-CO...) > "Paul E. McKenney" (supporter:SLEEPABLE > READ-CO...) > Dipankar Sarma (supporter:READ-COPY UPDATE...) > linux-kernel@vger.kernel.org (open list:SLEEPABLE READ-CO...) > > With the below change, it includes Josh, as expected based on the "R:" > entry I had previously added to MAINTAINERS in my local tree: > > Lai Jiangshan (supporter:SLEEPABLE READ-CO...) > "Paul E. McKenney" (supporter:SLEEPABLE > READ-CO...) > Josh Triplett (reviewer) > Dipankar Sarma (supporter:READ-COPY UPDATE...) > linux-kernel@vger.kernel.org (open list:SLEEPABLE READ-CO...) Paul, Sorry, I hadn't noticed it earlier. I can be dropped. Not that I am not interested, just that I am working on too many itsy bitsy things many outside the kernel and also herding cats, to find time :-) Thanks Dipankar -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@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: [PATCH RFC 1/2] MAINTAINERS: Add "R:" designated-reviewers tag
On Mon, Jun 02, 2014 at 12:40:38PM -0700, Randy Dunlap wrote: > On 06/02/2014 12:36 PM, Joe Perches wrote: > > On Mon, 2014-06-02 at 12:27 -0700, Paul E. McKenney wrote: > >> On Mon, Jun 02, 2014 at 12:11:55PM -0700, j...@joshtriplett.org wrote: > >>> > >>> I'd suggest adding Mathieu Desnoyers, Oleg Nesterov, and Lai Jiangshan > >>> as reviewers as well, with their consent. > >> > >> Mathieu, Oleg, Lai, any objections? > > > > I suggest not going overboard with R entries. > > If there are more than 1 or 2, create a mailing list. > > Yes, much better on the patch sender(s) to Cc: 1 or 2 people or lists > than to have to copy/paste 5 or 6 email addresses. A patch sender should never need to manually copy and paste email addresses, given get_maintainer.pl. - Josh Triplett -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@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: [PATCH RFC 1/2] MAINTAINERS: Add "R:" designated-reviewers tag
- Original Message - > From: "Paul E. McKenney" > To: j...@joshtriplett.org > Cc: "Joe Perches" , linux-kernel@vger.kernel.org, > mi...@kernel.org, la...@cn.fujitsu.com, > dipan...@in.ibm.com, a...@linux-foundation.org, "mathieu desnoyers" > , > n...@us.ibm.com, t...@linutronix.de, pet...@infradead.org, > rost...@goodmis.org, dhowe...@redhat.com, > eduma...@google.com, dvh...@linux.intel.com, fweis...@gmail.com, > o...@redhat.com, s...@mit.edu > Sent: Monday, June 2, 2014 3:27:29 PM > Subject: Re: [PATCH RFC 1/2] MAINTAINERS: Add "R:" designated-reviewers tag > > On Mon, Jun 02, 2014 at 12:11:55PM -0700, j...@joshtriplett.org wrote: > > On Mon, Jun 02, 2014 at 12:08:21PM -0700, Paul E. McKenney wrote: > > > On Mon, Jun 02, 2014 at 11:56:35AM -0700, j...@joshtriplett.org wrote: > > > > On Mon, Jun 02, 2014 at 11:16:58AM -0700, Paul E. McKenney wrote: > > > > > On Mon, Jun 02, 2014 at 10:59:28AM -0700, Joe Perches wrote: > > > > > > On Mon, 2014-06-02 at 10:48 -0700, j...@joshtriplett.org wrote: > > > > > > > On Mon, Jun 02, 2014 at 10:22:58AM -0700, Joe Perches wrote: > > > > > > > > On Mon, 2014-06-02 at 10:00 -0700, Paul E. McKenney wrote: > > > > > > > > > A ksummit-discuss email thread looked at the difficulty > > > > > > > > > recruiting > > > > > > > > > and retaining reviewers. > > > > > > > > > > > > > > > > [] > > > > > > > > > > > > > > > > > Paul Walmsley also noted the need for patch > > > > > > > > > submitters to know who the key reviewers are and suggested > > > > > > > > > adding an > > > > > > > > > "R:" tag to the MAINTAINERS file to record this information > > > > > > > > > on a > > > > > > > > > per-subsystem basis. > > > > > > > > > > > > > > > > I'm not sure of the value of this. > > > > > > > > > > > > > > > > Why not just mark the actual reviewers as maintainers? > > > > > > > > > > > > > > As discussed in the kernel summit discussion, being a regular > > > > > > > patch > > > > > > > reviewer isn't the same thing as being *the* maintainer. > > > > > > > > > > > > I think it's not particularly important or valuable > > > > > > here to make that distinction. > > > > > > > > > > > > What real difference does it make? > > > > > > > > > > In the particular case of Josh, none, at least from my viewpoint. He > > > > > of > > > > > course might or might not want to take on additional maintainership > > > > > responsibility at this particular point in time, in which case, I > > > > > would > > > > > be more than happy to have him as a designated maintainer. > > > > > > > > For the record, I'd be happy to be listed as a co-maintainer for RCU. > > > > :) > > > > > > I would be happy to put you down as maintainer and Steven down as > > > official reviewer. ;-) > > > > I'd suggest adding Mathieu Desnoyers, Oleg Nesterov, and Lai Jiangshan > > as reviewers as well, with their consent. > > Mathieu, Oleg, Lai, any objections? No objection from me. I'm always glad to help out reviewing RCU patches whenever I have some cycles available. Thanks, Mathieu -- Mathieu Desnoyers EfficiOS Inc. http://www.efficios.com -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@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: [PATCH RFC 1/2] MAINTAINERS: Add "R:" designated-reviewers tag
On Mon, Jun 02, 2014 at 12:55:50PM -0700, Joe Perches wrote: > On Mon, 2014-06-02 at 12:50 -0700, Paul E. McKenney wrote: > > I sometimes review patches in areas where > > I have no commits. > > Lots of people review patches all over the tree. > That doesn't mean they want to be or should be cc'd. True enough. And we probably need to rely on the maintainer's judgment for this. Thanx, Paul -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@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: [PATCH RFC 1/2] MAINTAINERS: Add "R:" designated-reviewers tag
On Mon, 2014-06-02 at 12:50 -0700, Paul E. McKenney wrote: > I sometimes review patches in areas where > I have no commits. Lots of people review patches all over the tree. That doesn't mean they want to be or should be cc'd. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@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: [PATCH RFC 1/2] MAINTAINERS: Add "R:" designated-reviewers tag
On Mon, Jun 02, 2014 at 12:36:22PM -0700, Joe Perches wrote: > On Mon, 2014-06-02 at 12:27 -0700, Paul E. McKenney wrote: > > On Mon, Jun 02, 2014 at 12:11:55PM -0700, j...@joshtriplett.org wrote: > > > > > > I'd suggest adding Mathieu Desnoyers, Oleg Nesterov, and Lai Jiangshan > > > as reviewers as well, with their consent. > > > > Mathieu, Oleg, Lai, any objections? > > I suggest not going overboard with R entries. > If there are more than 1 or 2, create a mailing list. Having a limit of two seems reasonable. First to the post, then. Or maybe last to object? ;-) > And more entries will just make the churn and name-decay > in MAINTAINERS that much more prevalent. > > None of those people have any significant > quantities of commit entries for rcu > > (with the get_maintainer patch posted) > > $ ./scripts/get_maintainer.pl -f kernel/rcu/ --git --git-min-percent=3 > Dipankar Sarma (supporter:READ-COPY UPDATE...) > "Paul E. McKenney" (supporter:READ-COPY > UPDATE...,commit_signer:82/85=96%,authored:58/85=68%) > Josh Triplett (reviewer,commit_signer:56/85=66%) > Ingo Molnar (commit_signer:5/85=6%) > Peter Zijlstra (commit_signer:5/85=6%) > Thomas Gleixner (commit_signer:3/85=4%) > Andreea-Cristina Bernat (authored:3/85=4%) > linux-kernel@vger.kernel.org (open list:READ-COPY UPDATE...) Good to know, but then again, I sometimes review patches in areas where I have no commits. Thanx, Paul -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@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: [PATCH RFC 1/2] MAINTAINERS: Add "R:" designated-reviewers tag
On 06/02/2014 12:36 PM, Joe Perches wrote: > On Mon, 2014-06-02 at 12:27 -0700, Paul E. McKenney wrote: >> On Mon, Jun 02, 2014 at 12:11:55PM -0700, j...@joshtriplett.org wrote: >>> >>> I'd suggest adding Mathieu Desnoyers, Oleg Nesterov, and Lai Jiangshan >>> as reviewers as well, with their consent. >> >> Mathieu, Oleg, Lai, any objections? > > I suggest not going overboard with R entries. > If there are more than 1 or 2, create a mailing list. Yes, much better on the patch sender(s) to Cc: 1 or 2 people or lists than to have to copy/paste 5 or 6 email addresses. > And more entries will just make the churn and name-decay > in MAINTAINERS that much more prevalent. > > None of those people have any significant > quantities of commit entries for rcu -- ~Randy -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@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: [PATCH RFC 1/2] MAINTAINERS: Add "R:" designated-reviewers tag
On Mon, 2014-06-02 at 12:27 -0700, Paul E. McKenney wrote: > On Mon, Jun 02, 2014 at 12:11:55PM -0700, j...@joshtriplett.org wrote: > > > > I'd suggest adding Mathieu Desnoyers, Oleg Nesterov, and Lai Jiangshan > > as reviewers as well, with their consent. > > Mathieu, Oleg, Lai, any objections? I suggest not going overboard with R entries. If there are more than 1 or 2, create a mailing list. And more entries will just make the churn and name-decay in MAINTAINERS that much more prevalent. None of those people have any significant quantities of commit entries for rcu (with the get_maintainer patch posted) $ ./scripts/get_maintainer.pl -f kernel/rcu/ --git --git-min-percent=3 Dipankar Sarma (supporter:READ-COPY UPDATE...) "Paul E. McKenney" (supporter:READ-COPY UPDATE...,commit_signer:82/85=96%,authored:58/85=68%) Josh Triplett (reviewer,commit_signer:56/85=66%) Ingo Molnar (commit_signer:5/85=6%) Peter Zijlstra (commit_signer:5/85=6%) Thomas Gleixner (commit_signer:3/85=4%) Andreea-Cristina Bernat (authored:3/85=4%) linux-kernel@vger.kernel.org (open list:READ-COPY UPDATE...) -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@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: [PATCH RFC 1/2] MAINTAINERS: Add "R:" designated-reviewers tag
On Mon, Jun 02, 2014 at 12:11:55PM -0700, j...@joshtriplett.org wrote: > On Mon, Jun 02, 2014 at 12:08:21PM -0700, Paul E. McKenney wrote: > > On Mon, Jun 02, 2014 at 11:56:35AM -0700, j...@joshtriplett.org wrote: > > > On Mon, Jun 02, 2014 at 11:16:58AM -0700, Paul E. McKenney wrote: > > > > On Mon, Jun 02, 2014 at 10:59:28AM -0700, Joe Perches wrote: > > > > > On Mon, 2014-06-02 at 10:48 -0700, j...@joshtriplett.org wrote: > > > > > > On Mon, Jun 02, 2014 at 10:22:58AM -0700, Joe Perches wrote: > > > > > > > On Mon, 2014-06-02 at 10:00 -0700, Paul E. McKenney wrote: > > > > > > > > A ksummit-discuss email thread looked at the difficulty > > > > > > > > recruiting > > > > > > > > and retaining reviewers. > > > > > > > > > > > > > > [] > > > > > > > > > > > > > > > Paul Walmsley also noted the need for patch > > > > > > > > submitters to know who the key reviewers are and suggested > > > > > > > > adding an > > > > > > > > "R:" tag to the MAINTAINERS file to record this information on a > > > > > > > > per-subsystem basis. > > > > > > > > > > > > > > I'm not sure of the value of this. > > > > > > > > > > > > > > Why not just mark the actual reviewers as maintainers? > > > > > > > > > > > > As discussed in the kernel summit discussion, being a regular patch > > > > > > reviewer isn't the same thing as being *the* maintainer. > > > > > > > > > > I think it's not particularly important or valuable > > > > > here to make that distinction. > > > > > > > > > > What real difference does it make? > > > > > > > > In the particular case of Josh, none, at least from my viewpoint. He of > > > > course might or might not want to take on additional maintainership > > > > responsibility at this particular point in time, in which case, I would > > > > be more than happy to have him as a designated maintainer. > > > > > > For the record, I'd be happy to be listed as a co-maintainer for RCU. :) > > > > I would be happy to put you down as maintainer and Steven down as > > official reviewer. ;-) > > I'd suggest adding Mathieu Desnoyers, Oleg Nesterov, and Lai Jiangshan > as reviewers as well, with their consent. Mathieu, Oleg, Lai, any objections? Thanx, Paul -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@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: [PATCH RFC 1/2] MAINTAINERS: Add "R:" designated-reviewers tag
On Mon, Jun 02, 2014 at 12:05:17PM -0700, Joe Perches wrote:
> On Mon, 2014-06-02 at 11:55 -0700, j...@joshtriplett.org wrote:
> > this should go along with a change to
> > get_maintainer.pl to add those folks to the CC list.
>
> Something like this:
To test this, I added a comment to kernel/rcu/srcu.c, then ran
scripts/get_maintainer.pl on the resulting diffs. Without the
below change to scripts/get_maintainer.pl, I get the following:
Lai Jiangshan (supporter:SLEEPABLE READ-CO...)
"Paul E. McKenney" (supporter:SLEEPABLE READ-CO...)
Dipankar Sarma (supporter:READ-COPY UPDATE...)
linux-kernel@vger.kernel.org (open list:SLEEPABLE READ-CO...)
With the below change, it includes Josh, as expected based on the "R:"
entry I had previously added to MAINTAINERS in my local tree:
Lai Jiangshan (supporter:SLEEPABLE READ-CO...)
"Paul E. McKenney" (supporter:SLEEPABLE READ-CO...)
Josh Triplett (reviewer)
Dipankar Sarma (supporter:READ-COPY UPDATE...)
linux-kernel@vger.kernel.org (open list:SLEEPABLE READ-CO...)
So:
Tested-by: Paul E. McKenney
> ---
> scripts/get_maintainer.pl | 22 +-
> 1 file changed, 21 insertions(+), 1 deletion(-)
>
> diff --git a/scripts/get_maintainer.pl b/scripts/get_maintainer.pl
> index 4198788..d701627 100755
> --- a/scripts/get_maintainer.pl
> +++ b/scripts/get_maintainer.pl
> @@ -21,6 +21,7 @@ my $lk_path = "./";
> my $email = 1;
> my $email_usename = 1;
> my $email_maintainer = 1;
> +my $email_reviewer = 1;
> my $email_list = 1;
> my $email_subscriber_list = 0;
> my $email_git_penguin_chiefs = 0;
> @@ -202,6 +203,7 @@ if (!GetOptions(
> 'remove-duplicates!' => \$email_remove_duplicates,
> 'mailmap!' => \$email_use_mailmap,
> 'm!' => \$email_maintainer,
> + 'r!' => \$email_reviewer,
> 'n!' => \$email_usename,
> 'l!' => \$email_list,
> 's!' => \$email_subscriber_list,
> @@ -260,7 +262,8 @@ if ($sections) {
> }
>
> if ($email &&
> -($email_maintainer + $email_list + $email_subscriber_list +
> +($email_maintainer + $email_reviewer +
> + $email_list + $email_subscriber_list +
> $email_git + $email_git_penguin_chiefs + $email_git_blame) == 0) {
> die "$P: Please select at least 1 email option\n";
> }
> @@ -750,6 +753,7 @@ MAINTAINER field selection options:
> --hg-since => hg history to use (default: $email_hg_since)
> --interactive => display a menu (mostly useful if used with the --git
> option)
> --m => include maintainer(s) if any
> +--r => include reviewer(s) if any
> --n => include name 'Full Name '
> --l => include list(s) if any
> --s => include subscriber only list(s) if any
> @@ -1064,6 +1068,22 @@ sub add_categories {
> my $role = get_maintainer_role($i);
> push_email_addresses($pvalue, $role);
> }
> + } elsif ($ptype eq "R") {
> + my ($name, $address) = parse_email($pvalue);
> + if ($name eq "") {
> + if ($i > 0) {
> + my $tv = $typevalue[$i - 1];
> + if ($tv =~ m/^(\C):\s*(.*)/) {
> + if ($1 eq "P") {
> + $name = $2;
> + $pvalue = format_email($name, $address,
> $email_usename);
> + }
> + }
> + }
> + }
> + if ($email_reviewer) {
> + push_email_addresses($pvalue, 'reviewer');
> + }
> } elsif ($ptype eq "T") {
> push(@scm, $pvalue);
> } elsif ($ptype eq "W") {
>
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@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: [PATCH RFC 1/2] MAINTAINERS: Add "R:" designated-reviewers tag
On Mon, 2014-06-02 at 12:09 -0700, j...@joshtriplett.org wrote: > On Mon, Jun 02, 2014 at 12:05:17PM -0700, Joe Perches wrote: > > On Mon, 2014-06-02 at 11:55 -0700, j...@joshtriplett.org wrote: > > > this should go along with a change to > > > get_maintainer.pl to add those folks to the CC list. > > > > Something like this: > > Yes, exactly. Given an appropriate commit message, > Reviewed-by: Josh Triplett That's the sort of patch where reviewing is pretty useless. What it needs is testing, not reviewing. I tested it for all of 10 seconds. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@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: [PATCH RFC 1/2] MAINTAINERS: Add "R:" designated-reviewers tag
On Mon, Jun 02, 2014 at 12:08:21PM -0700, Paul E. McKenney wrote: > On Mon, Jun 02, 2014 at 11:56:35AM -0700, j...@joshtriplett.org wrote: > > On Mon, Jun 02, 2014 at 11:16:58AM -0700, Paul E. McKenney wrote: > > > On Mon, Jun 02, 2014 at 10:59:28AM -0700, Joe Perches wrote: > > > > On Mon, 2014-06-02 at 10:48 -0700, j...@joshtriplett.org wrote: > > > > > On Mon, Jun 02, 2014 at 10:22:58AM -0700, Joe Perches wrote: > > > > > > On Mon, 2014-06-02 at 10:00 -0700, Paul E. McKenney wrote: > > > > > > > A ksummit-discuss email thread looked at the difficulty recruiting > > > > > > > and retaining reviewers. > > > > > > > > > > > > [] > > > > > > > > > > > > > Paul Walmsley also noted the need for patch > > > > > > > submitters to know who the key reviewers are and suggested adding > > > > > > > an > > > > > > > "R:" tag to the MAINTAINERS file to record this information on a > > > > > > > per-subsystem basis. > > > > > > > > > > > > I'm not sure of the value of this. > > > > > > > > > > > > Why not just mark the actual reviewers as maintainers? > > > > > > > > > > As discussed in the kernel summit discussion, being a regular patch > > > > > reviewer isn't the same thing as being *the* maintainer. > > > > > > > > I think it's not particularly important or valuable > > > > here to make that distinction. > > > > > > > > What real difference does it make? > > > > > > In the particular case of Josh, none, at least from my viewpoint. He of > > > course might or might not want to take on additional maintainership > > > responsibility at this particular point in time, in which case, I would > > > be more than happy to have him as a designated maintainer. > > > > For the record, I'd be happy to be listed as a co-maintainer for RCU. :) > > I would be happy to put you down as maintainer and Steven down as > official reviewer. ;-) I'd suggest adding Mathieu Desnoyers, Oleg Nesterov, and Lai Jiangshan as reviewers as well, with their consent. - Josh Triplett -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@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: [PATCH RFC 1/2] MAINTAINERS: Add "R:" designated-reviewers tag
On Mon, Jun 02, 2014 at 12:05:17PM -0700, Joe Perches wrote:
> On Mon, 2014-06-02 at 11:55 -0700, j...@joshtriplett.org wrote:
> > this should go along with a change to
> > get_maintainer.pl to add those folks to the CC list.
>
> Something like this:
Yes, exactly. Given an appropriate commit message,
Reviewed-by: Josh Triplett
> ---
> scripts/get_maintainer.pl | 22 +-
> 1 file changed, 21 insertions(+), 1 deletion(-)
>
> diff --git a/scripts/get_maintainer.pl b/scripts/get_maintainer.pl
> index 4198788..d701627 100755
> --- a/scripts/get_maintainer.pl
> +++ b/scripts/get_maintainer.pl
> @@ -21,6 +21,7 @@ my $lk_path = "./";
> my $email = 1;
> my $email_usename = 1;
> my $email_maintainer = 1;
> +my $email_reviewer = 1;
> my $email_list = 1;
> my $email_subscriber_list = 0;
> my $email_git_penguin_chiefs = 0;
> @@ -202,6 +203,7 @@ if (!GetOptions(
> 'remove-duplicates!' => \$email_remove_duplicates,
> 'mailmap!' => \$email_use_mailmap,
> 'm!' => \$email_maintainer,
> + 'r!' => \$email_reviewer,
> 'n!' => \$email_usename,
> 'l!' => \$email_list,
> 's!' => \$email_subscriber_list,
> @@ -260,7 +262,8 @@ if ($sections) {
> }
>
> if ($email &&
> -($email_maintainer + $email_list + $email_subscriber_list +
> +($email_maintainer + $email_reviewer +
> + $email_list + $email_subscriber_list +
> $email_git + $email_git_penguin_chiefs + $email_git_blame) == 0) {
> die "$P: Please select at least 1 email option\n";
> }
> @@ -750,6 +753,7 @@ MAINTAINER field selection options:
> --hg-since => hg history to use (default: $email_hg_since)
> --interactive => display a menu (mostly useful if used with the --git
> option)
> --m => include maintainer(s) if any
> +--r => include reviewer(s) if any
> --n => include name 'Full Name '
> --l => include list(s) if any
> --s => include subscriber only list(s) if any
> @@ -1064,6 +1068,22 @@ sub add_categories {
> my $role = get_maintainer_role($i);
> push_email_addresses($pvalue, $role);
> }
> + } elsif ($ptype eq "R") {
> + my ($name, $address) = parse_email($pvalue);
> + if ($name eq "") {
> + if ($i > 0) {
> + my $tv = $typevalue[$i - 1];
> + if ($tv =~ m/^(\C):\s*(.*)/) {
> + if ($1 eq "P") {
> + $name = $2;
> + $pvalue = format_email($name, $address,
> $email_usename);
> + }
> + }
> + }
> + }
> + if ($email_reviewer) {
> + push_email_addresses($pvalue, 'reviewer');
> + }
> } elsif ($ptype eq "T") {
> push(@scm, $pvalue);
> } elsif ($ptype eq "W") {
>
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@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: [PATCH RFC 1/2] MAINTAINERS: Add "R:" designated-reviewers tag
On Mon, Jun 02, 2014 at 11:56:35AM -0700, j...@joshtriplett.org wrote: > On Mon, Jun 02, 2014 at 11:16:58AM -0700, Paul E. McKenney wrote: > > On Mon, Jun 02, 2014 at 10:59:28AM -0700, Joe Perches wrote: > > > On Mon, 2014-06-02 at 10:48 -0700, j...@joshtriplett.org wrote: > > > > On Mon, Jun 02, 2014 at 10:22:58AM -0700, Joe Perches wrote: > > > > > On Mon, 2014-06-02 at 10:00 -0700, Paul E. McKenney wrote: > > > > > > A ksummit-discuss email thread looked at the difficulty recruiting > > > > > > and retaining reviewers. > > > > > > > > > > [] > > > > > > > > > > > Paul Walmsley also noted the need for patch > > > > > > submitters to know who the key reviewers are and suggested adding an > > > > > > "R:" tag to the MAINTAINERS file to record this information on a > > > > > > per-subsystem basis. > > > > > > > > > > I'm not sure of the value of this. > > > > > > > > > > Why not just mark the actual reviewers as maintainers? > > > > > > > > As discussed in the kernel summit discussion, being a regular patch > > > > reviewer isn't the same thing as being *the* maintainer. > > > > > > I think it's not particularly important or valuable > > > here to make that distinction. > > > > > > What real difference does it make? > > > > In the particular case of Josh, none, at least from my viewpoint. He of > > course might or might not want to take on additional maintainership > > responsibility at this particular point in time, in which case, I would > > be more than happy to have him as a designated maintainer. > > For the record, I'd be happy to be listed as a co-maintainer for RCU. :) I would be happy to put you down as maintainer and Steven down as official reviewer. ;-) Thanx, Paul -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@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: [PATCH RFC 1/2] MAINTAINERS: Add "R:" designated-reviewers tag
On Mon, Jun 02, 2014 at 02:50:20PM -0400, Steven Rostedt wrote: > On Mon, 02 Jun 2014 11:44:29 -0700 > Joe Perches wrote: > > > On Mon, 2014-06-02 at 11:16 -0700, Paul E. McKenney wrote: > > > But there have been people who have found serious issues in RCU patches > > > who I would not trust as full maintainers. The ability to find defects > > > is valuable in and of itself, and should be recognized as such, even > > > when not accompanied by the rest of the maintainership package. > > > > Maybe, but odd-lot reviewers are most likely going to find > > these same defects regardless of any "R" designation in > > MAINTAINERS. > > > > Actually, I'm thinking the R: tag is a good idea and we should have > people ask to be added to MAINTAINERS if they want to review certain > subsystems. Grant you, it should be people that the maintainers trust. > I can think of several people I would like to be added as R: in tracing. > > The point is, when patches go out, it is easy to see who the Cc list > should be. And perhaps this will get patches reviewed more. Maybe > maintainers of other subsystems should ask to have the R: tag added for > something they don't maintain but want to help out in. > > I may add myself to the x86, scheduler, time keeping and perhaps even > RCU, as I like to read those patches. I'm not at the level of > maintaining those subsystems, but I feel comfortable enough to review > any changes there. I would be quite happy to add you as official reviewer for RCU. Thanx, Paul -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@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: [PATCH RFC 1/2] MAINTAINERS: Add "R:" designated-reviewers tag
On Mon, 2014-06-02 at 11:55 -0700, j...@joshtriplett.org wrote:
> this should go along with a change to
> get_maintainer.pl to add those folks to the CC list.
Something like this:
---
scripts/get_maintainer.pl | 22 +-
1 file changed, 21 insertions(+), 1 deletion(-)
diff --git a/scripts/get_maintainer.pl b/scripts/get_maintainer.pl
index 4198788..d701627 100755
--- a/scripts/get_maintainer.pl
+++ b/scripts/get_maintainer.pl
@@ -21,6 +21,7 @@ my $lk_path = "./";
my $email = 1;
my $email_usename = 1;
my $email_maintainer = 1;
+my $email_reviewer = 1;
my $email_list = 1;
my $email_subscriber_list = 0;
my $email_git_penguin_chiefs = 0;
@@ -202,6 +203,7 @@ if (!GetOptions(
'remove-duplicates!' => \$email_remove_duplicates,
'mailmap!' => \$email_use_mailmap,
'm!' => \$email_maintainer,
+ 'r!' => \$email_reviewer,
'n!' => \$email_usename,
'l!' => \$email_list,
's!' => \$email_subscriber_list,
@@ -260,7 +262,8 @@ if ($sections) {
}
if ($email &&
-($email_maintainer + $email_list + $email_subscriber_list +
+($email_maintainer + $email_reviewer +
+ $email_list + $email_subscriber_list +
$email_git + $email_git_penguin_chiefs + $email_git_blame) == 0) {
die "$P: Please select at least 1 email option\n";
}
@@ -750,6 +753,7 @@ MAINTAINER field selection options:
--hg-since => hg history to use (default: $email_hg_since)
--interactive => display a menu (mostly useful if used with the --git
option)
--m => include maintainer(s) if any
+--r => include reviewer(s) if any
--n => include name 'Full Name '
--l => include list(s) if any
--s => include subscriber only list(s) if any
@@ -1064,6 +1068,22 @@ sub add_categories {
my $role = get_maintainer_role($i);
push_email_addresses($pvalue, $role);
}
+ } elsif ($ptype eq "R") {
+ my ($name, $address) = parse_email($pvalue);
+ if ($name eq "") {
+ if ($i > 0) {
+ my $tv = $typevalue[$i - 1];
+ if ($tv =~ m/^(\C):\s*(.*)/) {
+ if ($1 eq "P") {
+ $name = $2;
+ $pvalue = format_email($name, $address,
$email_usename);
+ }
+ }
+ }
+ }
+ if ($email_reviewer) {
+ push_email_addresses($pvalue, 'reviewer');
+ }
} elsif ($ptype eq "T") {
push(@scm, $pvalue);
} elsif ($ptype eq "W") {
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@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: [PATCH RFC 1/2] MAINTAINERS: Add "R:" designated-reviewers tag
On Mon, Jun 02, 2014 at 11:16:58AM -0700, Paul E. McKenney wrote: > On Mon, Jun 02, 2014 at 10:59:28AM -0700, Joe Perches wrote: > > On Mon, 2014-06-02 at 10:48 -0700, j...@joshtriplett.org wrote: > > > On Mon, Jun 02, 2014 at 10:22:58AM -0700, Joe Perches wrote: > > > > On Mon, 2014-06-02 at 10:00 -0700, Paul E. McKenney wrote: > > > > > A ksummit-discuss email thread looked at the difficulty recruiting > > > > > and retaining reviewers. > > > > > > > > [] > > > > > > > > > Paul Walmsley also noted the need for patch > > > > > submitters to know who the key reviewers are and suggested adding an > > > > > "R:" tag to the MAINTAINERS file to record this information on a > > > > > per-subsystem basis. > > > > > > > > I'm not sure of the value of this. > > > > > > > > Why not just mark the actual reviewers as maintainers? > > > > > > As discussed in the kernel summit discussion, being a regular patch > > > reviewer isn't the same thing as being *the* maintainer. > > > > I think it's not particularly important or valuable > > here to make that distinction. > > > > What real difference does it make? > > In the particular case of Josh, none, at least from my viewpoint. He of > course might or might not want to take on additional maintainership > responsibility at this particular point in time, in which case, I would > be more than happy to have him as a designated maintainer. For the record, I'd be happy to be listed as a co-maintainer for RCU. :) - Josh Triplett -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@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: [PATCH RFC 1/2] MAINTAINERS: Add "R:" designated-reviewers tag
On Mon, Jun 02, 2014 at 02:50:20PM -0400, Steven Rostedt wrote: > On Mon, 02 Jun 2014 11:44:29 -0700 > Joe Perches wrote: > > > On Mon, 2014-06-02 at 11:16 -0700, Paul E. McKenney wrote: > > > But there have been people who have found serious issues in RCU patches > > > who I would not trust as full maintainers. The ability to find defects > > > is valuable in and of itself, and should be recognized as such, even > > > when not accompanied by the rest of the maintainership package. > > > > Maybe, but odd-lot reviewers are most likely going to find > > these same defects regardless of any "R" designation in > > MAINTAINERS. > > > > Actually, I'm thinking the R: tag is a good idea and we should have > people ask to be added to MAINTAINERS if they want to review certain > subsystems. Grant you, it should be people that the maintainers trust. > I can think of several people I would like to be added as R: in tracing. > > The point is, when patches go out, it is easy to see who the Cc list > should be. And perhaps this will get patches reviewed more. Maybe > maintainers of other subsystems should ask to have the R: tag added for > something they don't maintain but want to help out in. That's exactly the idea: this should go along with a change to get_maintainer.pl to add those folks to the CC list. - Josh Triplett -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@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: [PATCH RFC 1/2] MAINTAINERS: Add "R:" designated-reviewers tag
On Mon, 02 Jun 2014 11:44:29 -0700 Joe Perches wrote: > On Mon, 2014-06-02 at 11:16 -0700, Paul E. McKenney wrote: > > But there have been people who have found serious issues in RCU patches > > who I would not trust as full maintainers. The ability to find defects > > is valuable in and of itself, and should be recognized as such, even > > when not accompanied by the rest of the maintainership package. > > Maybe, but odd-lot reviewers are most likely going to find > these same defects regardless of any "R" designation in > MAINTAINERS. > Actually, I'm thinking the R: tag is a good idea and we should have people ask to be added to MAINTAINERS if they want to review certain subsystems. Grant you, it should be people that the maintainers trust. I can think of several people I would like to be added as R: in tracing. The point is, when patches go out, it is easy to see who the Cc list should be. And perhaps this will get patches reviewed more. Maybe maintainers of other subsystems should ask to have the R: tag added for something they don't maintain but want to help out in. I may add myself to the x86, scheduler, time keeping and perhaps even RCU, as I like to read those patches. I'm not at the level of maintaining those subsystems, but I feel comfortable enough to review any changes there. -- Steve -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@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: [PATCH RFC 1/2] MAINTAINERS: Add "R:" designated-reviewers tag
On Mon, 2014-06-02 at 11:16 -0700, Paul E. McKenney wrote: > But there have been people who have found serious issues in RCU patches > who I would not trust as full maintainers. The ability to find defects > is valuable in and of itself, and should be recognized as such, even > when not accompanied by the rest of the maintainership package. Maybe, but odd-lot reviewers are most likely going to find these same defects regardless of any "R" designation in MAINTAINERS. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@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: [PATCH RFC 1/2] MAINTAINERS: Add "R:" designated-reviewers tag
On Mon, Jun 02, 2014 at 10:59:28AM -0700, Joe Perches wrote: > On Mon, 2014-06-02 at 10:48 -0700, j...@joshtriplett.org wrote: > > On Mon, Jun 02, 2014 at 10:22:58AM -0700, Joe Perches wrote: > > > On Mon, 2014-06-02 at 10:00 -0700, Paul E. McKenney wrote: > > > > A ksummit-discuss email thread looked at the difficulty recruiting > > > > and retaining reviewers. > > > > > > [] > > > > > > > Paul Walmsley also noted the need for patch > > > > submitters to know who the key reviewers are and suggested adding an > > > > "R:" tag to the MAINTAINERS file to record this information on a > > > > per-subsystem basis. > > > > > > I'm not sure of the value of this. > > > > > > Why not just mark the actual reviewers as maintainers? > > > > As discussed in the kernel summit discussion, being a regular patch > > reviewer isn't the same thing as being *the* maintainer. > > I think it's not particularly important or valuable > here to make that distinction. > > What real difference does it make? In the particular case of Josh, none, at least from my viewpoint. He of course might or might not want to take on additional maintainership responsibility at this particular point in time, in which case, I would be more than happy to have him as a designated maintainer. But there have been people who have found serious issues in RCU patches who I would not trust as full maintainers. The ability to find defects is valuable in and of itself, and should be recognized as such, even when not accompanied by the rest of the maintainership package. Thanx, Paul -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@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: [PATCH RFC 1/2] MAINTAINERS: Add "R:" designated-reviewers tag
On Mon, 2014-06-02 at 14:12 -0400, Josh Boyer wrote: > On Mon, Jun 2, 2014 at 1:59 PM, Joe Perches wrote: > > On Mon, 2014-06-02 at 10:48 -0700, j...@joshtriplett.org wrote: > >> On Mon, Jun 02, 2014 at 10:22:58AM -0700, Joe Perches wrote: > >> > On Mon, 2014-06-02 at 10:00 -0700, Paul E. McKenney wrote: > >> > > A ksummit-discuss email thread looked at the difficulty recruiting > >> > > and retaining reviewers. > >> > > >> > [] > >> > > >> > > Paul Walmsley also noted the need for patch > >> > > submitters to know who the key reviewers are and suggested adding an > >> > > "R:" tag to the MAINTAINERS file to record this information on a > >> > > per-subsystem basis. > >> > > >> > I'm not sure of the value of this. > >> > > >> > Why not just mark the actual reviewers as maintainers? > >> > >> As discussed in the kernel summit discussion, being a regular patch > >> reviewer isn't the same thing as being *the* maintainer. > > > > I think it's not particularly important or valuable > > here to make that distinction. > > > > What real difference does it make? > > It depends. If the Maintainer moves to a model where patches must be > reviewed before they are added to the tree, then having a designated > reviewer helps. It gives the patch submitter another person to > include, and if the Reviewer acks a patch, they know it's much more > likely to make it in-tree. > > If the tree isn't managed that way, then Reviewer/Maintainer is a bit > less distinctive, but it still provides at least some indication that > a "maintainer" looked at the patch instead of having it just sit on > the list. So effectively, nothing. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@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: [PATCH RFC 1/2] MAINTAINERS: Add "R:" designated-reviewers tag
On Mon, Jun 2, 2014 at 1:59 PM, Joe Perches wrote: > On Mon, 2014-06-02 at 10:48 -0700, j...@joshtriplett.org wrote: >> On Mon, Jun 02, 2014 at 10:22:58AM -0700, Joe Perches wrote: >> > On Mon, 2014-06-02 at 10:00 -0700, Paul E. McKenney wrote: >> > > A ksummit-discuss email thread looked at the difficulty recruiting >> > > and retaining reviewers. >> > >> > [] >> > >> > > Paul Walmsley also noted the need for patch >> > > submitters to know who the key reviewers are and suggested adding an >> > > "R:" tag to the MAINTAINERS file to record this information on a >> > > per-subsystem basis. >> > >> > I'm not sure of the value of this. >> > >> > Why not just mark the actual reviewers as maintainers? >> >> As discussed in the kernel summit discussion, being a regular patch >> reviewer isn't the same thing as being *the* maintainer. > > I think it's not particularly important or valuable > here to make that distinction. > > What real difference does it make? It depends. If the Maintainer moves to a model where patches must be reviewed before they are added to the tree, then having a designated reviewer helps. It gives the patch submitter another person to include, and if the Reviewer acks a patch, they know it's much more likely to make it in-tree. If the tree isn't managed that way, then Reviewer/Maintainer is a bit less distinctive, but it still provides at least some indication that a "maintainer" looked at the patch instead of having it just sit on the list. josh -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@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: [PATCH RFC 1/2] MAINTAINERS: Add "R:" designated-reviewers tag
On Mon, 2014-06-02 at 10:48 -0700, j...@joshtriplett.org wrote: > On Mon, Jun 02, 2014 at 10:22:58AM -0700, Joe Perches wrote: > > On Mon, 2014-06-02 at 10:00 -0700, Paul E. McKenney wrote: > > > A ksummit-discuss email thread looked at the difficulty recruiting > > > and retaining reviewers. > > > > [] > > > > > Paul Walmsley also noted the need for patch > > > submitters to know who the key reviewers are and suggested adding an > > > "R:" tag to the MAINTAINERS file to record this information on a > > > per-subsystem basis. > > > > I'm not sure of the value of this. > > > > Why not just mark the actual reviewers as maintainers? > > As discussed in the kernel summit discussion, being a regular patch > reviewer isn't the same thing as being *the* maintainer. I think it's not particularly important or valuable here to make that distinction. What real difference does it make? -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@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: [PATCH RFC 1/2] MAINTAINERS: Add "R:" designated-reviewers tag
On Mon, Jun 02, 2014 at 10:22:58AM -0700, Joe Perches wrote: > On Mon, 2014-06-02 at 10:00 -0700, Paul E. McKenney wrote: > > A ksummit-discuss email thread looked at the difficulty recruiting > > and retaining reviewers. > > [] > > > Paul Walmsley also noted the need for patch > > submitters to know who the key reviewers are and suggested adding an > > "R:" tag to the MAINTAINERS file to record this information on a > > per-subsystem basis. > > I'm not sure of the value of this. > > Why not just mark the actual reviewers as maintainers? As discussed in the kernel summit discussion, being a regular patch reviewer isn't the same thing as being *the* maintainer. > The lack of reviewers problem is entirely distinct from > the proposed solution. Not arguing with that; this change alone won't produce more reviewers, but it does provide a means of tracking regular reviewers. > I believe active reviewers will generally subscribe to a > subsystem specific mailing list. Not every driver or subsystem is large enough to have its own mailing list, and many would prefer to keep their discussion on LKML. Also, mailing lists introduce diffusion of responsibility. > Perhaps it'd be better to get a "linux-...@vger.kernel.org" > mailing list going. Perhaps. > > diff --git a/MAINTAINERS b/MAINTAINERS > [] > > @@ -73,6 +73,8 @@ Descriptions of section entries: > > L: Mailing list that is relevant to this area > > W: Web-page with status/info > > Q: Patchwork web based patch tracking system site > > + R: Designated reviewer, who should be CCed on patches, > > + format: FullName > > T: SCM tree type and location. > >Type is one of: git, hg, quilt, stgit, topgit > > S: Status, one of the following: > > If this is actually done, I suggest putting this > "R" entry description immediately after the "M" entry. Yeah, it does seem like these entries are not quite alphabetized, so putting this right after M makes sense. - Josh Triplett -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@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: [PATCH RFC 1/2] MAINTAINERS: Add "R:" designated-reviewers tag
On Mon, 2014-06-02 at 13:29 -0400, Steven Rostedt wrote: > On Mon, 02 Jun 2014 10:22:58 -0700 Joe Perches wrote: > > > Paul Walmsley also noted the need for patch > > > submitters to know who the key reviewers are and suggested adding an > > > "R:" tag to the MAINTAINERS file to record this information on a > > > per-subsystem basis. [] > > Why not just mark the actual reviewers as maintainers? [] > If you have a person that regularly reviews a patch and wants to be > Cc'd on all of them, then why not add them to MAINTAINERS? So we agree on that. > > > diff --git a/MAINTAINERS b/MAINTAINERS > > [] > > > @@ -73,6 +73,8 @@ Descriptions of section entries: > > > L: Mailing list that is relevant to this area > > > W: Web-page with status/info > > > Q: Patchwork web based patch tracking system site > > > + R: Designated reviewer, who should be CCed on patches, > > > +format: FullName > > > T: SCM tree type and location. > > > Type is one of: git, hg, quilt, stgit, topgit > > > S: Status, one of the following: > > > > If this is actually done, I suggest putting this > > "R" entry description immediately after the "M" entry. [] > Or perhaps after L: Either would be fine as long as the order is the same in the actual subsystem section entries that follow. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@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: [PATCH RFC 1/2] MAINTAINERS: Add "R:" designated-reviewers tag
On Mon, 02 Jun 2014 10:22:58 -0700 Joe Perches wrote: > > Paul Walmsley also noted the need for patch > > submitters to know who the key reviewers are and suggested adding an > > "R:" tag to the MAINTAINERS file to record this information on a > > per-subsystem basis. > > I'm not sure of the value of this. > > Why not just mark the actual reviewers as maintainers? > > The lack of reviewers problem is entirely distinct from > the proposed solution. > > I believe active reviewers will generally subscribe to a > subsystem specific mailing list. > > Perhaps it'd be better to get a "linux-...@vger.kernel.org" > mailing list going. I don't think we need a linux-rcu mailing list. I much rather see the patches on LKML. If you have a person that regularly reviews a patch and wants to be Cc'd on all of them, then why not add them to MAINTAINERS? > > > diff --git a/MAINTAINERS b/MAINTAINERS > [] > > @@ -73,6 +73,8 @@ Descriptions of section entries: > > L: Mailing list that is relevant to this area > > W: Web-page with status/info > > Q: Patchwork web based patch tracking system site > > + R: Designated reviewer, who should be CCed on patches, > > + format: FullName > > T: SCM tree type and location. > >Type is one of: git, hg, quilt, stgit, topgit > > S: Status, one of the following: > > If this is actually done, I suggest putting this > "R" entry description immediately after the "M" entry. > Or perhaps after L: -- Steve -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@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: [PATCH RFC 1/2] MAINTAINERS: Add "R:" designated-reviewers tag
On Mon, 2014-06-02 at 10:00 -0700, Paul E. McKenney wrote: > A ksummit-discuss email thread looked at the difficulty recruiting > and retaining reviewers. [] > Paul Walmsley also noted the need for patch > submitters to know who the key reviewers are and suggested adding an > "R:" tag to the MAINTAINERS file to record this information on a > per-subsystem basis. I'm not sure of the value of this. Why not just mark the actual reviewers as maintainers? The lack of reviewers problem is entirely distinct from the proposed solution. I believe active reviewers will generally subscribe to a subsystem specific mailing list. Perhaps it'd be better to get a "linux-...@vger.kernel.org" mailing list going. > diff --git a/MAINTAINERS b/MAINTAINERS [] > @@ -73,6 +73,8 @@ Descriptions of section entries: > L: Mailing list that is relevant to this area > W: Web-page with status/info > Q: Patchwork web based patch tracking system site > + R: Designated reviewer, who should be CCed on patches, > +format: FullName > T: SCM tree type and location. > Type is one of: git, hg, quilt, stgit, topgit > S: Status, one of the following: If this is actually done, I suggest putting this "R" entry description immediately after the "M" entry. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH RFC 1/2] MAINTAINERS: Add "R:" designated-reviewers tag
From: "Paul E. McKenney" A ksummit-discuss email thread looked at the difficulty recruiting and retaining reviewers. Paul Walmsley also noted the need for patch submitters to know who the key reviewers are and suggested adding an "R:" tag to the MAINTAINERS file to record this information on a per-subsystem basis. This commit does just that, and a subsequent commit tags the designated reviewer for the RCU-related subsystems. http://lists.linuxfoundation.org/pipermail/ksummit-discuss/2014-May/000830.html Suggested-by: Paul Walmsley Signed-off-by: Paul E. McKenney Reviewed-by: Josh Triplett --- MAINTAINERS | 2 ++ 1 file changed, 2 insertions(+) diff --git a/MAINTAINERS b/MAINTAINERS index 6dc67b1fdb50..57d681d1d7d1 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -73,6 +73,8 @@ Descriptions of section entries: L: Mailing list that is relevant to this area W: Web-page with status/info Q: Patchwork web based patch tracking system site + R: Designated reviewer, who should be CCed on patches, + format: FullName T: SCM tree type and location. Type is one of: git, hg, quilt, stgit, topgit S: Status, one of the following: -- 1.8.1.5 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH RFC 1/2] MAINTAINERS: Add R: designated-reviewers tag
From: Paul E. McKenney paul...@linux.vnet.ibm.com A ksummit-discuss email thread looked at the difficulty recruiting and retaining reviewers. Paul Walmsley also noted the need for patch submitters to know who the key reviewers are and suggested adding an R: tag to the MAINTAINERS file to record this information on a per-subsystem basis. This commit does just that, and a subsequent commit tags the designated reviewer for the RCU-related subsystems. http://lists.linuxfoundation.org/pipermail/ksummit-discuss/2014-May/000830.html Suggested-by: Paul Walmsley p...@pwsan.com Signed-off-by: Paul E. McKenney paul...@linux.vnet.ibm.com Reviewed-by: Josh Triplett j...@joshtriplett.org --- MAINTAINERS | 2 ++ 1 file changed, 2 insertions(+) diff --git a/MAINTAINERS b/MAINTAINERS index 6dc67b1fdb50..57d681d1d7d1 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -73,6 +73,8 @@ Descriptions of section entries: L: Mailing list that is relevant to this area W: Web-page with status/info Q: Patchwork web based patch tracking system site + R: Designated reviewer, who should be CCed on patches, + format: FullName address@domain T: SCM tree type and location. Type is one of: git, hg, quilt, stgit, topgit S: Status, one of the following: -- 1.8.1.5 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@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: [PATCH RFC 1/2] MAINTAINERS: Add R: designated-reviewers tag
On Mon, 2014-06-02 at 10:00 -0700, Paul E. McKenney wrote: A ksummit-discuss email thread looked at the difficulty recruiting and retaining reviewers. [] Paul Walmsley also noted the need for patch submitters to know who the key reviewers are and suggested adding an R: tag to the MAINTAINERS file to record this information on a per-subsystem basis. I'm not sure of the value of this. Why not just mark the actual reviewers as maintainers? The lack of reviewers problem is entirely distinct from the proposed solution. I believe active reviewers will generally subscribe to a subsystem specific mailing list. Perhaps it'd be better to get a linux-...@vger.kernel.org mailing list going. diff --git a/MAINTAINERS b/MAINTAINERS [] @@ -73,6 +73,8 @@ Descriptions of section entries: L: Mailing list that is relevant to this area W: Web-page with status/info Q: Patchwork web based patch tracking system site + R: Designated reviewer, who should be CCed on patches, +format: FullName address@domain T: SCM tree type and location. Type is one of: git, hg, quilt, stgit, topgit S: Status, one of the following: If this is actually done, I suggest putting this R entry description immediately after the M entry. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@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: [PATCH RFC 1/2] MAINTAINERS: Add R: designated-reviewers tag
On Mon, 02 Jun 2014 10:22:58 -0700 Joe Perches j...@perches.com wrote: Paul Walmsley also noted the need for patch submitters to know who the key reviewers are and suggested adding an R: tag to the MAINTAINERS file to record this information on a per-subsystem basis. I'm not sure of the value of this. Why not just mark the actual reviewers as maintainers? The lack of reviewers problem is entirely distinct from the proposed solution. I believe active reviewers will generally subscribe to a subsystem specific mailing list. Perhaps it'd be better to get a linux-...@vger.kernel.org mailing list going. I don't think we need a linux-rcu mailing list. I much rather see the patches on LKML. If you have a person that regularly reviews a patch and wants to be Cc'd on all of them, then why not add them to MAINTAINERS? diff --git a/MAINTAINERS b/MAINTAINERS [] @@ -73,6 +73,8 @@ Descriptions of section entries: L: Mailing list that is relevant to this area W: Web-page with status/info Q: Patchwork web based patch tracking system site + R: Designated reviewer, who should be CCed on patches, + format: FullName address@domain T: SCM tree type and location. Type is one of: git, hg, quilt, stgit, topgit S: Status, one of the following: If this is actually done, I suggest putting this R entry description immediately after the M entry. Or perhaps after L: -- Steve -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@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: [PATCH RFC 1/2] MAINTAINERS: Add R: designated-reviewers tag
On Mon, 2014-06-02 at 13:29 -0400, Steven Rostedt wrote: On Mon, 02 Jun 2014 10:22:58 -0700 Joe Perches j...@perches.com wrote: Paul Walmsley also noted the need for patch submitters to know who the key reviewers are and suggested adding an R: tag to the MAINTAINERS file to record this information on a per-subsystem basis. [] Why not just mark the actual reviewers as maintainers? [] If you have a person that regularly reviews a patch and wants to be Cc'd on all of them, then why not add them to MAINTAINERS? So we agree on that. diff --git a/MAINTAINERS b/MAINTAINERS [] @@ -73,6 +73,8 @@ Descriptions of section entries: L: Mailing list that is relevant to this area W: Web-page with status/info Q: Patchwork web based patch tracking system site + R: Designated reviewer, who should be CCed on patches, +format: FullName address@domain T: SCM tree type and location. Type is one of: git, hg, quilt, stgit, topgit S: Status, one of the following: If this is actually done, I suggest putting this R entry description immediately after the M entry. [] Or perhaps after L: Either would be fine as long as the order is the same in the actual subsystem section entries that follow. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@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: [PATCH RFC 1/2] MAINTAINERS: Add R: designated-reviewers tag
On Mon, Jun 02, 2014 at 10:22:58AM -0700, Joe Perches wrote: On Mon, 2014-06-02 at 10:00 -0700, Paul E. McKenney wrote: A ksummit-discuss email thread looked at the difficulty recruiting and retaining reviewers. [] Paul Walmsley also noted the need for patch submitters to know who the key reviewers are and suggested adding an R: tag to the MAINTAINERS file to record this information on a per-subsystem basis. I'm not sure of the value of this. Why not just mark the actual reviewers as maintainers? As discussed in the kernel summit discussion, being a regular patch reviewer isn't the same thing as being *the* maintainer. The lack of reviewers problem is entirely distinct from the proposed solution. Not arguing with that; this change alone won't produce more reviewers, but it does provide a means of tracking regular reviewers. I believe active reviewers will generally subscribe to a subsystem specific mailing list. Not every driver or subsystem is large enough to have its own mailing list, and many would prefer to keep their discussion on LKML. Also, mailing lists introduce diffusion of responsibility. Perhaps it'd be better to get a linux-...@vger.kernel.org mailing list going. Perhaps. diff --git a/MAINTAINERS b/MAINTAINERS [] @@ -73,6 +73,8 @@ Descriptions of section entries: L: Mailing list that is relevant to this area W: Web-page with status/info Q: Patchwork web based patch tracking system site + R: Designated reviewer, who should be CCed on patches, + format: FullName address@domain T: SCM tree type and location. Type is one of: git, hg, quilt, stgit, topgit S: Status, one of the following: If this is actually done, I suggest putting this R entry description immediately after the M entry. Yeah, it does seem like these entries are not quite alphabetized, so putting this right after M makes sense. - Josh Triplett -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@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: [PATCH RFC 1/2] MAINTAINERS: Add R: designated-reviewers tag
On Mon, 2014-06-02 at 10:48 -0700, j...@joshtriplett.org wrote: On Mon, Jun 02, 2014 at 10:22:58AM -0700, Joe Perches wrote: On Mon, 2014-06-02 at 10:00 -0700, Paul E. McKenney wrote: A ksummit-discuss email thread looked at the difficulty recruiting and retaining reviewers. [] Paul Walmsley also noted the need for patch submitters to know who the key reviewers are and suggested adding an R: tag to the MAINTAINERS file to record this information on a per-subsystem basis. I'm not sure of the value of this. Why not just mark the actual reviewers as maintainers? As discussed in the kernel summit discussion, being a regular patch reviewer isn't the same thing as being *the* maintainer. I think it's not particularly important or valuable here to make that distinction. What real difference does it make? -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@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: [PATCH RFC 1/2] MAINTAINERS: Add R: designated-reviewers tag
On Mon, Jun 2, 2014 at 1:59 PM, Joe Perches j...@perches.com wrote: On Mon, 2014-06-02 at 10:48 -0700, j...@joshtriplett.org wrote: On Mon, Jun 02, 2014 at 10:22:58AM -0700, Joe Perches wrote: On Mon, 2014-06-02 at 10:00 -0700, Paul E. McKenney wrote: A ksummit-discuss email thread looked at the difficulty recruiting and retaining reviewers. [] Paul Walmsley also noted the need for patch submitters to know who the key reviewers are and suggested adding an R: tag to the MAINTAINERS file to record this information on a per-subsystem basis. I'm not sure of the value of this. Why not just mark the actual reviewers as maintainers? As discussed in the kernel summit discussion, being a regular patch reviewer isn't the same thing as being *the* maintainer. I think it's not particularly important or valuable here to make that distinction. What real difference does it make? It depends. If the Maintainer moves to a model where patches must be reviewed before they are added to the tree, then having a designated reviewer helps. It gives the patch submitter another person to include, and if the Reviewer acks a patch, they know it's much more likely to make it in-tree. If the tree isn't managed that way, then Reviewer/Maintainer is a bit less distinctive, but it still provides at least some indication that a maintainer looked at the patch instead of having it just sit on the list. josh -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@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: [PATCH RFC 1/2] MAINTAINERS: Add R: designated-reviewers tag
On Mon, 2014-06-02 at 14:12 -0400, Josh Boyer wrote: On Mon, Jun 2, 2014 at 1:59 PM, Joe Perches j...@perches.com wrote: On Mon, 2014-06-02 at 10:48 -0700, j...@joshtriplett.org wrote: On Mon, Jun 02, 2014 at 10:22:58AM -0700, Joe Perches wrote: On Mon, 2014-06-02 at 10:00 -0700, Paul E. McKenney wrote: A ksummit-discuss email thread looked at the difficulty recruiting and retaining reviewers. [] Paul Walmsley also noted the need for patch submitters to know who the key reviewers are and suggested adding an R: tag to the MAINTAINERS file to record this information on a per-subsystem basis. I'm not sure of the value of this. Why not just mark the actual reviewers as maintainers? As discussed in the kernel summit discussion, being a regular patch reviewer isn't the same thing as being *the* maintainer. I think it's not particularly important or valuable here to make that distinction. What real difference does it make? It depends. If the Maintainer moves to a model where patches must be reviewed before they are added to the tree, then having a designated reviewer helps. It gives the patch submitter another person to include, and if the Reviewer acks a patch, they know it's much more likely to make it in-tree. If the tree isn't managed that way, then Reviewer/Maintainer is a bit less distinctive, but it still provides at least some indication that a maintainer looked at the patch instead of having it just sit on the list. So effectively, nothing. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@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: [PATCH RFC 1/2] MAINTAINERS: Add R: designated-reviewers tag
On Mon, Jun 02, 2014 at 10:59:28AM -0700, Joe Perches wrote: On Mon, 2014-06-02 at 10:48 -0700, j...@joshtriplett.org wrote: On Mon, Jun 02, 2014 at 10:22:58AM -0700, Joe Perches wrote: On Mon, 2014-06-02 at 10:00 -0700, Paul E. McKenney wrote: A ksummit-discuss email thread looked at the difficulty recruiting and retaining reviewers. [] Paul Walmsley also noted the need for patch submitters to know who the key reviewers are and suggested adding an R: tag to the MAINTAINERS file to record this information on a per-subsystem basis. I'm not sure of the value of this. Why not just mark the actual reviewers as maintainers? As discussed in the kernel summit discussion, being a regular patch reviewer isn't the same thing as being *the* maintainer. I think it's not particularly important or valuable here to make that distinction. What real difference does it make? In the particular case of Josh, none, at least from my viewpoint. He of course might or might not want to take on additional maintainership responsibility at this particular point in time, in which case, I would be more than happy to have him as a designated maintainer. But there have been people who have found serious issues in RCU patches who I would not trust as full maintainers. The ability to find defects is valuable in and of itself, and should be recognized as such, even when not accompanied by the rest of the maintainership package. Thanx, Paul -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@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: [PATCH RFC 1/2] MAINTAINERS: Add R: designated-reviewers tag
On Mon, 2014-06-02 at 11:16 -0700, Paul E. McKenney wrote: But there have been people who have found serious issues in RCU patches who I would not trust as full maintainers. The ability to find defects is valuable in and of itself, and should be recognized as such, even when not accompanied by the rest of the maintainership package. Maybe, but odd-lot reviewers are most likely going to find these same defects regardless of any R designation in MAINTAINERS. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@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: [PATCH RFC 1/2] MAINTAINERS: Add R: designated-reviewers tag
On Mon, 02 Jun 2014 11:44:29 -0700 Joe Perches j...@perches.com wrote: On Mon, 2014-06-02 at 11:16 -0700, Paul E. McKenney wrote: But there have been people who have found serious issues in RCU patches who I would not trust as full maintainers. The ability to find defects is valuable in and of itself, and should be recognized as such, even when not accompanied by the rest of the maintainership package. Maybe, but odd-lot reviewers are most likely going to find these same defects regardless of any R designation in MAINTAINERS. Actually, I'm thinking the R: tag is a good idea and we should have people ask to be added to MAINTAINERS if they want to review certain subsystems. Grant you, it should be people that the maintainers trust. I can think of several people I would like to be added as R: in tracing. The point is, when patches go out, it is easy to see who the Cc list should be. And perhaps this will get patches reviewed more. Maybe maintainers of other subsystems should ask to have the R: tag added for something they don't maintain but want to help out in. I may add myself to the x86, scheduler, time keeping and perhaps even RCU, as I like to read those patches. I'm not at the level of maintaining those subsystems, but I feel comfortable enough to review any changes there. -- Steve -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@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: [PATCH RFC 1/2] MAINTAINERS: Add R: designated-reviewers tag
On Mon, Jun 02, 2014 at 02:50:20PM -0400, Steven Rostedt wrote: On Mon, 02 Jun 2014 11:44:29 -0700 Joe Perches j...@perches.com wrote: On Mon, 2014-06-02 at 11:16 -0700, Paul E. McKenney wrote: But there have been people who have found serious issues in RCU patches who I would not trust as full maintainers. The ability to find defects is valuable in and of itself, and should be recognized as such, even when not accompanied by the rest of the maintainership package. Maybe, but odd-lot reviewers are most likely going to find these same defects regardless of any R designation in MAINTAINERS. Actually, I'm thinking the R: tag is a good idea and we should have people ask to be added to MAINTAINERS if they want to review certain subsystems. Grant you, it should be people that the maintainers trust. I can think of several people I would like to be added as R: in tracing. The point is, when patches go out, it is easy to see who the Cc list should be. And perhaps this will get patches reviewed more. Maybe maintainers of other subsystems should ask to have the R: tag added for something they don't maintain but want to help out in. That's exactly the idea: this should go along with a change to get_maintainer.pl to add those folks to the CC list. - Josh Triplett -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@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: [PATCH RFC 1/2] MAINTAINERS: Add R: designated-reviewers tag
On Mon, Jun 02, 2014 at 11:16:58AM -0700, Paul E. McKenney wrote: On Mon, Jun 02, 2014 at 10:59:28AM -0700, Joe Perches wrote: On Mon, 2014-06-02 at 10:48 -0700, j...@joshtriplett.org wrote: On Mon, Jun 02, 2014 at 10:22:58AM -0700, Joe Perches wrote: On Mon, 2014-06-02 at 10:00 -0700, Paul E. McKenney wrote: A ksummit-discuss email thread looked at the difficulty recruiting and retaining reviewers. [] Paul Walmsley also noted the need for patch submitters to know who the key reviewers are and suggested adding an R: tag to the MAINTAINERS file to record this information on a per-subsystem basis. I'm not sure of the value of this. Why not just mark the actual reviewers as maintainers? As discussed in the kernel summit discussion, being a regular patch reviewer isn't the same thing as being *the* maintainer. I think it's not particularly important or valuable here to make that distinction. What real difference does it make? In the particular case of Josh, none, at least from my viewpoint. He of course might or might not want to take on additional maintainership responsibility at this particular point in time, in which case, I would be more than happy to have him as a designated maintainer. For the record, I'd be happy to be listed as a co-maintainer for RCU. :) - Josh Triplett -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@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: [PATCH RFC 1/2] MAINTAINERS: Add R: designated-reviewers tag
On Mon, 2014-06-02 at 11:55 -0700, j...@joshtriplett.org wrote:
this should go along with a change to
get_maintainer.pl to add those folks to the CC list.
Something like this:
---
scripts/get_maintainer.pl | 22 +-
1 file changed, 21 insertions(+), 1 deletion(-)
diff --git a/scripts/get_maintainer.pl b/scripts/get_maintainer.pl
index 4198788..d701627 100755
--- a/scripts/get_maintainer.pl
+++ b/scripts/get_maintainer.pl
@@ -21,6 +21,7 @@ my $lk_path = ./;
my $email = 1;
my $email_usename = 1;
my $email_maintainer = 1;
+my $email_reviewer = 1;
my $email_list = 1;
my $email_subscriber_list = 0;
my $email_git_penguin_chiefs = 0;
@@ -202,6 +203,7 @@ if (!GetOptions(
'remove-duplicates!' = \$email_remove_duplicates,
'mailmap!' = \$email_use_mailmap,
'm!' = \$email_maintainer,
+ 'r!' = \$email_reviewer,
'n!' = \$email_usename,
'l!' = \$email_list,
's!' = \$email_subscriber_list,
@@ -260,7 +262,8 @@ if ($sections) {
}
if ($email
-($email_maintainer + $email_list + $email_subscriber_list +
+($email_maintainer + $email_reviewer +
+ $email_list + $email_subscriber_list +
$email_git + $email_git_penguin_chiefs + $email_git_blame) == 0) {
die $P: Please select at least 1 email option\n;
}
@@ -750,6 +753,7 @@ MAINTAINER field selection options:
--hg-since = hg history to use (default: $email_hg_since)
--interactive = display a menu (mostly useful if used with the --git
option)
--m = include maintainer(s) if any
+--r = include reviewer(s) if any
--n = include name 'Full Name addr\@domain.tld'
--l = include list(s) if any
--s = include subscriber only list(s) if any
@@ -1064,6 +1068,22 @@ sub add_categories {
my $role = get_maintainer_role($i);
push_email_addresses($pvalue, $role);
}
+ } elsif ($ptype eq R) {
+ my ($name, $address) = parse_email($pvalue);
+ if ($name eq ) {
+ if ($i 0) {
+ my $tv = $typevalue[$i - 1];
+ if ($tv =~ m/^(\C):\s*(.*)/) {
+ if ($1 eq P) {
+ $name = $2;
+ $pvalue = format_email($name, $address,
$email_usename);
+ }
+ }
+ }
+ }
+ if ($email_reviewer) {
+ push_email_addresses($pvalue, 'reviewer');
+ }
} elsif ($ptype eq T) {
push(@scm, $pvalue);
} elsif ($ptype eq W) {
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@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: [PATCH RFC 1/2] MAINTAINERS: Add R: designated-reviewers tag
On Mon, Jun 02, 2014 at 02:50:20PM -0400, Steven Rostedt wrote: On Mon, 02 Jun 2014 11:44:29 -0700 Joe Perches j...@perches.com wrote: On Mon, 2014-06-02 at 11:16 -0700, Paul E. McKenney wrote: But there have been people who have found serious issues in RCU patches who I would not trust as full maintainers. The ability to find defects is valuable in and of itself, and should be recognized as such, even when not accompanied by the rest of the maintainership package. Maybe, but odd-lot reviewers are most likely going to find these same defects regardless of any R designation in MAINTAINERS. Actually, I'm thinking the R: tag is a good idea and we should have people ask to be added to MAINTAINERS if they want to review certain subsystems. Grant you, it should be people that the maintainers trust. I can think of several people I would like to be added as R: in tracing. The point is, when patches go out, it is easy to see who the Cc list should be. And perhaps this will get patches reviewed more. Maybe maintainers of other subsystems should ask to have the R: tag added for something they don't maintain but want to help out in. I may add myself to the x86, scheduler, time keeping and perhaps even RCU, as I like to read those patches. I'm not at the level of maintaining those subsystems, but I feel comfortable enough to review any changes there. I would be quite happy to add you as official reviewer for RCU. Thanx, Paul -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@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: [PATCH RFC 1/2] MAINTAINERS: Add R: designated-reviewers tag
On Mon, Jun 02, 2014 at 11:56:35AM -0700, j...@joshtriplett.org wrote: On Mon, Jun 02, 2014 at 11:16:58AM -0700, Paul E. McKenney wrote: On Mon, Jun 02, 2014 at 10:59:28AM -0700, Joe Perches wrote: On Mon, 2014-06-02 at 10:48 -0700, j...@joshtriplett.org wrote: On Mon, Jun 02, 2014 at 10:22:58AM -0700, Joe Perches wrote: On Mon, 2014-06-02 at 10:00 -0700, Paul E. McKenney wrote: A ksummit-discuss email thread looked at the difficulty recruiting and retaining reviewers. [] Paul Walmsley also noted the need for patch submitters to know who the key reviewers are and suggested adding an R: tag to the MAINTAINERS file to record this information on a per-subsystem basis. I'm not sure of the value of this. Why not just mark the actual reviewers as maintainers? As discussed in the kernel summit discussion, being a regular patch reviewer isn't the same thing as being *the* maintainer. I think it's not particularly important or valuable here to make that distinction. What real difference does it make? In the particular case of Josh, none, at least from my viewpoint. He of course might or might not want to take on additional maintainership responsibility at this particular point in time, in which case, I would be more than happy to have him as a designated maintainer. For the record, I'd be happy to be listed as a co-maintainer for RCU. :) I would be happy to put you down as maintainer and Steven down as official reviewer. ;-) Thanx, Paul -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@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: [PATCH RFC 1/2] MAINTAINERS: Add R: designated-reviewers tag
On Mon, Jun 02, 2014 at 12:05:17PM -0700, Joe Perches wrote:
On Mon, 2014-06-02 at 11:55 -0700, j...@joshtriplett.org wrote:
this should go along with a change to
get_maintainer.pl to add those folks to the CC list.
Something like this:
Yes, exactly. Given an appropriate commit message,
Reviewed-by: Josh Triplett j...@joshtriplett.org
---
scripts/get_maintainer.pl | 22 +-
1 file changed, 21 insertions(+), 1 deletion(-)
diff --git a/scripts/get_maintainer.pl b/scripts/get_maintainer.pl
index 4198788..d701627 100755
--- a/scripts/get_maintainer.pl
+++ b/scripts/get_maintainer.pl
@@ -21,6 +21,7 @@ my $lk_path = ./;
my $email = 1;
my $email_usename = 1;
my $email_maintainer = 1;
+my $email_reviewer = 1;
my $email_list = 1;
my $email_subscriber_list = 0;
my $email_git_penguin_chiefs = 0;
@@ -202,6 +203,7 @@ if (!GetOptions(
'remove-duplicates!' = \$email_remove_duplicates,
'mailmap!' = \$email_use_mailmap,
'm!' = \$email_maintainer,
+ 'r!' = \$email_reviewer,
'n!' = \$email_usename,
'l!' = \$email_list,
's!' = \$email_subscriber_list,
@@ -260,7 +262,8 @@ if ($sections) {
}
if ($email
-($email_maintainer + $email_list + $email_subscriber_list +
+($email_maintainer + $email_reviewer +
+ $email_list + $email_subscriber_list +
$email_git + $email_git_penguin_chiefs + $email_git_blame) == 0) {
die $P: Please select at least 1 email option\n;
}
@@ -750,6 +753,7 @@ MAINTAINER field selection options:
--hg-since = hg history to use (default: $email_hg_since)
--interactive = display a menu (mostly useful if used with the --git
option)
--m = include maintainer(s) if any
+--r = include reviewer(s) if any
--n = include name 'Full Name addr\@domain.tld'
--l = include list(s) if any
--s = include subscriber only list(s) if any
@@ -1064,6 +1068,22 @@ sub add_categories {
my $role = get_maintainer_role($i);
push_email_addresses($pvalue, $role);
}
+ } elsif ($ptype eq R) {
+ my ($name, $address) = parse_email($pvalue);
+ if ($name eq ) {
+ if ($i 0) {
+ my $tv = $typevalue[$i - 1];
+ if ($tv =~ m/^(\C):\s*(.*)/) {
+ if ($1 eq P) {
+ $name = $2;
+ $pvalue = format_email($name, $address,
$email_usename);
+ }
+ }
+ }
+ }
+ if ($email_reviewer) {
+ push_email_addresses($pvalue, 'reviewer');
+ }
} elsif ($ptype eq T) {
push(@scm, $pvalue);
} elsif ($ptype eq W) {
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@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: [PATCH RFC 1/2] MAINTAINERS: Add R: designated-reviewers tag
On Mon, Jun 02, 2014 at 12:08:21PM -0700, Paul E. McKenney wrote: On Mon, Jun 02, 2014 at 11:56:35AM -0700, j...@joshtriplett.org wrote: On Mon, Jun 02, 2014 at 11:16:58AM -0700, Paul E. McKenney wrote: On Mon, Jun 02, 2014 at 10:59:28AM -0700, Joe Perches wrote: On Mon, 2014-06-02 at 10:48 -0700, j...@joshtriplett.org wrote: On Mon, Jun 02, 2014 at 10:22:58AM -0700, Joe Perches wrote: On Mon, 2014-06-02 at 10:00 -0700, Paul E. McKenney wrote: A ksummit-discuss email thread looked at the difficulty recruiting and retaining reviewers. [] Paul Walmsley also noted the need for patch submitters to know who the key reviewers are and suggested adding an R: tag to the MAINTAINERS file to record this information on a per-subsystem basis. I'm not sure of the value of this. Why not just mark the actual reviewers as maintainers? As discussed in the kernel summit discussion, being a regular patch reviewer isn't the same thing as being *the* maintainer. I think it's not particularly important or valuable here to make that distinction. What real difference does it make? In the particular case of Josh, none, at least from my viewpoint. He of course might or might not want to take on additional maintainership responsibility at this particular point in time, in which case, I would be more than happy to have him as a designated maintainer. For the record, I'd be happy to be listed as a co-maintainer for RCU. :) I would be happy to put you down as maintainer and Steven down as official reviewer. ;-) I'd suggest adding Mathieu Desnoyers, Oleg Nesterov, and Lai Jiangshan as reviewers as well, with their consent. - Josh Triplett -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@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: [PATCH RFC 1/2] MAINTAINERS: Add R: designated-reviewers tag
On Mon, 2014-06-02 at 12:09 -0700, j...@joshtriplett.org wrote: On Mon, Jun 02, 2014 at 12:05:17PM -0700, Joe Perches wrote: On Mon, 2014-06-02 at 11:55 -0700, j...@joshtriplett.org wrote: this should go along with a change to get_maintainer.pl to add those folks to the CC list. Something like this: Yes, exactly. Given an appropriate commit message, Reviewed-by: Josh Triplett j...@joshtriplett.org That's the sort of patch where reviewing is pretty useless. What it needs is testing, not reviewing. I tested it for all of 10 seconds. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@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: [PATCH RFC 1/2] MAINTAINERS: Add R: designated-reviewers tag
On Mon, Jun 02, 2014 at 12:05:17PM -0700, Joe Perches wrote:
On Mon, 2014-06-02 at 11:55 -0700, j...@joshtriplett.org wrote:
this should go along with a change to
get_maintainer.pl to add those folks to the CC list.
Something like this:
To test this, I added a comment to kernel/rcu/srcu.c, then ran
scripts/get_maintainer.pl on the resulting diffs. Without the
below change to scripts/get_maintainer.pl, I get the following:
Lai Jiangshan la...@cn.fujitsu.com (supporter:SLEEPABLE READ-CO...)
Paul E. McKenney paul...@linux.vnet.ibm.com (supporter:SLEEPABLE READ-CO...)
Dipankar Sarma dipan...@in.ibm.com (supporter:READ-COPY UPDATE...)
linux-kernel@vger.kernel.org (open list:SLEEPABLE READ-CO...)
With the below change, it includes Josh, as expected based on the R:
entry I had previously added to MAINTAINERS in my local tree:
Lai Jiangshan la...@cn.fujitsu.com (supporter:SLEEPABLE READ-CO...)
Paul E. McKenney paul...@linux.vnet.ibm.com (supporter:SLEEPABLE READ-CO...)
Josh Triplett j...@joshtriplett.org (reviewer)
Dipankar Sarma dipan...@in.ibm.com (supporter:READ-COPY UPDATE...)
linux-kernel@vger.kernel.org (open list:SLEEPABLE READ-CO...)
So:
Tested-by: Paul E. McKenney paul...@linux.vnet.ibm.com
---
scripts/get_maintainer.pl | 22 +-
1 file changed, 21 insertions(+), 1 deletion(-)
diff --git a/scripts/get_maintainer.pl b/scripts/get_maintainer.pl
index 4198788..d701627 100755
--- a/scripts/get_maintainer.pl
+++ b/scripts/get_maintainer.pl
@@ -21,6 +21,7 @@ my $lk_path = ./;
my $email = 1;
my $email_usename = 1;
my $email_maintainer = 1;
+my $email_reviewer = 1;
my $email_list = 1;
my $email_subscriber_list = 0;
my $email_git_penguin_chiefs = 0;
@@ -202,6 +203,7 @@ if (!GetOptions(
'remove-duplicates!' = \$email_remove_duplicates,
'mailmap!' = \$email_use_mailmap,
'm!' = \$email_maintainer,
+ 'r!' = \$email_reviewer,
'n!' = \$email_usename,
'l!' = \$email_list,
's!' = \$email_subscriber_list,
@@ -260,7 +262,8 @@ if ($sections) {
}
if ($email
-($email_maintainer + $email_list + $email_subscriber_list +
+($email_maintainer + $email_reviewer +
+ $email_list + $email_subscriber_list +
$email_git + $email_git_penguin_chiefs + $email_git_blame) == 0) {
die $P: Please select at least 1 email option\n;
}
@@ -750,6 +753,7 @@ MAINTAINER field selection options:
--hg-since = hg history to use (default: $email_hg_since)
--interactive = display a menu (mostly useful if used with the --git
option)
--m = include maintainer(s) if any
+--r = include reviewer(s) if any
--n = include name 'Full Name addr\@domain.tld'
--l = include list(s) if any
--s = include subscriber only list(s) if any
@@ -1064,6 +1068,22 @@ sub add_categories {
my $role = get_maintainer_role($i);
push_email_addresses($pvalue, $role);
}
+ } elsif ($ptype eq R) {
+ my ($name, $address) = parse_email($pvalue);
+ if ($name eq ) {
+ if ($i 0) {
+ my $tv = $typevalue[$i - 1];
+ if ($tv =~ m/^(\C):\s*(.*)/) {
+ if ($1 eq P) {
+ $name = $2;
+ $pvalue = format_email($name, $address,
$email_usename);
+ }
+ }
+ }
+ }
+ if ($email_reviewer) {
+ push_email_addresses($pvalue, 'reviewer');
+ }
} elsif ($ptype eq T) {
push(@scm, $pvalue);
} elsif ($ptype eq W) {
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@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: [PATCH RFC 1/2] MAINTAINERS: Add R: designated-reviewers tag
On Mon, Jun 02, 2014 at 12:11:55PM -0700, j...@joshtriplett.org wrote: On Mon, Jun 02, 2014 at 12:08:21PM -0700, Paul E. McKenney wrote: On Mon, Jun 02, 2014 at 11:56:35AM -0700, j...@joshtriplett.org wrote: On Mon, Jun 02, 2014 at 11:16:58AM -0700, Paul E. McKenney wrote: On Mon, Jun 02, 2014 at 10:59:28AM -0700, Joe Perches wrote: On Mon, 2014-06-02 at 10:48 -0700, j...@joshtriplett.org wrote: On Mon, Jun 02, 2014 at 10:22:58AM -0700, Joe Perches wrote: On Mon, 2014-06-02 at 10:00 -0700, Paul E. McKenney wrote: A ksummit-discuss email thread looked at the difficulty recruiting and retaining reviewers. [] Paul Walmsley also noted the need for patch submitters to know who the key reviewers are and suggested adding an R: tag to the MAINTAINERS file to record this information on a per-subsystem basis. I'm not sure of the value of this. Why not just mark the actual reviewers as maintainers? As discussed in the kernel summit discussion, being a regular patch reviewer isn't the same thing as being *the* maintainer. I think it's not particularly important or valuable here to make that distinction. What real difference does it make? In the particular case of Josh, none, at least from my viewpoint. He of course might or might not want to take on additional maintainership responsibility at this particular point in time, in which case, I would be more than happy to have him as a designated maintainer. For the record, I'd be happy to be listed as a co-maintainer for RCU. :) I would be happy to put you down as maintainer and Steven down as official reviewer. ;-) I'd suggest adding Mathieu Desnoyers, Oleg Nesterov, and Lai Jiangshan as reviewers as well, with their consent. Mathieu, Oleg, Lai, any objections? Thanx, Paul -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@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: [PATCH RFC 1/2] MAINTAINERS: Add R: designated-reviewers tag
On Mon, 2014-06-02 at 12:27 -0700, Paul E. McKenney wrote: On Mon, Jun 02, 2014 at 12:11:55PM -0700, j...@joshtriplett.org wrote: I'd suggest adding Mathieu Desnoyers, Oleg Nesterov, and Lai Jiangshan as reviewers as well, with their consent. Mathieu, Oleg, Lai, any objections? I suggest not going overboard with R entries. If there are more than 1 or 2, create a mailing list. And more entries will just make the churn and name-decay in MAINTAINERS that much more prevalent. None of those people have any significant quantities of commit entries for rcu (with the get_maintainer patch posted) $ ./scripts/get_maintainer.pl -f kernel/rcu/ --git --git-min-percent=3 Dipankar Sarma dipan...@in.ibm.com (supporter:READ-COPY UPDATE...) Paul E. McKenney paul...@linux.vnet.ibm.com (supporter:READ-COPY UPDATE...,commit_signer:82/85=96%,authored:58/85=68%) Josh Triplett j...@joshtriplett.org (reviewer,commit_signer:56/85=66%) Ingo Molnar mi...@kernel.org (commit_signer:5/85=6%) Peter Zijlstra pet...@infradead.org (commit_signer:5/85=6%) Thomas Gleixner t...@linutronix.de (commit_signer:3/85=4%) Andreea-Cristina Bernat bernat@gmail.com (authored:3/85=4%) linux-kernel@vger.kernel.org (open list:READ-COPY UPDATE...) -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@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: [PATCH RFC 1/2] MAINTAINERS: Add R: designated-reviewers tag
On 06/02/2014 12:36 PM, Joe Perches wrote: On Mon, 2014-06-02 at 12:27 -0700, Paul E. McKenney wrote: On Mon, Jun 02, 2014 at 12:11:55PM -0700, j...@joshtriplett.org wrote: I'd suggest adding Mathieu Desnoyers, Oleg Nesterov, and Lai Jiangshan as reviewers as well, with their consent. Mathieu, Oleg, Lai, any objections? I suggest not going overboard with R entries. If there are more than 1 or 2, create a mailing list. Yes, much better on the patch sender(s) to Cc: 1 or 2 people or lists than to have to copy/paste 5 or 6 email addresses. And more entries will just make the churn and name-decay in MAINTAINERS that much more prevalent. None of those people have any significant quantities of commit entries for rcu -- ~Randy -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@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: [PATCH RFC 1/2] MAINTAINERS: Add R: designated-reviewers tag
On Mon, Jun 02, 2014 at 12:36:22PM -0700, Joe Perches wrote: On Mon, 2014-06-02 at 12:27 -0700, Paul E. McKenney wrote: On Mon, Jun 02, 2014 at 12:11:55PM -0700, j...@joshtriplett.org wrote: I'd suggest adding Mathieu Desnoyers, Oleg Nesterov, and Lai Jiangshan as reviewers as well, with their consent. Mathieu, Oleg, Lai, any objections? I suggest not going overboard with R entries. If there are more than 1 or 2, create a mailing list. Having a limit of two seems reasonable. First to the post, then. Or maybe last to object? ;-) And more entries will just make the churn and name-decay in MAINTAINERS that much more prevalent. None of those people have any significant quantities of commit entries for rcu (with the get_maintainer patch posted) $ ./scripts/get_maintainer.pl -f kernel/rcu/ --git --git-min-percent=3 Dipankar Sarma dipan...@in.ibm.com (supporter:READ-COPY UPDATE...) Paul E. McKenney paul...@linux.vnet.ibm.com (supporter:READ-COPY UPDATE...,commit_signer:82/85=96%,authored:58/85=68%) Josh Triplett j...@joshtriplett.org (reviewer,commit_signer:56/85=66%) Ingo Molnar mi...@kernel.org (commit_signer:5/85=6%) Peter Zijlstra pet...@infradead.org (commit_signer:5/85=6%) Thomas Gleixner t...@linutronix.de (commit_signer:3/85=4%) Andreea-Cristina Bernat bernat@gmail.com (authored:3/85=4%) linux-kernel@vger.kernel.org (open list:READ-COPY UPDATE...) Good to know, but then again, I sometimes review patches in areas where I have no commits. Thanx, Paul -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@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: [PATCH RFC 1/2] MAINTAINERS: Add R: designated-reviewers tag
On Mon, 2014-06-02 at 12:50 -0700, Paul E. McKenney wrote: I sometimes review patches in areas where I have no commits. Lots of people review patches all over the tree. That doesn't mean they want to be or should be cc'd. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@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: [PATCH RFC 1/2] MAINTAINERS: Add R: designated-reviewers tag
On Mon, Jun 02, 2014 at 12:55:50PM -0700, Joe Perches wrote: On Mon, 2014-06-02 at 12:50 -0700, Paul E. McKenney wrote: I sometimes review patches in areas where I have no commits. Lots of people review patches all over the tree. That doesn't mean they want to be or should be cc'd. True enough. And we probably need to rely on the maintainer's judgment for this. Thanx, Paul -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@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: [PATCH RFC 1/2] MAINTAINERS: Add R: designated-reviewers tag
- Original Message - From: Paul E. McKenney paul...@linux.vnet.ibm.com To: j...@joshtriplett.org Cc: Joe Perches j...@perches.com, linux-kernel@vger.kernel.org, mi...@kernel.org, la...@cn.fujitsu.com, dipan...@in.ibm.com, a...@linux-foundation.org, mathieu desnoyers mathieu.desnoy...@efficios.com, n...@us.ibm.com, t...@linutronix.de, pet...@infradead.org, rost...@goodmis.org, dhowe...@redhat.com, eduma...@google.com, dvh...@linux.intel.com, fweis...@gmail.com, o...@redhat.com, s...@mit.edu Sent: Monday, June 2, 2014 3:27:29 PM Subject: Re: [PATCH RFC 1/2] MAINTAINERS: Add R: designated-reviewers tag On Mon, Jun 02, 2014 at 12:11:55PM -0700, j...@joshtriplett.org wrote: On Mon, Jun 02, 2014 at 12:08:21PM -0700, Paul E. McKenney wrote: On Mon, Jun 02, 2014 at 11:56:35AM -0700, j...@joshtriplett.org wrote: On Mon, Jun 02, 2014 at 11:16:58AM -0700, Paul E. McKenney wrote: On Mon, Jun 02, 2014 at 10:59:28AM -0700, Joe Perches wrote: On Mon, 2014-06-02 at 10:48 -0700, j...@joshtriplett.org wrote: On Mon, Jun 02, 2014 at 10:22:58AM -0700, Joe Perches wrote: On Mon, 2014-06-02 at 10:00 -0700, Paul E. McKenney wrote: A ksummit-discuss email thread looked at the difficulty recruiting and retaining reviewers. [] Paul Walmsley also noted the need for patch submitters to know who the key reviewers are and suggested adding an R: tag to the MAINTAINERS file to record this information on a per-subsystem basis. I'm not sure of the value of this. Why not just mark the actual reviewers as maintainers? As discussed in the kernel summit discussion, being a regular patch reviewer isn't the same thing as being *the* maintainer. I think it's not particularly important or valuable here to make that distinction. What real difference does it make? In the particular case of Josh, none, at least from my viewpoint. He of course might or might not want to take on additional maintainership responsibility at this particular point in time, in which case, I would be more than happy to have him as a designated maintainer. For the record, I'd be happy to be listed as a co-maintainer for RCU. :) I would be happy to put you down as maintainer and Steven down as official reviewer. ;-) I'd suggest adding Mathieu Desnoyers, Oleg Nesterov, and Lai Jiangshan as reviewers as well, with their consent. Mathieu, Oleg, Lai, any objections? No objection from me. I'm always glad to help out reviewing RCU patches whenever I have some cycles available. Thanks, Mathieu -- Mathieu Desnoyers EfficiOS Inc. http://www.efficios.com -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@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: [PATCH RFC 1/2] MAINTAINERS: Add R: designated-reviewers tag
On Mon, Jun 02, 2014 at 12:40:38PM -0700, Randy Dunlap wrote: On 06/02/2014 12:36 PM, Joe Perches wrote: On Mon, 2014-06-02 at 12:27 -0700, Paul E. McKenney wrote: On Mon, Jun 02, 2014 at 12:11:55PM -0700, j...@joshtriplett.org wrote: I'd suggest adding Mathieu Desnoyers, Oleg Nesterov, and Lai Jiangshan as reviewers as well, with their consent. Mathieu, Oleg, Lai, any objections? I suggest not going overboard with R entries. If there are more than 1 or 2, create a mailing list. Yes, much better on the patch sender(s) to Cc: 1 or 2 people or lists than to have to copy/paste 5 or 6 email addresses. A patch sender should never need to manually copy and paste email addresses, given get_maintainer.pl. - Josh Triplett -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@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: [PATCH RFC 1/2] MAINTAINERS: Add R: designated-reviewers tag
On Mon, Jun 02, 2014 at 12:26:31PM -0700, Paul E. McKenney wrote: On Mon, Jun 02, 2014 at 12:05:17PM -0700, Joe Perches wrote: On Mon, 2014-06-02 at 11:55 -0700, j...@joshtriplett.org wrote: this should go along with a change to get_maintainer.pl to add those folks to the CC list. Something like this: To test this, I added a comment to kernel/rcu/srcu.c, then ran scripts/get_maintainer.pl on the resulting diffs. Without the below change to scripts/get_maintainer.pl, I get the following: Lai Jiangshan la...@cn.fujitsu.com (supporter:SLEEPABLE READ-CO...) Paul E. McKenney paul...@linux.vnet.ibm.com (supporter:SLEEPABLE READ-CO...) Dipankar Sarma dipan...@in.ibm.com (supporter:READ-COPY UPDATE...) linux-kernel@vger.kernel.org (open list:SLEEPABLE READ-CO...) With the below change, it includes Josh, as expected based on the R: entry I had previously added to MAINTAINERS in my local tree: Lai Jiangshan la...@cn.fujitsu.com (supporter:SLEEPABLE READ-CO...) Paul E. McKenney paul...@linux.vnet.ibm.com (supporter:SLEEPABLE READ-CO...) Josh Triplett j...@joshtriplett.org (reviewer) Dipankar Sarma dipan...@in.ibm.com (supporter:READ-COPY UPDATE...) linux-kernel@vger.kernel.org (open list:SLEEPABLE READ-CO...) Paul, Sorry, I hadn't noticed it earlier. I can be dropped. Not that I am not interested, just that I am working on too many itsy bitsy things many outside the kernel and also herding cats, to find time :-) Thanks Dipankar -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@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: [PATCH RFC 1/2] MAINTAINERS: Add R: designated-reviewers tag
On Mon, Jun 02, 2014 at 12:17:46PM -0700, Joe Perches wrote: On Mon, 2014-06-02 at 12:09 -0700, j...@joshtriplett.org wrote: On Mon, Jun 02, 2014 at 12:05:17PM -0700, Joe Perches wrote: On Mon, 2014-06-02 at 11:55 -0700, j...@joshtriplett.org wrote: this should go along with a change to get_maintainer.pl to add those folks to the CC list. Something like this: Yes, exactly. Given an appropriate commit message, Reviewed-by: Josh Triplett j...@joshtriplett.org That's the sort of patch where reviewing is pretty useless. What it needs is testing, not reviewing. I tested it for all of 10 seconds. From Documentation/SubmittingPatches: (c) While there may be things that could be improved with this submission, I believe that it is, at this time, (1) a worthwhile modification to the kernel, and (2) free of known issues which would argue against its inclusion. . 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. So, for someone to say they have reviewed the code and are able to say it is free of known issues and has no remaining technical issues, they would have had to apply, compile and test the patch, yes? i.e. Reviewed-by implies both Acked-by, Tested-by and that the code is technically sound. Anyone using Reviewed-by without having actually applied and tested the patch is mis-using the tag - they should be using Acked-by: if all they have done is read the code in their mail program Cheers, Dave. -- Dave Chinner da...@fromorbit.com -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
