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

[Paolo Bonzini]

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