[PATCH] get_maintainer: add support for using an alternate MAINTAINERS file

[PATCH] get_maintainer: add support for using an alternate MAINTAINERS file

[Jani Nikula]

There are large and/or complex subsystems/drivers that have domain
experts that should review patches in their domain. One such example is
drm/i915. We'd like to be able to document this in a way that can be
automatically queried for each patch, so people know who to ping for
reviews. This is what get_maintainer.pl already solves.

However, documenting all of this in the main kernel MAINTAINERS file is
just too much noise, and potentially confusing for community
contributors. Add support for specifying and using an alternate
MAINTAINERS file with --maintainers option.

Signed-off-by: Jani Nikula <jani.nikula@intel.com>

---

Note that I don't know perl, at all. This is all cargo culted copy
paste, but seems to work. Please be gentle. :)
---
 scripts/get_maintainer.pl | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/scripts/get_maintainer.pl b/scripts/get_maintainer.pl
index 98bae869f6d0..0dae244f26eb 100755
--- a/scripts/get_maintainer.pl
+++ b/scripts/get_maintainer.pl
@@ -18,6 +18,7 @@ my $V = '0.26';
 use Getopt::Long qw(:config no_auto_abbrev);
 
 my $lk_path = "./";
+my $maintainers_path = "${lk_path}MAINTAINERS";
 my $email = 1;
 my $email_usename = 1;
 my $email_maintainer = 1;
@@ -229,6 +230,7 @@ if (!GetOptions(
 		'n!' => \$email_usename,
 		'l!' => \$email_list,
 		's!' => \$email_subscriber_list,
+		'maintainers=s' => \$maintainers_path,
 		'multiline!' => \$output_multiline,
 		'roles!' => \$output_roles,
 		'rolestats!' => \$output_rolestats,
@@ -300,8 +302,8 @@ if (!top_of_kernel_tree($lk_path)) {
 my @typevalue = ();
 my %keyword_hash;
 
-open (my $maint, '<', "${lk_path}MAINTAINERS")
-    or die "$P: Can't open MAINTAINERS: $!\n";
+open (my $maint, '<', $maintainers_path)
+    or die "$P: Can't open ${maintainers_path}: $!\n";
 while (<$maint>) {
     my $line = $_;
 
@@ -808,6 +810,7 @@ Other options:
   --keywords => scan patch for keywords (default: $keywords)
   --sections => print all of the subsystem sections with pattern matches
   --mailmap => use .mailmap file (default: $email_use_mailmap)
+  --maintainers => specify alternate MAINTAINERS file to use
   --version => show version
   --help => show this help information
 
-- 
2.1.4

Re: [PATCH] get_maintainer: add support for using an alternate MAINTAINERS file

[Joe Perches]

On Fri, 2015-10-16 at 11:36 +0300, Jani Nikula wrote:
> There are large and/or complex subsystems/drivers that have domain
> experts that should review patches in their domain. One such example is
> drm/i915. We'd like to be able to document this in a way that can be
> automatically queried for each patch, so people know who to ping for
> reviews. This is what get_maintainer.pl already solves.
> 
> However, documenting all of this in the main kernel MAINTAINERS file is
> just too much noise, and potentially confusing for community
> contributors. Add support for specifying and using an alternate
> MAINTAINERS file with --maintainers option.

Is this really useful for the community at large?

This seems like something that might be useful for an
organization but not others.

Why is specifying whatever is necessary in the existing
MAINTAINERS file noisy or confusing?

Re: [PATCH] get_maintainer: add support for using an alternate MAINTAINERS file

[Jani Nikula]

On Fri, 16 Oct 2015, Joe Perches <joe@perches.com> wrote:
> On Fri, 2015-10-16 at 11:36 +0300, Jani Nikula wrote:
>> There are large and/or complex subsystems/drivers that have domain
>> experts that should review patches in their domain. One such example is
>> drm/i915. We'd like to be able to document this in a way that can be
>> automatically queried for each patch, so people know who to ping for
>> reviews. This is what get_maintainer.pl already solves.
>> 
>> However, documenting all of this in the main kernel MAINTAINERS file is
>> just too much noise, and potentially confusing for community
>> contributors. Add support for specifying and using an alternate
>> MAINTAINERS file with --maintainers option.
>
> Is this really useful for the community at large?

Probably not.

> This seems like something that might be useful for an
> organization but not others.

It may be useful for several organizations contributing to the kernel.

> Why is specifying whatever is necessary in the existing
> MAINTAINERS file noisy or confusing?

IIUC you can't specify file patterns for specific reviewers within one
entry. I think we'd have to split up the driver entry to several, mostly
duplicated and possibly overlapping entries, with their own designated
reviewers and file patterns. I think that would be noisy and confusing.

Perhaps we could have detailed maintainers files within drivers,
included from the top MAINTAINERS file; however that would be a much
more intrusive change (and definitely beyond my perl cargo culting
skills). I just thought what I proposed here would be a rather harmless
change.


BR,
Jani.


-- 
Jani Nikula, Intel Open Source Technology Center

Re: [PATCH] get_maintainer: add support for using an alternate MAINTAINERS file

[Joe Perches]

On Fri, 2015-10-16 at 12:14 +0300, Jani Nikula wrote:
> On Fri, 16 Oct 2015, Joe Perches <joe@perches.com> wrote:
> > On Fri, 2015-10-16 at 11:36 +0300, Jani Nikula wrote:
> >> There are large and/or complex subsystems/drivers that have domain
> >> experts that should review patches in their domain. One such example is
> >> drm/i915. We'd like to be able to document this in a way that can be
> >> automatically queried for each patch, so people know who to ping for
> >> reviews. This is what get_maintainer.pl already solves.
> >> 
> >> However, documenting all of this in the main kernel MAINTAINERS file is
> >> just too much noise, and potentially confusing for community
> >> contributors. Add support for specifying and using an alternate
> >> MAINTAINERS file with --maintainers option.
> >
> > Is this really useful for the community at large?
> 
> Probably not.
> 
> > This seems like something that might be useful for an
> > organization but not others.
> 
> It may be useful for several organizations contributing to the kernel.
> 
> > Why is specifying whatever is necessary in the existing
> > MAINTAINERS file noisy or confusing?
> 
> IIUC you can't specify file patterns for specific reviewers within one
> entry. I think we'd have to split up the driver entry to several, mostly
> duplicated and possibly overlapping entries, with their own designated
> reviewers and file patterns. I think that would be noisy and confusing.

I find the concept of adding separate MAINTAINERS files odd
and at best and not good for the community.

Internal to an organization, if this is for subject matter
expert reviewers, perhaps it'd be better to add an optional
"REVIEWERS" file (or maybe multiple REVIEWER.* files) with
simpler patterns.

Using just R: F: X: N: K: would probably work.

This wouldn't have to be section ordered like MAINTAINERS,
but could also be ordered by reviewer skills.

For example:

$ cat REVIEWERS
BIGCO PHYS
M:	Joe Schmo <joe.schmo@bigco.com>
F:	drivers/net/ethernet/bigco/*/*phy*

Re: [PATCH] get_maintainer: add support for using an alternate MAINTAINERS file

[Joe Perches]

On Fri, 2015-10-16 at 09:23 -0700, Joe Perches wrote:
> On Fri, 2015-10-16 at 12:14 +0300, Jani Nikula wrote:
> > On Fri, 16 Oct 2015, Joe Perches <joe@perches.com> wrote:
> > > On Fri, 2015-10-16 at 11:36 +0300, Jani Nikula wrote:
> > >> There are large and/or complex subsystems/drivers that have domain
> > >> experts that should review patches in their domain. One such example is
> > >> drm/i915. We'd like to be able to document this in a way that can be
> > >> automatically queried for each patch, so people know who to ping for
> > >> reviews. This is what get_maintainer.pl already solves.

Maybe this:

It reads optional REVIEWER files in the same style
as MAINTAINERS.

Most of the change is just making a block of code
into a subroutine.

---
 scripts/get_maintainer.pl | 60 +++++++++++++++++++++++++++--------------------
 1 file changed, 35 insertions(+), 25 deletions(-)

diff --git a/scripts/get_maintainer.pl b/scripts/get_maintainer.pl
index 98bae86..79ba49a 100755
--- a/scripts/get_maintainer.pl
+++ b/scripts/get_maintainer.pl
@@ -300,35 +300,45 @@ if (!top_of_kernel_tree($lk_path)) {
 my @typevalue = ();
 my %keyword_hash;
 
-open (my $maint, '<', "${lk_path}MAINTAINERS")
-    or die "$P: Can't open MAINTAINERS: $!\n";
-while (<$maint>) {
-    my $line = $_;
-
-    if ($line =~ m/^([A-Z]):\s*(.*)/) {
-	my $type = $1;
-	my $value = $2;
-
-	##Filename pattern matching
-	if ($type eq "F" || $type eq "X") {
-	    $value =~ s@\.@\\\.@g;       ##Convert . to \.
-	    $value =~ s/\*/\.\*/g;       ##Convert * to .*
-	    $value =~ s/\?/\./g;         ##Convert ? to .
-	    ##if pattern is a directory and it lacks a trailing slash, add one
-	    if ((-d $value)) {
-		$value =~ s@([^/])$@$1/@;
+read_maintainer_file("${lk_path}MAINTAINERS");
+
+## Read optional REVIEWERS file(s) too
+my @reviewer_files = glob("${lk_path}REVIEWERS*");
+foreach my $reviewer_file (@reviewer_files) {
+    read_maintainer_file($reviewer_file);
+}
+
+sub read_maintainer_file {
+    my ($file) = @_;
+    open (my $maint, '<', "$file")
+	or die "$P: Can't open $file: $!\n";
+    while (<$maint>) {
+	my $line = $_;
+
+	if ($line =~ m/^([A-Z]):\s*(.*)/) {
+	    my $type = $1;
+	    my $value = $2;
+
+	    ##Filename pattern matching
+	    if ($type eq "F" || $type eq "X") {
+		$value =~ s@\.@\\\.@g;       ##Convert . to \.
+		$value =~ s/\*/\.\*/g;       ##Convert * to .*
+		$value =~ s/\?/\./g;         ##Convert ? to .
+		##if pattern is a directory and it lacks a trailing slash, add one
+		if ((-d $value)) {
+		    $value =~ s@([^/])$@$1/@;
+		}
+	    } elsif ($type eq "K") {
+		$keyword_hash{@typevalue} = $value;
 	    }
-	} elsif ($type eq "K") {
-	    $keyword_hash{@typevalue} = $value;
+	    push(@typevalue, "$type:$value");
+	} elsif (!/^(\s)*$/) {
+	    $line =~ s/\n$//g;
+	    push(@typevalue, $line);
 	}
-	push(@typevalue, "$type:$value");
-    } elsif (!/^(\s)*$/) {
-	$line =~ s/\n$//g;
-	push(@typevalue, $line);
     }
+    close($maint);
 }
-close($maint);
-
 
 #
 # Read mail address map

 

Re: [PATCH] get_maintainer: add support for using an alternate MAINTAINERS file

[Jani Nikula]

On Fri, 16 Oct 2015, Joe Perches <joe@perches.com> wrote:
> On Fri, 2015-10-16 at 12:14 +0300, Jani Nikula wrote:
>> On Fri, 16 Oct 2015, Joe Perches <joe@perches.com> wrote:
>> > On Fri, 2015-10-16 at 11:36 +0300, Jani Nikula wrote:
>> >> There are large and/or complex subsystems/drivers that have domain
>> >> experts that should review patches in their domain. One such example is
>> >> drm/i915. We'd like to be able to document this in a way that can be
>> >> automatically queried for each patch, so people know who to ping for
>> >> reviews. This is what get_maintainer.pl already solves.
>> >> 
>> >> However, documenting all of this in the main kernel MAINTAINERS file is
>> >> just too much noise, and potentially confusing for community
>> >> contributors. Add support for specifying and using an alternate
>> >> MAINTAINERS file with --maintainers option.
>> >
>> > Is this really useful for the community at large?
>> 
>> Probably not.
>> 
>> > This seems like something that might be useful for an
>> > organization but not others.
>> 
>> It may be useful for several organizations contributing to the kernel.
>> 
>> > Why is specifying whatever is necessary in the existing
>> > MAINTAINERS file noisy or confusing?
>> 
>> IIUC you can't specify file patterns for specific reviewers within one
>> entry. I think we'd have to split up the driver entry to several, mostly
>> duplicated and possibly overlapping entries, with their own designated
>> reviewers and file patterns. I think that would be noisy and confusing.
>
> I find the concept of adding separate MAINTAINERS files odd
> and at best and not good for the community.

Let me get this straight. You're rejecting a trivial patch increasing
the usefulness of a simple script to a number of kernel developers not
on technical grounds but because in your view the intended use is not
good for the community? So had I said, this patch enables one to write
unit tests for the script with various input files, the outcome might
have been different?

> Internal to an organization, if this is for subject matter
> expert reviewers, perhaps it'd be better to add an optional
> "REVIEWERS" file (or maybe multiple REVIEWER.* files) with
> simpler patterns.

It's fine if there's an option to specify additional reviewers files
(that don't need to reside in the kernel tree) on the command line.

Thanks for your time and review.


BR,
Jani.


-- 
Jani Nikula, Intel Open Source Technology Center

Re: [PATCH] get_maintainer: add support for using an alternate MAINTAINERS file

[Joe Perches]

On Fri, 2015-10-16 at 21:35 +0300, Jani Nikula wrote:
> On Fri, 16 Oct 2015, Joe Perches <joe@perches.com> wrote:
> > On Fri, 2015-10-16 at 12:14 +0300, Jani Nikula wrote:
> >> On Fri, 16 Oct 2015, Joe Perches <joe@perches.com> wrote:
> >> > On Fri, 2015-10-16 at 11:36 +0300, Jani Nikula wrote:
> >> >> There are large and/or complex subsystems/drivers that have domain
> >> >> experts that should review patches in their domain. One such example is
> >> >> drm/i915. We'd like to be able to document this in a way that can be
> >> >> automatically queried for each patch, so people know who to ping for
> >> >> reviews. This is what get_maintainer.pl already solves.
> >> >> 
> >> >> However, documenting all of this in the main kernel MAINTAINERS file is
> >> >> just too much noise, and potentially confusing for community
> >> >> contributors. Add support for specifying and using an alternate
> >> >> MAINTAINERS file with --maintainers option.
> >> >
> >> > Is this really useful for the community at large?
> >> 
> >> Probably not.
> >> 
> >> > This seems like something that might be useful for an
> >> > organization but not others.
> >> 
> >> It may be useful for several organizations contributing to the kernel.
> >> 
> >> > Why is specifying whatever is necessary in the existing
> >> > MAINTAINERS file noisy or confusing?
> >> 
> >> IIUC you can't specify file patterns for specific reviewers within one
> >> entry. I think we'd have to split up the driver entry to several, mostly
> >> duplicated and possibly overlapping entries, with their own designated
> >> reviewers and file patterns. I think that would be noisy and confusing.
> >
> > I find the concept of adding separate MAINTAINERS files odd
> > and at best and not good for the community.
> 
> Let me get this straight. You're rejecting a trivial patch increasing
> the usefulness of a simple script to a number of kernel developers not
> on technical grounds but because in your view the intended use is not
> good for the community?

Yes.

I think it's perfectly fine to keep something like this
out-of-tree in your own repository.

>  So had I said, this patch enables one to write
> unit tests for the script with various input files, the outcome might
> have been different?

No.  Nice try though.

Re: [PATCH] get_maintainer: add support for using an alternate MAINTAINERS file

[Jani Nikula]

On Fri, 16 Oct 2015, Joe Perches <joe@perches.com> wrote:
> On Fri, 2015-10-16 at 21:35 +0300, Jani Nikula wrote:
>> Let me get this straight. You're rejecting a trivial patch increasing
>> the usefulness of a simple script to a number of kernel developers not
>> on technical grounds but because in your view the intended use is not
>> good for the community?
>
> Yes.

Thanks for the straight answer.

> I think it's perfectly fine to keep something like this
> out-of-tree in your own repository.

Now that is a relief. I am not sure what I would have done if you'd told
me I can't have it in my own repository either! ;)


Cheers,
Jani.

-- 
Jani Nikula, Intel Open Source Technology Center