[Qemu-devel] [PATCH 0/4] small improvements to get_maintainer.pl

[Qemu-devel] [PATCH 0/4] small improvements to get_maintainer.pl

[Paolo Bonzini]

See individual patches.  My Perl-fu is limited, but the individual
changes are easy enough.

Paolo Bonzini (4):
  get_maintainer.pl: exit with status 1 if no maintainer found
  get_maintainer.pl: treat all M entries the same
  get_maintainer.pl: move git loop under "if ($email) {"
  get_maintainer.pl: point at --git-fallback instead of enabling it
    automatically

 scripts/get_maintainer.pl | 52 +++++++++++++++++++----------------------------
 1 file changed, 21 insertions(+), 31 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH 1/4] get_maintainer.pl: exit with status 1 if no maintainer found

[Paolo Bonzini]

This can be useful, at the very least, for debugging.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 scripts/get_maintainer.pl | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/scripts/get_maintainer.pl b/scripts/get_maintainer.pl
index 38334de..a3a16d8 100755
--- a/scripts/get_maintainer.pl
+++ b/scripts/get_maintainer.pl
@@ -55,7 +55,7 @@ my $help = 0;
 
 my $vcs_used = 0;
 
-my $exit = 0;
+my $exit = 1;
 
 my %commit_author_hash;
 my %commit_signer_hash;
@@ -2061,6 +2061,7 @@ sub output {
 	print(join($output_separator, @parms));
 	print("\n");
     }
+    $exit = 0 if @parms > 0;
 }
 
 my $rfc822re;
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 1/4] get_maintainer.pl: exit with status 1 if no maintainer found

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> This can be useful, at the very least, for debugging.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  scripts/get_maintainer.pl | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/scripts/get_maintainer.pl b/scripts/get_maintainer.pl
> index 38334de..a3a16d8 100755
> --- a/scripts/get_maintainer.pl
> +++ b/scripts/get_maintainer.pl
> @@ -55,7 +55,7 @@ my $help = 0;
>  
>  my $vcs_used = 0;
>  
> -my $exit = 0;
> +my $exit = 1;
>  
>  my %commit_author_hash;
>  my %commit_signer_hash;
> @@ -2061,6 +2061,7 @@ sub output {
>  	print(join($output_separator, @parms));
>  	print("\n");
>      }
> +    $exit = 0 if @parms > 0;
>  }
>  
>  my $rfc822re;

Not sure about "if no maintainer found".

Here's how output() gets called:

    if (@maintainers) {
        @maintainers = merge_email(@maintainers);
        output(@maintainers);
    }

    if ($scm) {
        @scm = uniq(@scm);
        output(@scm);
    }

    if ($status) {
        @status = uniq(@status);
        output(@status);
    }

    if ($subsystem) {
        @subsystem = uniq(@subsystem);
        output(@subsystem);
    }

    if ($web) {
        @web = uniq(@web);
        output(@web);
    }

    exit($exit);

Thus, the patch makes the script exit with status 1 when none of the
above output() calls produces output.

Maybe it exits with status 1 when it fails to produce output?  Obvious
ouput elsewhere: printing the version (okay), and a print in
get_maintainers().  Can't tell what the latter does.  Hmm.

Re: [Qemu-devel] [PATCH 1/4] get_maintainer.pl: exit with status 1 if no maintainer found

[Paolo Bonzini]

> Thus, the patch makes the script exit with status 1 when none of the
> above output() calls produces output.
> 
> Maybe it exits with status 1 when it fails to produce output?  Obvious
> ouput elsewhere: printing the version (okay),

That exits with 0.

> and a print in get_maintainers().  Can't tell what the latter does.  Hmm.

That is for --sections, and you're right, this is missing:

diff --git a/scripts/get_maintainer.pl b/scripts/get_maintainer.pl
index f4fee27..e682d1f 100755
--- a/scripts/get_maintainer.pl
+++ b/scripts/get_maintainer.pl
@@ -617,6 +617,7 @@ sub get_maintainers {
 		    print("$line\n");
 		}
 		print("\n");
+		$exit = 0;
 	    }
 	}
     }

Paolo

[Qemu-devel] [PATCH 2/4] get_maintainer.pl: treat all M entries the same

[Paolo Bonzini]

Why should get_maintainer bother if someone puts his name as maintainer
on an "Orphan" entry?  Garbage in, email out.  The results from the git
fallback are likely to be from someone doing sweeping changes on the
whole tree, and thus not really interesting.

Reported-by: Gerd Hoffmann <kraxel@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 scripts/get_maintainer.pl | 19 -------------------
 1 file changed, 19 deletions(-)

diff --git a/scripts/get_maintainer.pl b/scripts/get_maintainer.pl
index a3a16d8..6c5a73b 100755
--- a/scripts/get_maintainer.pl
+++ b/scripts/get_maintainer.pl
@@ -505,24 +505,6 @@ if ($web) {
 
 exit($exit);
 
-sub range_is_maintained {
-    my ($start, $end) = @_;
-
-    for (my $i = $start; $i < $end; $i++) {
-	my $line = $typevalue[$i];
-	if ($line =~ m/^(\C):\s*(.*)/) {
-	    my $type = $1;
-	    my $value = $2;
-	    if ($type eq 'S') {
-		if ($value =~ /(maintain|support)/i) {
-		    return 1;
-		}
-	    }
-	}
-    }
-    return 0;
-}
-
 sub range_has_maintainer {
     my ($start, $end) = @_;
 
@@ -602,7 +584,6 @@ sub get_maintainers {
 				$value_pd++ if (substr($value,-1,1) ne "/");
 				$value_pd = -1 if ($value =~ /^\.\*/);
 				if ($value_pd >= $file_pd &&
-				    range_is_maintained($start, $end) &&
 				    range_has_maintainer($start, $end)) {
 				    $exact_pattern_match_hash{$file} = 1;
 				}
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 2/4] get_maintainer.pl: treat all M entries the same

[Michael S. Tsirkin]

On Wed, Oct 22, 2014 at 11:08:20AM +0200, Paolo Bonzini wrote:
> Why should get_maintainer bother if someone puts his name as maintainer
> on an "Orphan" entry?  Garbage in, email out.  The results from the git
> fallback are likely to be from someone doing sweeping changes on the
> whole tree, and thus not really interesting.
> 
> Reported-by: Gerd Hoffmann <kraxel@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Looks good to me

Reviewed-by: Michael S. Tsirkin <mst@redhat.com>

I will add text that explains that the effect for current tree is
disabling fallback for Odd fixes projects.


> ---
>  scripts/get_maintainer.pl | 19 -------------------
>  1 file changed, 19 deletions(-)
> 
> diff --git a/scripts/get_maintainer.pl b/scripts/get_maintainer.pl
> index a3a16d8..6c5a73b 100755
> --- a/scripts/get_maintainer.pl
> +++ b/scripts/get_maintainer.pl
> @@ -505,24 +505,6 @@ if ($web) {
>  
>  exit($exit);
>  
> -sub range_is_maintained {
> -    my ($start, $end) = @_;
> -
> -    for (my $i = $start; $i < $end; $i++) {
> -	my $line = $typevalue[$i];
> -	if ($line =~ m/^(\C):\s*(.*)/) {
> -	    my $type = $1;
> -	    my $value = $2;
> -	    if ($type eq 'S') {
> -		if ($value =~ /(maintain|support)/i) {
> -		    return 1;
> -		}
> -	    }
> -	}
> -    }
> -    return 0;
> -}
> -
>  sub range_has_maintainer {
>      my ($start, $end) = @_;
>  
> @@ -602,7 +584,6 @@ sub get_maintainers {
>  				$value_pd++ if (substr($value,-1,1) ne "/");
>  				$value_pd = -1 if ($value =~ /^\.\*/);
>  				if ($value_pd >= $file_pd &&
> -				    range_is_maintained($start, $end) &&
>  				    range_has_maintainer($start, $end)) {
>  				    $exact_pattern_match_hash{$file} = 1;
>  				}
> -- 
> 1.8.3.1
> 

[Qemu-devel] [PATCH 3/4] get_maintainer.pl: move git loop under "if ($email) {"

[Paolo Bonzini]

All checks in the loop are guarded by that condition, and there is a
handy "if" just below.  Simplify the code.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 scripts/get_maintainer.pl | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/scripts/get_maintainer.pl b/scripts/get_maintainer.pl
index 6c5a73b..7c6d186 100755
--- a/scripts/get_maintainer.pl
+++ b/scripts/get_maintainer.pl
@@ -632,18 +632,17 @@ sub get_maintainers {
 	$email->[0] = deduplicate_email($email->[0]);
     }
 
-    foreach my $file (@files) {
-	if ($email &&
-	    ($email_git || ($email_git_fallback &&
-			    !$exact_pattern_match_hash{$file}))) {
-	    vcs_file_signoffs($file);
-	}
-	if ($email && $email_git_blame) {
-	    vcs_file_blame($file);
+    if ($email) {
+	foreach my $file (@files) {
+	    if ($email_git || ($email_git_fallback &&
+			       !$exact_pattern_match_hash{$file})) {
+	        vcs_file_signoffs($file);
+	    }
+	    if ($email_git_blame) {
+	        vcs_file_blame($file);
+	    }
 	}
-    }
 
-    if ($email) {
 	foreach my $chief (@penguin_chief) {
 	    if ($chief =~ m/^(.*):(.*)/) {
 		my $email_address;
-- 
1.8.3.1

[Qemu-devel] [PATCH 4/4] get_maintainer.pl: point at --git-fallback instead of enabling it automatically

[Paolo Bonzini]

The list emitted by --git-fallback often leads inexperienced contributors
to add pointless CCs.  While not discouraging usage of --git-fallback,
we want to warn the contributors about using their common sense.

So, default to *not* enabling --git-fallback, but print a message if
none of the files has a match against MAINTAINERS.  Of course the
message is hidden by --no-git-fallback.

Examples:

1) No maintainer for all specified files, print message:

    $ scripts/get_maintainer.pl -f util/cutils.c
    No maintainers found.
    You may want to try --git-fallback to find recent contributors.
    Do not blindly cc: them on patches!  Use common sense.

2) No maintainer for some of the specified files, behave entirely
as if the user specified --no-git-fallback.

    $ scripts/get_maintainer.pl -f util/cutils.c hw/ide/core.c
    Kevin Wolf <kwolf@redhat.com> (odd fixer:IDE)
    Stefan Hajnoczi <stefanha@redhat.com> (odd fixer:IDE)

3) Explicit disable, do not print message:

    $ scripts/get_maintainer.pl -f util/cutils.c   --no-git-fallback
    $ echo $?
    1

Suggested-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 scripts/get_maintainer.pl | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/scripts/get_maintainer.pl b/scripts/get_maintainer.pl
index 7c6d186..c8537fd 100755
--- a/scripts/get_maintainer.pl
+++ b/scripts/get_maintainer.pl
@@ -28,7 +28,7 @@ my $email_git = 0;
 my $email_git_all_signature_types = 0;
 my $email_git_blame = 0;
 my $email_git_blame_signatures = 1;
-my $email_git_fallback = 1;
+my $email_git_fallback = undef;
 my $email_git_min_signatures = 1;
 my $email_git_max_maintainers = 5;
 my $email_git_min_percent = 5;
@@ -235,6 +235,7 @@ if (-t STDIN && !@ARGV) {
 }
 
 $output_multiline = 0 if ($output_separator ne ", ");
+$email_git_fallback = 1 if ($interactive && ! defined $email_git_fallback);
 $output_rolestats = 1 if ($interactive);
 $output_roles = 1 if ($output_rolestats);
 
@@ -633,6 +634,14 @@ sub get_maintainers {
     }
 
     if ($email) {
+	if (@email_to == 0 && @list_to == 0 &&
+            ! $email_git && ! $email_git_blame && ! defined $email_git_fallback) {
+	    print STDERR "No maintainers found.\n";
+	    print STDERR "You may want to try --git-fallback to find recent contributors.\n";
+	    print STDERR "Do not blindly cc: them on patches!  Use common sense.\n";
+	}
+
+	$email_git_fallback = 0 if ! defined $email_git_fallback;
 	foreach my $file (@files) {
 	    if ($email_git || ($email_git_fallback &&
 			       !$exact_pattern_match_hash{$file})) {
@@ -711,7 +720,7 @@ MAINTAINER field selection options:
     --git => include recent git \*-by: signers
     --git-all-signature-types => include signers regardless of signature type
         or use only ${signature_pattern} signers (default: $email_git_all_signature_types)
-    --git-fallback => use git when no exact MAINTAINERS pattern (default: $email_git_fallback)
+    --git-fallback => use git when no exact MAINTAINERS pattern (default: same value as --interactive)
     --git-chief-penguins => include ${penguin_chiefs}
     --git-min-signatures => number of signatures required (default: $email_git_min_signatures)
     --git-max-maintainers => maximum maintainers to add (default: $email_git_max_maintainers)
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 4/4] get_maintainer.pl: point at --git-fallback instead of enabling it automatically

[Michael S. Tsirkin]

On Wed, Oct 22, 2014 at 11:08:22AM +0200, Paolo Bonzini wrote:
> The list emitted by --git-fallback often leads inexperienced contributors
> to add pointless CCs.  While not discouraging usage of --git-fallback,
> we want to warn the contributors about using their common sense.
> 
> So, default to *not* enabling --git-fallback, but print a message if
> none of the files has a match against MAINTAINERS.  Of course the
> message is hidden by --no-git-fallback.
> 
> Examples:
> 
> 1) No maintainer for all specified files, print message:
> 
>     $ scripts/get_maintainer.pl -f util/cutils.c
>     No maintainers found.
>     You may want to try --git-fallback to find recent contributors.
>     Do not blindly cc: them on patches!  Use common sense.
>

Does it make sense for util/cutils.c?
I doubt it, so we are just giving useless advice?

--git-blame might be a better fallback here?
How about an entry in MAINTAINERS to trigger git-blame?

 
> 2) No maintainer for some of the specified files, behave entirely
> as if the user specified --no-git-fallback.
> 
>     $ scripts/get_maintainer.pl -f util/cutils.c hw/ide/core.c
>     Kevin Wolf <kwolf@redhat.com> (odd fixer:IDE)
>     Stefan Hajnoczi <stefanha@redhat.com> (odd fixer:IDE)
> 
> 3) Explicit disable, do not print message:
> 
>     $ scripts/get_maintainer.pl -f util/cutils.c   --no-git-fallback
>     $ echo $?
>     1
> 
> Suggested-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  scripts/get_maintainer.pl | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/scripts/get_maintainer.pl b/scripts/get_maintainer.pl
> index 7c6d186..c8537fd 100755
> --- a/scripts/get_maintainer.pl
> +++ b/scripts/get_maintainer.pl
> @@ -28,7 +28,7 @@ my $email_git = 0;
>  my $email_git_all_signature_types = 0;
>  my $email_git_blame = 0;
>  my $email_git_blame_signatures = 1;
> -my $email_git_fallback = 1;
> +my $email_git_fallback = undef;
>  my $email_git_min_signatures = 1;
>  my $email_git_max_maintainers = 5;
>  my $email_git_min_percent = 5;
> @@ -235,6 +235,7 @@ if (-t STDIN && !@ARGV) {
>  }
>  
>  $output_multiline = 0 if ($output_separator ne ", ");
> +$email_git_fallback = 1 if ($interactive && ! defined $email_git_fallback);
>  $output_rolestats = 1 if ($interactive);
>  $output_roles = 1 if ($output_rolestats);
>  
> @@ -633,6 +634,14 @@ sub get_maintainers {
>      }
>  
>      if ($email) {
> +	if (@email_to == 0 && @list_to == 0 &&
> +            ! $email_git && ! $email_git_blame && ! defined $email_git_fallback) {
> +	    print STDERR "No maintainers found.\n";
> +	    print STDERR "You may want to try --git-fallback to find recent contributors.\n";

So let's just do this for the user?

> +	    print STDERR "Do not blindly cc: them on patches!  Use common sense.\n";

Can we do better than "Use common sense"?

I doubt such advice accomplishes much.

> +	}
> +
> +	$email_git_fallback = 0 if ! defined $email_git_fallback;
>  	foreach my $file (@files) {
>  	    if ($email_git || ($email_git_fallback &&
>  			       !$exact_pattern_match_hash{$file})) {
> @@ -711,7 +720,7 @@ MAINTAINER field selection options:
>      --git => include recent git \*-by: signers
>      --git-all-signature-types => include signers regardless of signature type
>          or use only ${signature_pattern} signers (default: $email_git_all_signature_types)
> -    --git-fallback => use git when no exact MAINTAINERS pattern (default: $email_git_fallback)
> +    --git-fallback => use git when no exact MAINTAINERS pattern (default: same value as --interactive)
>      --git-chief-penguins => include ${penguin_chiefs}
>      --git-min-signatures => number of signatures required (default: $email_git_min_signatures)
>      --git-max-maintainers => maximum maintainers to add (default: $email_git_max_maintainers)
> -- 
> 1.8.3.1

Re: [Qemu-devel] [PATCH 4/4] get_maintainer.pl: point at --git-fallback instead of enabling it automatically

[Paolo Bonzini]

On 10/22/2014 11:33 AM, Michael S. Tsirkin wrote:
> On Wed, Oct 22, 2014 at 11:08:22AM +0200, Paolo Bonzini wrote:
>> The list emitted by --git-fallback often leads inexperienced contributors
>> to add pointless CCs.  While not discouraging usage of --git-fallback,
>> we want to warn the contributors about using their common sense.
>>
>> So, default to *not* enabling --git-fallback, but print a message if
>> none of the files has a match against MAINTAINERS.  Of course the
>> message is hidden by --no-git-fallback.
>>
>> Examples:
>>
>> 1) No maintainer for all specified files, print message:
>>
>>     $ scripts/get_maintainer.pl -f util/cutils.c
>>     No maintainers found.
>>     You may want to try --git-fallback to find recent contributors.
>>     Do not blindly cc: them on patches!  Use common sense.
>>
> 
> Does it make sense for util/cutils.c?
> I doubt it, so we are just giving useless advice?
> 
> --git-blame might be a better fallback here?
> How about an entry in MAINTAINERS to trigger git-blame?

We cannot know which is better.  The right thing to do would be to use
git-blame *manually*, so as to find who touched the function you are
touching now.

But for larger patches, one can hope that at least one file is covered
by MAINTAINERS, in which case the error will not be shown.  One can also
hope that removing range_is_maintained avoids triggering the error
message too often.

>  
>> 2) No maintainer for some of the specified files, behave entirely
>> as if the user specified --no-git-fallback.
>>
>>     $ scripts/get_maintainer.pl -f util/cutils.c hw/ide/core.c
>>     Kevin Wolf <kwolf@redhat.com> (odd fixer:IDE)
>>     Stefan Hajnoczi <stefanha@redhat.com> (odd fixer:IDE)
>>
>> 3) Explicit disable, do not print message:
>>
>>     $ scripts/get_maintainer.pl -f util/cutils.c   --no-git-fallback
>>     $ echo $?
>>     1
>>
>> Suggested-by: Markus Armbruster <armbru@redhat.com>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> ---
>>  scripts/get_maintainer.pl | 13 +++++++++++--
>>  1 file changed, 11 insertions(+), 2 deletions(-)
>>
>> diff --git a/scripts/get_maintainer.pl b/scripts/get_maintainer.pl
>> index 7c6d186..c8537fd 100755
>> --- a/scripts/get_maintainer.pl
>> +++ b/scripts/get_maintainer.pl
>> @@ -28,7 +28,7 @@ my $email_git = 0;
>>  my $email_git_all_signature_types = 0;
>>  my $email_git_blame = 0;
>>  my $email_git_blame_signatures = 1;
>> -my $email_git_fallback = 1;
>> +my $email_git_fallback = undef;
>>  my $email_git_min_signatures = 1;
>>  my $email_git_max_maintainers = 5;
>>  my $email_git_min_percent = 5;
>> @@ -235,6 +235,7 @@ if (-t STDIN && !@ARGV) {
>>  }
>>  
>>  $output_multiline = 0 if ($output_separator ne ", ");
>> +$email_git_fallback = 1 if ($interactive && ! defined $email_git_fallback);
>>  $output_rolestats = 1 if ($interactive);
>>  $output_roles = 1 if ($output_rolestats);
>>  
>> @@ -633,6 +634,14 @@ sub get_maintainers {
>>      }
>>  
>>      if ($email) {
>> +	if (@email_to == 0 && @list_to == 0 &&
>> +            ! $email_git && ! $email_git_blame && ! defined $email_git_fallback) {
>> +	    print STDERR "No maintainers found.\n";
>> +	    print STDERR "You may want to try --git-fallback to find recent contributors.\n";
> 
> So let's just do this for the user?

That would be the current behavior.  You do want users to think about
*why* they are CCing a bunch of people, especially those that use
get_maintainer.pl as a cccmd.

>> +	    print STDERR "Do not blindly cc: them on patches!  Use common sense.\n";
> 
> Can we do better than "Use common sense"?
> 
> I doubt such advice accomplishes much.

Perhaps not, but you have to start somewhere.  This solution seemed to
have at least some consensus.

Paolo

>> +	}
>> +
>> +	$email_git_fallback = 0 if ! defined $email_git_fallback;
>>  	foreach my $file (@files) {
>>  	    if ($email_git || ($email_git_fallback &&
>>  			       !$exact_pattern_match_hash{$file})) {
>> @@ -711,7 +720,7 @@ MAINTAINER field selection options:
>>      --git => include recent git \*-by: signers
>>      --git-all-signature-types => include signers regardless of signature type
>>          or use only ${signature_pattern} signers (default: $email_git_all_signature_types)
>> -    --git-fallback => use git when no exact MAINTAINERS pattern (default: $email_git_fallback)
>> +    --git-fallback => use git when no exact MAINTAINERS pattern (default: same value as --interactive)
>>      --git-chief-penguins => include ${penguin_chiefs}
>>      --git-min-signatures => number of signatures required (default: $email_git_min_signatures)
>>      --git-max-maintainers => maximum maintainers to add (default: $email_git_max_maintainers)
>> -- 
>> 1.8.3.1
> 
> 

Re: [Qemu-devel] [PATCH 4/4] get_maintainer.pl: point at --git-fallback instead of enabling it automatically

[Thomas Huth]

On Wed, 22 Oct 2014 11:56:30 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> 
> 
> On 10/22/2014 11:33 AM, Michael S. Tsirkin wrote:
> > On Wed, Oct 22, 2014 at 11:08:22AM +0200, Paolo Bonzini wrote:
> >> The list emitted by --git-fallback often leads inexperienced contributors
> >> to add pointless CCs.  While not discouraging usage of --git-fallback,
> >> we want to warn the contributors about using their common sense.
> >>
> >> So, default to *not* enabling --git-fallback, but print a message if
> >> none of the files has a match against MAINTAINERS.  Of course the
> >> message is hidden by --no-git-fallback.
> >>
> >> Examples:
> >>
> >> 1) No maintainer for all specified files, print message:
> >>
> >>     $ scripts/get_maintainer.pl -f util/cutils.c
> >>     No maintainers found.
> >>     You may want to try --git-fallback to find recent contributors.
> >>     Do not blindly cc: them on patches!  Use common sense.
> >>
> > 
> > Does it make sense for util/cutils.c?
> > I doubt it, so we are just giving useless advice?
> > 
> > --git-blame might be a better fallback here?
> > How about an entry in MAINTAINERS to trigger git-blame?
> 
> We cannot know which is better.  The right thing to do would be to use
> git-blame *manually*, so as to find who touched the function you are
> touching now.

Maybe you could at least also add "--git-blame" to the message that is
printed out here - so in case --git-fallback does not print anything at
all due to the 1-year limitation, the user has at least another thing to
try?

 Thomas

Re: [Qemu-devel] [PATCH 4/4] get_maintainer.pl: point at --git-fallback instead of enabling it automatically

[Michael S. Tsirkin]

On Wed, Oct 22, 2014 at 11:56:30AM +0200, Paolo Bonzini wrote:
> 
> 
> On 10/22/2014 11:33 AM, Michael S. Tsirkin wrote:
> > On Wed, Oct 22, 2014 at 11:08:22AM +0200, Paolo Bonzini wrote:
> >> The list emitted by --git-fallback often leads inexperienced contributors
> >> to add pointless CCs.  While not discouraging usage of --git-fallback,
> >> we want to warn the contributors about using their common sense.
> >>
> >> So, default to *not* enabling --git-fallback, but print a message if
> >> none of the files has a match against MAINTAINERS.  Of course the
> >> message is hidden by --no-git-fallback.
> >>
> >> Examples:
> >>
> >> 1) No maintainer for all specified files, print message:
> >>
> >>     $ scripts/get_maintainer.pl -f util/cutils.c
> >>     No maintainers found.
> >>     You may want to try --git-fallback to find recent contributors.
> >>     Do not blindly cc: them on patches!  Use common sense.
> >>
> > 
> > Does it make sense for util/cutils.c?
> > I doubt it, so we are just giving useless advice?
> > 
> > --git-blame might be a better fallback here?
> > How about an entry in MAINTAINERS to trigger git-blame?
> 
> We cannot know which is better.  The right thing to do would be to use
> git-blame *manually*, so as to find who touched the function you are
> touching now.

Why would doing it manually be any better than doing it automatically?

> But for larger patches, one can hope that at least one file is covered
> by MAINTAINERS, in which case the error will not be shown.

Well if you are saying it's a rare condition, can we ignore it for now?

We don't need to get it 100% right and should err preferably on the side
of Cc too many people.

>  One can also
> hope that removing range_is_maintained avoids triggering the error
> message too often.

I picked that one up.
Thanks!

> >  
> >> 2) No maintainer for some of the specified files, behave entirely
> >> as if the user specified --no-git-fallback.
> >>
> >>     $ scripts/get_maintainer.pl -f util/cutils.c hw/ide/core.c
> >>     Kevin Wolf <kwolf@redhat.com> (odd fixer:IDE)
> >>     Stefan Hajnoczi <stefanha@redhat.com> (odd fixer:IDE)
> >>
> >> 3) Explicit disable, do not print message:
> >>
> >>     $ scripts/get_maintainer.pl -f util/cutils.c   --no-git-fallback
> >>     $ echo $?
> >>     1
> >>
> >> Suggested-by: Markus Armbruster <armbru@redhat.com>
> >> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> >> ---
> >>  scripts/get_maintainer.pl | 13 +++++++++++--
> >>  1 file changed, 11 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/scripts/get_maintainer.pl b/scripts/get_maintainer.pl
> >> index 7c6d186..c8537fd 100755
> >> --- a/scripts/get_maintainer.pl
> >> +++ b/scripts/get_maintainer.pl
> >> @@ -28,7 +28,7 @@ my $email_git = 0;
> >>  my $email_git_all_signature_types = 0;
> >>  my $email_git_blame = 0;
> >>  my $email_git_blame_signatures = 1;
> >> -my $email_git_fallback = 1;
> >> +my $email_git_fallback = undef;
> >>  my $email_git_min_signatures = 1;
> >>  my $email_git_max_maintainers = 5;
> >>  my $email_git_min_percent = 5;
> >> @@ -235,6 +235,7 @@ if (-t STDIN && !@ARGV) {
> >>  }
> >>  
> >>  $output_multiline = 0 if ($output_separator ne ", ");
> >> +$email_git_fallback = 1 if ($interactive && ! defined $email_git_fallback);
> >>  $output_rolestats = 1 if ($interactive);
> >>  $output_roles = 1 if ($output_rolestats);
> >>  
> >> @@ -633,6 +634,14 @@ sub get_maintainers {
> >>      }
> >>  
> >>      if ($email) {
> >> +	if (@email_to == 0 && @list_to == 0 &&
> >> +            ! $email_git && ! $email_git_blame && ! defined $email_git_fallback) {
> >> +	    print STDERR "No maintainers found.\n";
> >> +	    print STDERR "You may want to try --git-fallback to find recent contributors.\n";
> > 
> > So let's just do this for the user?
> 
> That would be the current behavior.  You do want users to think about
> *why* they are CCing a bunch of people, especially those that use
> get_maintainer.pl as a cccmd.

Interesting that you should mention cccmd.
First time one hits it, one adds --git-fallback in cccmd and never
sees this again.
Does not sounds too useful.


> >> +	    print STDERR "Do not blindly cc: them on patches!  Use common sense.\n";
> > 
> > Can we do better than "Use common sense"?
> > 
> > I doubt such advice accomplishes much.
> 
> Perhaps not, but you have to start somewhere.  This solution seemed to
> have at least some consensus.
> 
> Paolo

We already found the "Odd fixes" bug because of the current behaviour.

So I think if we keep the status quo, some contributors will be
sufficiently annoyed to take real action, and either enhance
MAINTAINERS, or get_maintainer, or write a wiki page,
or refactor code, to make finding maintainers easier ;).

Applying this badaid will make the problem go away, and
no one will work on a solution.


> >> +	}
> >> +
> >> +	$email_git_fallback = 0 if ! defined $email_git_fallback;
> >>  	foreach my $file (@files) {
> >>  	    if ($email_git || ($email_git_fallback &&
> >>  			       !$exact_pattern_match_hash{$file})) {
> >> @@ -711,7 +720,7 @@ MAINTAINER field selection options:
> >>      --git => include recent git \*-by: signers
> >>      --git-all-signature-types => include signers regardless of signature type
> >>          or use only ${signature_pattern} signers (default: $email_git_all_signature_types)
> >> -    --git-fallback => use git when no exact MAINTAINERS pattern (default: $email_git_fallback)
> >> +    --git-fallback => use git when no exact MAINTAINERS pattern (default: same value as --interactive)
> >>      --git-chief-penguins => include ${penguin_chiefs}
> >>      --git-min-signatures => number of signatures required (default: $email_git_min_signatures)
> >>      --git-max-maintainers => maximum maintainers to add (default: $email_git_max_maintainers)
> >> -- 
> >> 1.8.3.1
> > 
> > 

Re: [Qemu-devel] [PATCH 4/4] get_maintainer.pl: point at --git-fallback instead of enabling it automatically

[Paolo Bonzini]

On 10/22/2014 12:17 PM, Michael S. Tsirkin wrote:
> On Wed, Oct 22, 2014 at 11:56:30AM +0200, Paolo Bonzini wrote:
>>
>>
>> On 10/22/2014 11:33 AM, Michael S. Tsirkin wrote:
>>> On Wed, Oct 22, 2014 at 11:08:22AM +0200, Paolo Bonzini wrote:
>>>> The list emitted by --git-fallback often leads inexperienced contributors
>>>> to add pointless CCs.  While not discouraging usage of --git-fallback,
>>>> we want to warn the contributors about using their common sense.
>>>>
>>>> So, default to *not* enabling --git-fallback, but print a message if
>>>> none of the files has a match against MAINTAINERS.  Of course the
>>>> message is hidden by --no-git-fallback.
>>>>
>>>> Examples:
>>>>
>>>> 1) No maintainer for all specified files, print message:
>>>>
>>>>     $ scripts/get_maintainer.pl -f util/cutils.c
>>>>     No maintainers found.
>>>>     You may want to try --git-fallback to find recent contributors.
>>>>     Do not blindly cc: them on patches!  Use common sense.
>>>>
>>>
>>> Does it make sense for util/cutils.c?
>>> I doubt it, so we are just giving useless advice?
>>>
>>> --git-blame might be a better fallback here?
>>> How about an entry in MAINTAINERS to trigger git-blame?
>>
>> We cannot know which is better.  The right thing to do would be to use
>> git-blame *manually*, so as to find who touched the function you are
>> touching now.
> 
> Why would doing it manually be any better than doing it automatically?

As far as I understand, --git-blame looks at the overall author of a
file.  What you usually want is to find the author of the _function_
that you are modifying.

>> But for larger patches, one can hope that at least one file is covered
>> by MAINTAINERS, in which case the error will not be shown.
> 
> Well if you are saying it's a rare condition, can we ignore it for now?

The error message is shown rarely.  But the patch also has an
improvement in the case mentioned above:

    $ scripts/get_maintainer.pl -f util/cutils.c hw/ide/core.c

Without the patch, it falls back to the commit_signer algorithm.  With
the patch, it expects that the IDE maintainers will do a decent job with
utils/cutils.c as well.  And it does not print any message.  I think
this is something we definitely want to keep, so I'll send v2.

> We don't need to get it 100% right and should err preferably on the side
> of Cc too many people.

You and I do not mind being CCed spuriously, but others have expressed
dissatisfaction.

>>>>      if ($email) {
>>>> +	if (@email_to == 0 && @list_to == 0 &&
>>>> +            ! $email_git && ! $email_git_blame && ! defined $email_git_fallback) {
>>>> +	    print STDERR "No maintainers found.\n";
>>>> +	    print STDERR "You may want to try --git-fallback to find recent contributors.\n";
>>>
>>> So let's just do this for the user?
>>
>> That would be the current behavior.  You do want users to think about
>> *why* they are CCing a bunch of people, especially those that use
>> get_maintainer.pl as a cccmd.
> 
> Interesting that you should mention cccmd.
> First time one hits it, one adds --git-fallback in cccmd and never
> sees this again.
> Does not sounds too useful.

Perhaps.  Or perhaps they also read the last line of the error:

Paolo

> 
>>>> +	}
>>>> +
>>>> +	$email_git_fallback = 0 if ! defined $email_git_fallback;
>>>>  	foreach my $file (@files) {
>>>>  	    if ($email_git || ($email_git_fallback &&
>>>>  			       !$exact_pattern_match_hash{$file})) {
>>>> @@ -711,7 +720,7 @@ MAINTAINER field selection options:
>>>>      --git => include recent git \*-by: signers
>>>>      --git-all-signature-types => include signers regardless of signature type
>>>>          or use only ${signature_pattern} signers (default: $email_git_all_signature_types)
>>>> -    --git-fallback => use git when no exact MAINTAINERS pattern (default: $email_git_fallback)
>>>> +    --git-fallback => use git when no exact MAINTAINERS pattern (default: same value as --interactive)
>>>>      --git-chief-penguins => include ${penguin_chiefs}
>>>>      --git-min-signatures => number of signatures required (default: $email_git_min_signatures)
>>>>      --git-max-maintainers => maximum maintainers to add (default: $email_git_max_maintainers)
>>>> -- 
>>>> 1.8.3.1
>>>
>>>
> 
> 

Re: [Qemu-devel] [PATCH 4/4] get_maintainer.pl: point at --git-fallback instead of enabling it automatically

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 10/22/2014 12:17 PM, Michael S. Tsirkin wrote:
>> On Wed, Oct 22, 2014 at 11:56:30AM +0200, Paolo Bonzini wrote:
>>>
>>>
>>> On 10/22/2014 11:33 AM, Michael S. Tsirkin wrote:
>>>> On Wed, Oct 22, 2014 at 11:08:22AM +0200, Paolo Bonzini wrote:
>>>>> The list emitted by --git-fallback often leads inexperienced contributors
>>>>> to add pointless CCs.  While not discouraging usage of --git-fallback,
>>>>> we want to warn the contributors about using their common sense.
>>>>>
>>>>> So, default to *not* enabling --git-fallback, but print a message if
>>>>> none of the files has a match against MAINTAINERS.  Of course the
>>>>> message is hidden by --no-git-fallback.
>>>>>
>>>>> Examples:
>>>>>
>>>>> 1) No maintainer for all specified files, print message:
>>>>>
>>>>>     $ scripts/get_maintainer.pl -f util/cutils.c
>>>>>     No maintainers found.
>>>>>     You may want to try --git-fallback to find recent contributors.
>>>>>     Do not blindly cc: them on patches!  Use common sense.
>>>>>
>>>>
>>>> Does it make sense for util/cutils.c?
>>>> I doubt it, so we are just giving useless advice?
>>>>
>>>> --git-blame might be a better fallback here?
>>>> How about an entry in MAINTAINERS to trigger git-blame?
>>>
>>> We cannot know which is better.  The right thing to do would be to use
>>> git-blame *manually*, so as to find who touched the function you are
>>> touching now.
>> 
>> Why would doing it manually be any better than doing it automatically?
>
> As far as I understand, --git-blame looks at the overall author of a
> file.  What you usually want is to find the author of the _function_
> that you are modifying.

When there's no maintainer, we want to make patch submitters stop and
*think*.  Automation achieves the opposite.  Making them do a bit of
manual work is a feature.

If you know what you're doing, you can put --git-fallback into your
.get_maintainer.conf.

>>> But for larger patches, one can hope that at least one file is covered
>>> by MAINTAINERS, in which case the error will not be shown.
>> 
>> Well if you are saying it's a rare condition, can we ignore it for now?
>
> The error message is shown rarely.  But the patch also has an
> improvement in the case mentioned above:
>
>     $ scripts/get_maintainer.pl -f util/cutils.c hw/ide/core.c
>
> Without the patch, it falls back to the commit_signer algorithm.  With
> the patch, it expects that the IDE maintainers will do a decent job with
> utils/cutils.c as well.  And it does not print any message.  I think
> this is something we definitely want to keep, so I'll send v2.
>
>> We don't need to get it 100% right and should err preferably on the side
>> of Cc too many people.
>
> You and I do not mind being CCed spuriously, but others have expressed
> dissatisfaction.

Michael, you seem to focus like a laser on optimizing the chance that a
patch gets reviewed.  That's worth optimizing, no argument.  But it
needs to be weighed against annoying contributors.

Moreover, reducing the number of useless copies increases the chances
the useful copies actually get read.  Applies even to me, and I believe
I'm fairly good at ignoring useless copies without getting annoyed.

>>>>>      if ($email) {
>>>>> +	if (@email_to == 0 && @list_to == 0 &&
>>>>> + ! $email_git && ! $email_git_blame && ! defined
>>>>> $email_git_fallback) {
>>>>> +	    print STDERR "No maintainers found.\n";
>>>>> + print STDERR "You may want to try --git-fallback to find recent
>>>>> contributors.\n";
>>>>
>>>> So let's just do this for the user?
>>>
>>> That would be the current behavior.  You do want users to think about
>>> *why* they are CCing a bunch of people, especially those that use
>>> get_maintainer.pl as a cccmd.
>> 
>> Interesting that you should mention cccmd.
>> First time one hits it, one adds --git-fallback in cccmd and never
>> sees this again.

It's not this script's job to stop people from indiscriminately copying
people when they want to.  The job is to guide towards good practice, as
far as that's practical here.

We can't stop people from putting --git-fallback in cccmd, and spray
random committers with copies on full auto.  When it happens, we'll tell
them to knock it off.

>> Does not sounds too useful.
>
> Perhaps.  Or perhaps they also read the last line of the error:

I like it, thanks for coding this up!

Re: [Qemu-devel] [PATCH 0/4] small improvements to get_maintainer.pl

[Thomas Huth]

On Wed, 22 Oct 2014 11:08:18 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> See individual patches.  My Perl-fu is limited, but the individual
> changes are easy enough.
> 
> Paolo Bonzini (4):
>   get_maintainer.pl: exit with status 1 if no maintainer found
>   get_maintainer.pl: treat all M entries the same
>   get_maintainer.pl: move git loop under "if ($email) {"
>   get_maintainer.pl: point at --git-fallback instead of enabling it
>     automatically
> 
>  scripts/get_maintainer.pl | 52 +++++++++++++++++++----------------------------
>  1 file changed, 21 insertions(+), 31 deletions(-)

I think these are good patches ... but while you're at it, I wonder
whether you should/could also replace the entry for "penguin_chief" in
that file. It still points to Linus Torvalds - but I assume he does not
like to be bothered with patches related to QEMU... ?

 Thomas

Re: [Qemu-devel] [PATCH 0/4] small improvements to get_maintainer.pl

[Michael S. Tsirkin]

On Wed, Oct 22, 2014 at 12:17:47PM +0200, Thomas Huth wrote:
> On Wed, 22 Oct 2014 11:08:18 +0200
> Paolo Bonzini <pbonzini@redhat.com> wrote:
> 
> > See individual patches.  My Perl-fu is limited, but the individual
> > changes are easy enough.
> > 
> > Paolo Bonzini (4):
> >   get_maintainer.pl: exit with status 1 if no maintainer found
> >   get_maintainer.pl: treat all M entries the same
> >   get_maintainer.pl: move git loop under "if ($email) {"
> >   get_maintainer.pl: point at --git-fallback instead of enabling it
> >     automatically
> > 
> >  scripts/get_maintainer.pl | 52 +++++++++++++++++++----------------------------
> >  1 file changed, 21 insertions(+), 31 deletions(-)
> 
> I think these are good patches ... but while you're at it, I wonder
> whether you should/could also replace the entry for "penguin_chief" in
> that file. It still points to Linus Torvalds - but I assume he does not
> like to be bothered with patches related to QEMU... ?
> 
>  Thomas

Let's just drop this flag.

-- 
MST

Re: [Qemu-devel] [PATCH 0/4] small improvements to get_maintainer.pl

[Paolo Bonzini]

On 10/22/2014 12:28 PM, Michael S. Tsirkin wrote:
> On Wed, Oct 22, 2014 at 12:17:47PM +0200, Thomas Huth wrote:
>> On Wed, 22 Oct 2014 11:08:18 +0200
>> Paolo Bonzini <pbonzini@redhat.com> wrote:
>>
>>> See individual patches.  My Perl-fu is limited, but the individual
>>> changes are easy enough.
>>>
>>> Paolo Bonzini (4):
>>>   get_maintainer.pl: exit with status 1 if no maintainer found
>>>   get_maintainer.pl: treat all M entries the same
>>>   get_maintainer.pl: move git loop under "if ($email) {"
>>>   get_maintainer.pl: point at --git-fallback instead of enabling it
>>>     automatically
>>>
>>>  scripts/get_maintainer.pl | 52 +++++++++++++++++++----------------------------
>>>  1 file changed, 21 insertions(+), 31 deletions(-)
>>
>> I think these are good patches ... but while you're at it, I wonder
>> whether you should/could also replace the entry for "penguin_chief" in
>> that file. It still points to Linus Torvalds - but I assume he does not
>> like to be bothered with patches related to QEMU... ?
>>
>>  Thomas
> 
> Let's just drop this flag.

I'll gladly leave that to someone else. :)

Paolo

Re: [Qemu-devel] [PATCH 0/4] small improvements to get_maintainer.pl

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> See individual patches.  My Perl-fu is limited, but the individual
> changes are easy enough.
>
> Paolo Bonzini (4):
>   get_maintainer.pl: exit with status 1 if no maintainer found
>   get_maintainer.pl: treat all M entries the same
>   get_maintainer.pl: move git loop under "if ($email) {"
>   get_maintainer.pl: point at --git-fallback instead of enabling it
>     automatically
>
>  scripts/get_maintainer.pl | 52 +++++++++++++++++++----------------------------
>  1 file changed, 21 insertions(+), 31 deletions(-)

Commit message of 1/4 could perhaps be improved slightly.  Regardless:

Reviewed-by: Markus Armbruster <armbru@redhat.com>