On Thu, Aug 03, 2023 at 01:23:16AM -0700, Guru Das Srinagesh wrote:
> This script runs get_maintainer.py on a given patch file and adds its
> output to the patch file in place with the appropriate email headers
> "To: " or "Cc: " as the case may be. These new headers are added after
> the "From: " line in the patch.
>
> Currently, for a single patch, maintainers are added as "To: ", mailing
> lists and all other roles are addded as "Cc: ".
>
> For a series of patches, however, a set-union scheme is employed in
> order to solve the all-too-common problem of sending subsets of a patch
> series to some lists, which results in important pieces of context such
> as the cover letter being dropped. This scheme is as follows:
> - Create set-union of all mailing lists corresponding to all patches and
> add this to all patches as "Cc: "
> - Create set-union of all other roles corresponding to all patches and
> add this to all patches as "Cc: "
> - Create set-union of all maintainers from all patches and use this to
> do the following per patch:
> - add only that specific patch's maintainers as "To: ", and
> - the other maintainers from the other patches as "Cc: "
>
Thanks. I have tested this logic by running this script on two patches
from different subsystems. It does what it says.
> Please note that patch files that don't have any "Maintainer"s
> explicitly listed in their `get_maintainer.pl` output will not have any
> "To: " entries added to them; developers are expected to manually make
> edits to the added entries in such cases to convert some "Cc: " entries
> to "To: " as desired.
>
> The script is quiet by default (only prints errors) and its verbosity
> can be adjusted via an optional parameter.
>
> Signed-off-by: Guru Das Srinagesh <[email protected]>
> ---
> scripts/add-maintainer.py | 113 ++++++++++++++++++++++++++++++++++++++
> 1 file changed, 113 insertions(+)
> create mode 100755 scripts/add-maintainer.py
>
Do you need to update MAINTAINERS file?
> diff --git a/scripts/add-maintainer.py b/scripts/add-maintainer.py
> new file mode 100755
> index 000000000000..b1682c2945f9
> --- /dev/null
> +++ b/scripts/add-maintainer.py
> @@ -0,0 +1,113 @@
> +#! /usr/bin/env python3
> +
> +import argparse
> +import logging
> +import os
> +import sys
> +import subprocess
> +import re
> +
> +def gather_maintainers_of_file(patch_file):
> + all_entities_of_patch = dict()
> +
> + # Run get_maintainer.pl on patch file
> + logging.info("GET: Patch: {}".format(os.path.basename(patch_file)))
> + cmd = ['scripts/get_maintainer.pl']
> + cmd.extend([patch_file])
> + p = subprocess.run(cmd, stdout=subprocess.PIPE, check=True)
> + logging.debug("\n{}".format(p.stdout.decode()))
> +
> + entries = p.stdout.decode().splitlines()
> +
> + maintainers = []
> + lists = []
> + others = []
> +
> + for entry in entries:
> + entity = entry.split('(')[0].strip()
> + if "maintainer" in entry:
> + maintainers.append(entity)
> + elif "list" in entry:
> + lists.append(entity)
> + else:
> + others.append(entity)
> +
> + all_entities_of_patch["maintainers"] = set(maintainers)
> + all_entities_of_patch["lists"] = set(lists)
> + all_entities_of_patch["others"] = set(others)
> +
> + return all_entities_of_patch
> +
> +def add_maintainers_to_file(patch_file, entities_per_file, all_entities_union):
> + logging.info("ADD: Patch: {}".format(os.path.basename(patch_file)))
> +
> + # For each patch:
> + # - Add all lists from all patches in series as Cc:
> + # - Add all others from all patches in series as Cc:
> + # - Add only maintainers of that patch as To:
> + # - Add maintainers of other patches in series as Cc:
> +
> + lists = list(all_entities_union["all_lists"])
> + others = list(all_entities_union["all_others"])
> + file_maintainers = all_entities_union["all_maintainers"].intersection(entities_per_file[os.path.basename(patch_file)].get("maintainers"))
> + other_maintainers = all_entities_union["all_maintainers"].difference(entities_per_file[os.path.basename(patch_file)].get("maintainers"))
> +
> + # Specify email headers appropriately
> + cc_lists = ["Cc: " + l for l in lists]
> + cc_others = ["Cc: " + o for o in others]
> + to_maintainers = ["To: " + m for m in file_maintainers]
> + cc_maintainers = ["Cc: " + om for om in other_maintainers]
> + logging.debug("Cc Lists:\n{}".format('\n'.join(cc_lists)))
> + logging.debug("Cc Others:\n{}".format('\n'.join(cc_others)))
> + logging.debug("Cc Maintainers:\n{}".format('\n'.join(cc_maintainers) or None))
> + logging.debug("To Maintainers:\n{}\n".format('\n'.join(to_maintainers) or None))
> +
> + # Edit patch file in place to add maintainers
> + with open(patch_file, "r") as pf:
> + lines = pf.readlines()
> +
> + from_line = [i for i, line in enumerate(lines) if re.search("From: ", line)]
> + if len(from_line) > 1:
> + logging.error("Only one From: line is allowed in a patch file")
> + sys.exit(1)
> +
Few minor issues from my limited testing:
- It is very unlikely, but for whatever reason if "From:" is present in
the patch (commit description), this script bails out. Pls try running
this script on the current patch. May be you should also look for a
proper email address on this line.
- When this script is run on a file (get_maintainer.pl allows this), it
throws a runtime warning. may be good to bail out much earlier.
- When this script runs on a non-existent file, it does not bail out
early.
Thanks,
Pavan
On Aug 03 2023 14:34, Pavan Kondeti wrote:
> On Thu, Aug 03, 2023 at 01:23:16AM -0700, Guru Das Srinagesh wrote:
> > This script runs get_maintainer.py on a given patch file and adds its
> > output to the patch file in place with the appropriate email headers
> > "To: " or "Cc: " as the case may be. These new headers are added after
> > the "From: " line in the patch.
> >
> > Currently, for a single patch, maintainers are added as "To: ", mailing
> > lists and all other roles are addded as "Cc: ".
> >
> > For a series of patches, however, a set-union scheme is employed in
> > order to solve the all-too-common problem of sending subsets of a patch
> > series to some lists, which results in important pieces of context such
> > as the cover letter being dropped. This scheme is as follows:
> > - Create set-union of all mailing lists corresponding to all patches and
> > add this to all patches as "Cc: "
> > - Create set-union of all other roles corresponding to all patches and
> > add this to all patches as "Cc: "
> > - Create set-union of all maintainers from all patches and use this to
> > do the following per patch:
> > - add only that specific patch's maintainers as "To: ", and
> > - the other maintainers from the other patches as "Cc: "
> >
>
> Thanks. I have tested this logic by running this script on two patches
> from different subsystems. It does what it says.
Thanks for testing this v2!
>
> > Please note that patch files that don't have any "Maintainer"s
> > explicitly listed in their `get_maintainer.pl` output will not have any
> > "To: " entries added to them; developers are expected to manually make
> > edits to the added entries in such cases to convert some "Cc: " entries
> > to "To: " as desired.
> >
> > The script is quiet by default (only prints errors) and its verbosity
> > can be adjusted via an optional parameter.
> >
> > Signed-off-by: Guru Das Srinagesh <[email protected]>
> > ---
> > scripts/add-maintainer.py | 113 ++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 113 insertions(+)
> > create mode 100755 scripts/add-maintainer.py
> >
>
> Do you need to update MAINTAINERS file?
Noted.
>
[...]
>
> Few minor issues from my limited testing:
>
> - It is very unlikely, but for whatever reason if "From:" is present in
> the patch (commit description), this script bails out. Pls try running
> this script on the current patch. May be you should also look for a
> proper email address on this line.
>
> - When this script is run on a file (get_maintainer.pl allows this), it
> throws a runtime warning. may be good to bail out much earlier.
>
> - When this script runs on a non-existent file, it does not bail out
> early.
Will fix these.
Thank you.
Guru Das.