[PATCH for-4.11 v3 0/1] Add new add_maintainers.pl script to optimise the workflow when using git format-patch with get_maintainer.pl

[PATCH for-4.11 v3 0/1] Add new add_maintainers.pl script to optimise the workflow when using git format-patch with get_maintainer.pl

[Lars Kurth]

This version of this series addresses all comments on the mailing list, as well
as some feedback I got in various personal conversations and/or on IRC. For
the people who asked for specific features/workflows:

Ian Jackson: use ./scripts/add_maintainers.pl -p none [-c top]
Reads CCs from unmodified *.patch files and inserts them into the cover letter

George Dunlap: use ./scripts/add_maintainers.pl -p cc---
Tends to add CC blocks after the --- line in *.patches. This option achieves
this behavior/

Julien Grall: use ./scripts/add_maintainers.pl -c ccend
As far as I recall, Julien adds CC blocks into the body of the cover letter.
This option achieves this, but there is no place that always exists other
than before "-- " where the CC block can be insterted.

I made the processing code easily extendable via policies. So if there is any
missed behavior, the tool can easily be extended.

Also note that git send-email does not automatically add people in *=by:
tags to CC lists (with the exception of Singed-off-by). For this I added
the options -t|--tags and --tagscc.

v2 of this patch contained some cleanup to MAINTAINERS which has been broken
out into a separate series: see
https://lists.xenproject.org/archives/html/xen-devel/2018-05/threads.html#00028

Lars Kurth (1):
  Add new add_maintainers.pl script to optimise the workflow when using
    git format-patch with get_maintainer.pl

 scripts/add_maintainers.pl | 512 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 512 insertions(+)
 create mode 100755 scripts/add_maintainers.pl

-- 
2.13.0


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

[PATCH for-4.11 v3 1/1] Add new add_maintainers.pl script to optimise the workflow when using git format-patch with get_maintainer.pl

[Lars Kurth]

The tool covers step 2 of the following workflow

  Step 1: git format-patch ... -o <patchdir> ...
  Step 2: ./scripts/add_maintainers.pl -d <patchdir>
          This overwrites  *.patch files in <patchdir>
  Step 3: git send-email -to xen-devel@lists.xenproject.org <patchdir>/*.patchxm

I manually tested all options and the most common combinations
on Mac.

Changes since v1:
- Added RAB (indicated by Juergen on IRC that this is OK)
- Remove trailing whitespaces
- Renamed --prefix to --reroll-count
- Cleaned up short options -v, ... to be in line with git
- Added --tags|-t option to add AB, RAB and RB emails to CC list
- Added --insert|-i mode to allow for people adding CCs to commit message
  instead of the e-mail header (the header is the default)
- Moved common code into functions
- Added logic, such that the tool only insert's To: and Cc: statements
  which were not there before, allowing for running the tool multiple times
  on the same <patchdir>

Changes since v2:
- Deleted --version and related infrastructure
- Added subroutine prototypes
- Removed AT and @lists declaration and used \@ in literals
- Changed usage message and options based on feedback
- Improved error handling
- Removed occurances of index() and replaced with regex
- Removed non-perl idioms
- Moved uniq statements to normalize and added info on what normalize does
- Read L: tags from MAINTAINERS file instead of using heuristic
- Fixed issues related to metacharacters in getmaintainers()
- Allow multiple -a | --arg values (because of this renamed --args)
- Identify tags via regex
- CC's from tags are only inserted in the mail header, never the body
- That is unless the new option --tagscc is used
- Added policy processing which includes reworking insert()
- Replaced -i|--insert with -p|--inspatch and -c|--inscover now using policies
- Added new policies to cover for all user requests
- Rewrote help message to center around usage of policies
- Reordered some code (e.g. help string first to make code more easily readable)

Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: George Dunlap <George.Dunlap@eu.citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Julien Grall <julien.grall@arm.com>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Tim Deegan <tim@xen.org>
Cc: Wei Liu <wei.liu2@citrix.com>
Signed-off-by: Lars Kurth <lars.kurth@citrix.com>
Release-acked-by: Juergen Gross <jgross@suse.com>
---
 scripts/add_maintainers.pl | 512 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 512 insertions(+)
 create mode 100755 scripts/add_maintainers.pl

diff --git a/scripts/add_maintainers.pl b/scripts/add_maintainers.pl
new file mode 100755
index 0000000000..11ae60d888
--- /dev/null
+++ b/scripts/add_maintainers.pl
@@ -0,0 +1,512 @@
+#!/usr/bin/perl -w
+# (c) 2018, Lars Kurth <lars.kurth@citrix.com>
+#
+# Add maintainers to patches generated with git format-patch
+#
+# Usage: perl scripts/add_maintainers.pl [OPTIONS] -patchdir <patchdir>
+#
+# Prerequisites: Execute
+#                git format-patch ... -o <patchdir> ...
+#
+#                ./scripts/get_maintainer.pl is present in the tree
+#
+# Licensed under the terms of the GNU GPL License version 2
+
+use strict;
+
+use Getopt::Long qw(:config no_auto_abbrev);
+use File::Basename;
+use List::MoreUtils qw(uniq);
+
+sub getmaintainers ($$$);
+sub gettagsfrompatch ($$$;$);
+sub normalize ($$);
+sub insert ($$$$);
+sub hastag ($$);
+
+# Tool Variables
+my $tool = $0;
+my $usage = <<EOT;
+USAGE: $tool [options] (--patchdir | -d) <patchdir>
+
+OPTIONS:
+--------
+  --reroll-count <n> | -v <n>
+    Choose patch files for specific version. This results into the
+    following filters on <patchdir>
+    0: default - *.patch
+    >1: v<n>*.patch
+  --inspatch (top|ccbody|cc---|none) | -p (top|ccbody|cc---|none)
+    Insert email addresses into *.patch files according to the POLICY
+    See section POLICY:
+  --inscover (top|ccend|none) | -c (top|ccend|none)
+    Insert email addresses into cover letteraccording to the POLICY
+    See section PROCESSING POLICY:
+  --tags | -t
+    Read email addresses from tags and add to CC list.
+    Note that git send-email does not do this. It will add the senders
+    email adress to the CC list though
+  --tagscc
+    Same as tags, only that in this case CCs extracted from tags
+    are treated like CCs that have come from the *.patch file
+  --arg <argument> | -a <argument> ...
+    Arguments passed on to get_maintainer.pl
+    This option can be used multiple times, e.g. -a <a1> -a <a2> ...
+  --verbose
+    Show more output
+  --help | -h
+    Show this help information
+
+PROCESSING POLICY:
+------------------
+  *.patch files consist of several sections relevant to processing:
+  <top>:   This is the email header containing email related information
+           It ends with the Subject: line
+  <body>:  This is the body that ends up in the commit message
+           It ends with ---
+  <--->:   This section contains the actual patches. CCs added here are
+           processed by git send-email, but are not stored in the commit
+           message. Some people add CCs into this section
+  <ccend>: It ends with '-- '
+
+  Note that cover letters do not have the <body> section.
+
+  The following options specifiy how CCs are insertied into *.patch files
+  top:    Insert CCs into the email header
+          Insert CCs from *-by: tags and TOs from mailing lists into the header
+          (this is the default)
+  ccbody: Insert CCs into body
+          Insert CCs from *-by: tags and TOs from mailing lists into the header
+          unless specified otherwise (via --tagscc).
+  cc---:  Insert CCs just after the --- line
+          Insert CCs from *-by: tags and TOs from mailing lists into the header
+          unless specified otherwise (via --tagscc).
+  ccend:  Insert CCs before the '-- ' line
+          Insert CCs from *-by: tags and TOs from mailing lists into the header
+          unless specified otherwise (via --tagscc).
+  none:   Neither insert TO, CCs from --tags nor other CCs
+
+WORKFLOW:
+---------
+  This script is intended to be used as part of the following workflow
+
+  Step 1: git format-patch ... -o <patchdir> ...
+  Step 2: ./scripts/add_maintainers.pl -d <patchdir>
+          This overwrites *.patch files in <patchdir> but makes a backup
+  Step 3: git send-email -to xen-devel\@lists.xenproject.org <patchdir>/*.patch
+EOT
+
+# Constants and functions related to policies
+
+# Constants for -p|--inspatch and -c|--inscover option processing
+my @ppolicies = ("top", "ccbody", "cc---", "none");
+my @cpolicies = ("top", "ccend", "none");
+
+# Hash is used to determine which mode value maps onto which search string
+my %inssearch = (
+    "top"    => "Date:",          # Insert before Date:
+    "ccbody" => "Signed-off-by:", # Insert before Signed-off-by:
+    "cc---"  => "---",            # Insert after ---
+    "ccend"  => "-- ",            # Insert before '-- '
+);
+
+# Hash is used to determine whether for a given mode we insert CCs after
+# the search string or before
+my %insafter = (
+    "top"    => 0,
+    "ccbody" => 0,
+    "cc---"  => 1,
+    "ccend"  => 0,
+);
+
+# The following subroutines take a areference to arrays of
+# - @top: contains CCs from *-by: tags and TOs from mailing lists
+# - @cc:  contains all other CC's
+# It will then apply the corect policies on the input file
+
+sub applypolicy_top ($$$) {
+    my ($file, $rtop, $rcc) = @_;
+    my $insert = join("\n", uniq (@$rtop, @$rcc));
+    insert($file , $insert, $inssearch{top}, $insafter{top});
+}
+
+sub applymixedpolicy ($$$$) {
+    my ($file, $rtop, $rcc, $mode) = @_;
+    my $top = join("\n", @$rtop);
+    my $cc  = join("\n", @$rcc);
+    # Insert snippets into files
+    insert($file , $cc, $inssearch{$mode}, $insafter{$mode});
+    # The top
+    insert($file , $top, $inssearch{top}, $insafter{top});
+}
+
+sub applypolicy_ccbody($$$) {
+    my ($file, $rtop, $rcc) = @_;
+    applymixedpolicy($file, $rtop, $rcc, "ccbody");
+}
+
+# Use a different name to make sure perl doesn't throw a syntax error
+sub applypolicy_cc3minus ($$$) {
+    my ($file, $rtop, $rcc) = @_;
+    applymixedpolicy($file, $rtop, $rcc, "cc---");
+}
+
+sub applypolicy_ccend ($$$) {
+    my ($file, $rtop, $rcc) = @_;
+    applymixedpolicy($file, $rtop, $rcc, "ccend");
+}
+
+sub applypolicy_none ($$$) {
+    return;
+}
+
+# Hash for policy functions
+my %applypolicy = (
+    "top"    => \&applypolicy_top,
+    "ccbody" => \&applypolicy_ccbody,
+    "cc---"  => \&applypolicy_cc3minus,
+    "ccend"  => \&applypolicy_ccend,
+    "none"   => \&applypolicy_none,
+);
+
+# Arguments / Options
+my $help = 0;
+my $patch_dir = 0;
+my @get_maintainer_args = ();
+my $verbose = 0;
+my $rerollcount = 0;
+my $patch_prefix = "0"; # Use a 0, such that v* does not get picked up
+                        # Obviously this will only work for series with
+                        # < 999 patches, which should be fine
+my $tags = 0;
+my $tagscc = 0;
+my $ppolicy = "top";
+my $cpolicy = "top";
+
+# Constants
+# Keep these as constants, in case we want to make these configurable
+# in future
+my $CC                  = "Cc:"; # Note: git-send-mail requires Cc:
+my $TO                  = "To:";
+my $cover_letter        = "0000-cover-letter.patch";
+my $get_maintainer      = "./scripts/get_maintainer.pl";
+my $patch_ext           = ".patch";
+my $maintainers         = "MAINTAINERS";
+
+if (!GetOptions(
+                'd|patchdir=s'     => \$patch_dir,
+                'v|reroll-count=i' => \$rerollcount,
+                'p|inspatch=s'     => \$ppolicy,
+                'c|inscover=s'     => \$cpolicy,
+                't|tags'           => \$tags,
+                'tagscc'           => \$tagscc,
+                'a|arg=s'          => \@get_maintainer_args,
+                'verbose'          => \$verbose,
+                'h|help'           => \$help,
+                )) {
+    die "$tool: invalid argument - use --help if necessary\n";
+}
+
+if ($help) {
+    print $usage;
+    exit 0;
+}
+
+if (!$patch_dir) {
+    die "$tool: Directory -d|--patchdir not specified\n";
+}
+
+if (! -e $patch_dir) {
+    die "$tool: Directory $patch_dir does not exist\n";
+}
+
+if ($rerollcount > 0) {
+    $patch_prefix = "v".$rerollcount;
+}
+
+if ( ! grep $_ eq $ppolicy, @ppolicies ) {
+    die "$tool: Invalid -p|--inspatch value\n";
+}
+if ( ! grep $_ eq $cpolicy, @cpolicies ) {
+    die "$tool: Invalid -c|--inscover value\n";
+}
+
+# Get the list of patches
+my $has_cover_letter = 0;
+my $cover_letter_file;
+my $pattern = $patch_dir.'/'.$patch_prefix.'*'.$patch_ext;
+
+$!=0;
+my @patches = glob($pattern);
+if ($!) {
+    die "$tool: Directory $patch_dir contains no patches\n";
+}
+if (!scalar @patches) {
+    die "$tool: Directory $patch_dir contains no matching patches\n";
+}
+
+# Do the actual processing
+my $file;
+my @combined_top;
+my @combined_cc;
+
+foreach my $file (@patches) {
+    if ($file =~ /\Q$cover_letter\E/) {
+        $has_cover_letter = 1;
+        $cover_letter_file = $file;
+    } else {
+        my @top;        # To: lists returned by get_maintainers.pl
+        my @toppatch;   # To: entries in *.patch
+                        #
+                        # Also includes CC's from tags as we do not want
+                        # entries in the body such as
+                        # CC: lars.kurth@citrix.com
+                        # ...
+                        # Tested-by: lars.kurth@citrix.com
+
+        my @cc;         # Cc: maintainers returned by get_maintainers.pl
+        my @ccpatch;    # Cc: entries in *.patch
+        my @extrapatch; # Cc: for AB, RB, RAB in *.patch
+
+        print "Processing: ".basename($file)."\n";
+
+        # Read tags from output of get_maintainers.pl
+        # Lists go into @top and everything else into @cc
+        getmaintainers($file, \@top, \@cc);
+
+        # Read all lines with CC & TO from the patch file (these will
+        # likely come from the commit message). Also read tags.
+        gettagsfrompatch($file, \@toppatch, \@ccpatch, \@extrapatch);
+
+        # With -t|--tags only add @extrapatch to @top and @combined_top
+        # With --tagscc treat tags as CC that came from the *.patch file
+        if ($tags & !$tagscc) {
+            # Copy these always onto the TO related arrays
+            push @top, @extrapatch;
+            push @combined_top, @extrapatch;
+        } elsif ($tagscc) {
+            # Treat these as if they came from CC's
+            push @ccpatch, @extrapatch;
+            push @combined_cc, @extrapatch;
+        }
+
+        # In this section we normalize the lists. We remove entries
+        # that are already in the patch, from @cc and @to
+        my @top_only = normalize(\@top, \@toppatch);
+        my @cc_only  = normalize(\@cc, \@ccpatch);
+
+        # Apply the policy
+        $applypolicy{$ppolicy}($file, \@top_only, \@cc_only);
+    }
+}
+
+# Deal with the cover letter
+if ($has_cover_letter) {
+    my @toppatch;   # Entries inserted at the top
+    my @ccpatch;    # Cc: entries in *.patch
+
+    print "Processing: ".basename($cover_letter_file)."\n";
+    
+    # Read all lines with CC & TO from the patch file such that subsequent
+    # calls don't lead to duplication
+    gettagsfrompatch($cover_letter_file, \@toppatch, \@ccpatch);
+
+    # In this section we normalize the lists. We remove entries
+    # that are already in the patch, from @cc and @to
+    my @top_only = normalize(\@combined_top, \@toppatch);
+    my @cc_only  = normalize(\@combined_cc, \@ccpatch);
+
+    # Apply the policy
+    $applypolicy{$cpolicy}($cover_letter_file, \@top_only, \@cc_only);
+
+    print "\nDon't forget to add the subject and message to ".
+          $cover_letter_file."\n";
+}
+
+print "Then perform:\n".
+      "git send-email -to xen-devel\@lists.xenproject.org ".
+      $patch_dir.'/'.$patch_prefix."*.patch"."\n";
+
+exit 0;
+
+my $readmailinglists = 0;
+my @mailinglists = ();
+
+sub getmailinglists () {
+   # Read mailing list from MAINTAINERS file and copy
+   # a list of e-mail addresses to @mailinglists
+    if (!$readmailinglists) {
+        if (-e $maintainers) {
+            my $fh;
+            my $line;
+            open($fh, "<", $maintainers) or die $!;
+            while (my $line = <$fh>) {
+                chomp $line;
+                if ($line =~ /^L:[[:blank:]]+/m) {
+                   push @mailinglists, $';
+                }
+            }
+            close $fh or die $!;
+        } else {
+            print "Warning: file '$maintainers' does not exist\n";
+            print "Warning: Mailing lists will be treated as CC's\n";
+        }
+    # Don't try again, even if the MAINTAINERS file does not exist
+    $readmailinglists = 1;
+    # Remove any duplicates
+    @mailinglists = uniq @mailinglists;
+    }
+}
+
+sub ismailinglist ($) {
+    my ($check) = @_;
+    # Get the mailing list information
+    getmailinglists();
+    # Do the check
+    if ( grep { $_ eq $check} @mailinglists) {
+        return 1;
+    }
+    return 0;
+}
+
+sub getmaintainers ($$$) {
+    my ($file, $rto, $rcc) = @_;
+    my $get_maintainer_args = join " ", @get_maintainer_args;
+    my $cmd = "$get_maintainer $get_maintainer_args <$file";
+    my $fh;
+
+    if (! -e $get_maintainer) {
+        die "$tool: The tool requires $get_maintainer\n";
+    }
+
+    open($fh, "-|", $cmd)
+        or die "Failed to open '$cmd'\n";
+    while(my $line = <$fh>) {
+        chomp $line;
+        # Keep lists and CC's separately as we dont want them in
+        # the commit message under a Cc: line
+        if (ismailinglist($line)) {
+            push @$rto, $TO." ".$line;
+            push @combined_top, $TO." ".$line;
+        } else {
+            push @$rcc, $CC." ".$line;
+            push @combined_cc, $CC." ".$line;
+        }
+    }
+    close $fh;
+}
+
+sub gettagsfrompatch ($$$;$) {
+    my ($file, $rto, $rcc, $rextra) = @_;
+    my $fh;
+
+    open($fh, "<", $file)
+        or die "Failed to open '$file'\n";
+    while(<$fh>) {
+        chomp;
+        my $line = $_;
+        my $nline;
+
+        if (hastag($line, $TO)) {
+            push @$rto, $line;
+            push @combined_top, $line;
+        }
+        if (hastag($line, $CC)) {
+            push @$rcc, $line;
+            push @combined_cc, $line;
+        }
+        # If there is an $rextra, then get various tags and add
+        # email addresses to the CC list
+        if ($rextra && $line =~ /^[-0-9a-z]+-by:[[:blank:]]+/mi) {
+            push @$rextra, $CC." ".$';
+        }
+    }
+    close $fh;
+}
+
+sub hastag ($$) {
+    my ($line, $tag) = @_;
+    if ($line =~ m{^\Q$tag\E}i) {
+        return 1;
+    }
+    return 0;
+}
+
+sub normalize ($$) {
+    # This function is used to normalize lists of tags or CC / TO lists
+    # - It removes duplicates in the input arrays
+    # - It ensures that elements in the second list are not in the first
+    my ($ra, $rb) = @_;
+    my @aonly = ();
+    my %seen;
+    my $item;
+
+    @$ra = uniq @$ra;
+    @$rb = uniq @$rb;
+    foreach $item (@$rb) {
+        $seen{$item} = 1;
+    }
+    foreach $item (@$ra) {
+        unless ($seen{$item}) {
+            # it's not in %seen, so add to @aonly
+            push @aonly, $item;
+        }
+    }
+
+    return @aonly;
+}
+
+sub readfile ($) {
+    my ($file) = @_;
+    my $fh;
+    my $content;
+    open($fh, "<", $file)
+         or die "Could not open file '$file' $!";
+    $content = do { local $/; <$fh> };
+    close $fh or die $!;
+
+    return $content;
+}
+
+sub writefile ($$) {
+    my ($content, $file) = @_;
+    my $fh;
+    open($fh, ">", $file)
+         or die "Could not open file '$file' $!";
+    print $fh $content or die $!;
+    close $fh or die $!;
+}
+
+sub insert ($$$$) {
+    my ($file, $insert, $delimiter, $insafter) = @_;
+    my $content;
+
+    if ($insert eq "") {
+        # Nothing to insert
+        return;
+    }
+    # Read file
+    $content = readfile($file);
+
+    # Split the string and generate new content
+    if ($content =~ /^\Q$delimiter\E/mi) {
+        if ($insafter) {
+            writefile($`.$delimiter."\n".$insert."\n".$', $file);
+
+            if ($verbose) {
+                print "\nInserted into ".basename($file).' after "'.
+                      $delimiter."'"."\n-----\n".$insert."\n-----\n";
+            }
+        } else {
+            writefile($`.$insert."\n".$delimiter.$', $file);
+
+            if ($verbose) {
+                print "\nInserted into ".basename($file).' before "'.
+                      $delimiter."'"."\n-----\n".$insert."\n-----\n";
+            }
+        }
+
+    } else {
+       print "Error: Didn't find '$delimiter' in '$file'\n";
+    }
+}
-- 
2.13.0


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

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

[Ian Jackson]

Thanks.  This is much better :-).  I have reviewed this for style,
obvious bugs, and the semantics in the doc comment.  I haven't tried
to follow the algorithm in detail, but I reckon it's probably OK.

I have reordered the patch (and hence the file) to make the grouping
of my comments make more sense.

Lars Kurth writes ("[PATCH for-4.11 v3 1/1] Add new add_maintainers.pl script to optimise the workflow when using git format-patch with get_maintainer.pl"):
> +  --inspatch (top|ccbody|cc---|none) | -p (top|ccbody|cc---|none)
> +    Insert email addresses into *.patch files according to the POLICY
> +    See section POLICY:
> +  --inscover (top|ccend|none) | -c (top|ccend|none)
> +    Insert email addresses into cover letteraccording to the POLICY
> +    See section PROCESSING POLICY:

I'm afraid that I don't understand which addresses are added where,
from the documentation.  In particular, what happens without --tags or
--tagscc ?  Also you should define `tag'; it has a lot of different
meanings, some subtly different, and it is not completely clear which
one you mean.

I think you should formally state the default behaviour.  Something
like:

  By default:
  * get_maintainer is called on each patch to find email addresses
    of maintainers/reviewers for that patch; these are added
    to the patch body near the CC section.
  * further email addresses are found in each patch's commit
    message tags (CC, signed-off-by, reviewed-by, etc.)
  * All of the above addresses are added to the CC mail headers
    of each patch
  * All of the above addresses are added to the CC mail headers
    of the cover letter

I suspect that what I have above is not the real behaviour.  You
should write what is true in that kind of style :-).

> +my $patch_prefix = "0"; # Use a 0, such that v* does not get picked up
> +                        # Obviously this will only work for series with
> +                        # < 999 patches, which should be fine

I don't understand the purpose of this:

> +if ($rerollcount > 0) {
> +    $patch_prefix = "v".$rerollcount;
> +}
...
> +my $pattern = $patch_dir.'/'.$patch_prefix.'*'.$patch_ext;
> +print "Then perform:\n".
> +      "git send-email -to xen-devel\@lists.xenproject.org ".
> +      $patch_dir.'/'.$patch_prefix."*.patch"."\n";

What files matching *.patch exist here that should not be processed ?
If the answer is none then $patch_prefix is redundant, I think ?


> +foreach my $file (@patches) {
> +    if ($file =~ /\Q$cover_letter\E/) {

I know you had me look at this over your shoulder and I said it was
right, but I think in fact this would match hypothetical files
     $patch_dir/0020-do-something-about-0000-cover-letter.patch

I think you need to expect a /.  So one of

  +    if ($file =~ /\/\Q$cover_letter\E/) {
  +    if ($file =~ m{/\Q$cover_letter\E}) {

> +print "Then perform:\n".
> +      "git send-email -to xen-devel\@lists.xenproject.org ".
> +      $patch_dir.'/'.$patch_prefix."*.patch"."\n";
> +
> +exit 0;
> +
> +my $readmailinglists = 0;
> +my @mailinglists = ();

This is a very curious structure.  These assignments are never
executed (but I guess the program works anyway).  I would recommend
moving the main program to the bottom of the file.

> +sub getmailinglists () {
> +   # Read mailing list from MAINTAINERS file and copy
> +   # a list of e-mail addresses to @mailinglists
> +    if (!$readmailinglists) {

I suggest you rename this variable $getmailinglists_done or
something.  As it is it is confusing because `read' might be the
present tense, but then the sense is wrong.

Also, you might find it better to use a structure like one of
      if ($getmailingslists_done) { return; }
      return if $getmailingslists_done;

> +        if (-e $maintainers) {
...
> +            print "Warning: Mailing lists will be treated as CC's\n";
> +        }
> +    # Don't try again, even if the MAINTAINERS file does not exist
> +    $readmailinglists = 1;
> +    # Remove any duplicates
> +    @mailinglists = uniq @mailinglists;
> +    }

Indentation here is misleading.  (But this will go away if you adopt
my suggestion above).

> +sub ismailinglist ($) {
> +    my ($check) = @_;
> +    # Get the mailing list information
> +    getmailinglists();
> +    # Do the check
> +    if ( grep { $_ eq $check} @mailinglists) {

Rather than uniq above, and then grep here, you could use a hash
%mailinglists.  That would be more idiomatic and also less code and
faster.  But as it is is tolerable.

> +sub getmaintainers ($$$) {
> +    my ($file, $rto, $rcc) = @_;
> +    my $get_maintainer_args = join " ", @get_maintainer_args;
> +    my $cmd = "$get_maintainer $get_maintainer_args <$file";
...
> +    open($fh, "-|", $cmd)
> +        or die "Failed to open '$cmd'\n";

You should use the array form of piped open, rather than this string
joining.  That way arguments containing spaces make their way through
correct.

> +    if (! -e $get_maintainer) {
> +        die "$tool: The tool requires $get_maintainer\n";

I still don't like this check.  What if the user specifies an
implementation of $get_maintainer which is on the PATH ?

> +    while(my $line = <$fh>) {
...
> +    }
> +    close $fh;

You need to check the errors here.  See the `perldoc -f close'.

> +        if ($tags & !$tagscc) {

You mean &&, not &.

Thanks,
Ian.

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

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

[Lars Kurth]

On 04/05/2018, 18:43, "Ian Jackson" <ian.jackson@citrix.com> wrote:

    Thanks.  This is much better :-).  I have reviewed this for style,
    obvious bugs, and the semantics in the doc comment.  I haven't tried
    to follow the algorithm in detail, but I reckon it's probably OK.
    
    I have reordered the patch (and hence the file) to make the grouping
    of my comments make more sense.
    
    Lars Kurth writes ("[PATCH for-4.11 v3 1/1] Add new add_maintainers.pl script to optimise the workflow when using git format-patch with get_maintainer.pl"):
    > +  --inspatch (top|ccbody|cc---|none) | -p (top|ccbody|cc---|none)
    > +    Insert email addresses into *.patch files according to the POLICY
    > +    See section POLICY:
    > +  --inscover (top|ccend|none) | -c (top|ccend|none)
    > +    Insert email addresses into cover letteraccording to the POLICY
    > +    See section PROCESSING POLICY:
    
    I'm afraid that I don't understand which addresses are added where,
    from the documentation.  In particular, what happens without --tags or
    --tagscc ?  Also you should define `tag'; it has a lot of different
    meanings, some subtly different, and it is not completely clear which
    one you mean.

Is that because of the way I structured it? The actual behaviour
Is stated under PROCESSING POLICY. Sure: I can explain what is read
and what is done with the data.

    I think you should formally state the default behaviour.  Something
    like:
    
      By default:
      * get_maintainer is called on each patch to find email addresses
        of maintainers/reviewers for that patch; these are added
        to the patch body near the CC section.
      * further email addresses are found in each patch's commit
        message tags (CC, signed-off-by, reviewed-by, etc.)
      * All of the above addresses are added to the CC mail headers
        of each patch
      * All of the above addresses are added to the CC mail headers
        of the cover letter
    
    I suspect that what I have above is not the real behaviour.  You
    should write what is true in that kind of style :-).

Sure. That makes things a lot clearer.
    
    > +my $patch_prefix = "0"; # Use a 0, such that v* does not get picked up
    > +                        # Obviously this will only work for series with
    > +                        # < 999 patches, which should be fine
    
    I don't understand the purpose of this:
 
This is a bit of a hack! 

There are several different usage patterns for g-f-p when working on a series,
which result in $patch_dir being used differently. In one case
a) the user stores patches for multiple series in $patch_dir
Thus, $patchdir may contain files starting with 0000*, 0001*, ... v1-000*, v2-000*
I have directories that contain entries generated by 

Case a1)
g-f-p  ...
g-f-p  --reroll-count=2 ...
Etc.

and 
Case a2) g-f-p  --reroll-count=1 ...
Etc.

b) the user stores patches in separate directories, aka one directory per g-f-p

What I was trying to do here is to use $patch_prefix to select what to process
in $patchdir. The problem is that in case a1, when g-f-p was called with no 
--reroll-count, I need to select entries 0000*, 0001*, ... as otherwise the entire
directory is processed.

The only way to identify these is via 0*.patch. But I may have missed something.

    > +if ($rerollcount > 0) {
    > +    $patch_prefix = "v".$rerollcount;
    > +}
    ...
    > +my $pattern = $patch_dir.'/'.$patch_prefix.'*'.$patch_ext;
    > +print "Then perform:\n".
    > +      "git send-email -to xen-devel\@lists.xenproject.org ".
    > +      $patch_dir.'/'.$patch_prefix."*.patch"."\n";
    
    What files matching *.patch exist here that should not be processed ?
    If the answer is none then $patch_prefix is redundant, I think ?

Well, it depends. G-s-m will send everything in $patch_dir.
I have not checked whether it ignores backups (~ .bak), but I assume it does.
In any case, for scenario a1) and a2) I do need to select which files
to select in g-s-e.
    
    > +foreach my $file (@patches) {
    > +    if ($file =~ /\Q$cover_letter\E/) {
    
    I know you had me look at this over your shoulder and I said it was
    right, but I think in fact this would match hypothetical files
         $patch_dir/0020-do-something-about-0000-cover-letter.patch
    
    I think you need to expect a /.  So one of
    
      +    if ($file =~ /\/\Q$cover_letter\E/) {
      +    if ($file =~ m{/\Q$cover_letter\E}) {
    
Sure

    > +print "Then perform:\n".
    > +      "git send-email -to xen-devel\@lists.xenproject.org ".
    > +      $patch_dir.'/'.$patch_prefix."*.patch"."\n";
    > +
    > +exit 0;
    > +
    > +my $readmailinglists = 0;
    > +my @mailinglists = ();
    
    This is a very curious structure.  These assignments are never
    executed (but I guess the program works anyway).  I would recommend
    moving the main program to the bottom of the file.
    
    > +sub getmailinglists () {
    > +   # Read mailing list from MAINTAINERS file and copy
    > +   # a list of e-mail addresses to @mailinglists
    > +    if (!$readmailinglists) {
    
    I suggest you rename this variable $getmailinglists_done or
    something.  As it is it is confusing because `read' might be the
    present tense, but then the sense is wrong.

Sure
    
    Also, you might find it better to use a structure like one of
          if ($getmailingslists_done) { return; }
          return if $getmailingslists_done;
    
    > +        if (-e $maintainers) {
    ...
    > +            print "Warning: Mailing lists will be treated as CC's\n";
    > +        }
    > +    # Don't try again, even if the MAINTAINERS file does not exist
    > +    $readmailinglists = 1;
    > +    # Remove any duplicates
    > +    @mailinglists = uniq @mailinglists;
    > +    }
    
    Indentation here is misleading.  (But this will go away if you adopt
    my suggestion above).
    
    > +sub ismailinglist ($) {
    > +    my ($check) = @_;
    > +    # Get the mailing list information
    > +    getmailinglists();
    > +    # Do the check
    > +    if ( grep { $_ eq $check} @mailinglists) {
    
    Rather than uniq above, and then grep here, you could use a hash
    %mailinglists.  That would be more idiomatic and also less code and
    faster.  But as it is is tolerable.
    
    > +sub getmaintainers ($$$) {
    > +    my ($file, $rto, $rcc) = @_;
    > +    my $get_maintainer_args = join " ", @get_maintainer_args;
    > +    my $cmd = "$get_maintainer $get_maintainer_args <$file";
    ...
    > +    open($fh, "-|", $cmd)
    > +        or die "Failed to open '$cmd'\n";
    
    You should use the array form of piped open, rather than this string
    joining.  That way arguments containing spaces make their way through
    correct.

Will look into this.
    
    > +    if (! -e $get_maintainer) {
    > +        die "$tool: The tool requires $get_maintainer\n";
    
    I still don't like this check.  What if the user specifies an
    implementation of $get_maintainer which is on the PATH ?

The way get_maintainer.pl works is that it has to be called in the root
directory of the Xen and Linux trees. There are some checks in the
tool that throw an error when you call it from another location.

The relevant code in get_maintainer.pl is

 268 if (!top_of_tree($xen_path)) {
 269     die "$P: The current directory does not appear to be "
 270         . "a Xen source tree.\n";
 271 }

Right now, $get_maintainer is not really configurable. So this
is not an issue now, but could become one in future. 

I think for now, I will leave this as-is, but add a comment.
    
    > +    while(my $line = <$fh>) {
    ...
    > +    }
    > +    close $fh;
    
    You need to check the errors here.  See the `perldoc -f close'.

OK
    
    > +        if ($tags & !$tagscc) {
    
    You mean &&, not &.

Indeed

Thanks for the review

Lars
 

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

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

[Ian Jackson]

Lars Kurth writes ("Re: [PATCH for-4.11 v3 1/1] Add new add_maintainers.pl script to optimise the workflow when using git format-patch with get_maintainer.pl"):
> On 04/05/2018, 18:43, "Ian Jackson" <ian.jackson@citrix.com> wrote:
>     > +my $patch_prefix = "0"; # Use a 0, such that v* does not get picked up
>     > +                        # Obviously this will only work for series with
>     > +                        # < 999 patches, which should be fine
>     
>     I don't understand the purpose of this:
>  
> This is a bit of a hack! 
> 
> There are several different usage patterns for g-f-p when working on a series,
> which result in $patch_dir being used differently. In one case
> a) the user stores patches for multiple series in $patch_dir
> Thus, $patchdir may contain files starting with 0000*, 0001*, ... v1-000*, v2-000*

OMG.  It had not even occurred to me that anyone would do that.  But I
think this workflow choice is independent of --reroll-count, which is
mainly used to control patch subject lines.

A workflow where *different* patch series are mingled in the same
directory cannot be supported, because what when their reroll counts
collide?  So these must be different versions (different rerolls) of
the same series.

I suggest the following approach:

Test whether v<reroll-count>-0000-cover-letter.patch exists.  If it
does, expect every patch to start with v<reroll-count>.  If it does
not, expect simply 0000-cover-letter.patch to exist, and every patch
to start with just the patch number.

So I guess $patch_prefix would be '[0-9]' or "v${reroll_count}-[0-9]".

>     > +if ($rerollcount > 0) {
>     > +    $patch_prefix = "v".$rerollcount;
>     > +}
>     ...
>     > +my $pattern = $patch_dir.'/'.$patch_prefix.'*'.$patch_ext;
>     > +print "Then perform:\n".
>     > +      "git send-email -to xen-devel\@lists.xenproject.org ".
>     > +      $patch_dir.'/'.$patch_prefix."*.patch"."\n";
>     
>     What files matching *.patch exist here that should not be processed ?
>     If the answer is none then $patch_prefix is redundant, I think ?
> 
> Well, it depends. G-s-m will send everything in $patch_dir.
> I have not checked whether it ignores backups (~ .bak), but I assume it does.
> In any case, for scenario a1) and a2) I do need to select which files
> to select in g-s-e.

For the purposes of documentation and informative messages like this
one, I think you can assume workflow (b).  I think workflow (a) is not
to be recommended.

(Anyway, who keeps their g-f-p output directories ?  I don't...)

>     > +    if (! -e $get_maintainer) {
>     > +        die "$tool: The tool requires $get_maintainer\n";
>     
>     I still don't like this check.  What if the user specifies an
>     implementation of $get_maintainer which is on the PATH ?
> 
> The way get_maintainer.pl works is that it has to be called in the root
> directory of the Xen and Linux trees. There are some checks in the
> tool that throw an error when you call it from another location.

You have misunderstood my remark.  I am not talking about the cwd.

I mean, if the user says --get-maintainer=generic-get-maintaienr
where /usr/bin/generic-get-maintaienr exists.

Thanks,
Ian.

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

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

[Lars Kurth]

On 08/05/2018, 11:48, "Ian Jackson" <ian.jackson@citrix.com> wrote:

    Lars Kurth writes ("Re: [PATCH for-4.11 v3 1/1] Add new add_maintainers.pl script to optimise the workflow when using git format-patch with get_maintainer.pl"):
    > On 04/05/2018, 18:43, "Ian Jackson" <ian.jackson@citrix.com> wrote:
    >     > +my $patch_prefix = "0"; # Use a 0, such that v* does not get picked up
    >     > +                        # Obviously this will only work for series with
    >     > +                        # < 999 patches, which should be fine
    >     
    >     I don't understand the purpose of this:
    >  
    > This is a bit of a hack! 
    > 
    > There are several different usage patterns for g-f-p when working on a series,
    > which result in $patch_dir being used differently. In one case
    > a) the user stores patches for multiple series in $patch_dir
    > Thus, $patchdir may contain files starting with 0000*, 0001*, ... v1-000*, v2-000*
    
    OMG.  It had not even occurred to me that anyone would do that.  But I
    think this workflow choice is independent of --reroll-count, which is
    mainly used to control patch subject lines.
    
    A workflow where *different* patch series are mingled in the same
    directory cannot be supported, because what when their reroll counts
    collide?  So these must be different versions (different rerolls) of
    the same series.
    
    I suggest the following approach:
    
    Test whether v<reroll-count>-0000-cover-letter.patch exists.  If it
    does, expect every patch to start with v<reroll-count>.  If it does
    not, expect simply 0000-cover-letter.patch to exist, and every patch
    to start with just the patch number.
    
    So I guess $patch_prefix would be '[0-9]' or "v${reroll_count}-[0-9]".

That makes sense
    
    >     > +if ($rerollcount > 0) {
    >     > +    $patch_prefix = "v".$rerollcount;
    >     > +}
    >     ...
    >     > +my $pattern = $patch_dir.'/'.$patch_prefix.'*'.$patch_ext;
    >     > +print "Then perform:\n".
    >     > +      "git send-email -to xen-devel\@lists.xenproject.org ".
    >     > +      $patch_dir.'/'.$patch_prefix."*.patch"."\n";
    >     
    >     What files matching *.patch exist here that should not be processed ?
    >     If the answer is none then $patch_prefix is redundant, I think ?
    > 
    > Well, it depends. G-s-m will send everything in $patch_dir.
    > I have not checked whether it ignores backups (~ .bak), but I assume it does.
    > In any case, for scenario a1) and a2) I do need to select which files
    > to select in g-s-e.
    
    For the purposes of documentation and informative messages like this
    one, I think you can assume workflow (b).  I think workflow (a) is not
    to be recommended.
    
    (Anyway, who keeps their g-f-p output directories ?  I don't...)

Sure: I think right now the documentation and messages don't cover this. So if we move to the approach you suggested, all should be fine.
    
    >     > +    if (! -e $get_maintainer) {
    >     > +        die "$tool: The tool requires $get_maintainer\n";
    >     
    >     I still don't like this check.  What if the user specifies an
    >     implementation of $get_maintainer which is on the PATH ?
    > 
    > The way get_maintainer.pl works is that it has to be called in the root
    > directory of the Xen and Linux trees. There are some checks in the
    > tool that throw an error when you call it from another location.
    
    You have misunderstood my remark.  I am not talking about the cwd.
    
    I mean, if the user says --get-maintainer=generic-get-maintaienr
    where /usr/bin/generic-get-maintaienr exists.

OK: simply removing the check would just do this

Lars 

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

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

[George Dunlap]

On 05/04/2018 09:36 AM, Lars Kurth wrote:
> The tool covers step 2 of the following workflow
> 
>   Step 1: git format-patch ... -o <patchdir> ...
>   Step 2: ./scripts/add_maintainers.pl -d <patchdir>
>           This overwrites  *.patch files in <patchdir>
>   Step 3: git send-email -to xen-devel@lists.xenproject.org <patchdir>/*.patchxm
> 
> I manually tested all options and the most common combinations
> on Mac.
> 
> Changes since v1:
> - Added RAB (indicated by Juergen on IRC that this is OK)
> - Remove trailing whitespaces
> - Renamed --prefix to --reroll-count
> - Cleaned up short options -v, ... to be in line with git
> - Added --tags|-t option to add AB, RAB and RB emails to CC list
> - Added --insert|-i mode to allow for people adding CCs to commit message
>   instead of the e-mail header (the header is the default)
> - Moved common code into functions
> - Added logic, such that the tool only insert's To: and Cc: statements
>   which were not there before, allowing for running the tool multiple times
>   on the same <patchdir>
> 
> Changes since v2:
> - Deleted --version and related infrastructure
> - Added subroutine prototypes
> - Removed AT and @lists declaration and used \@ in literals
> - Changed usage message and options based on feedback
> - Improved error handling
> - Removed occurances of index() and replaced with regex
> - Removed non-perl idioms
> - Moved uniq statements to normalize and added info on what normalize does
> - Read L: tags from MAINTAINERS file instead of using heuristic
> - Fixed issues related to metacharacters in getmaintainers()
> - Allow multiple -a | --arg values (because of this renamed --args)
> - Identify tags via regex
> - CC's from tags are only inserted in the mail header, never the body
> - That is unless the new option --tagscc is used
> - Added policy processing which includes reworking insert()
> - Replaced -i|--insert with -p|--inspatch and -c|--inscover now using policies
> - Added new policies to cover for all user requests
> - Rewrote help message to center around usage of policies
> - Reordered some code (e.g. help string first to make code more easily readable)
> 
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> Cc: George Dunlap <George.Dunlap@eu.citrix.com>
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Julien Grall <julien.grall@arm.com>
> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Cc: Stefano Stabellini <sstabellini@kernel.org>
> Cc: Tim Deegan <tim@xen.org>
> Cc: Wei Liu <wei.liu2@citrix.com>
> Signed-off-by: Lars Kurth <lars.kurth@citrix.com>
> Release-acked-by: Juergen Gross <jgross@suse.com>
> ---
>  scripts/add_maintainers.pl | 512 +++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 512 insertions(+)
>  create mode 100755 scripts/add_maintainers.pl
> 
> diff --git a/scripts/add_maintainers.pl b/scripts/add_maintainers.pl
> new file mode 100755
> index 0000000000..11ae60d888
> --- /dev/null
> +++ b/scripts/add_maintainers.pl
> @@ -0,0 +1,512 @@
> +#!/usr/bin/perl -w
> +# (c) 2018, Lars Kurth <lars.kurth@citrix.com>
> +#
> +# Add maintainers to patches generated with git format-patch
> +#
> +# Usage: perl scripts/add_maintainers.pl [OPTIONS] -patchdir <patchdir>
> +#
> +# Prerequisites: Execute
> +#                git format-patch ... -o <patchdir> ...
> +#
> +#                ./scripts/get_maintainer.pl is present in the tree
> +#
> +# Licensed under the terms of the GNU GPL License version 2
> +
> +use strict;
> +
> +use Getopt::Long qw(:config no_auto_abbrev);
> +use File::Basename;
> +use List::MoreUtils qw(uniq);
> +
> +sub getmaintainers ($$$);
> +sub gettagsfrompatch ($$$;$);
> +sub normalize ($$);
> +sub insert ($$$$);
> +sub hastag ($$);
> +
> +# Tool Variables
> +my $tool = $0;
> +my $usage = <<EOT;
> +USAGE: $tool [options] (--patchdir | -d) <patchdir>
> +
> +OPTIONS:
> +--------
> +  --reroll-count <n> | -v <n>
> +    Choose patch files for specific version. This results into the
> +    following filters on <patchdir>
> +    0: default - *.patch
> +    >1: v<n>*.patch
> +  --inspatch (top|ccbody|cc---|none) | -p (top|ccbody|cc---|none)
> +    Insert email addresses into *.patch files according to the POLICY
> +    See section POLICY:
> +  --inscover (top|ccend|none) | -c (top|ccend|none)
> +    Insert email addresses into cover letteraccording to the POLICY
> +    See section PROCESSING POLICY:
> +  --tags | -t
> +    Read email addresses from tags and add to CC list.
> +    Note that git send-email does not do this. It will add the senders
> +    email adress to the CC list though
> +  --tagscc
> +    Same as tags, only that in this case CCs extracted from tags
> +    are treated like CCs that have come from the *.patch file

Not clear on the difference between these.

> +  --arg <argument> | -a <argument> ...
> +    Arguments passed on to get_maintainer.pl
> +    This option can be used multiple times, e.g. -a <a1> -a <a2> ...
> +  --verbose
> +    Show more output
> +  --help | -h
> +    Show this help information
> +
> +PROCESSING POLICY:

Why is this called 'policy'?  This seems to be definitions.

> +------------------
> +  *.patch files consist of several sections relevant to processing:
> +  <top>:   This is the email header containing email related information
> +           It ends with the Subject: line
> +  <body>:  This is the body that ends up in the commit message
> +           It ends with ---
> +  <--->:   This section contains the actual patches. CCs added here are
> +           processed by git send-email, but are not stored in the commit
> +           message. Some people add CCs into this section

<---> is not a normal name (how do you say it? "dash-dash-dash"?), and
worse yet might be confused with an option.  `--inspatch cc---` looks
like there was some sort of mistake.  "Top" would normally mean, "Top of
the body of the mail".

I think it would be better to call these sections <header>, <commit> and
"commit message", and <comment> and "reviewer comment section",
respectively.


> +  <ccend>: It ends with '-- '
> +
> +  Note that cover letters do not have the <body> section.
> +
> +  The following options specifiy how CCs are insertied into *.patch files
> +  top:    Insert CCs into the email header
> +          Insert CCs from *-by: tags and TOs from mailing lists into the header
> +          (this is the default)
> +  ccbody: Insert CCs into body
> +          Insert CCs from *-by: tags and TOs from mailing lists into the header
> +          unless specified otherwise (via --tagscc).
> +  cc---:  Insert CCs just after the --- line
> +          Insert CCs from *-by: tags and TOs from mailing lists into the header
> +          unless specified otherwise (via --tagscc).
> +  ccend:  Insert CCs before the '-- ' line
> +          Insert CCs from *-by: tags and TOs from mailing lists into the header
> +          unless specified otherwise (via --tagscc).
> +  none:   Neither insert TO, CCs from --tags nor other CCs

I don't really get this section.

What about having the functionality be something like this?  (Obvious
this would need some code changes as well.  Also I guessed what the
significance of the `-- ` is in the cover letter, so correct me if I'm
wrong.)

---
USAGE: $tool [options] (--patchdir | -d) <patchdir>

OPTIONS:
--------

  --reroll-count <n> | -v <n>
    Choose patch files for specific version. This results into the
    following filters on <patchdir>
    0: default - *.patch
    >1: v<n>*.patch

  --patchcc (top|commit|comment|none) | -p (top|commit|comment|none)

    Insert CC lines into *.patch files in the specified location.
    See LOCATIONS for a definition of the various locations.

    The default is `top`.

  --covercc (top|end|none) | -c (top|end|none)

    Insert CC lines into cover letter in the specified location. See
    LOCATIONS for a definition of the various locations.

    The default is `top`.

  --tags | -t

    In addition to the output of get_maintainer.pl, include email
    addresses from commit tags (e.g., Reviewed-by, Tested-by, &c) in
    the list of CC lines to insert.

  --arg <argument> | -a <argument> ...
    Arguments passed on to get_maintainer.pl
    This option can be used multiple times, e.g. -a <a1> -a <a2> ...

  --verbose
    Show more output

  --help | -h
    Show this help information

LOCATIONS
---------

  *.patch and cover letters files consist of several sections relevant
   to processing:

  <header>:  This is the email header containing email related information
             It ends with the Subject: line

  <commit>: This is the email body that ends up in the commit message.
             It ends with ---.  CC lines added here will be checked
             into the git tree on commit.  Only applicable to normal
             patch files.

  <comment>: This is the 'comment for reviewers' section, after the
             --- but before the diff actually starts. CCs added here
             are processed by git send-email, but are not checked into
             the git tree on commit.  Only applicable to normal patch
             files.

  <end>:     The part of a cover letter just before `-- ` (which normally
             begins a diffstat).  Only applicable to cover letters.

---

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

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

[George Dunlap]

On 05/10/2018 12:38 PM, George Dunlap wrote:

>   --patchcc (top|commit|comment|none) | -p (top|commit|comment|none)
> 
>     Insert CC lines into *.patch files in the specified location.
>     See LOCATIONS for a definition of the various locations.
> 
>     The default is `top`.
> 
>   --covercc (top|end|none) | -c (top|end|none)

s/top/header/g; for these

 -George

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

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

[George Dunlap]

On 05/10/2018 12:38 PM, George Dunlap wrote:
> On 05/04/2018 09:36 AM, Lars Kurth wrote:
>> The tool covers step 2 of the following workflow
>>
>>   Step 1: git format-patch ... -o <patchdir> ...
>>   Step 2: ./scripts/add_maintainers.pl -d <patchdir>
>>           This overwrites  *.patch files in <patchdir>
>>   Step 3: git send-email -to xen-devel@lists.xenproject.org <patchdir>/*.patchxm
>>
>> I manually tested all options and the most common combinations
>> on Mac.
>>
>> Changes since v1:
>> - Added RAB (indicated by Juergen on IRC that this is OK)
>> - Remove trailing whitespaces
>> - Renamed --prefix to --reroll-count
>> - Cleaned up short options -v, ... to be in line with git
>> - Added --tags|-t option to add AB, RAB and RB emails to CC list
>> - Added --insert|-i mode to allow for people adding CCs to commit message
>>   instead of the e-mail header (the header is the default)
>> - Moved common code into functions
>> - Added logic, such that the tool only insert's To: and Cc: statements
>>   which were not there before, allowing for running the tool multiple times
>>   on the same <patchdir>
>>
>> Changes since v2:
>> - Deleted --version and related infrastructure
>> - Added subroutine prototypes
>> - Removed AT and @lists declaration and used \@ in literals
>> - Changed usage message and options based on feedback
>> - Improved error handling
>> - Removed occurances of index() and replaced with regex
>> - Removed non-perl idioms
>> - Moved uniq statements to normalize and added info on what normalize does
>> - Read L: tags from MAINTAINERS file instead of using heuristic
>> - Fixed issues related to metacharacters in getmaintainers()
>> - Allow multiple -a | --arg values (because of this renamed --args)
>> - Identify tags via regex
>> - CC's from tags are only inserted in the mail header, never the body
>> - That is unless the new option --tagscc is used
>> - Added policy processing which includes reworking insert()
>> - Replaced -i|--insert with -p|--inspatch and -c|--inscover now using policies
>> - Added new policies to cover for all user requests
>> - Rewrote help message to center around usage of policies
>> - Reordered some code (e.g. help string first to make code more easily readable)
>>
>> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
>> Cc: George Dunlap <George.Dunlap@eu.citrix.com>
>> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
>> Cc: Jan Beulich <jbeulich@suse.com>
>> Cc: Julien Grall <julien.grall@arm.com>
>> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>> Cc: Stefano Stabellini <sstabellini@kernel.org>
>> Cc: Tim Deegan <tim@xen.org>
>> Cc: Wei Liu <wei.liu2@citrix.com>
>> Signed-off-by: Lars Kurth <lars.kurth@citrix.com>
>> Release-acked-by: Juergen Gross <jgross@suse.com>
>> ---
>>  scripts/add_maintainers.pl | 512 +++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 512 insertions(+)
>>  create mode 100755 scripts/add_maintainers.pl
>>
>> diff --git a/scripts/add_maintainers.pl b/scripts/add_maintainers.pl
>> new file mode 100755
>> index 0000000000..11ae60d888
>> --- /dev/null
>> +++ b/scripts/add_maintainers.pl
>> @@ -0,0 +1,512 @@
>> +#!/usr/bin/perl -w
>> +# (c) 2018, Lars Kurth <lars.kurth@citrix.com>
>> +#
>> +# Add maintainers to patches generated with git format-patch
>> +#
>> +# Usage: perl scripts/add_maintainers.pl [OPTIONS] -patchdir <patchdir>
>> +#
>> +# Prerequisites: Execute
>> +#                git format-patch ... -o <patchdir> ...
>> +#
>> +#                ./scripts/get_maintainer.pl is present in the tree
>> +#
>> +# Licensed under the terms of the GNU GPL License version 2
>> +
>> +use strict;
>> +
>> +use Getopt::Long qw(:config no_auto_abbrev);
>> +use File::Basename;
>> +use List::MoreUtils qw(uniq);
>> +
>> +sub getmaintainers ($$$);
>> +sub gettagsfrompatch ($$$;$);
>> +sub normalize ($$);
>> +sub insert ($$$$);
>> +sub hastag ($$);
>> +
>> +# Tool Variables
>> +my $tool = $0;
>> +my $usage = <<EOT;
>> +USAGE: $tool [options] (--patchdir | -d) <patchdir>
>> +
>> +OPTIONS:
>> +--------
>> +  --reroll-count <n> | -v <n>
>> +    Choose patch files for specific version. This results into the
>> +    following filters on <patchdir>
>> +    0: default - *.patch
>> +    >1: v<n>*.patch
>> +  --inspatch (top|ccbody|cc---|none) | -p (top|ccbody|cc---|none)
>> +    Insert email addresses into *.patch files according to the POLICY
>> +    See section POLICY:
>> +  --inscover (top|ccend|none) | -c (top|ccend|none)
>> +    Insert email addresses into cover letteraccording to the POLICY
>> +    See section PROCESSING POLICY:
>> +  --tags | -t
>> +    Read email addresses from tags and add to CC list.
>> +    Note that git send-email does not do this. It will add the senders
>> +    email adress to the CC list though
>> +  --tagscc
>> +    Same as tags, only that in this case CCs extracted from tags
>> +    are treated like CCs that have come from the *.patch file
> 
> Not clear on the difference between these.
> 
>> +  --arg <argument> | -a <argument> ...
>> +    Arguments passed on to get_maintainer.pl
>> +    This option can be used multiple times, e.g. -a <a1> -a <a2> ...
>> +  --verbose
>> +    Show more output
>> +  --help | -h
>> +    Show this help information
>> +
>> +PROCESSING POLICY:
> 
> Why is this called 'policy'?  This seems to be definitions.
> 
>> +------------------
>> +  *.patch files consist of several sections relevant to processing:
>> +  <top>:   This is the email header containing email related information
>> +           It ends with the Subject: line
>> +  <body>:  This is the body that ends up in the commit message
>> +           It ends with ---
>> +  <--->:   This section contains the actual patches. CCs added here are
>> +           processed by git send-email, but are not stored in the commit
>> +           message. Some people add CCs into this section
> 
> <---> is not a normal name (how do you say it? "dash-dash-dash"?), and
> worse yet might be confused with an option.  `--inspatch cc---` looks
> like there was some sort of mistake.  "Top" would normally mean, "Top of
> the body of the mail".
> 
> I think it would be better to call these sections <header>, <commit> and
> "commit message", and <comment> and "reviewer comment section",
> respectively.
> 
> 
>> +  <ccend>: It ends with '-- '
>> +
>> +  Note that cover letters do not have the <body> section.
>> +
>> +  The following options specifiy how CCs are insertied into *.patch files
>> +  top:    Insert CCs into the email header
>> +          Insert CCs from *-by: tags and TOs from mailing lists into the header
>> +          (this is the default)
>> +  ccbody: Insert CCs into body
>> +          Insert CCs from *-by: tags and TOs from mailing lists into the header
>> +          unless specified otherwise (via --tagscc).
>> +  cc---:  Insert CCs just after the --- line
>> +          Insert CCs from *-by: tags and TOs from mailing lists into the header
>> +          unless specified otherwise (via --tagscc).
>> +  ccend:  Insert CCs before the '-- ' line
>> +          Insert CCs from *-by: tags and TOs from mailing lists into the header
>> +          unless specified otherwise (via --tagscc).
>> +  none:   Neither insert TO, CCs from --tags nor other CCs
> 
> I don't really get this section.
> 
> What about having the functionality be something like this?  (Obvious
> this would need some code changes as well.  Also I guessed what the
> significance of the `-- ` is in the cover letter, so correct me if I'm
> wrong.)
> 
> ---
> USAGE: $tool [options] (--patchdir | -d) <patchdir>
> 
> OPTIONS:
> --------
> 
>   --reroll-count <n> | -v <n>
>     Choose patch files for specific version. This results into the
>     following filters on <patchdir>
>     0: default - *.patch
>     >1: v<n>*.patch
> 
>   --patchcc (top|commit|comment|none) | -p (top|commit|comment|none)
> 
>     Insert CC lines into *.patch files in the specified location.
>     See LOCATIONS for a definition of the various locations.
> 
>     The default is `top`.
> 
>   --covercc (top|end|none) | -c (top|end|none)
> 
>     Insert CC lines into cover letter in the specified location. See
>     LOCATIONS for a definition of the various locations.
> 
>     The default is `top`.
> 
>   --tags | -t
> 
>     In addition to the output of get_maintainer.pl, include email
>     addresses from commit tags (e.g., Reviewed-by, Tested-by, &c) in
>     the list of CC lines to insert.

OK, having talked to you IRL I now understand what these are about.  How
about this:

---
  --tags | -t

    In addition to the output of get_maintainer.pl, include email
    addresses from commit tags (e.g., Reviewed-by, Tested-by, &c) in
    the list of CC lines to insert.

    For patches, these extra lines will be inserted as specified by
    the --patchcc option, *unless* that option is set to `commit`.  In
    that case, rather than duplicate tags in the commit message with
    CC lines, the extra CC lines will be added to he header instead.

  --tagscc

    As above, but `commit` location is not special-cased: In the case
    of `commit`, all CCs will be added to the bottom of the commit
    message, even if they duplicate other tags in the commit message.
---

Or, perhaps even better:

---
  --tags[=(split|combined)] | -t

    In addition to the output of get_maintainer.pl, collect email
    addresses from commit tags (e.g., Reviewed-by, Tested-by, &c) in
    the list of CC lines to insert.

    If 'split' is specified, these tag CC lines will be added to the
    header, rather than in the location specified by --patchcc.  In
    the case that `commit` is specified, this will avoid duplicating
    tags and CC lines in the commit message.  This is the default when
    `--patchcc commit` is specified.

    If 'combined' is specified, these tag CC lines will be added along
    with the maintainer CC's, wherever specified by --patchcc.  This
    is the default when "header" or "comment" are specified.
---

 -George

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

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

[Lars Kurth]

On 10/05/2018, 14:03, "George Dunlap" <george.dunlap@citrix.com> wrote:

    On 05/10/2018 12:38 PM, George Dunlap wrote:
    > On 05/04/2018 09:36 AM, Lars Kurth wrote:
    >> The tool covers step 2 of the following workflow
    >>
    >>   Step 1: git format-patch ... -o <patchdir> ...
    >>   Step 2: ./scripts/add_maintainers.pl -d <patchdir>
    >>           This overwrites  *.patch files in <patchdir>
    >>   Step 3: git send-email -to xen-devel@lists.xenproject.org <patchdir>/*.patchxm
    >>
    >> I manually tested all options and the most common combinations
    >> on Mac.
    >>
    >> Changes since v1:
    >> - Added RAB (indicated by Juergen on IRC that this is OK)
    >> - Remove trailing whitespaces
    >> - Renamed --prefix to --reroll-count
    >> - Cleaned up short options -v, ... to be in line with git
    >> - Added --tags|-t option to add AB, RAB and RB emails to CC list
    >> - Added --insert|-i mode to allow for people adding CCs to commit message
    >>   instead of the e-mail header (the header is the default)
    >> - Moved common code into functions
    >> - Added logic, such that the tool only insert's To: and Cc: statements
    >>   which were not there before, allowing for running the tool multiple times
    >>   on the same <patchdir>
    >>
    >> Changes since v2:
    >> - Deleted --version and related infrastructure
    >> - Added subroutine prototypes
    >> - Removed AT and @lists declaration and used \@ in literals
    >> - Changed usage message and options based on feedback
    >> - Improved error handling
    >> - Removed occurances of index() and replaced with regex
    >> - Removed non-perl idioms
    >> - Moved uniq statements to normalize and added info on what normalize does
    >> - Read L: tags from MAINTAINERS file instead of using heuristic
    >> - Fixed issues related to metacharacters in getmaintainers()
    >> - Allow multiple -a | --arg values (because of this renamed --args)
    >> - Identify tags via regex
    >> - CC's from tags are only inserted in the mail header, never the body
    >> - That is unless the new option --tagscc is used
    >> - Added policy processing which includes reworking insert()
    >> - Replaced -i|--insert with -p|--inspatch and -c|--inscover now using policies
    >> - Added new policies to cover for all user requests
    >> - Rewrote help message to center around usage of policies
    >> - Reordered some code (e.g. help string first to make code more easily readable)
    >>
    >> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
    >> Cc: George Dunlap <George.Dunlap@eu.citrix.com>
    >> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
    >> Cc: Jan Beulich <jbeulich@suse.com>
    >> Cc: Julien Grall <julien.grall@arm.com>
    >> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
    >> Cc: Stefano Stabellini <sstabellini@kernel.org>
    >> Cc: Tim Deegan <tim@xen.org>
    >> Cc: Wei Liu <wei.liu2@citrix.com>
    >> Signed-off-by: Lars Kurth <lars.kurth@citrix.com>
    >> Release-acked-by: Juergen Gross <jgross@suse.com>
    >> ---
    >>  scripts/add_maintainers.pl | 512 +++++++++++++++++++++++++++++++++++++++++++++
    >>  1 file changed, 512 insertions(+)
    >>  create mode 100755 scripts/add_maintainers.pl
    >>
    >> diff --git a/scripts/add_maintainers.pl b/scripts/add_maintainers.pl
    >> new file mode 100755
    >> index 0000000000..11ae60d888
    >> --- /dev/null
    >> +++ b/scripts/add_maintainers.pl
    >> @@ -0,0 +1,512 @@
    >> +#!/usr/bin/perl -w
    >> +# (c) 2018, Lars Kurth <lars.kurth@citrix.com>
    >> +#
    >> +# Add maintainers to patches generated with git format-patch
    >> +#
    >> +# Usage: perl scripts/add_maintainers.pl [OPTIONS] -patchdir <patchdir>
    >> +#
    >> +# Prerequisites: Execute
    >> +#                git format-patch ... -o <patchdir> ...
    >> +#
    >> +#                ./scripts/get_maintainer.pl is present in the tree
    >> +#
    >> +# Licensed under the terms of the GNU GPL License version 2
    >> +
    >> +use strict;
    >> +
    >> +use Getopt::Long qw(:config no_auto_abbrev);
    >> +use File::Basename;
    >> +use List::MoreUtils qw(uniq);
    >> +
    >> +sub getmaintainers ($$$);
    >> +sub gettagsfrompatch ($$$;$);
    >> +sub normalize ($$);
    >> +sub insert ($$$$);
    >> +sub hastag ($$);
    >> +
    >> +# Tool Variables
    >> +my $tool = $0;
    >> +my $usage = <<EOT;
    >> +USAGE: $tool [options] (--patchdir | -d) <patchdir>
    >> +
    >> +OPTIONS:
    >> +--------
    >> +  --reroll-count <n> | -v <n>
    >> +    Choose patch files for specific version. This results into the
    >> +    following filters on <patchdir>
    >> +    0: default - *.patch
    >> +    >1: v<n>*.patch
    >> +  --inspatch (top|ccbody|cc---|none) | -p (top|ccbody|cc---|none)
    >> +    Insert email addresses into *.patch files according to the POLICY
    >> +    See section POLICY:
    >> +  --inscover (top|ccend|none) | -c (top|ccend|none)
    >> +    Insert email addresses into cover letteraccording to the POLICY
    >> +    See section PROCESSING POLICY:
    >> +  --tags | -t
    >> +    Read email addresses from tags and add to CC list.
    >> +    Note that git send-email does not do this. It will add the senders
    >> +    email adress to the CC list though
    >> +  --tagscc
    >> +    Same as tags, only that in this case CCs extracted from tags
    >> +    are treated like CCs that have come from the *.patch file
    > 
    > Not clear on the difference between these.
    > 
    >> +  --arg <argument> | -a <argument> ...
    >> +    Arguments passed on to get_maintainer.pl
    >> +    This option can be used multiple times, e.g. -a <a1> -a <a2> ...
    >> +  --verbose
    >> +    Show more output
    >> +  --help | -h
    >> +    Show this help information
    >> +
    >> +PROCESSING POLICY:
    > 
    > Why is this called 'policy'?  This seems to be definitions.
    > 
    >> +------------------
    >> +  *.patch files consist of several sections relevant to processing:
    >> +  <top>:   This is the email header containing email related information
    >> +           It ends with the Subject: line
    >> +  <body>:  This is the body that ends up in the commit message
    >> +           It ends with ---
    >> +  <--->:   This section contains the actual patches. CCs added here are
    >> +           processed by git send-email, but are not stored in the commit
    >> +           message. Some people add CCs into this section
    > 
    > <---> is not a normal name (how do you say it? "dash-dash-dash"?), and
    > worse yet might be confused with an option.  `--inspatch cc---` looks
    > like there was some sort of mistake.  "Top" would normally mean, "Top of
    > the body of the mail".
    > 
    > I think it would be better to call these sections <header>, <commit> and
    > "commit message", and <comment> and "reviewer comment section",
    > respectively.
    > 
    > 
    >> +  <ccend>: It ends with '-- '
    >> +
    >> +  Note that cover letters do not have the <body> section.
    >> +
    >> +  The following options specifiy how CCs are insertied into *.patch files
    >> +  top:    Insert CCs into the email header
    >> +          Insert CCs from *-by: tags and TOs from mailing lists into the header
    >> +          (this is the default)
    >> +  ccbody: Insert CCs into body
    >> +          Insert CCs from *-by: tags and TOs from mailing lists into the header
    >> +          unless specified otherwise (via --tagscc).
    >> +  cc---:  Insert CCs just after the --- line
    >> +          Insert CCs from *-by: tags and TOs from mailing lists into the header
    >> +          unless specified otherwise (via --tagscc).
    >> +  ccend:  Insert CCs before the '-- ' line
    >> +          Insert CCs from *-by: tags and TOs from mailing lists into the header
    >> +          unless specified otherwise (via --tagscc).
    >> +  none:   Neither insert TO, CCs from --tags nor other CCs
    > 
    > I don't really get this section.
    > 
    > What about having the functionality be something like this?  (Obvious
    > this would need some code changes as well.  Also I guessed what the
    > significance of the `-- ` is in the cover letter, so correct me if I'm
    > wrong.)
    > 
    > ---
    > USAGE: $tool [options] (--patchdir | -d) <patchdir>
    > 
    > OPTIONS:
    > --------
    > 
    >   --reroll-count <n> | -v <n>
    >     Choose patch files for specific version. This results into the
    >     following filters on <patchdir>
    >     0: default - *.patch
    >     >1: v<n>*.patch
    > 
    >   --patchcc (top|commit|comment|none) | -p (top|commit|comment|none)
    > 
    >     Insert CC lines into *.patch files in the specified location.
    >     See LOCATIONS for a definition of the various locations.
    > 
    >     The default is `top`.
    > 
    >   --covercc (top|end|none) | -c (top|end|none)
    > 
    >     Insert CC lines into cover letter in the specified location. See
    >     LOCATIONS for a definition of the various locations.
    > 
    >     The default is `top`.
    > 
    >   --tags | -t
    > 
    >     In addition to the output of get_maintainer.pl, include email
    >     addresses from commit tags (e.g., Reviewed-by, Tested-by, &c) in
    >     the list of CC lines to insert.
    
    OK, having talked to you IRL I now understand what these are about.  How
    about this:
    
    ---
      --tags | -t
    
        In addition to the output of get_maintainer.pl, include email
        addresses from commit tags (e.g., Reviewed-by, Tested-by, &c) in
        the list of CC lines to insert.
    
        For patches, these extra lines will be inserted as specified by
        the --patchcc option, *unless* that option is set to `commit`.  In
        that case, rather than duplicate tags in the commit message with
        CC lines, the extra CC lines will be added to he header instead.
    
      --tagscc
    
        As above, but `commit` location is not special-cased: In the case
        of `commit`, all CCs will be added to the bottom of the commit
        message, even if they duplicate other tags in the commit message.
    ---
    
    Or, perhaps even better:
    
    ---
      --tags[=(split|combined)] | -t
    
        In addition to the output of get_maintainer.pl, collect email
        addresses from commit tags (e.g., Reviewed-by, Tested-by, &c) in
        the list of CC lines to insert.
    
        If 'split' is specified, these tag CC lines will be added to the
        header, rather than in the location specified by --patchcc.  In
        the case that `commit` is specified, this will avoid duplicating
        tags and CC lines in the commit message.  This is the default when
        `--patchcc commit` is specified.
    
        If 'combined' is specified, these tag CC lines will be added along
        with the maintainer CC's, wherever specified by --patchcc.  This
        is the default when "header" or "comment" are specified.
    ---

George: thanks for the feedback. I think that's an improvement. 

Although the behaviour you describe is not quite correct as
--tags will always insert any CC lines into the top of a mail aka
simulating --tagscc=top and --covercc=top
, whereas
--tagscc follows the behaviour specified by --patchcc and --covercc

I don't mind renaming the options (and some of the code which 
uses the term policy in variable names, as that makes things clearer. 

Lars

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

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

[Lars Kurth]

On 10/05/2018, 12:38, "George Dunlap" <george.dunlap@citrix.com> wrote:

    On 05/04/2018 09:36 AM, Lars Kurth wrote:
    > The tool covers step 2 of the following workflow
    > 
    >   Step 1: git format-patch ... -o <patchdir> ...
    >   Step 2: ./scripts/add_maintainers.pl -d <patchdir>
    >           This overwrites  *.patch files in <patchdir>
    >   Step 3: git send-email -to xen-devel@lists.xenproject.org <patchdir>/*.patchxm
    > 
    > I manually tested all options and the most common combinations
    > on Mac.
    > 
    > Changes since v1:
    > - Added RAB (indicated by Juergen on IRC that this is OK)
    > - Remove trailing whitespaces
    > - Renamed --prefix to --reroll-count
    > - Cleaned up short options -v, ... to be in line with git
    > - Added --tags|-t option to add AB, RAB and RB emails to CC list
    > - Added --insert|-i mode to allow for people adding CCs to commit message
    >   instead of the e-mail header (the header is the default)
    > - Moved common code into functions
    > - Added logic, such that the tool only insert's To: and Cc: statements
    >   which were not there before, allowing for running the tool multiple times
    >   on the same <patchdir>
    > 
    > Changes since v2:
    > - Deleted --version and related infrastructure
    > - Added subroutine prototypes
    > - Removed AT and @lists declaration and used \@ in literals
    > - Changed usage message and options based on feedback
    > - Improved error handling
    > - Removed occurances of index() and replaced with regex
    > - Removed non-perl idioms
    > - Moved uniq statements to normalize and added info on what normalize does
    > - Read L: tags from MAINTAINERS file instead of using heuristic
    > - Fixed issues related to metacharacters in getmaintainers()
    > - Allow multiple -a | --arg values (because of this renamed --args)
    > - Identify tags via regex
    > - CC's from tags are only inserted in the mail header, never the body
    > - That is unless the new option --tagscc is used
    > - Added policy processing which includes reworking insert()
    > - Replaced -i|--insert with -p|--inspatch and -c|--inscover now using policies
    > - Added new policies to cover for all user requests
    > - Rewrote help message to center around usage of policies
    > - Reordered some code (e.g. help string first to make code more easily readable)
    > 
    > Cc: Andrew Cooper <andrew.cooper3@citrix.com>
    > Cc: George Dunlap <George.Dunlap@eu.citrix.com>
    > Cc: Ian Jackson <ian.jackson@eu.citrix.com>
    > Cc: Jan Beulich <jbeulich@suse.com>
    > Cc: Julien Grall <julien.grall@arm.com>
    > Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
    > Cc: Stefano Stabellini <sstabellini@kernel.org>
    > Cc: Tim Deegan <tim@xen.org>
    > Cc: Wei Liu <wei.liu2@citrix.com>
    > Signed-off-by: Lars Kurth <lars.kurth@citrix.com>
    > Release-acked-by: Juergen Gross <jgross@suse.com>
    > ---
    >  scripts/add_maintainers.pl | 512 +++++++++++++++++++++++++++++++++++++++++++++
    >  1 file changed, 512 insertions(+)
    >  create mode 100755 scripts/add_maintainers.pl
    > 
    > diff --git a/scripts/add_maintainers.pl b/scripts/add_maintainers.pl
    > new file mode 100755
    > index 0000000000..11ae60d888
    > --- /dev/null
    > +++ b/scripts/add_maintainers.pl
    > @@ -0,0 +1,512 @@
    > +#!/usr/bin/perl -w
    > +# (c) 2018, Lars Kurth <lars.kurth@citrix.com>
    > +#
    > +# Add maintainers to patches generated with git format-patch
    > +#
    > +# Usage: perl scripts/add_maintainers.pl [OPTIONS] -patchdir <patchdir>
    > +#
    > +# Prerequisites: Execute
    > +#                git format-patch ... -o <patchdir> ...
    > +#
    > +#                ./scripts/get_maintainer.pl is present in the tree
    > +#
    > +# Licensed under the terms of the GNU GPL License version 2
    > +
    > +use strict;
    > +
    > +use Getopt::Long qw(:config no_auto_abbrev);
    > +use File::Basename;
    > +use List::MoreUtils qw(uniq);
    > +
    > +sub getmaintainers ($$$);
    > +sub gettagsfrompatch ($$$;$);
    > +sub normalize ($$);
    > +sub insert ($$$$);
    > +sub hastag ($$);
    > +
    > +# Tool Variables
    > +my $tool = $0;
    > +my $usage = <<EOT;
    > +USAGE: $tool [options] (--patchdir | -d) <patchdir>
    > +
    > +OPTIONS:
    > +--------
    > +  --reroll-count <n> | -v <n>
    > +    Choose patch files for specific version. This results into the
    > +    following filters on <patchdir>
    > +    0: default - *.patch
    > +    >1: v<n>*.patch
    > +  --inspatch (top|ccbody|cc---|none) | -p (top|ccbody|cc---|none)
    > +    Insert email addresses into *.patch files according to the POLICY
    > +    See section POLICY:
    > +  --inscover (top|ccend|none) | -c (top|ccend|none)
    > +    Insert email addresses into cover letteraccording to the POLICY
    > +    See section PROCESSING POLICY:
    > +  --tags | -t
    > +    Read email addresses from tags and add to CC list.
    > +    Note that git send-email does not do this. It will add the senders
    > +    email adress to the CC list though
    > +  --tagscc
    > +    Same as tags, only that in this case CCs extracted from tags
    > +    are treated like CCs that have come from the *.patch file
    
    Not clear on the difference between these.
    
    > +  --arg <argument> | -a <argument> ...
    > +    Arguments passed on to get_maintainer.pl
    > +    This option can be used multiple times, e.g. -a <a1> -a <a2> ...
    > +  --verbose
    > +    Show more output
    > +  --help | -h
    > +    Show this help information
    > +
    > +PROCESSING POLICY:
    
    Why is this called 'policy'?  This seems to be definitions.
    
    > +------------------
    > +  *.patch files consist of several sections relevant to processing:
    > +  <top>:   This is the email header containing email related information
    > +           It ends with the Subject: line
    > +  <body>:  This is the body that ends up in the commit message
    > +           It ends with ---
    > +  <--->:   This section contains the actual patches. CCs added here are
    > +           processed by git send-email, but are not stored in the commit
    > +           message. Some people add CCs into this section
    
    <---> is not a normal name (how do you say it? "dash-dash-dash"?), and
    worse yet might be confused with an option.  `--inspatch cc---` looks
    like there was some sort of mistake.  "Top" would normally mean, "Top of
    the body of the mail".
    
    I think it would be better to call these sections <header>, <commit> and
    "commit message", and <comment> and "reviewer comment section",
    respectively.
    
    
    > +  <ccend>: It ends with '-- '
    > +
    > +  Note that cover letters do not have the <body> section.
    > +
    > +  The following options specifiy how CCs are insertied into *.patch files
    > +  top:    Insert CCs into the email header
    > +          Insert CCs from *-by: tags and TOs from mailing lists into the header
    > +          (this is the default)
    > +  ccbody: Insert CCs into body
    > +          Insert CCs from *-by: tags and TOs from mailing lists into the header
    > +          unless specified otherwise (via --tagscc).
    > +  cc---:  Insert CCs just after the --- line
    > +          Insert CCs from *-by: tags and TOs from mailing lists into the header
    > +          unless specified otherwise (via --tagscc).
    > +  ccend:  Insert CCs before the '-- ' line
    > +          Insert CCs from *-by: tags and TOs from mailing lists into the header
    > +          unless specified otherwise (via --tagscc).
    > +  none:   Neither insert TO, CCs from --tags nor other CCs
    
    I don't really get this section.
    
    What about having the functionality be something like this?  (Obvious
    this would need some code changes as well.  Also I guessed what the
    significance of the `-- ` is in the cover letter, so correct me if I'm
    wrong.)
    
    ---
    USAGE: $tool [options] (--patchdir | -d) <patchdir>
    
    OPTIONS:
    --------
    
      --reroll-count <n> | -v <n>
        Choose patch files for specific version. This results into the
        following filters on <patchdir>
        0: default - *.patch
        >1: v<n>*.patch
    
      --patchcc (top|commit|comment|none) | -p (top|commit|comment|none)
    
        Insert CC lines into *.patch files in the specified location.
        See LOCATIONS for a definition of the various locations.
    
        The default is `top`.
    
      --covercc (top|end|none) | -c (top|end|none)
    
        Insert CC lines into cover letter in the specified location. See
        LOCATIONS for a definition of the various locations.
    
        The default is `top`.
    
      --tags | -t
    
        In addition to the output of get_maintainer.pl, include email
        addresses from commit tags (e.g., Reviewed-by, Tested-by, &c) in
        the list of CC lines to insert.
    
      --arg <argument> | -a <argument> ...
        Arguments passed on to get_maintainer.pl
        This option can be used multiple times, e.g. -a <a1> -a <a2> ...
    
      --verbose
        Show more output
    
      --help | -h
        Show this help information
    
    LOCATIONS
    ---------
    
      *.patch and cover letters files consist of several sections relevant
       to processing:
    
      <header>:  This is the email header containing email related information
                 It ends with the Subject: line
    
      <commit>: This is the email body that ends up in the commit message.
                 It ends with ---.  CC lines added here will be checked
                 into the git tree on commit.  Only applicable to normal
                 patch files.
    
      <comment>: This is the 'comment for reviewers' section, after the
                 --- but before the diff actually starts. CCs added here
                 are processed by git send-email, but are not checked into
                 the git tree on commit.  Only applicable to normal patch
                 files.
    
      <end>:     The part of a cover letter just before `-- ` (which normally
                 begins a diffstat).  Only applicable to cover letters.
    
    ---

That looks like a good improvement. 
Let me have a go at this with the other comments also

Lars

    

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