|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] 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
On 10/05/2018, 14:03, "George Dunlap" <george.dunlap@xxxxxxxxxx> wrote:
On 05/10/2018 12:38 PM, George Dunlap wrote:
> On 05/04/2018 09:36 AM, Lars Kurth wrote:
>> The tool covers step 2 of the following workflow
>>
>> Step 1: git format-patch ... -o <patchdir> ...
>> Step 2: ./scripts/add_maintainers.pl -d <patchdir>
>> This overwrites *.patch files in <patchdir>
>> Step 3: git send-email -to xen-devel@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: thanks for the feedback. I think that's an improvement.
Although the behaviour you describe is not quite correct as
--tags will always insert any CC lines into the top of a mail aka
simulating --tagscc=top and --covercc=top
, whereas
--tagscc follows the behaviour specified by --patchcc and --covercc
I don't mind renaming the options (and some of the code which
uses the term policy in variable names, as that makes things clearer.
Lars
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |