[Xen-devel] scripts/get_maintainers.pl: Don't blindly drop "THE REST" maintainers

Message ID 20170717181844.12970-1-julien.grall@arm.com
State Superseded
Headers show

Commit Message

Julien Grall July 17, 2017, 6:18 p.m.
"THE REST" maintainers should always be CCed for any modification that
don't fall under the responsability of a specific component maintainer.

However, the script get_maintainers.pl will remove "THE REST"
maintainers as soon as one maintainer of a specific component will be
present.

Fix the script once for all.

Signed-off-by: Julien Grall <julien.grall@arm.com>

---

I am getting annoyed at requesting contributors to CC "THE REST" because
the script didn't properly return the list of maintainers. This should
now be fixed.
---
 scripts/get_maintainer.pl | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

Comments

Andrew Cooper July 18, 2017, 9:11 a.m. | #1
On 17/07/17 19:18, Julien Grall wrote:
> "THE REST" maintainers should always be CCed for any modification that
> don't fall under the responsability of a specific component maintainer.
>
> However, the script get_maintainers.pl will remove "THE REST"
> maintainers as soon as one maintainer of a specific component will be
> present.
>
> Fix the script once for all.
>
> Signed-off-by: Julien Grall <julien.grall@arm.com>

Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>

This is definitely an improvement in behaviour.

You should probably get a review from someone who speaks better perl
than I do.

~Andrew
Julien Grall July 26, 2017, 12:08 p.m. | #2
On 18/07/17 10:11, Andrew Cooper wrote:
> On 17/07/17 19:18, Julien Grall wrote:
>> "THE REST" maintainers should always be CCed for any modification that
>> don't fall under the responsability of a specific component maintainer.
>>
>> However, the script get_maintainers.pl will remove "THE REST"
>> maintainers as soon as one maintainer of a specific component will be
>> present.
>>
>> Fix the script once for all.
>>
>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>
> Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>
>
> This is definitely an improvement in behaviour.
>
> You should probably get a review from someone who speaks better perl
> than I do.

Anyone up to review perl? :)

Cheers,
Ian Jackson July 26, 2017, 2:27 p.m. | #3
Julien Grall writes ("[Xen-devel] [PATCH] scripts/get_maintainers.pl: Don't blindly drop "THE REST" maintainers"):
> "THE REST" maintainers should always be CCed for any modification that
> don't fall under the responsability of a specific component maintainer.
> 
> However, the script get_maintainers.pl will remove "THE REST"
> maintainers as soon as one maintainer of a specific component will be
> present.
> 
> Fix the script once for all.

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

Although:

> +    # By default "THE REST" will be suppressed.
> +    my $do_not_suppress_the_rest = 0;

I normally find flag variables whose names contain negations to be
quite confusing.

Ian.
Julien Grall July 26, 2017, 5:13 p.m. | #4
Hi Ian,

On 26/07/17 15:27, Ian Jackson wrote:
> Julien Grall writes ("[Xen-devel] [PATCH] scripts/get_maintainers.pl: Don't blindly drop "THE REST" maintainers"):
>> "THE REST" maintainers should always be CCed for any modification that
>> don't fall under the responsability of a specific component maintainer.
>>
>> However, the script get_maintainers.pl will remove "THE REST"
>> maintainers as soon as one maintainer of a specific component will be
>> present.
>>
>> Fix the script once for all.
>
> Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
>
> Although:
>
>> +    # By default "THE REST" will be suppressed.
>> +    my $do_not_suppress_the_rest = 0;
>
> I normally find flag variables whose names contain negations to be
> quite confusing.

I am happy to rename to "suppress_the_rest". I will send a new version.

Cheers,

Patch

diff --git a/scripts/get_maintainer.pl b/scripts/get_maintainer.pl
index 2804a5b5df..d3076adfd8 100755
--- a/scripts/get_maintainer.pl
+++ b/scripts/get_maintainer.pl
@@ -571,11 +571,15 @@  sub get_maintainers {
     # Find responsible parties
 
     my %exact_pattern_match_hash = ();
+    # By default "THE REST" will be suppressed.
+    my $do_not_suppress_the_rest = 0;
 
     foreach my $file (@files) {
 
 	my %hash;
 	my $tvi = find_first_section();
+	# Unless stated otherwise, a file is maintained by "THE REST"
+	my $file_maintained_by_the_rest = 1;
 	while ($tvi < @typevalue) {
 	    my $start = find_starting_index($tvi);
 	    my $end = find_ending_index($tvi);
@@ -633,6 +637,14 @@  sub get_maintainers {
 
 	foreach my $line (sort {$hash{$b} <=> $hash{$a}} keys %hash) {
 	    add_categories($line);
+	    my $role = get_maintainer_role($line);
+
+	    # Check the role, if it is not "THE REST" then the file is not
+	    # only maintained by "THE REST".
+	    if ( get_maintainer_role($line) ne "supporter:THE REST" ) {
+		    $file_maintained_by_the_rest = 0;
+	    }
+
 	    if ($sections) {
 		my $i;
 		my $start = find_starting_index($line);
@@ -657,6 +669,9 @@  sub get_maintainers {
 		print("\n");
 	    }
 	}
+	# If the file is only maintained by "THE REST", then CC all of them on
+	# the patch.
+	$do_not_suppress_the_rest = 1 if $file_maintained_by_the_rest;
     }
 
     if ($keywords) {
@@ -666,7 +681,8 @@  sub get_maintainers {
 	}
     }
 
-    if ($email_drop_the_rest_supporter_if_supporter_found && $#email_to > 0) {
+    if ($email_drop_the_rest_supporter_if_supporter_found &&
+	!$do_not_suppress_the_rest && $#email_to > 0) {
         my @email_new;
         my $do_replace = 0;
         foreach my $email (@email_to) {