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

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

[Lars Kurth]

To avoid issues with duplicate e-mail sending, I also cleaned up MAINTAINERS

I made significant changes to the UI to simplify its usage.
I also 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>

Lars Kurth (2):
  Replace occurances of xen.org with xenproject.org
  Add new add_maintainers.pl script to optimise the workflow when using
    git format-patch with get_maintainer.pl

 MAINTAINERS                |  24 +--
 scripts/add_maintainers.pl | 380 +++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 392 insertions(+), 12 deletions(-)
 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 v2 1/2] Replace occurances of xen.org with xenproject.org

[Lars Kurth]

This is a general clean-up activity. It also avoids mails being
sent to xen-devel@lists.xenproject.org and xen-devel@lists.xen.org
when used with add_maintainers.pl/git send-email

Changes in v2
- Added RAB
- Left Tim Deegan's mail as is

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>
Cc: Juergen Gross <jgross@suse.com>
Signed-off-by: Lars Kurth <lars.kurth@citrix.com>

Released-acked-by: Juergen Gross <jgross@suse.com>
---
 MAINTAINERS | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index bbda4b9f43..90aa759145 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -18,7 +18,7 @@ trivial patch so apply some common sense.
 	and variable names.  These aren't as silly as they seem. One
 	job the maintainers do is to keep things looking the same.
 
-	PLEASE see http://wiki.xen.org/wiki/Submitting_Xen_Patches for
+	PLEASE see http://wiki.xenproject.org/wiki/Submitting_Xen_Patches for
 	hints on how to submit a patch to xen-unstable in a suitable
 	form.
 
@@ -46,7 +46,7 @@ trivial patch so apply some common sense.
 The policy for inclusion in a Xen stable release is different to that
 for inclusion in xen-unstable.
 
-Please see http://wiki.xen.org/wiki/Xen_Maintenance_Releases for more
+Please see http://wiki.xenproject.org/wiki/Xen_Maintenance_Releases for more
 information.
 
 Backport requests should be made on the xen-devel@lists.xenproject.org
@@ -163,7 +163,7 @@ ARM (W/ VIRTUALISATION EXTENSIONS) ARCHITECTURE
 M:	Stefano Stabellini <sstabellini@kernel.org>
 M:	Julien Grall <julien.grall@arm.com>
 S:	Supported
-L:	xen-devel@lists.xen.org
+L:	xen-devel@lists.xenproject.org
 F:	docs/misc/arm/
 F:	xen/arch/arm/
 F:	xen/drivers/char/arm-uart.c
@@ -290,7 +290,7 @@ MINI-OS
 M:	Samuel Thibault <samuel.thibault@ens-lyon.org>
 S:	Supported
 L:	minios-devel@lists.xenproject.org
-T:	git git://xenbits.xen.org/mini-os.git
+T:	git git://xenbits.xenproject.org/mini-os.git
 F:	config/MiniOS.mk
 
 OCAML TOOLS
@@ -303,7 +303,7 @@ OVMF UPSTREAM
 M:	Anthony PERARD <anthony.perard@citrix.com>
 M:	Wei Liu <wei.liu2@citrix.com>
 S:	Supported
-T:	git git://xenbits.xen.org/ovmf.git
+T:	git git://xenbits.xenproject.org/ovmf.git
 
 POWER MANAGEMENT
 M:	Jan Beulich <jbeulich@suse.com>
@@ -327,13 +327,13 @@ F:	tools/python
 QEMU-DM
 M:	Ian Jackson <ian.jackson@eu.citrix.com>
 S:	Supported
-T:	git git://xenbits.xen.org/qemu-xen-traditional.git
+T:	git git://xenbits.xenproject.org/qemu-xen-traditional.git
 
 QEMU UPSTREAM
 M:	Stefano Stabellini <sstabellini@kernel.org>
 M:	Anthony Perard <anthony.perard@citrix.com>
 S:	Supported
-T:	git git://xenbits.xen.org/qemu-xen.git
+T:	git git://xenbits.xenproject.org/qemu-xen.git
 
 REMUS
 M:	Shriram Rajagopalan <rshriram@cs.ubc.ca>
@@ -361,7 +361,7 @@ F:	xen/common/sched*
 SEABIOS UPSTREAM
 M:	Wei Liu <wei.liu2@citrix.com>
 S:	Supported
-T:	git git://xenbits.xen.org/seabios.git
+T:	git git://xenbits.xenproject.org/seabios.git
 
 STUB DOMAINS
 M:	Samuel Thibault <samuel.thibault@ens-lyon.org>
@@ -398,13 +398,13 @@ F:	docs/misc/tmem*
 UNMODIFIED LINUX PV DRIVERS
 M:	Jan Beulich <jbeulich@suse.com>
 S:	Obsolete
-L:	xen-devel@lists.xen.org
+L:	xen-devel@lists.xenproject.org
 F:	unmodified_drivers/linux-2.6/
 
 USB PV DRIVERS
 M:	Noboru Iwamatsu <n_iwamatsu@jp.fujitsu.com>
 S:	Supported
-T:	hg http://xenbits.xen.org/linux-2.6.18-xen.hg
+T:	hg http://xenbits.xenproject.org/linux-2.6.18-xen.hg
 F:	drivers/xen/usb*/
 
 VM EVENT, MEM ACCESS and MONITOR
@@ -441,7 +441,7 @@ X86 ARCHITECTURE
 M:	Jan Beulich <jbeulich@suse.com>
 M:	Andrew Cooper <andrew.cooper3@citrix.com>
 S:	Supported
-L:	xen-devel@lists.xen.org
+L:	xen-devel@lists.xenproject.org
 F:	xen/arch/x86/
 F:	xen/include/asm-x86/
 F:	xen/include/public/arch-x86/
@@ -513,7 +513,7 @@ M:	Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
 M:	Stefano Stabellini <sstabellini@kernel.org>
 M:	Tim Deegan <tim@xen.org>
 M:	Wei Liu <wei.liu2@citrix.com>
-L:	xen-devel@lists.xen.org
+L:	xen-devel@lists.xenproject.org
 S:	Supported
 F:	*
 F:	*/
-- 
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 v2 1/2] Replace occurances of xen.org with xenproject.org

[Ian Jackson]

Lars Kurth writes ("[PATCH for-4.11 v2 1/2] Replace occurances of xen.org with xenproject.org"):
> This is a general clean-up activity. It also avoids mails being
> sent to xen-devel@lists.xenproject.org and xen-devel@lists.xen.org
> when used with add_maintainers.pl/git send-email

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

It would be nice to replace many of the http:// urls with https.  But
that shouldn't block this patch.

Ian.

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

Re: [PATCH for-4.11 v2 1/2] Replace occurances of xen.org with xenproject.org

[Lars Kurth]

On 30/04/2018, 14:38, "Ian Jackson" <ian.jackson@citrix.com> wrote:

    Lars Kurth writes ("[PATCH for-4.11 v2 1/2] Replace occurances of xen.org with xenproject.org"):
    > This is a general clean-up activity. It also avoids mails being
    > sent to xen-devel@lists.xenproject.org and xen-devel@lists.xen.org
    > when used with add_maintainers.pl/git send-email
    
    Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
    
    It would be nice to replace many of the http:// urls with https.  But
    that shouldn't block this patch.
   
Sure, I can do that. I don't mind re-sending it with those changes.

Lars
    

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

Re: [PATCH for-4.11 v2 1/2] Replace occurances of xen.org with xenproject.org

[Ian Jackson]

Lars Kurth writes ("Re: [PATCH for-4.11 v2 1/2] Replace occurances of xen.org with xenproject.org"):
> On 30/04/2018, 14:38, "Ian Jackson" <ian.jackson@citrix.com> wrote:
> 
>     Lars Kurth writes ("[PATCH for-4.11 v2 1/2] Replace occurances of xen.org with xenproject.org"):
>     > This is a general clean-up activity. It also avoids mails being
>     > sent to xen-devel@lists.xenproject.org and xen-devel@lists.xen.org
>     > when used with add_maintainers.pl/git send-email
>     
>     Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
>     
>     It would be nice to replace many of the http:// urls with https.  But
>     that shouldn't block this patch.
>    
> Sure, I can do that. I don't mind re-sending it with those changes.

Thanks - but as a separate patch, if you do, please !

Thanks,
Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/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]

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>/*.patch

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

Changes in v2
- 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>

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>
Cc: Juergen Gross <jgross@suse.com>
Signed-off-by: Lars Kurth <lars.kurth@citrix.com>

Release-acked-by: Juergen Gross <jgross@suse.com>
---
 scripts/add_maintainers.pl | 380 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 380 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..616c13caa1
--- /dev/null
+++ b/scripts/add_maintainers.pl
@@ -0,0 +1,380 @@
+#!/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);
+
+# Tool Variables
+my $tool = $0;
+my $toolversion = "1.0";
+
+# Arguments / Options
+my $version = 0;
+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 $mode = "top";
+my @modes = ("top", "ccbody");
+my $tags = 0;
+
+# Constants
+my @tags                = ("Acked-by:",
+                           "Release-acked-by:",
+                           "Reviewed-by:");
+my $CC                  = "Cc:"; # Note: git-send-mail requires Cc:
+my $TO                  = "To:";
+my $AT                  = "@";
+my $cc_insert_before    = "Signed-off-by:";
+my $to_insert_before    = "Date:";
+my $cover_letter        = "0000-cover-letter.patch";
+my $get_maintainer      = "./scripts/get_maintainer.pl";
+my $patch_ext           = ".patch";
+my $mailing_lists       = $AT."lists.";
+
+my @lists; #Needed for <<EOT
+my $usage = <<EOT;
+USAGE: $tool [options] (--patchdir|-d) <patchdir>
+VERSION: $toolversion
+
+OPTIONS:
+--------
+  [(--reroll-count|-v) <n>]
+    Choose patch files for specific version. This results into the
+    following filters on <patchdir>
+    0: default - *.patch
+    >1: v<n>*.patch
+  [(--insert|-i) (top|ccbody)]
+    top: default - insert everything in the e-mail header
+    ccbody: insert CCs into body (this means the CC list will be stored in
+            the commit message
+  [(--tags|-t)]
+    Read email addresses from tags and add to CC list.
+  [(--args|-a) <arguments>]
+    Arguments passed on to $get_maintainer
+  [--verbose]
+    Show more output
+  [--version]
+    Show version
+  --h|help|usage
+    Show this help information
+
+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
+
+if (!GetOptions(
+                'd|patchdir=s'     => \$patch_dir,
+                'v|reroll-count=i' => \$rerollcount,
+                'i|insert=s'       => \$mode,
+                't|tags'           => \$tags,
+                'a|args=s'         => \$get_maintainer_args,
+                'verbose'          => \$verbose,
+                'version'          => \$version,
+                'h|help|usage'     => \$help,
+                )) {
+    die "$tool: invalid argument - use --help if necessary\n";
+}
+
+if ($help != 0) {
+    print $usage;
+    exit 0;
+}
+
+if ($version != 0) {
+    print("$tool: version $toolversion\n");
+    exit 0;
+}
+
+if (! -e $get_maintainer) {
+    die "$tool: The tool requires $get_maintainer\n";
+}
+
+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 $mode, @modes ) {
+    die "$tool: Invalid (-i|--insert) value\n";
+}
+
+# Get the list of patches
+my $pattern = $patch_dir.'/'.$patch_prefix.'*'.$patch_ext;
+my @patches = glob($pattern);
+my $has_cover_letter = 0;
+my $cover_letter_file;
+
+if (!scalar @patches) {
+    die "$tool: Directory $patch_dir contains no patches\n";
+}
+
+# Do the actual processing
+my $file;
+my @combined_to;
+my @combined_cc;
+
+foreach my $file (@patches) {
+    if (index($file, $cover_letter) != -1) {
+        $has_cover_letter = 1;
+        $cover_letter_file = $file;
+    } else {
+        print "Processing: ".basename($file)."\n";
+        my @to;         # To: lists returned by get_maintainers.pl
+        my @topatch;    # To: entries in *.patch
+        my @cc;         # Cc: maintainers returned by get_maintainers.pl
+        my @ccpatch;    # Cc: entries in *.patch
+        my @extrapatch; # Cc: for AB, RB, RAB in *.patch
+
+        # Read tags from output of get_maintainers.pl
+        getmaintainers($file, \@to, \@cc);
+
+        # Read all lines with CC & TO from the patch file (these will
+        # likely come from the commit message). Also read tags.
+        gettagsfrompatch($file, \@topatch, \@ccpatch, \@extrapatch);
+
+        # With -t|--tags add @extrapatch to @cc and @combined_cc
+        if ($tags) {
+            push @cc, @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 @to_only = normalize(\@to, \@topatch);
+        my @cc_only = normalize(\@cc, \@ccpatch);
+
+        # Insert CC's and TO's at the top of the *.patch file
+        if ($mode eq "top") {     
+            my $insert = join("\n", uniq (@to_only, @cc_only))."\n";
+            # Insert snippets into files
+            # $insert before "Signed-off-by:"
+            insert_before($file , $insert, $to_insert_before);
+        }
+        elsif ($mode eq "ccbody") {
+            my $to = join("\n", uniq @to_only)."\n";
+            my $cc = join("\n", uniq @cc_only)."\n";
+
+            # Insert snippets into files
+            # $cc before "Signed-off-by:"f
+            insert_before($file , $cc, $cc_insert_before);
+            # $to before suitable header line
+            insert_before($file , $to, $to_insert_before);
+        }
+    }
+}
+
+# Deal with the cover letter
+if ($has_cover_letter) {
+    my @topatch;    # To: entries in *.patch
+    my @ccpatch;    # Cc: entries in *.patch
+
+    # Read all lines with CC & TO from the patch file such that subsequent
+    # calls don't lead to duplication
+    gettagsfrompatch($cover_letter_file, \@topatch, \@ccpatch);
+
+    # In this section we normalize the lists. We remove entries
+    # that are already in the patch, from @cc and @to
+    my @to_only = normalize(\@combined_to, \@topatch);
+    my @cc_only = normalize(\@combined_cc, \@ccpatch);
+
+    my $insert = join("\n", uniq (@to_only, @cc_only))."\n";
+
+    print "Processing: ".basename($cover_letter_file)."\n";
+
+    # Insert snippets into files
+    # $to and $cc before suitable header line
+    insert_before($cover_letter_file, $insert, $to_insert_before);
+
+    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".$AT."lists.xenproject.org ".
+      $patch_dir.'/'.$patch_prefix."*.patch"."\n";
+
+exit 1;
+
+sub getmaintainers {
+    my ($file, $rto, $rcc) = @_;
+    my $cmd = $get_maintainer." ".$get_maintainer_args." < ".$file;
+    my $fh;
+
+    open($fh, "$cmd|")
+        or die "Failed to open '$cmd'\n";
+    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) {
+            push @{$rto}, $TO." ".$_;
+            push @combined_to, $TO." ".$_;
+        } else {
+            push @{$rcc}, $CC." ".$_;
+            push @combined_cc, $CC." ".$_;
+        }
+    }
+    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, \$nline)) {
+            push @{$rto}, $nline;
+            push @combined_to, $nline;
+        }
+        if (hastag($line, $CC, \$nline)) {
+            push @{$rcc}, $nline;
+            push @combined_cc, $nline;
+        }
+        # If there is an $rextra, then get various tags and add
+        # email addresses to the CC list
+        if ($rextra) {
+            my $item;
+            foreach $item (@tags) {
+                if (hastag($line, $item, \$nline)) {
+                    # Replace tag with CC, then push
+                    $nline =~ s/$item/$CC/;
+                    push @{$rextra}, $nline;
+                }
+            }
+        }
+    }
+    close $fh;
+}
+
+sub hastag ()
+{
+    my ($line, $tag, $rline) = @_;
+    my $index = index(lc $line, lc $tag);
+    if ($index != -1) {
+        if ($index == 0) {
+            # If at first position, then just return
+            ${$rline} = $line;
+            return 1;
+        } else {
+            # If not at first position, then remove white
+            # spaces before the tag and return a normalized
+            # string
+            if (substr($line, 0, $index) =~ /^\s*$/) {
+                ${$rline} = substr($line, $index);
+                return 1;
+            }
+        }
+    }
+    return 0;
+}
+
+sub normalize {
+    # Note: you cannot pass two arrays to a subroutine without loosing the
+    # information which entry belongs to which array. Thus, pass as references.
+    my ($ra, $rb) = @_;
+    my @aonly = ();
+    my %seen;
+    my $item;
+
+    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;
+
+    return $content;
+}
+
+sub writefile {
+    my ($content, $file) = @_;
+    my $fh;
+    open($fh, ">", $file)
+         or die "Could not open file '$file' $!";
+    print $fh $content;
+    close $fh;
+
+    return 1;
+}
+
+sub insert_before {
+    my ($file, $ins, $before) = @_;
+    my $content;
+
+    if ($ins eq "\n") {
+        # Not inserting anything
+        # I added this here such that I don't have to run the check
+        # when calling the function.
+        return 0;
+    }
+
+    # Read file
+    $content = readfile($file);
+
+    # Split the string and generate new content
+    my $i  = index($content, $before);
+    my $p1 = substr $content, 0, $i;
+    my $p2 = substr $content, $i;
+
+    writefile($p1.$ins.$p2, $file);
+
+    if ($verbose) {
+        print "\nInserted into ".basename($file).' before "'.$before."'".
+              "\n-----\n".$ins."-----\n";
+    }
+
+    return 1;
+}
-- 
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 v2 2/2] Add new add_maintainers.pl script to optimise the workflow when using git format-patch with get_maintainer.pl

[Ian Jackson]

Thanks for this.  It looks like it will be very useful.  I have
reviewed your script in detail.

My review is quite picky, mainly about error handling.

Also I have a lot of style comments: where I say "so and so would be
more perlish" you should feel free to leave it as it is if you prefer.
(I would need some convincing that the continued use of `index' was
appropriate.)

Lars Kurth writes ("[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"):
...
> +my $toolversion = "1.0";

I think in practice this version number will quickly become out of
date as it won't be updated.  I think it is harmless but I would drop
it.

> +my @tags                = ("Acked-by:",
> +                           "Release-acked-by:",
> +                           "Reviewed-by:");
> +my $CC                  = "Cc:"; # Note: git-send-mail requires Cc:
> +my $TO                  = "To:";
> +my $AT                  = "@";

This is a very odd variable and needs to be abolished.

> +my $cc_insert_before    = "Signed-off-by:";
> +my $to_insert_before    = "Date:";
> +my $cover_letter        = "0000-cover-letter.patch";
> +my $get_maintainer      = "./scripts/get_maintainer.pl";
> +my $patch_ext           = ".patch";
> +my $mailing_lists       = $AT."lists.";

I notice that your usual quoting character for literal strings is "
rather than '.  You might find it easier to make more use of ', which
does much less interpolation.

> +my @lists; #Needed for <<EOT

Erk.  No, you need to write \@lists.  The warning you are suppressing
here was trying to tell you that
  xen-devel@lists.xenproject.org
causes the value of the array variable @lists to be interpolated.
Which is not what you want.  I'm slightly surprised you didn't
notice...

> +my $usage = <<EOT;
> +USAGE: $tool [options] (--patchdir|-d) <patchdir>
> +VERSION: $toolversion
> +
> +OPTIONS:
> +--------
> +  [(--reroll-count|-v) <n>]

This syntax with the ( ) seems clumsy to me.  I would just write

> +  --reroll-count <n> | -v<n>

but this is a matter of taste, so no reason to withhold my ack.

> +  [--verbose]
> +    Show more output

These [ ] are clearly wrong.

> +  [--version]
> +    Show version
> +  --h|help|usage

This is clearly wrong because it's -h, not --h.  And spaces make this
easier to read.  I suggest

  +  -h | --help | --usage

(TBH I don't see the need to support --usage, but whatever.)

> +if ($help != 0) {

Conventional (idiomatic perl) style would be

   if ($help) {

(throughout).  But what you have is not wrong.

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

You may remember me saying I wanted a mode that simply transfers
maintainer information already provided in the patches.  That is
useful when the CCs have been manually specified.

I don't think such a mode is essential for me to ack this patch.

But in that mode get_maintainer is not essential.

In any case, if you do want to check it, you should stat, rather than
using a file test.  File tests on other than _ make it hard to produce
correct and useful messages on failure.  In this case, you fail to
print the errno, which you could do if you called stat.

Anyway, is it really worthwhile specifically testing that
get_maintainer exists ?  If it doesn't presumably you will try to run
it, and then get an error later which will print something sensible ?

> +if (! -e $patch_dir) {
> +    die "$tool: Directory $patch_dir does not exist\n";
> +}

Same comment as above about stat.  Also, again, it might be better not
to check and simply allow the later code to handle errors correctly.

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

> +my @patches = glob($pattern);

You should set $!=0 first because that makes proper error handling
possible.  The error handling should come immediately after the call,
so dealing with that now:

> +if (!scalar @patches) {
> +    die "$tool: Directory $patch_dir contains no patches\n";
> +}

Here you should check $!.  If $! then there was a read error on the
directory, which should be reported (with the $! value).  Doing this
also obviates the need to check $patch_dir's existence, because if it
doesn't exist you get $!=ENOENT.

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

> +    if (index($file, $cover_letter) != -1) {

This is quite an un-perlish way to do things.  Also it goes wrong if
the patch *directory* is called 0000-cover-letter.patch (which would
be mad, but it would be better not to make things worse), or if there
is a 0000-cover-letter.patch~ but no 0000-cover-letter.patch.

I suggest something like
       if ($file =~ m{/\Q$cover_letter\E$}) {
> +        # Insert CC's and TO's at the top of the *.patch file
> +        if ($mode eq "top") {     
> +            my $insert = join("\n", uniq (@to_only, @cc_only))."\n";
> +            # Insert snippets into files
> +            # $insert before "Signed-off-by:"
> +            insert_before($file , $insert, $to_insert_before);
> +        }
> +        elsif ($mode eq "ccbody") {

Conventional style would be elsif on the same line as }.

> +print "Then perform:\n".
> +      "git send-email -to xen-devel".$AT."lists.xenproject.org ".
> +      $patch_dir.'/'.$patch_prefix."*.patch"."\n";
> +
> +exit 1;

Why do you exit 1 here rather than 0 ?  It seems that your script has
succeeded.

> +sub getmaintainers {

Should have a prototype.  See perlsub(1).  You will find that you
probably need to move it earlier in the file, or add an
pre-declaration.

This applies to several of your other functions too.

> +    my ($file, $rto, $rcc) = @_;
> +    my $cmd = $get_maintainer." ".$get_maintainer_args." < ".$file;

More perlish might be
        "$get_maintainer $get_maintainer_args <$file"

But, worse, this is quite dodgy if any of this contains shell
metacharacters.  IMO you should use
    open $fh, "-|", $get_maintainer, ...
instead.

get_maintainer_args should become an array and use the appropriate
GetOptions thing for a repeatable argument.

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

> +        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]+: ?

Perl programs should nearly never use `index'.

> +                    push @{$rextra}, $nline;

Again, @{$var} can be spelled @$var and conventionally is.

> +sub hastag ()
> +{
> +    my ($line, $tag, $rline) = @_;
> +    my $index = index(lc $line, lc $tag);

ITYM
       if ($line =~ m{^\Q$tag\E}i) {
?

> +    if ($index != -1) {
> +        if ($index == 0) {
> +            # If at first position, then just return
> +            ${$rline} = $line;
> +            return 1;
> +        } else {
> +            # If not at first position, then remove white
> +            # spaces before the tag and return a normalized
> +            # string

How do you ever get a tag not in the left hand column ?  I think such
a thing, if ever found, has probably been indented specifically to
protect it from automatic processing ?

> +sub normalize {
> +    # Note: you cannot pass two arrays to a subroutine without loosing the
> +    # information which entry belongs to which array. Thus, pass as references.

I think this function needs a comment to say what it does!

So AFAICT normalize(\@x,\@y) returns the elements of @x that do not
appear in @y.

If you made normalize strip repeats from @x, which would be easy, then
it would do the work of uniq too.  That might be simpler to follow.

(The thing about arrays is standard perl but I don't mind you
mentioning it.)

> +    my ($ra, $rb) = @_;
> +    my @aonly = ();
> +    my %seen;
> +    my $item;
> +
> +    foreach $item (@{$rb}) {

Conventional idiomatic syntax would be

  +    foreach $item (@$rb) {

> +    foreach $item (@{$ra}) {
> +        unless ($seen{$item}) {
> +            # it's not in %seen, so add to @aonly
> +            push @aonly, $item;
> +        }
> +    }

This can be written much more perlishly with grep.  But if that is too
obtuse I don't mind this open-coding.

> +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 $!
> +
> +    return 1;

Why this when none of your callers check the return value ?

Returning 1 for success is in any case not a very perlish error
handling pattern.

> +sub insert_before {
> +    my ($file, $ins, $before) = @_;
> +    my $content;
> +
> +    if ($ins eq "\n") {
> +        # Not inserting anything
> +        # I added this here such that I don't have to run the check
> +        # when calling the function.
> +        return 0;
> +    }

Instead of this extra code here, I would make $ins contain as many \n
as lines.  Something like (earlier):

+    my $insert = map { "$_\n" } uniq (@to_only, @cc_only);

Except of course subject to my other comments about uniq.

Also it would be nice if the variable name here ($ins) was the same as
the earlier one ($insert).

> +    # Read file
> +    $content = readfile($file);
> +
> +    # Split the string and generate new content
> +    my $i  = index($content, $before);

Again, do this with regexps.  Something like one of these (untested):

       $content =~ s{^(?=\Q$insert_before\E)}{$content}m;

       $content =~ s{^\Q$insert_before\E)}{$content$&}m;

(Your "index" is wrong because it should match only at start of line.)

> +    return 1;
> +}

Again, why the return ?


I hope this detailed critique is helpful...

Regards,
Ian.

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

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

[Lars Kurth]

On 30/04/2018, 15:36, "Ian Jackson" <ian.jackson@citrix.com> wrote:

    > +if (! -e $get_maintainer) {
    > +    die "$tool: The tool requires $get_maintainer\n";
    > +}
    
    You may remember me saying I wanted a mode that simply transfers
    maintainer information already provided in the patches.  That is
    useful when the CCs have been manually specified.
    
    I don't think such a mode is essential for me to ack this patch.

I think I may have misunderstood you.

Given that git send-email reads CC's anywhere in the body of a *.patch file, 
I am not sure why this is useful. Can you enlighten me? It does also have all 
the bits to ensure that I can do this. Aka I can make sure that all the CC's from 
the commit-message/*.patch files are added to the e-mail block. Right now
I avoid duplication: aka I only add stuff to the header if it is not already 
In the commit-message/*.patch.  

I can see though, that some of the tool functionality is useful in non-xen 
trees. Thus, changing it such that it doesn't fall over when get_maintainers.pl 
isn't there is probably a good idea.

Lars  

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

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

[Ian Jackson]

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"):
> Given that git send-email reads CC's anywhere in the body of a *.patch file, 
> I am not sure why this is useful. Can you enlighten me? It does also have all 
> the bits to ensure that I can do this. Aka I can make sure that all the CC's from 
> the commit-message/*.patch files are added to the e-mail block. Right now
> I avoid duplication: aka I only add stuff to the header if it is not already 
> In the commit-message/*.patch.  
> 
> I can see though, that some of the tool functionality is useful in non-xen 
> trees. Thus, changing it such that it doesn't fall over when get_maintainers.pl 
> isn't there is probably a good idea.

The function of your tool is to invoke get_maintainer and put the
addresses from there everywhere appropriate including, in particular,
the CCs of the cover letter.

The only reason your tool is needed is because there is no other tool
that gets the cover letter right.

But I often have a set of patches where I have manually decided who
they should be CCd to, and put appropriate tags in my commit
messages.  I don't blindly use get_maintainer.

When I do this, there is nothing that gets the CC for the cover letter
right.  (I sometimes bodge it.)  Your tool already knows how to
extract CCs from the individual non-cover-letter patches and add them
to the cover letter.  That is the function I want - to do that, but
not run get_maintainer.

Nor do I need, I think, your tool to edit any of the non-cover-letter
patches, since git-send-email will find CCs in their bodies and use
them for the email recipients.

I don't think I really mind where the CCs end up in the cover letter
(whether they are in the body, or just in the email header), but
others have made a convincing argument that they should be in the
body, so that is fine.

Does that make sense ?

Thanks,
Ian.

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

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

[Lars Kurth]

On 30/04/2018, 17:21, "Ian Jackson" <ian.jackson@citrix.com> wrote:

    Lars Kurth writes ("Re: [PATCH for-4.11 v2 2/2] Add new add_maintainers.pl script to optimise the workflow when using git format-patch with get_maintainer.pl"):
    > Given that git send-email reads CC's anywhere in the body of a *.patch file, 
    > I am not sure why this is useful. Can you enlighten me? It does also have all 
    > the bits to ensure that I can do this. Aka I can make sure that all the CC's from 
    > the commit-message/*.patch files are added to the e-mail block. Right now
    > I avoid duplication: aka I only add stuff to the header if it is not already 
    > In the commit-message/*.patch.  
    > 
    > I can see though, that some of the tool functionality is useful in non-xen 
    > trees. Thus, changing it such that it doesn't fall over when get_maintainers.pl 
    > isn't there is probably a good idea.
    
    The function of your tool is to invoke get_maintainer and put the
    addresses from there everywhere appropriate including, in particular,
    the CCs of the cover letter.
    
    The only reason your tool is needed is because there is no other tool
    that gets the cover letter right.
    
    But I often have a set of patches where I have manually decided who
    they should be CCd to, and put appropriate tags in my commit
    messages.  I don't blindly use get_maintainer.
    
    When I do this, there is nothing that gets the CC for the cover letter
    right.  (I sometimes bodge it.)  Your tool already knows how to
    extract CCs from the individual non-cover-letter patches and add them
    to the cover letter.  That is the function I want - to do that, but
    not run get_maintainer.

That makes sense and can be easily done via an option: e.g.
--insert cover|-i cover or a separate option. Let me know whether
you have a preference regarding naming/options.
    
    Nor do I need, I think, your tool to edit any of the non-cover-letter
    patches, since git-send-email will find CCs in their bodies and use
    them for the email recipients.
    
    I don't think I really mind where the CCs end up in the cover letter
    (whether they are in the body, or just in the email header), but
    others have made a convincing argument that they should be in the
    body, so that is fine.
    
    Does that make sense ?
    
Yes

Lars    

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

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

[Ian Jackson]

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, 17:21, "Ian Jackson" <ian.jackson@citrix.com> wrote:
> 
>     When I do this, there is nothing that gets the CC for the cover letter
>     right.  (I sometimes bodge it.)  Your tool already knows how to
>     extract CCs from the individual non-cover-letter patches and add them
>     to the cover letter.  That is the function I want - to do that, but
>     not run get_maintainer.
> 
> That makes sense and can be easily done via an option: e.g.
> --insert cover|-i cover or a separate option. Let me know whether
> you have a preference regarding naming/options.

I think this is orthogonal to -i.  That is, a user who wants your tool
to only edit the cover letter might want either to have it add the CCs
only to the mail header, or into the body as well.

Perhaps --transfer-only aka -T ?  Not sure I can think of a good name.

As an aside, I guess with `-i ccbody' your tool still puts the CCs in
the mail headers of the cover letter ?  Or will git-send-email do that ?

Ian.

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

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

[Lars Kurth]

On 01/05/2018, 13:52, "Ian Jackson" <ian.jackson@citrix.com> wrote:

    Lars Kurth writes ("Re: [PATCH for-4.11 v2 2/2] Add new add_maintainers.pl script to optimise the workflow when using git format-patch with get_maintainer.pl"):
    > On 30/04/2018, 17:21, "Ian Jackson" <ian.jackson@citrix.com> wrote:
    > 
    >     When I do this, there is nothing that gets the CC for the cover letter
    >     right.  (I sometimes bodge it.)  Your tool already knows how to
    >     extract CCs from the individual non-cover-letter patches and add them
    >     to the cover letter.  That is the function I want - to do that, but
    >     not run get_maintainer.
    > 
    > That makes sense and can be easily done via an option: e.g.
    > --insert cover|-i cover or a separate option. Let me know whether
    > you have a preference regarding naming/options.
    
    I think this is orthogonal to -i.  That is, a user who wants your tool
    to only edit the cover letter might want either to have it add the CCs
    only to the mail header, or into the body as well.
    
    Perhaps --transfer-only aka -T ?  Not sure I can think of a good name.

I can do either: in fact looking at the code -i only operates on files that
are not the cover letter. 

So:
-i top: adds CC's to each *.patch except the cover letter
-i ccbody: adds CC's to the body of each *.patch file except the cover letter

Thus
-i none: could just not add anything to each *.patch file except the
 cover letter

I think that is probably the most logical way to do this
I probably need to clarify the scope of -i in the help section

In the cover letter, the tool always insert at the top as it is a transient
thing that does not end up in git.

    As an aside, I guess with `-i ccbody' your tool still puts the CCs in
    the mail headers of the cover letter ?  Or will git-send-email do that ?

git-send-email does that. No need to add extra logic
I will add a comment into the tool's code

Lars
    

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

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

[Ian Jackson]

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"):
> In the cover letter, the tool always insert at the top as it is a transient
> thing that does not end up in git.
> 
> On 01/05/2018, 13:52, "Ian Jackson" <ian.jackson@citrix.com> wrote:
>
>     As an aside, I guess with `-i ccbody' your tool still puts the CCs in
>     the mail headers of the cover letter ?  Or will git-send-email do that ?
> 
> git-send-email does that.

These replies seem to be contradictory.

Also, IIRC someone wrote earlier in the thread that they prefer the
practice of including CCs in the body of the cover letter too.  So
assuming that the first reply is accurate, it may not be desirable.

Ian.

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

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

[Lars Kurth]

On 01/05/2018, 16:16, "Ian Jackson" <ian.jackson@citrix.com> wrote:

    Lars Kurth writes ("Re: [PATCH for-4.11 v2 2/2] Add new add_maintainers.pl script to optimise the workflow when using git format-patch with get_maintainer.pl"):
    > In the cover letter, the tool always insert at the top as it is a transient
    > thing that does not end up in git.
    > 
    > On 01/05/2018, 13:52, "Ian Jackson" <ian.jackson@citrix.com> wrote:
    >
    >     As an aside, I guess with `-i ccbody' your tool still puts the CCs in
    >     the mail headers of the cover letter ?  Or will git-send-email do that ?
    > 
    > git-send-email does that.
    
    These replies seem to be contradictory.

That's because I mis-read the question. So the correct answer is:
the tool does that. But it does this regardless of -i *

    Also, IIRC someone wrote earlier in the thread that they prefer the
    practice of including CCs in the body of the cover letter too.  So
    assuming that the first reply is accurate, it may not be desirable.

That can be done easily. Maybe the best way to approach this is
to have two options that control how CCs are inserted into the cover
letter and a separate one that controls how CCs are inserted into
patches.

In that case I would probably rename 
--insert | -i to --insert-patch | -p with top, ccbody, none as admissible values
and
--insert-cover | -c with top, ccbody, none as admissible values


Cheers
Lars


In 

Lars
 

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

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

[Ian Jackson]

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"):
>  Maybe the best way to approach this is
> to have two options that control how CCs are inserted into the cover
> letter and a separate one that controls how CCs are inserted into
> patches.

I don't mind the options.  I think I care about the default, which I
think should be to include all CCs in both bodies and headers.

Ian.

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

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

[Lars Kurth]

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@citrix.com> 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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

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

[Ian Jackson]

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@citrix.com> 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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

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

[Lars Kurth]

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

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

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

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

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

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

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

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

Lars

    

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

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

[Ian Jackson]

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 02/05/2018, 11:50, "Ian Jackson" <ian.jackson@citrix.com> wrote:
>     If the address is a mailing list for some other stanza in MAINTAINERS,
>     ie for a stanza whose files are not touched by this patch, then it
>     should still be treated as a list.
>     
>     So what I meant was that you should search the whole of MAINTAINERS
>     yourself for all the L: lines, regardless of where they appear.
> 
> But that is exactly what get_maintainers.pl --nom --nor does

  mariner:xen.git> scripts/get_maintainer.pl --nom --nor
  scripts/get_maintainer.pl: missing patchfile or -f file - use --help if necessary

It requires a patch.  But for this purpose, we don't want any patch.

>     That avoids calling get_maintainer.pl twice too, so helpfully the
>     other difficulties you discuss go away.
> 
> I suppose it does. But it also makes the script re-implement bits
> of get_maintainers.pl 

Finding lines starting with L: is so simple that I wouldn't bother
calling get_maintainer.pl for it.

>     > We could make this configurable:
>     > Default: all tags, except signed-off-by (unless of course this should be added to the CC)
>     
>     Why not CC the S-o-b ?  Usually that will be the author anyway.
>     
> I wasn't sure. 
> 
> Particularly if you used -i ccbody, which we agreed in another mail should
> be the default, you then will end up with
> CC: lars.kurth@citrix.com
> ...
> Signed-off-by: lars.kurth@citrix.com
> 
> Which would annoy me personally.

Ideally no-one should be mentioned in the CC that is mentioned in some
other tag.

  CC: lars.kurth@citrix.com
  Tested-by: lars.kurth@citrix.com

is just as silly.

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

Search for "options with multiple values" in the FM.

> So I am thinking of addressing core features and the rest as separate oatches.

Sure.

Ian.

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

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

[Lars Kurth]

On 02/05/2018, 12:45, "Ian Jackson" <ian.jackson@citrix.com> wrote:

    Lars Kurth writes ("Re: [PATCH for-4.11 v2 2/2] Add new add_maintainers.pl script to optimise the workflow when using git format-patch with get_maintainer.pl"):
    > On 02/05/2018, 11:50, "Ian Jackson" <ian.jackson@citrix.com> wrote:
    >     If the address is a mailing list for some other stanza in MAINTAINERS,
    >     ie for a stanza whose files are not touched by this patch, then it
    >     should still be treated as a list.
    >     
    >     So what I meant was that you should search the whole of MAINTAINERS
    >     yourself for all the L: lines, regardless of where they appear.
    > 
    > But that is exactly what get_maintainers.pl --nom --nor does
    
      mariner:xen.git> scripts/get_maintainer.pl --nom --nor
      scripts/get_maintainer.pl: missing patchfile or -f file - use --help if necessary
    
    It requires a patch.  But for this purpose, we don't want any patch.
    
    >     That avoids calling get_maintainer.pl twice too, so helpfully the
    >     other difficulties you discuss go away.
    > 
    > I suppose it does. But it also makes the script re-implement bits
    > of get_maintainers.pl 
    
    Finding lines starting with L: is so simple that I wouldn't bother
    calling get_maintainer.pl for it.

OK. I will do that then.
    
    >     > We could make this configurable:
    >     > Default: all tags, except signed-off-by (unless of course this should be added to the CC)
    >     
    >     Why not CC the S-o-b ?  Usually that will be the author anyway.
    >     
    > I wasn't sure. 
    > 
    > Particularly if you used -i ccbody, which we agreed in another mail should
    > be the default, you then will end up with
    > CC: lars.kurth@citrix.com
    > ...
    > Signed-off-by: lars.kurth@citrix.com
    > 
    > Which would annoy me personally.
    
    Ideally no-one should be mentioned in the CC that is mentioned in some
    other tag.
    
      CC: lars.kurth@citrix.com
      Tested-by: lars.kurth@citrix.com
    
    is just as silly.

That tells me that that entire --tags infrastructure should only be used
when inserting into the header and ignored otherwise. This makes
everything simpler. No need to include extra options, etc.

If --tags is there, then
a) Retrieve any tags via the *-by: regex
b) Only insert at the top of the mail

Then there is really no reason why to do anything more complex.

Regards
Lars    

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