| Message ID | 141b9fcab2208ace3001df4fc10e3dfd42b9f5d9.1693037031.git.quic_gurus@quicinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add add-maintainer.py script | expand |
On 26/08/2023 10:07, Guru Das Srinagesh wrote: > This script runs get_maintainer.py on a given patch file (or multiple > patch files) 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 and reviewers are added as > "To: ", mailing lists and all other roles are added as "Cc: ". > > For a series of patches, however, a set-union scheme is employed in > order to solve the all-too-common problem of ending up sending only > subsets of a patch series to some lists, which results in important > pieces of context such as the cover letter (or other patches in the > series) being dropped from those lists. This scheme is as follows: > > - Create set-union of all maintainers and reviewers from all patches and > use this to do the following per patch: > - add only that specific patch's maintainers and reviewers as "To: " > - add the other maintainers and reviewers from the other patches as "Cc: " > > - 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: " > > Please note that patch files that don't have any "Maintainer"s or > "Reviewers" explicitly listed in their `get_maintainer.pl` output will So before you will ignoring the reviewers, right? One more reason to not get it right... > 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 <quic_gurus@quicinc.com> > --- > MAINTAINERS | 5 ++ > scripts/add-maintainer.py | 164 ++++++++++++++++++++++++++++++++++++++ > 2 files changed, 169 insertions(+) > create mode 100755 scripts/add-maintainer.py > I do not see the benefits of this script. For me - it's unnecessarily more complicated instead of my simple bash function which makes everything one command less than here. One more thing to maintain. I don't see the benefits of this for newcomers, either. They should use b4. b4 can do much, much more, so anyone creating his workflow should start from b4, not from this script. Best regards, Krzysztof
On Aug 28 2023 10:21, Krzysztof Kozlowski wrote: > On 26/08/2023 10:07, Guru Das Srinagesh wrote: > > This script runs get_maintainer.py on a given patch file (or multiple > > patch files) 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 and reviewers are added as > > "To: ", mailing lists and all other roles are added as "Cc: ". > > > > For a series of patches, however, a set-union scheme is employed in > > order to solve the all-too-common problem of ending up sending only > > subsets of a patch series to some lists, which results in important > > pieces of context such as the cover letter (or other patches in the > > series) being dropped from those lists. This scheme is as follows: > > > > - Create set-union of all maintainers and reviewers from all patches and > > use this to do the following per patch: > > - add only that specific patch's maintainers and reviewers as "To: " > > - add the other maintainers and reviewers from the other patches as "Cc: " > > > > - 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: " > > > > Please note that patch files that don't have any "Maintainer"s or > > "Reviewers" explicitly listed in their `get_maintainer.pl` output will > > So before you will ignoring the reviewers, right? One more reason to not > get it right... In v2, Reviewers were added as "Cc:" whereas here in v3 they are added as "To:". Not sure where you're getting "ignoring the reviewers" from. > > 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 <quic_gurus@quicinc.com> > > --- > > MAINTAINERS | 5 ++ > > scripts/add-maintainer.py | 164 ++++++++++++++++++++++++++++++++++++++ > > 2 files changed, 169 insertions(+) > > create mode 100755 scripts/add-maintainer.py > > > > I do not see the benefits of this script. For me - it's unnecessarily > more complicated instead of my simple bash function which makes Your function adds mailing lists also in "To:" which is not ideal, in my view. You've mentioned before that To or Cc doesn't matter [1] which I disagree with: it doesn't matter, why does Cc exist as a concept at all? [1] https://lore.kernel.org/lkml/af1eca37-9fd2-1e83-ab27-ebb51480904b@linaro.org/ Thank you. Guru Das.
On Sat, Aug 26, 2023 at 01:07:42AM -0700, Guru Das Srinagesh wrote: > +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]) > + > + try: > + p = subprocess.run(cmd, stdout=subprocess.PIPE, check=True) > + except: > + sys.exit(1) > + > + 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 any(role in entry for role in ["maintainer", "reviewer"]): > + 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 > + FYI, there are couple of issues found while playing around. - Some entries in MAINTAINERS could be "supporter" - When names contain ("company"), the script fails to extract name. Thanks, Pavan diff --git a/scripts/add-maintainer.py b/scripts/add-maintainer.py index 5a5cc9482b06..6aa5e7941172 100755 --- a/scripts/add-maintainer.py +++ b/scripts/add-maintainer.py @@ -29,8 +29,8 @@ def gather_maintainers_of_file(patch_file): others = [] for entry in entries: - entity = entry.split('(')[0].strip() - if any(role in entry for role in ["maintainer", "reviewer"]): + entity = entry.rsplit('(', 1)[0].strip() + if any(role in entry for role in ["maintainer", "reviewer", "supporter"]): maintainers.append(entity) elif "list" in entry: lists.append(entity) Thanks, Pavan
diff --git a/MAINTAINERS b/MAINTAINERS index 0903d87b17cb..b670e9733f03 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -8721,6 +8721,11 @@ M: Joe Perches <joe@perches.com> S: Maintained F: scripts/get_maintainer.pl +ADD MAINTAINER SCRIPT +M: Guru Das Srinagesh <quic_gurus@quicinc.com> +S: Maintained +F: scripts/add-maintainer.py + GFS2 FILE SYSTEM M: Bob Peterson <rpeterso@redhat.com> M: Andreas Gruenbacher <agruenba@redhat.com> diff --git a/scripts/add-maintainer.py b/scripts/add-maintainer.py new file mode 100755 index 000000000000..5a5cc9482b06 --- /dev/null +++ b/scripts/add-maintainer.py @@ -0,0 +1,164 @@ +#! /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]) + + try: + p = subprocess.run(cmd, stdout=subprocess.PIPE, check=True) + except: + sys.exit(1) + + 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 any(role in entry for role in ["maintainer", "reviewer"]): + 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 find_pattern_in_lines(pattern, lines): + index = 0 + for line in lines: + if re.search(pattern, line): + break; + index = index + 1 + + if index == len(lines): + logging.error("Couldn't find pattern {} in patch".format(pattern)) + sys.exit(1) + + return index + +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() + + # Get the index of the first "From: <email address>" line in patch + from_line = find_pattern_in_lines("^(From: )(.*)<(.*)@(.*)>", lines) + + # Insert our To: and Cc: headers after it. + next_line_after_from = from_line + 1 + + for l in cc_lists: + lines.insert(next_line_after_from, l + "\n") + for o in cc_others: + lines.insert(next_line_after_from, o + "\n") + for om in cc_maintainers: + lines.insert(next_line_after_from, om + "\n") + for m in to_maintainers: + lines.insert(next_line_after_from, m + "\n") + + with open(patch_file, "w") as pf: + pf.writelines(lines) + +def add_maintainers(patch_files): + entities_per_file = dict() + + for patch in patch_files: + entities_per_file[os.path.basename(patch)] = gather_maintainers_of_file(patch) + + all_entities_union = {"all_maintainers": set(), "all_lists": set(), "all_others": set()} + for patch in patch_files: + all_entities_union["all_maintainers"] = all_entities_union["all_maintainers"].union(entities_per_file[os.path.basename(patch)].get("maintainers")) + all_entities_union["all_lists"] = all_entities_union["all_lists"].union(entities_per_file[os.path.basename(patch)].get("lists")) + all_entities_union["all_others"] = all_entities_union["all_others"].union(entities_per_file[os.path.basename(patch)].get("others")) + + for patch in patch_files: + add_maintainers_to_file(patch, entities_per_file, all_entities_union) + + logging.info("Maintainers added to all patch files successfully") + +def remove_to_cc_from_header(patch_files): + for patch in patch_files: + logging.info("UNDO: Patch: {}".format(os.path.basename(patch))) + with open(patch, "r") as pf: + lines = pf.readlines() + + # Get the index of the first "From: <email address>" line in patch + from_line = find_pattern_in_lines("^(From: )(.*)<(.*)@(.*)>", lines) + + # Get the index of the first "Date: " line in patch + date_line = find_pattern_in_lines("^(Date: )", lines) + + # Delete everything in between From: and Date: + # These are the lines that this script adds - any To: or Cc: anywhere + # else in the patch will not be removed. + del lines[(from_line + 1):date_line] + + with open(patch, "w") as pf: + pf.writelines(lines) + + logging.info("Maintainers removed from all patch files successfully") + +def main(): + parser = argparse.ArgumentParser(description='Add the respective maintainers and mailing lists to patch files') + parser.add_argument('patches', nargs='+', help="One or more patch files") + parser.add_argument('-v', '--verbosity', choices=['debug', 'info', 'error'], default='error', help="Verbosity level of script output") + parser.add_argument('-u', '--undo', action='store_true', help="Remove maintainers added by this script from patch(es)") + args = parser.parse_args() + + logging.basicConfig(level=args.verbosity.upper(), format='%(levelname)s: %(message)s') + + for patch in args.patches: + if not os.path.isfile(patch): + logging.error("File does not exist: {}".format(patch)) + sys.exit(1) + + if args.undo: + remove_to_cc_from_header(args.patches) + else: + add_maintainers(args.patches) + +if __name__ == "__main__": + main()
This script runs get_maintainer.py on a given patch file (or multiple patch files) 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 and reviewers are added as "To: ", mailing lists and all other roles are added as "Cc: ". For a series of patches, however, a set-union scheme is employed in order to solve the all-too-common problem of ending up sending only subsets of a patch series to some lists, which results in important pieces of context such as the cover letter (or other patches in the series) being dropped from those lists. This scheme is as follows: - Create set-union of all maintainers and reviewers from all patches and use this to do the following per patch: - add only that specific patch's maintainers and reviewers as "To: " - add the other maintainers and reviewers from the other patches as "Cc: " - 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: " Please note that patch files that don't have any "Maintainer"s or "Reviewers" 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 <quic_gurus@quicinc.com> --- MAINTAINERS | 5 ++ scripts/add-maintainer.py | 164 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 169 insertions(+) create mode 100755 scripts/add-maintainer.py