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 <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?

> +         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