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

Ian,

I addressed most of these locally, but have not dealt with the more functional 
changes such as options, etc.  However there are a few areas I was not planning 
to address or have questions.

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.

        +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

        +    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

However, I there is a conflict with arguments passed via the --args option.
I don't really want to add extra logic to deal with this, which means that
--l, --nol, --m, --nom, --r and --nor will be documented as options you
can't pass to get_maintainers.pl. I don't think anyone uses these. So
this should be fine.

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

Which included things like CC: To: ...
It can also pick up strings such as "Changed since v1:"
Etc.

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.

We could make this configurable:
Default: all tags, except signed-off-by (unless of course this should be added 
to the CC)
An option such as --mytags "reviewed-by: release-acked-by: tested-by: ..."

Regards
Lars

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