Re: [Xen-devel] [PATCH for-4.11 v2 2/2] Add new add_maintainers.pl script to optimise the workflow when using git format-patch with get_maintainer.pl

[Lars Kurth <lars.kurth@xxxxxxxxxx>]

On 02/05/2018, 11:50, "Ian Jackson" <ian.jackson@xxxxxxxxxx> wrote:

    Lars Kurth writes ("Re: [PATCH for-4.11 v2 2/2] Add new add_maintainers.pl 
script to optimise the workflow when using git format-patch with 
get_maintainer.pl"):
    > On 30/04/2018, 15:36, "Ian Jackson" <ian.jackson@xxxxxxxxxx> wrote:
    >     
    
    >         +    while(<$fh>) {
    >         +        chomp;
    >         +        # Keep lists and CC's separately as we dont want them in
    >         +        # the commit message under a Cc: line
    >         +        if (index($_, $mailing_lists) != -1) {
    > 
    >         This is really very strange.  Firstly, I had to look for the
    >         definition of $mailing_lists.  It seems to be a variable for 
little
    >         reason, as it is not configurable.
    > 
    >         Secondly, what this is trying to do is look for the string 
'@lists.'
    >         anywhere in the CC.  But that is not a reliable way of 
identifying a
    >         mailing list.  I think in general it is not possible to do this
    >         reliably, but this is rather a suboptimal heuristic.
    > 
    >         Instead, I would additionally check to see if the address is 
mentioned
    >         in any L: line in MAINTAINERS.
    > 
    >         I would also allow the user to specify regexps for addresses to be
    >         treated as lists.  If you did that the the regexp \@lists\. would 
be a
    >         good default starting point.
    > 
    > What I am going to do there then is the following: call get_maintainers.pl
    > twice, with the options
    > --nol => that gets me the R: and M: e-mail addresses
    > --nom --nor => that gets me the L: e-mail addresses
    
    Err, I don't think this is quite right.  The question you are trying
    to ask in this bit of your script is "is this address a mailing list".

That is correct. And get_maintainers.pl --nom --nor gets me a list
of list addresses.
    
    If the address is a mailing list for some other stanza in MAINTAINERS,
    ie for a stanza whose files are not touched by this patch, then it
    should still be treated as a list.
    
    So what I meant was that you should search the whole of MAINTAINERS
    yourself for all the L: lines, regardless of where they appear.

But that is exactly what get_maintainers.pl --nom --nor does
    
    That avoids calling get_maintainer.pl twice too, so helpfully the
    other difficulties you discuss go away.

I suppose it does. But it also makes the script re-implement bits
of get_maintainers.pl 
    
    >         +        if ($rextra) {
    >         +            my $item;
    >         +            foreach $item (@tags) {
    >         +                if (hastag($line, $item, \$nline)) {
    >         +                    # Replace tag with CC, then push
    >         +                    $nline =~ s/$item/$CC/;
    > 
    >         I think this is not a sensible way to identify the tag part of the
    >         line.  Instead, why not use a regexp like ^[-A-Z0-9a-z]+: ?
    > 
    > I think I will leave this as-is for now. Right now, we pick up a 
    > known list of tags and add these to the CC list. What you propose 
    > would add every tag (including signed off by to the CC list).  
    
    Err, no ?  What I meant was something like this:
    
        sub hastag ($$)
        {
            my ($line, $tag) = @_;
            return $line =~ m{^\Q$tag\E\s*}i;
        }
    
        foreach my $tag (@tags) {
            if (hastag($line, $tag)) {
                my n$line = $line;
                $nline =~ s{^[-0-9a-z]+:}{}i;
                push @$rextra, $CC.$nline;
    
    or 
    
        sub hastag ($$;$)
        {
            my ($line, $tag, $rhs_r) = @_;
            my $hastag = $line =~ m{^\Q$tag\E*}i;
            $$rhs_r = $' if $rhs_r;
            return $hastag;
        }
    
        foreach my $tag (@tags) {
            my $rhs;
            if (hastag($line, $tag, \$rhs)) {
                push @$rextra, $CC.$rhs;
    
    (Other things I noticed while writing this:
     - if you say `foreach $something (@tags) {', $something should
       probably be $tag just so it's less confusing.
     - you want `foreach my $something ...' usually
     - you want /i on your regexps because you want to match case-insensitively
    
    > Maybe more appropriate would be
    > <tag>-by: <email>
    > Although I don't know what the reg-ex is: 
    > ^[-A-Z0-9a-z]-by+: does not work.
    
    I like your <something>-by idea.  That would catch "suggested-by",
    "reported-by", etc., and it's really nice to CC those people
    automatically.  I think the regexp is:
    
      /^[-0-9a-z]+-by:/i
    
    The + needs to come after the [ ] because it's what lets that part
    match more than one character.
    
    > We could make this configurable:
    > Default: all tags, except signed-off-by (unless of course this should be 
added to the CC)
    
    Why not CC the S-o-b ?  Usually that will be the author anyway.
    
I wasn't sure. 

Particularly if you used -i ccbody, which we agreed in another mail should
be the default, you then will end up with
CC: lars.kurth@xxxxxxxxxx
...
Signed-off-by: lars.kurth@xxxxxxxxxx

Which would annoy me personally.
 
    > An option such as --mytags "reviewed-by: release-acked-by: tested-by: ..."
    
    That would be a fancy feature, certainly.  How about
       --tag original-author
    which would add "Original-Author:" to the set of things recognised,
    and can be repeated, and
       --no-by-tags
    which suppresses "...-by" from the list.  So your example would be
       --no-by-tags --tag reviewed-by --tag release-acked-by --tag tested-by
    
How do I handle multiple --tag options in GetOptions

Let me think about this otherwise. In any case, we have a fairly long list of 
extra features and behaviours and I spent already far too much time on this. 
So I am thinking of addressing core features and the rest as separate oatches.

Lars

    

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel