Add new add_maintainers.pl script to optimise the workflow when using git format-patch with get_maintainer.pl

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
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel

This is a general clean-up activity. It also avoids mails being sent to [email protected] and [email protected] when used with add_maintainers.pl/git send-email

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

---
 MAINTAINERS | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

index bbda4b9f43..90aa759145 100644
@@ -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 [email protected]

@@ -163,7 +163,7 @@ ARM (W/ VIRTUALISATION EXTENSIONS) ARCHITECTURE
 M:	Stefano Stabellini <[email protected]>
 M:	Julien Grall <[email protected]>
 S:	Supported
-L:	[email protected]
+L:	[email protected]
 F:	docs/misc/arm/
 F:	xen/arch/arm/
 F:	xen/drivers/char/arm-uart.c

@@ -290,7 +290,7 @@ MINI-OS
 M:	Samuel Thibault <[email protected]>
 S:	Supported
 L:	[email protected]
-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 <[email protected]>
 M:	Wei Liu <[email protected]>
 S:	Supported
-T:	git git://xenbits.xen.org/ovmf.git
+T:	git git://xenbits.xenproject.org/ovmf.git
 
 POWER MANAGEMENT
 M:	Jan Beulich <[email protected]>

@@ -327,13 +327,13 @@ F:	tools/python
 QEMU-DM
 M:	Ian Jackson <[email protected]>
 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 <[email protected]>
 M:	Anthony Perard <[email protected]>
 S:	Supported
-T:	git git://xenbits.xen.org/qemu-xen.git
+T:	git git://xenbits.xenproject.org/qemu-xen.git
 
 REMUS
 M:	Shriram Rajagopalan <[email protected]>

@@ -361,7 +361,7 @@ F:	xen/common/sched*
 SEABIOS UPSTREAM
 M:	Wei Liu <[email protected]>
 S:	Supported
-T:	git git://xenbits.xen.org/seabios.git
+T:	git git://xenbits.xenproject.org/seabios.git
 
 STUB DOMAINS
 M:	Samuel Thibault <[email protected]>

@@ -398,13 +398,13 @@ F:	docs/misc/tmem*
 UNMODIFIED LINUX PV DRIVERS
 M:	Jan Beulich <[email protected]>
 S:	Obsolete
-L:	[email protected]
+L:	[email protected]
 F:	unmodified_drivers/linux-2.6/
 
 USB PV DRIVERS
 M:	Noboru Iwamatsu <[email protected]>
 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 <[email protected]>
 M:	Andrew Cooper <[email protected]>
 S:	Supported
-L:	[email protected]
+L:	[email protected]
 F:	xen/arch/x86/
 F:	xen/include/asm-x86/
 F:	xen/include/public/arch-x86/

@@ -513,7 +513,7 @@ M:	Konrad Rzeszutek Wilk <[email protected]>
 M:	Stefano Stabellini <[email protected]>
 M:	Tim Deegan <[email protected]>
 M:	Wei Liu <[email protected]>
-L:	[email protected]
+L:	[email protected]
 S:	Supported
 F:	*
 F:	*/

2.13.0

_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel

Lars Kurth writes ("[PATCH for-4.11 v2 1/2] Replace occurances of xen.org with xenproject.org"):

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
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel

On 30/04/2018, 14:38, "Ian Jackson" <[email protected]> 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 [email protected] and [email protected]
> when used with add_maintainers.pl/git send-email

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
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel

Lars Kurth writes ("Re: [PATCH for-4.11 v2 1/2] Replace occurances of xen.org with xenproject.org"):

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

Thanks,
Ian.

_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel

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 [email protected] <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>

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

new file mode 100755
index 0000000000..616c13caa1
@@ -0,0 +1,380 @@
+#!/usr/bin/perl -w
+# (c) 2018, Lars Kurth <[email protected]>
+#
+# 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 [email protected] <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
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel

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"):
...

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.

Erk. No, you need to write \@lists. The warning you are suppressing here was trying to tell you that
[email protected]
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...

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

Conventional (idiomatic perl) style would be

   if ($help) {

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

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:

It would be nice to exclude ~ and .bak files here. That way manually editing files won't require trickery to exclude them.

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$}) {

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.

    open $fh, "-|", $get_maintainer, ...

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

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

or die $!

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

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

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"):

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
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel

On 30/04/2018, 17:21, "Ian Jackson" <[email protected]> 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
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel

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"):

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
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel

On 01/05/2018, 13:52, "Ian Jackson" <[email protected]> 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" <[email protected]> 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
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel

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"):

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
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel

On 01/05/2018, 16:16, "Ian Jackson" <[email protected]> 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" <[email protected]> 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
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel

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"):

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
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel

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" <[email protected]> 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
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel

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"):

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

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

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.

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

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.

Why not CC the S-o-b ? Usually that will be the author anyway.

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
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel

On 02/05/2018, 11:50, "Ian Jackson" <[email protected]> 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" <[email protected]> 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

    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: [email protected]
...

Which would annoy me personally.

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
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel

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"):

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.

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

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

is just as silly.

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

Sure.

Ian.

_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel

On 02/05/2018, 12:45, "Ian Jackson" <[email protected]> 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" <[email protected]> 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.

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

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
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel