[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 4/4] get_maintainer.pl: point at --git-fallback
From: |
Michael S. Tsirkin |
Subject: |
Re: [Qemu-devel] [PATCH 4/4] get_maintainer.pl: point at --git-fallback instead of enabling it automatically |
Date: |
Wed, 22 Oct 2014 13:17:54 +0300 |
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 <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.
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
> >
> >
[Qemu-devel] [PATCH 2/4] get_maintainer.pl: treat all M entries the same, Paolo Bonzini, 2014/10/22
Re: [Qemu-devel] [PATCH 0/4] small improvements to get_maintainer.pl, Thomas Huth, 2014/10/22
Re: [Qemu-devel] [PATCH 0/4] small improvements to get_maintainer.pl, Markus Armbruster, 2014/10/22