From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:45297) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Xgsvy-00059l-FC for qemu-devel@nongnu.org; Wed, 22 Oct 2014 06:14:32 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Xgsvs-0004dt-Vf for qemu-devel@nongnu.org; Wed, 22 Oct 2014 06:14:26 -0400 Received: from mx1.redhat.com ([209.132.183.28]:22767) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Xgsvs-0004df-Ow for qemu-devel@nongnu.org; Wed, 22 Oct 2014 06:14:20 -0400 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id s9MAEKKi014078 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL) for ; Wed, 22 Oct 2014 06:14:20 -0400 Date: Wed, 22 Oct 2014 13:17:54 +0300 From: "Michael S. Tsirkin" Message-ID: <20141022101754.GA9701@redhat.com> References: <1413968902-24094-1-git-send-email-pbonzini@redhat.com> <1413968902-24094-5-git-send-email-pbonzini@redhat.com> <20141022093339.GB9425@redhat.com> <54477F4E.6020002@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <54477F4E.6020002@redhat.com> Subject: Re: [Qemu-devel] [PATCH 4/4] get_maintainer.pl: point at --git-fallback instead of enabling it automatically List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini Cc: Markus Armbruster , qemu-devel 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 (odd fixer:IDE) > >> Stefan Hajnoczi (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 > >> Signed-off-by: Paolo Bonzini > >> --- > >> 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 > > > >