From: "Paul E. McKenney" <[email protected]>
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 <[email protected]>
Signed-off-by: Paul E. McKenney <[email protected]>
Reviewed-by: Josh Triplett <[email protected]>
---
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
From: "Paul E. McKenney" <[email protected]>
Signed-off-by: Paul E. McKenney <[email protected]>
Reviewed-by: Josh Triplett <[email protected]>
---
MAINTAINERS | 3 +++
1 file changed, 3 insertions(+)
diff --git a/MAINTAINERS b/MAINTAINERS
index 57d681d1d7d1..93054c4864d7 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -7321,6 +7321,7 @@ F: kernel/rcu/torture.c
RCUTORTURE TEST FRAMEWORK
M: "Paul E. McKenney" <[email protected]>
+R: Josh Triplett <[email protected]>
L: [email protected]
S: Supported
T: git git://git.kernel.org/pub/scm/linux/kernel/git/paulmck/linux-rcu.git
@@ -7345,6 +7346,7 @@ F: net/rds/
READ-COPY UPDATE (RCU)
M: Dipankar Sarma <[email protected]>
M: "Paul E. McKenney" <[email protected]>
+R: Josh Triplett <[email protected]>
L: [email protected]
W: http://www.rdrop.com/users/paulmck/RCU/
S: Supported
@@ -8098,6 +8100,7 @@ F: mm/sl?b.c
SLEEPABLE READ-COPY UPDATE (SRCU)
M: Lai Jiangshan <[email protected]>
M: "Paul E. McKenney" <[email protected]>
+R: Josh Triplett <[email protected]>
L: [email protected]
W: http://www.rdrop.com/users/paulmck/RCU/
S: Supported
--
1.8.1.5
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 "[email protected]"
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.
On Mon, 02 Jun 2014 10:22:58 -0700
Joe Perches <[email protected]> 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 "[email protected]"
> 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
On Mon, 2014-06-02 at 13:29 -0400, Steven Rostedt wrote:
> On Mon, 02 Jun 2014 10:22:58 -0700 Joe Perches <[email protected]> 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.
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 "[email protected]"
> 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
On Mon, 2014-06-02 at 10:48 -0700, [email protected] 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?
On Mon, Jun 2, 2014 at 1:59 PM, Joe Perches <[email protected]> wrote:
> On Mon, 2014-06-02 at 10:48 -0700, [email protected] 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
On Mon, 2014-06-02 at 14:12 -0400, Josh Boyer wrote:
> On Mon, Jun 2, 2014 at 1:59 PM, Joe Perches <[email protected]> wrote:
> > On Mon, 2014-06-02 at 10:48 -0700, [email protected] 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.
On Mon, Jun 02, 2014 at 10:59:28AM -0700, Joe Perches wrote:
> On Mon, 2014-06-02 at 10:48 -0700, [email protected] 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
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.
On Mon, 02 Jun 2014 11:44:29 -0700
Joe Perches <[email protected]> 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
On Mon, Jun 02, 2014 at 02:50:20PM -0400, Steven Rostedt wrote:
> On Mon, 02 Jun 2014 11:44:29 -0700
> Joe Perches <[email protected]> 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
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, [email protected] 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
On Mon, 2014-06-02 at 11:55 -0700, [email protected] 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") {
On Mon, Jun 02, 2014 at 02:50:20PM -0400, Steven Rostedt wrote:
> On Mon, 02 Jun 2014 11:44:29 -0700
> Joe Perches <[email protected]> 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
On Mon, Jun 02, 2014 at 11:56:35AM -0700, [email protected] 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, [email protected] 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
On Mon, Jun 02, 2014 at 12:05:17PM -0700, Joe Perches wrote:
> On Mon, 2014-06-02 at 11:55 -0700, [email protected] 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 <[email protected]>
> ---
> 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") {
>
>
>
On Mon, Jun 02, 2014 at 12:08:21PM -0700, Paul E. McKenney wrote:
> On Mon, Jun 02, 2014 at 11:56:35AM -0700, [email protected] 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, [email protected] 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
On Mon, 2014-06-02 at 12:09 -0700, [email protected] wrote:
> On Mon, Jun 02, 2014 at 12:05:17PM -0700, Joe Perches wrote:
> > On Mon, 2014-06-02 at 11:55 -0700, [email protected] 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 <[email protected]>
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.
On Mon, Jun 02, 2014 at 12:05:17PM -0700, Joe Perches wrote:
> On Mon, 2014-06-02 at 11:55 -0700, [email protected] 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 <[email protected]> (supporter:SLEEPABLE READ-CO...)
"Paul E. McKenney" <[email protected]> (supporter:SLEEPABLE READ-CO...)
Dipankar Sarma <[email protected]> (supporter:READ-COPY UPDATE...)
[email protected] (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 <[email protected]> (supporter:SLEEPABLE READ-CO...)
"Paul E. McKenney" <[email protected]> (supporter:SLEEPABLE READ-CO...)
Josh Triplett <[email protected]> (reviewer)
Dipankar Sarma <[email protected]> (supporter:READ-COPY UPDATE...)
[email protected] (open list:SLEEPABLE READ-CO...)
So:
Tested-by: Paul E. McKenney <[email protected]>
> ---
> 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") {
>
>
>
On Mon, Jun 02, 2014 at 12:11:55PM -0700, [email protected] 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, [email protected] 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, [email protected] 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
On Mon, 2014-06-02 at 12:27 -0700, Paul E. McKenney wrote:
> On Mon, Jun 02, 2014 at 12:11:55PM -0700, [email protected] 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 <[email protected]> (supporter:READ-COPY UPDATE...)
"Paul E. McKenney" <[email protected]> (supporter:READ-COPY UPDATE...,commit_signer:82/85=96%,authored:58/85=68%)
Josh Triplett <[email protected]> (reviewer,commit_signer:56/85=66%)
Ingo Molnar <[email protected]> (commit_signer:5/85=6%)
Peter Zijlstra <[email protected]> (commit_signer:5/85=6%)
Thomas Gleixner <[email protected]> (commit_signer:3/85=4%)
Andreea-Cristina Bernat <[email protected]> (authored:3/85=4%)
[email protected] (open list:READ-COPY UPDATE...)
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, [email protected] 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
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, [email protected] 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 <[email protected]> (supporter:READ-COPY UPDATE...)
> "Paul E. McKenney" <[email protected]> (supporter:READ-COPY UPDATE...,commit_signer:82/85=96%,authored:58/85=68%)
> Josh Triplett <[email protected]> (reviewer,commit_signer:56/85=66%)
> Ingo Molnar <[email protected]> (commit_signer:5/85=6%)
> Peter Zijlstra <[email protected]> (commit_signer:5/85=6%)
> Thomas Gleixner <[email protected]> (commit_signer:3/85=4%)
> Andreea-Cristina Bernat <[email protected]> (authored:3/85=4%)
> [email protected] (open list:READ-COPY UPDATE...)
Good to know, but then again, I sometimes review patches in areas where
I have no commits.
Thanx, Paul
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.
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
----- Original Message -----
> From: "Paul E. McKenney" <[email protected]>
> To: [email protected]
> Cc: "Joe Perches" <[email protected]>, [email protected], [email protected], [email protected],
> [email protected], [email protected], "mathieu desnoyers" <[email protected]>,
> [email protected], [email protected], [email protected], [email protected], [email protected],
> [email protected], [email protected], [email protected], [email protected], [email protected]
> 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, [email protected] 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, [email protected] 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, [email protected] 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
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, [email protected] 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
On Mon, 2 Jun 2014 10:00:20 -0700 "Paul E. McKenney" <[email protected]> wrote:
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -7321,6 +7321,7 @@ F: kernel/rcu/torture.c
>
> RCUTORTURE TEST FRAMEWORK
> M: "Paul E. McKenney" <[email protected]>
> +R: Josh Triplett <[email protected]>
> L: [email protected]
> S: Supported
> T: git git://git.kernel.org/pub/scm/linux/kernel/git/paulmck/linux-rcu.git
I like the general principle - knowing who to poke regarding a kernel
change is useful.
I don't care much whether it's "M:" or "R:", although "R:" carries more
meaning and hence is probably better.
But why not "Cc:"? That's meaningful too and is more copy-n-paste friendly.
On Mon, 2014-06-02 at 13:35 -0700, Andrew Morton wrote:
> On Mon, 2 Jun 2014 10:00:20 -0700 "Paul E. McKenney" <[email protected]> wrote:
>
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -7321,6 +7321,7 @@ F: kernel/rcu/torture.c
> >
> > RCUTORTURE TEST FRAMEWORK
> > M: "Paul E. McKenney" <[email protected]>
> > +R: Josh Triplett <[email protected]>
> > L: [email protected]
> > S: Supported
> > T: git git://git.kernel.org/pub/scm/linux/kernel/git/paulmck/linux-rcu.git
>
> I like the general principle - knowing who to poke regarding a kernel
> change is useful.
>
> I don't care much whether it's "M:" or "R:", although "R:" carries more
> meaning and hence is probably better.
>
> But why not "Cc:"? That's meaningful too and is more copy-n-paste friendly.
For one, it'd take an extra bit of rework to get_maintainer.
On 06/02/2014 01:36 PM, Joe Perches wrote:
> On Mon, 2014-06-02 at 13:35 -0700, Andrew Morton wrote:
>> On Mon, 2 Jun 2014 10:00:20 -0700 "Paul E. McKenney" <[email protected]> wrote:
>>
>>> --- a/MAINTAINERS
>>> +++ b/MAINTAINERS
>>> @@ -7321,6 +7321,7 @@ F: kernel/rcu/torture.c
>>>
>>> RCUTORTURE TEST FRAMEWORK
>>> M: "Paul E. McKenney" <[email protected]>
>>> +R: Josh Triplett <[email protected]>
>>> L: [email protected]
>>> S: Supported
>>> T: git git://git.kernel.org/pub/scm/linux/kernel/git/paulmck/linux-rcu.git
>>
>> I like the general principle - knowing who to poke regarding a kernel
>> change is useful.
>>
>> I don't care much whether it's "M:" or "R:", although "R:" carries more
>> meaning and hence is probably better.
>>
>> But why not "Cc:"? That's meaningful too and is more copy-n-paste friendly.
Josh, what are you assuming that Andrew and I did not?
--
~Randy
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, [email protected] 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 <[email protected]> (supporter:SLEEPABLE READ-CO...)
> "Paul E. McKenney" <[email protected]> (supporter:SLEEPABLE READ-CO...)
> Dipankar Sarma <[email protected]> (supporter:READ-COPY UPDATE...)
> [email protected] (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 <[email protected]> (supporter:SLEEPABLE READ-CO...)
> "Paul E. McKenney" <[email protected]> (supporter:SLEEPABLE READ-CO...)
> Josh Triplett <[email protected]> (reviewer)
> Dipankar Sarma <[email protected]> (supporter:READ-COPY UPDATE...)
> [email protected] (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
On Mon, Jun 02, 2014 at 12:17:46PM -0700, Joe Perches wrote:
> On Mon, 2014-06-02 at 12:09 -0700, [email protected] wrote:
> > On Mon, Jun 02, 2014 at 12:05:17PM -0700, Joe Perches wrote:
> > > On Mon, 2014-06-02 at 11:55 -0700, [email protected] 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 <[email protected]>
>
> 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
[email protected]
On Tue, 3 Jun 2014 09:19:49 +1000 Dave Chinner <[email protected]> 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....
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, [email protected] wrote:
> > > On Mon, Jun 02, 2014 at 12:05:17PM -0700, Joe Perches wrote:
> > > > On Mon, 2014-06-02 at 11:55 -0700, [email protected] 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 <[email protected]>
> >
> > 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
On Mon, Jun 02, 2014 at 01:38:56PM -0700, Randy Dunlap wrote:
> On 06/02/2014 01:36 PM, Joe Perches wrote:
> > On Mon, 2014-06-02 at 13:35 -0700, Andrew Morton wrote:
> >> On Mon, 2 Jun 2014 10:00:20 -0700 "Paul E. McKenney" <[email protected]> wrote:
> >>
> >>> --- a/MAINTAINERS
> >>> +++ b/MAINTAINERS
> >>> @@ -7321,6 +7321,7 @@ F: kernel/rcu/torture.c
> >>>
> >>> RCUTORTURE TEST FRAMEWORK
> >>> M: "Paul E. McKenney" <[email protected]>
> >>> +R: Josh Triplett <[email protected]>
> >>> L: [email protected]
> >>> S: Supported
> >>> T: git git://git.kernel.org/pub/scm/linux/kernel/git/paulmck/linux-rcu.git
> >>
> >> I like the general principle - knowing who to poke regarding a kernel
> >> change is useful.
> >>
> >> I don't care much whether it's "M:" or "R:", although "R:" carries more
> >> meaning and hence is probably better.
> >>
> >> But why not "Cc:"? That's meaningful too and is more copy-n-paste friendly.
>
> Josh, what are you assuming that Andrew and I did not?
Not sure what you mean here. Responding to the text you quoted: I have
no particular need to bikeshed the tag name, so if you prefer "Cc" and
can convince get_maintainer.pl to handle it, fine by me.
- Josh Triplett
On Mon, 2014-06-02 at 16:59 -0700, [email protected] 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.
On Mon, 2 Jun 2014 16:24:24 -0700
Andrew Morton <[email protected]> wrote:
> On Tue, 3 Jun 2014 09:19:49 +1000 Dave Chinner <[email protected]> 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
On 06/02/2014 05:02 PM, [email protected] wrote:
> On Mon, Jun 02, 2014 at 01:38:56PM -0700, Randy Dunlap wrote:
>> On 06/02/2014 01:36 PM, Joe Perches wrote:
>>> On Mon, 2014-06-02 at 13:35 -0700, Andrew Morton wrote:
>>>> On Mon, 2 Jun 2014 10:00:20 -0700 "Paul E. McKenney" <[email protected]> wrote:
>>>>
>>>>> --- a/MAINTAINERS
>>>>> +++ b/MAINTAINERS
>>>>> @@ -7321,6 +7321,7 @@ F: kernel/rcu/torture.c
>>>>>
>>>>> RCUTORTURE TEST FRAMEWORK
>>>>> M: "Paul E. McKenney" <[email protected]>
>>>>> +R: Josh Triplett <[email protected]>
>>>>> L: [email protected]
>>>>> S: Supported
>>>>> T: git git://git.kernel.org/pub/scm/linux/kernel/git/paulmck/linux-rcu.git
>>>>
>>>> I like the general principle - knowing who to poke regarding a kernel
>>>> change is useful.
>>>>
>>>> I don't care much whether it's "M:" or "R:", although "R:" carries more
>>>> meaning and hence is probably better.
>>>>
>>>> But why not "Cc:"? That's meaningful too and is more copy-n-paste friendly.
>>
>> Josh, what are you assuming that Andrew and I did not?
>
> Not sure what you mean here. Responding to the text you quoted: I have
> no particular need to bikeshed the tag name, so if you prefer "Cc" and
> can convince get_maintainer.pl to handle it, fine by me.
Sorry, what I meant is that Andrew and I both mentioned copy-paste and
you replied earlier (and I have already deleted it) that copy-paste shouldn't
be necessary for someone who is using get_maintainer.pl.
Do you redirect its output to your patch file and then edit it or does
get_maintainer.pl work with git-send-email or something else? if something
else, what is it, please?
thanks,
--
~Randy
On Mon, Jun 02, 2014 at 04:59:15PM -0700, [email protected] 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
[email protected]
On Tue, 3 Jun 2014 11:11:25 +1000
Dave Chinner <[email protected]> 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
On Mon, Jun 02, 2014 at 06:07:18PM -0700, Randy Dunlap wrote:
> On 06/02/2014 05:02 PM, [email protected] wrote:
> > On Mon, Jun 02, 2014 at 01:38:56PM -0700, Randy Dunlap wrote:
> >> On 06/02/2014 01:36 PM, Joe Perches wrote:
> >>> On Mon, 2014-06-02 at 13:35 -0700, Andrew Morton wrote:
> >>>> On Mon, 2 Jun 2014 10:00:20 -0700 "Paul E. McKenney" <[email protected]> wrote:
> >>>>
> >>>>> --- a/MAINTAINERS
> >>>>> +++ b/MAINTAINERS
> >>>>> @@ -7321,6 +7321,7 @@ F: kernel/rcu/torture.c
> >>>>>
> >>>>> RCUTORTURE TEST FRAMEWORK
> >>>>> M: "Paul E. McKenney" <[email protected]>
> >>>>> +R: Josh Triplett <[email protected]>
> >>>>> L: [email protected]
> >>>>> S: Supported
> >>>>> T: git git://git.kernel.org/pub/scm/linux/kernel/git/paulmck/linux-rcu.git
> >>>>
> >>>> I like the general principle - knowing who to poke regarding a kernel
> >>>> change is useful.
> >>>>
> >>>> I don't care much whether it's "M:" or "R:", although "R:" carries more
> >>>> meaning and hence is probably better.
> >>>>
> >>>> But why not "Cc:"? That's meaningful too and is more copy-n-paste friendly.
> >>
> >> Josh, what are you assuming that Andrew and I did not?
> >
> > Not sure what you mean here. Responding to the text you quoted: I have
> > no particular need to bikeshed the tag name, so if you prefer "Cc" and
> > can convince get_maintainer.pl to handle it, fine by me.
>
> Sorry, what I meant is that Andrew and I both mentioned copy-paste and
> you replied earlier (and I have already deleted it) that copy-paste shouldn't
> be necessary for someone who is using get_maintainer.pl.
>
> Do you redirect its output to your patch file and then edit it or does
> get_maintainer.pl work with git-send-email or something else? if something
> else, what is it, please?
Oh, I see; that was in text you hadn't quoted, so I didn't know what you
were asking. :)
git send-email can invoke 'scripts/get_maintainer.pl --no-rolestats'
directly via --to-cmd or -cc-cmd; that works fine as long as you don't
have a cover letter.
Depending on the system I'm running on, and whether it's more convenient
to invoke git-send-email or to edit patch mails and send them with 'mutt
-H', I have a shell pipeline which invokes get_maintainer.pl on an
entire patch series, collects all the email addresses it returns, and
inserts them all into each mail as CCs. (That way, when I send a
cross-subsystem patch series, I don't get a pile of maintainers confused
that they only received a couple of the numbered patches.) One example:
{ echo -n "To: " ; for x in *.patch ; do scripts/get_maintainer.pl --no-rolestats < $x | fgrep -v [email protected] ; done | sort -u | sed 's/$/, /;$s/, $//' | tr -d '\n' ; echo ; } | sed -i '/^From:/r/dev/stdin'
Personally, I'd find it handy if one of the following happened:
- git send-email (and ideally also git format-patch) grew an option to
collect *all* the to-cmd and cc-cmd output from each patch and apply
it to every patch (including the cover letter).
- get_maintainer.pl accepted multiple patchfile names and output the
union of the results. Ideally, get_maintainer.pl would also have a -i
option to edit the patch files and insert the addresses in the mail
headers.
On Mon, 2014-06-02 at 18:51 -0700, Josh Triplett wrote:
> git send-email can invoke 'scripts/get_maintainer.pl --no-rolestats'
> directly via --to-cmd or -cc-cmd; that works fine as long as you don't
> have a cover letter.
>
> Depending on the system I'm running on, and whether it's more convenient
> to invoke git-send-email or to edit patch mails and send them with 'mutt
> -H', I have a shell pipeline which invokes get_maintainer.pl on an
> entire patch series, collects all the email addresses it returns, and
> inserts them all into each mail as CCs. (That way, when I send a
> cross-subsystem patch series, I don't get a pile of maintainers confused
> that they only received a couple of the numbered patches.) One example:
I think that as long as the appropriate mailing lists receive
the cover letter, any real maintainer won't be confused.
> { echo -n "To: " ; for x in *.patch ; do scripts/get_maintainer.pl --no-rolestats < $x | fgrep -v [email protected] ; done | sort -u | sed 's/$/, /;$s/, $//' | tr -d '\n' ; echo ; } | sed -i '/^From:/r/dev/stdin'
>
> Personally, I'd find it handy if one of the following happened:
>
> - git send-email (and ideally also git format-patch) grew an option to
> collect *all* the to-cmd and cc-cmd output from each patch and apply
> it to every patch (including the cover letter).
The biggest issue with doing that is the
quantity of names and addresses on the [0/n]
patch can easily exceed vger's 1024 byte
maximum header size limit.
I drop all but the primary maintainers and
just cc lists.
I use a couple of scripts for that (attached)
for the "--to_cmd" and "--cc_cmd" options
Another possibility is to add a new "--bcc_cmd"
to git send-email so that vger's header limit
can be worked around.
I had patches to git to do that awhile ago.
> - get_maintainer.pl accepted multiple patchfile names and output the
> union of the results. Ideally, get_maintainer.pl would also have a -i
> option to edit the patch files and insert the addresses in the mail
> headers.
Why would get_maintainer.pl have any option like that?
Tools for uses. Scripting.
Aren't we good at that sort of thing?
On Mon, Jun 02, 2014 at 08:11:36PM -0700, Joe Perches wrote:
> On Mon, 2014-06-02 at 18:51 -0700, Josh Triplett wrote:
> > git send-email can invoke 'scripts/get_maintainer.pl --no-rolestats'
> > directly via --to-cmd or -cc-cmd; that works fine as long as you don't
> > have a cover letter.
> >
> > Depending on the system I'm running on, and whether it's more convenient
> > to invoke git-send-email or to edit patch mails and send them with 'mutt
> > -H', I have a shell pipeline which invokes get_maintainer.pl on an
> > entire patch series, collects all the email addresses it returns, and
> > inserts them all into each mail as CCs. (That way, when I send a
> > cross-subsystem patch series, I don't get a pile of maintainers confused
> > that they only received a couple of the numbered patches.) One example:
>
> I think that as long as the appropriate mailing lists receive
> the cover letter, any real maintainer won't be confused.
Not so much "confused" as "annoyed"; I've had people specifically
complain about getting one or two patches but not the cover letter, for
instance. (And "the appropriate mailing lists" often just mean LKML,
for many patches I've sent; almost nobody sees a patch only sent to
LKML, unless they specifically go looking for it.)
> > { echo -n "To: " ; for x in *.patch ; do scripts/get_maintainer.pl --no-rolestats < $x | fgrep -v [email protected] ; done | sort -u | sed 's/$/, /;$s/, $//' | tr -d '\n' ; echo ; } | sed -i '/^From:/r/dev/stdin'
> >
> > Personally, I'd find it handy if one of the following happened:
> >
> > - git send-email (and ideally also git format-patch) grew an option to
> > collect *all* the to-cmd and cc-cmd output from each patch and apply
> > it to every patch (including the cover letter).
>
> The biggest issue with doing that is the
> quantity of names and addresses on the [0/n]
> patch can easily exceed vger's 1024 byte
> maximum header size limit.
Is that the limit on the size of any *one* header, or on the size of all
headers combined? If the former, there's an easy way around that. And
if the latter, that seems absurdly small.
Might also help to strip out the names and just insert the addresses;
annoying, but a handy workaround to make it likely that a sensibly sized
patch series won't hit the limit.
> Another possibility is to add a new "--bcc_cmd"
> to git send-email so that vger's header limit
> can be worked around.
That breaks the ability for the recipients to see replies.
> I had patches to git to do that awhile ago.
That'd be handy; did you try submitting them upstream? What reception
did you get?
> > - get_maintainer.pl accepted multiple patchfile names and output the
> > union of the results. Ideally, get_maintainer.pl would also have a -i
> > option to edit the patch files and insert the addresses in the mail
> > headers.
>
> Why would get_maintainer.pl have any option like that?
>
> Tools for uses. Scripting.
> Aren't we good at that sort of thing?
Yes, we're good at scripting; we put scripts many people might wish to
use in a scripts/ directory, such as the extremely handy script
get_maintainer.pl that does that sort of thing with patches. And since
it currently does nothing useful at all with cover letters, it'd be nice
to change that.
If you have objections to putting it directly in get_maintainer.pl, it'd
be easy enough to make a secondary script (patch_add_maintainers?) to
drive it.
- Josh Triplett
On Mon, 2014-06-02 at 22:10 -0700, Josh Triplett wrote:
> On Mon, Jun 02, 2014 at 08:11:36PM -0700, Joe Perches wrote:
[]
> "the appropriate mailing lists" often just mean LKML,
> for many patches I've sent; almost nobody sees a patch only sent to
> LKML, unless they specifically go looking for it.)
It's a good thing then that Andre Morton is all-seeing.
> > the
> > quantity of names and addresses on the [0/n]
> > patch can easily exceed vger's 1024 byte
> > maximum header size limit.
>
> Is that the limit on the size of any *one* header, or on the size of all
> headers combined?
All headers
> > Another possibility is to add a new "--bcc_cmd"
> > to git send-email so that vger's header limit
> > can be worked around.
>
> That breaks the ability for the recipients to see replies.
<shrug> If interested, the recipient knows where to look.
> > I had patches to git to do that awhile ago.
>
> That'd be handy; did you try submitting them upstream? What reception
> did you get?
I don't recall.
It was a bit after I added the cc-cmd stuff.
Nearly 7 years ago now.
> If you have objections to putting it directly in get_maintainer.pl,
I do.
> it'd
> be easy enough to make a secondary script (patch_add_maintainers?) to
> drive it.
Go for it.
cheers, Joe
On Mon, Jun 02, 2014 at 09:30:45PM -0400, Steven Rostedt wrote:
> On Tue, 3 Jun 2014 11:11:25 +1000
> Dave Chinner <[email protected]> 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 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? :/
> 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.
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.
Similarly, I would expect any other maintainer to give me the same
"go read the guidelines - you know better than that" line if I did
the same thing for a patch that I clearly don't have a clue about or
are able to test.
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. :/
Cheers,
Dave.
--
Dave Chinner
[email protected]
----- Original Message -----
> From: "Dave Chinner" <[email protected]>
> To: "Steven Rostedt" <[email protected]>
> Cc: [email protected], "Joe Perches" <[email protected]>, [email protected], [email protected],
> [email protected], [email protected], [email protected], [email protected], "mathieu desnoyers"
> <[email protected]>, [email protected], [email protected], [email protected], [email protected],
> [email protected], [email protected], [email protected], [email protected], [email protected]
> 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 <[email protected]> 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 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? :/
>
> > 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.
>
> 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.
>
> Similarly, I would expect any other maintainer to give me the same
> "go read the guidelines - you know better than that" line if I did
> the same thing for a patch that I clearly don't have a clue about or
> are able to test.
>
> 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. :/
Hi Dave,
For various kernel subsystems, the situation appears to be like this:
there are few reviewers who have the technical ability to understand
the code. The reason why this email thread started is indeed the
"difficulty recruiting and retaining reviewers" (quoting Paul E.
McKenney).
On the other hand, testing can be automated, baby-sitted in a
continuous integration infrastructure. Code review cannot be automated
in that way.
You argue that anyone doing "review" should be running tests in order
to abide by the Reviewer's statement of oversight. Even if your
understanding of the Documentation/SubmittingPatches wording was
accurate, it sounds counter-productive to me, because it would keep
away people who have the technical knowledge to review code, but limited
time and hardware available to test it.
I also disagree with your interpretation that "free of known issues
which would argue against its inclusion" imply that testing needs to
be performed by the reviewer. It merely states that if unresolved issues
were brought to the knowledge of the reviewer, then the patch would not
be "free of known issues". It does not imply any active involvement in
testing, semantically speaking.
Thanks,
Mathieu
>
> Cheers,
>
> Dave.
> --
> Dave Chinner
> [email protected]
>
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
On Mon, Jun 02, 2014 at 08:25:51PM +0000, Mathieu Desnoyers wrote:
> > From: "Paul E. McKenney" <[email protected]>
> > On Mon, Jun 02, 2014 at 12:11:55PM -0700, [email protected] 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, [email protected] 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, [email protected] 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
On Tue, Jun 03, 2014 at 01:24:23PM +0000, Mathieu Desnoyers wrote:
> ----- Original Message -----
> > From: "Dave Chinner" <[email protected]>
> > To: "Steven Rostedt" <[email protected]>
> > Cc: [email protected], "Joe Perches" <[email protected]>, [email protected], [email protected],
> > [email protected], [email protected], [email protected], [email protected], "mathieu desnoyers"
> > <[email protected]>, [email protected], [email protected], [email protected], [email protected],
> > [email protected], [email protected], [email protected], [email protected], [email protected]
> > 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 <[email protected]> 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 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? :/
> >
> > > 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.
> >
> > 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.
> >
> > Similarly, I would expect any other maintainer to give me the same
> > "go read the guidelines - you know better than that" line if I did
> > the same thing for a patch that I clearly don't have a clue about or
> > are able to test.
> >
> > 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. :/
>
> Hi Dave,
>
> For various kernel subsystems, the situation appears to be like this:
> there are few reviewers who have the technical ability to understand
> the code. The reason why this email thread started is indeed the
> "difficulty recruiting and retaining reviewers" (quoting Paul E.
> McKenney).
>
> On the other hand, testing can be automated, baby-sitted in a
> continuous integration infrastructure. Code review cannot be automated
> in that way.
>
> You argue that anyone doing "review" should be running tests in order
> to abide by the Reviewer's statement of oversight. Even if your
> understanding of the Documentation/SubmittingPatches wording was
> accurate, it sounds counter-productive to me, because it would keep
> away people who have the technical knowledge to review code, but limited
> time and hardware available to test it.
>
> I also disagree with your interpretation that "free of known issues
> which would argue against its inclusion" imply that testing needs to
> be performed by the reviewer. It merely states that if unresolved issues
> were brought to the knowledge of the reviewer, then the patch would not
> be "free of known issues". It does not imply any active involvement in
> testing, semantically speaking.
I believe that this will vary from subsystem to subsystem. I expect that
some subsystems are more amenable to straight testing than others. I would
not agree that a mostly-testing approach to validation is a good match for
RCU, but it probably is for other subsystems.
Thanx, Paul
On Tue, 3 Jun 2014 08:37:55 -0700
"Paul E. McKenney" <[email protected]> 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
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" <[email protected]> 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
On 06/02/2014 06:51 PM, Josh Triplett wrote:
> On Mon, Jun 02, 2014 at 06:07:18PM -0700, Randy Dunlap wrote:
>> On 06/02/2014 05:02 PM, [email protected] wrote:
>>> On Mon, Jun 02, 2014 at 01:38:56PM -0700, Randy Dunlap wrote:
>>>> On 06/02/2014 01:36 PM, Joe Perches wrote:
>>>>> On Mon, 2014-06-02 at 13:35 -0700, Andrew Morton wrote:
>>>>>> On Mon, 2 Jun 2014 10:00:20 -0700 "Paul E. McKenney" <[email protected]> wrote:
>>>>>>
>>>>>>> --- a/MAINTAINERS
>>>>>>> +++ b/MAINTAINERS
>>>>>>> @@ -7321,6 +7321,7 @@ F: kernel/rcu/torture.c
>>>>>>>
>>>>>>> RCUTORTURE TEST FRAMEWORK
>>>>>>> M: "Paul E. McKenney" <[email protected]>
>>>>>>> +R: Josh Triplett <[email protected]>
>>>>>>> L: [email protected]
>>>>>>> S: Supported
>>>>>>> T: git git://git.kernel.org/pub/scm/linux/kernel/git/paulmck/linux-rcu.git
>>>>>>
>>>>>> I like the general principle - knowing who to poke regarding a kernel
>>>>>> change is useful.
>>>>>>
>>>>>> I don't care much whether it's "M:" or "R:", although "R:" carries more
>>>>>> meaning and hence is probably better.
>>>>>>
>>>>>> But why not "Cc:"? That's meaningful too and is more copy-n-paste friendly.
>>>>
>>>> Josh, what are you assuming that Andrew and I did not?
>>>
>>> Not sure what you mean here. Responding to the text you quoted: I have
>>> no particular need to bikeshed the tag name, so if you prefer "Cc" and
>>> can convince get_maintainer.pl to handle it, fine by me.
>>
>> Sorry, what I meant is that Andrew and I both mentioned copy-paste and
>> you replied earlier (and I have already deleted it) that copy-paste shouldn't
>> be necessary for someone who is using get_maintainer.pl.
>>
>> Do you redirect its output to your patch file and then edit it or does
>> get_maintainer.pl work with git-send-email or something else? if something
>> else, what is it, please?
>
> Oh, I see; that was in text you hadn't quoted, so I didn't know what you
> were asking. :)
>
> git send-email can invoke 'scripts/get_maintainer.pl --no-rolestats'
> directly via --to-cmd or -cc-cmd; that works fine as long as you don't
> have a cover letter.
>
> Depending on the system I'm running on, and whether it's more convenient
> to invoke git-send-email or to edit patch mails and send them with 'mutt
> -H', I have a shell pipeline which invokes get_maintainer.pl on an
> entire patch series, collects all the email addresses it returns, and
> inserts them all into each mail as CCs. (That way, when I send a
> cross-subsystem patch series, I don't get a pile of maintainers confused
> that they only received a couple of the numbered patches.) One example:
>
> { echo -n "To: " ; for x in *.patch ; do scripts/get_maintainer.pl --no-rolestats < $x | fgrep -v [email protected] ; done | sort -u | sed 's/$/, /;$s/, $//' | tr -d '\n' ; echo ; } | sed -i '/^From:/r/dev/stdin'
I see. Thanks for the summary.
> Personally, I'd find it handy if one of the following happened:
>
> - git send-email (and ideally also git format-patch) grew an option to
> collect *all* the to-cmd and cc-cmd output from each patch and apply
> it to every patch (including the cover letter).
>
> - get_maintainer.pl accepted multiple patchfile names and output the
> union of the results. Ideally, get_maintainer.pl would also have a -i
> option to edit the patch files and insert the addresses in the mail
> headers.
--
~Randy
On Tue, 3 Jun 2014 17:16:54 +1000
Dave Chinner <[email protected]> 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. 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 day to
> test their own code. IOWs, in most cases "tester" and the "reviewer"
> roles are performed by the same people.
Again, you are limiting you supply of reviewers with this requirement.
>
> > 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.
>
> > 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.
>
> 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 <[email protected]> # for the
tracing part". before.
I may not test them. I may compile it, but not boot it. I understand
tracing enough to know what will work and what wont. When I give
reviews for other archs, I seldom even compile their code.
>
> Similarly, I would expect any other maintainer to give me the same
> "go read the guidelines - you know better than that" line if I did
> the same thing for a patch that I clearly don't have a clue about or
> are able to test.
>
> 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.
I'm not saying that you can't have your own meaning of Reviewed-by. You
are in charge of a filesystem, and to me, that's one of the most
crucial parts of the kernel, as a corrupt filesystem can lead to huge
loss of data.
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 there that
bothers me". I does not mean that I looked over every change in detail.
-- Steve
On 06/03/2014 10:43 AM, Steven Rostedt wrote:
> On Tue, 3 Jun 2014 17:16:54 +1000
> Dave Chinner <[email protected]> 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 day to
>> test their own code. IOWs, in most cases "tester" and the "reviewer"
>> roles are performed by the same people.
>
> Again, you are limiting you supply of reviewers with this requirement.
>
>>
>>> 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.
> I'm not saying that you can't have your own meaning of Reviewed-by. You
> are in charge of a filesystem, and to me, that's one of the most
> crucial parts of the kernel, as a corrupt filesystem can lead to huge
> loss of data.
>
> 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 there that
> bothers me". I does not mean that I looked over every change in detail.
Agreed.
--
~Randy
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
On Tue, 3 Jun 2014 16:52:53 -0400
Theodore Ts'o <[email protected]> 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
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 <[email protected]> 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
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.
On Wed, 4 Jun 2014 00:48:16 +0100
Ken Moffat <[email protected]> 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
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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
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
On 06/03/2014 03:27 AM, Paul E. McKenney wrote:
> On Mon, Jun 02, 2014 at 12:11:55PM -0700, [email protected] 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, [email protected] 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, [email protected] 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
On Tue, Jun 03, 2014 at 01:43:47PM -0400, Steven Rostedt wrote:
> On Tue, 3 Jun 2014 17:16:54 +1000
> Dave Chinner <[email protected]> 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 <[email protected]>
> # 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 there that bothers me". I does not mean that I
> looked over every change in detail.
That's exactly how I use the two tags as well. If I'm not going to
(or can't) fully commit to a review, Acked-by is what I use. But you
or I are not the problem here....
Cheers,
Dave.
--
Dave Chinner
[email protected]
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 <[email protected]> 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