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

[George Dunlap <george.dunlap@xxxxxxxxxx>]

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

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

---
  --tags | -t

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

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

  --tagscc

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

Or, perhaps even better:

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

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

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

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

 -George

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