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 <address@hidden> (odd fixer:IDE)
>>     Stefan Hajnoczi <address@hidden> (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 <address@hidden>
>> Signed-off-by: Paolo Bonzini <address@hidden>
>> ---
>>  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 && address@hidden) {
>>  }
>>  
>>  $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
> 
>