From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:49766) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XgtCA-0003bA-DE for qemu-devel@nongnu.org; Wed, 22 Oct 2014 06:31:17 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XgtC0-0003vw-Q2 for qemu-devel@nongnu.org; Wed, 22 Oct 2014 06:31:10 -0400 Received: from mail-wi0-x231.google.com ([2a00:1450:400c:c05::231]:38379) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XgtC0-0003vZ-FQ for qemu-devel@nongnu.org; Wed, 22 Oct 2014 06:31:00 -0400 Received: by mail-wi0-f177.google.com with SMTP id fb4so918391wid.10 for ; Wed, 22 Oct 2014 03:30:59 -0700 (PDT) Sender: Paolo Bonzini Message-ID: <54478760.1030807@redhat.com> Date: Wed, 22 Oct 2014 12:30:56 +0200 From: Paolo Bonzini MIME-Version: 1.0 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> <20141022101754.GA9701@redhat.com> In-Reply-To: <20141022101754.GA9701@redhat.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 8bit 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: "Michael S. Tsirkin" Cc: Markus Armbruster , qemu-devel 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 >>> >>> > >