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

[Ian Jackson <ian.jackson@xxxxxxxxxx>]

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:
> 
>         +# Get the list of patches
>         +my $pattern = $patch_dir.'/'.$patch_prefix.'*'.$patch_ext;
> 
>         This goes wrong if $patch_dir (or $patch_prefix) contains whitespace
>         or glob characters.  This will be fine in any reasonable Unix
>         environment, but there are corner cases where it may go wrong.  For
>         example, I am told that modern desktop environments mount removeable
>         storage media on a pathname containing the volume label (this seems
>         very unwise to me, but there you are).
> 
>         I don't think this is a problem for this script, but I thought I would
>         mention it.
> 
> I think I won't address this for now, but just out of interest, how would I 
> address this?
> If easy, I will just fix it.

Personally I would use opendir/readdir rather than glob, or perhaps
chdir to the patch directory (although whether that is a good idea
depends on whether there are other filename arguments to the script,
because their meaning would change).

>         +foreach my $file (@patches) {
> 
>         It would be nice to exclude ~ and .bak files here.  That way manually
>         editing files won't require trickery to exclude them.
> 
> I was not planning to address this, as it is not an issue, because of the 
> filter
> used to get @patches, which is <patch_dir>/0*.patch or <patch_dir>/vx*.patch 
> As such, *.patch~ and *.patch.bak are already excluded

Oh, yes of course.  Indeed, I was just wrong.

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

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.

That avoids calling get_maintainer.pl twice too, so helpfully the
other difficulties you discuss go away.

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

> 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

Ian.

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