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

[Markus Armbruster]

Paolo Bonzini <address@hidden> writes:

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

When there's no maintainer, we want to make patch submitters stop and
*think*.  Automation achieves the opposite.  Making them do a bit of
manual work is a feature.

If you know what you're doing, you can put --git-fallback into your
.get_maintainer.conf.

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

Michael, you seem to focus like a laser on optimizing the chance that a
patch gets reviewed.  That's worth optimizing, no argument.  But it
needs to be weighed against annoying contributors.

Moreover, reducing the number of useless copies increases the chances
the useful copies actually get read.  Applies even to me, and I believe
I'm fairly good at ignoring useless copies without getting annoyed.

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

It's not this script's job to stop people from indiscriminately copying
people when they want to.  The job is to guide towards good practice, as
far as that's practical here.

We can't stop people from putting --git-fallback in cccmd, and spray
random committers with copies on full auto.  When it happens, we'll tell
them to knock it off.

>> Does not sounds too useful.
>
> Perhaps.  Or perhaps they also read the last line of the error:

I like it, thanks for coding this up!