| Message ID | 20170717181844.12970-1-julien.grall@arm.com |
|---|---|
| State | Superseded |
| Headers | show |
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
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,
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.
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,
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) {
"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(-)