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

["Michael S. Tsirkin" <mst@redhat.com>]

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