| Message ID | 829b08342568735095bbd3f8c44f435f44688018.1691049436.git.quic_gurus@quicinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add add-maintainer.py script | expand |
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 <quic_gurus@quicinc.com> > > --- > > 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.
Hi Nicolas, Thank you so much for reviewing this script! On Aug 23 2023 17:14, Nicolas Schier wrote: > Hi Guru, > > thanks for your patch! I really to appreciate the discussion about how to > lower the burden for first-time contributors; might you consider cc-ing > workflows@vger.kernel.org when sending v3? Certainly, will do. The archives for this list are very interesting to read! [...] > Some additional thoughts to the feedback from Pavan: > > 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: ". > > typo: addded -> added Done. > > The script is quiet by default (only prints errors) and its verbosity > > can be adjusted via an optional parameter. > > IMO, it would be nice to see which addresses are effectively added, e.g. > comparable to the output of git send-email. Perhaps somehing like: > > $ scripts/add-maintainer.py *.patch > 0001-fixup-scripts-Add-add-maintainer.py.patch: Adding 'To: Guru Das Srinagesh <quic_gurus@quicinc.com>' (maintainer) > 0001-fixup-scripts-Add-add-maintainer.py.patch: Adding 'Cc: linux-kernel@vger.kernel.org' (list) > > Perhaps verbosity should then be configurable. Yes, this is already implemented - you just need to pass "--verbosity debug" to the script. Example based on commit 8648aeb5d7b7 ("power: supply: add Qualcomm PMI8998 SMB2 Charger driver") converted to a patch: $ ./scripts/add-maintainer.py --verbosity debug $u/upstream/patches/test2/0001-power-supply-add-Qualcomm-PMI8998-SMB2-Charger-drive.patch INFO: GET: Patch: 0001-power-supply-add-Qualcomm-PMI8998-SMB2-Charger-drive.patch DEBUG: Sebastian Reichel <sre@kernel.org> (maintainer:POWER SUPPLY CLASS/SUBSYSTEM and DRIVERS) Andy Gross <agross@kernel.org> (maintainer:ARM/QUALCOMM SUPPORT) Bjorn Andersson <andersson@kernel.org> (maintainer:ARM/QUALCOMM SUPPORT) Konrad Dybcio <konrad.dybcio@linaro.org> (maintainer:ARM/QUALCOMM SUPPORT) Nathan Chancellor <nathan@kernel.org> (supporter:CLANG/LLVM BUILD SUPPORT) Nick Desaulniers <ndesaulniers@google.com> (supporter:CLANG/LLVM BUILD SUPPORT) Tom Rix <trix@redhat.com> (reviewer:CLANG/LLVM BUILD SUPPORT) linux-kernel@vger.kernel.org (open list) linux-pm@vger.kernel.org (open list:POWER SUPPLY CLASS/SUBSYSTEM and DRIVERS) linux-arm-msm@vger.kernel.org (open list:ARM/QUALCOMM SUPPORT) llvm@lists.linux.dev (open list:CLANG/LLVM BUILD SUPPORT) INFO: ADD: Patch: 0001-power-supply-add-Qualcomm-PMI8998-SMB2-Charger-drive.patch DEBUG: Cc Lists: Cc: linux-arm-msm@vger.kernel.org Cc: llvm@lists.linux.dev Cc: linux-pm@vger.kernel.org Cc: linux-kernel@vger.kernel.org DEBUG: Cc Others: Cc: Tom Rix <trix@redhat.com> Cc: Nick Desaulniers <ndesaulniers@google.com> Cc: Nathan Chancellor <nathan@kernel.org> DEBUG: Cc Maintainers: None DEBUG: To Maintainers: To: Sebastian Reichel <sre@kernel.org> To: Andy Gross <agross@kernel.org> To: Bjorn Andersson <andersson@kernel.org> To: Konrad Dybcio <konrad.dybcio@linaro.org> INFO: Maintainers added to all patch files successfully The first "GET:" output prints the output of `get_maintainer.pl` verbatim, and the "ADD:" output shows what exactly is getting added to that patch. Hope this is what you were expecting. Please let me know if you'd prefer any other modifications to this debug output. [...] > > +def add_maintainers_to_file(patch_file, entities_per_file, all_entities_union): > > + logging.info("ADD: Patch: {}".format(os.path.basename(patch_file))) > > + # 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)] > > (extending Pavan comment on "From:" handling:) > > Please use something like line.startswith("From:"), otherwise this catches any > "From: " in the whole file (that's the reason why add-maintainer.py fails on > this very patch). Actually, you only want to search through the patch (mail) > header block, not through the whole commit msg and the patch body. I see the issue. I will use a simple regex to search for the first occurrence of a valid "From: <email address>" and stop there. [...] > > +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") > > nargs='+' is one or more > nargs='*' is zero, one or more Thank you - fixed. > While testing, I thought that adding addresses without filtering-out duplicates > was odd; but as git-send-email does the unique filtering, it doesn't matter. Since I'm using `set()` in this script, the uniqueness is guaranteed here as well - there won't be any duplicates. > For my own workflow, I would rather prefer a git-send-email wrapper, similiar > to the shell alias Krzysztof shared (but I like 'b4' even more). Do you have > some thoughts about a "smoother" workflow integration? The best one I could > come up with is > > ln -sr scripts/add-maintainer.py .git/hooks/sendemail-validate > git config --add --local sendemail.validate true This looks really useful! I haven't explored git hooks enough to comment on this though, sorry. Guru Das.
On Thu 24 Aug 2023 14:44:36 GMT, Guru Das Srinagesh wrote: [...] > > > The script is quiet by default (only prints errors) and its verbosity > > > can be adjusted via an optional parameter. > > > > IMO, it would be nice to see which addresses are effectively added, e.g. > > comparable to the output of git send-email. Perhaps somehing like: > > > > $ scripts/add-maintainer.py *.patch > > 0001-fixup-scripts-Add-add-maintainer.py.patch: Adding 'To: Guru Das Srinagesh <quic_gurus@quicinc.com>' (maintainer) > > 0001-fixup-scripts-Add-add-maintainer.py.patch: Adding 'Cc: linux-kernel@vger.kernel.org' (list) > > > > Perhaps verbosity should then be configurable. > > Yes, this is already implemented - you just need to pass "--verbosity debug" to > the script. Example based on commit 8648aeb5d7b7 ("power: supply: add Qualcomm > PMI8998 SMB2 Charger driver") converted to a patch: > > $ ./scripts/add-maintainer.py --verbosity debug $u/upstream/patches/test2/0001-power-supply-add-Qualcomm-PMI8998-SMB2-Charger-drive.patch > INFO: GET: Patch: 0001-power-supply-add-Qualcomm-PMI8998-SMB2-Charger-drive.patch > DEBUG: > Sebastian Reichel <sre@kernel.org> (maintainer:POWER SUPPLY CLASS/SUBSYSTEM and DRIVERS) > Andy Gross <agross@kernel.org> (maintainer:ARM/QUALCOMM SUPPORT) > Bjorn Andersson <andersson@kernel.org> (maintainer:ARM/QUALCOMM SUPPORT) > Konrad Dybcio <konrad.dybcio@linaro.org> (maintainer:ARM/QUALCOMM SUPPORT) > Nathan Chancellor <nathan@kernel.org> (supporter:CLANG/LLVM BUILD SUPPORT) > Nick Desaulniers <ndesaulniers@google.com> (supporter:CLANG/LLVM BUILD SUPPORT) > Tom Rix <trix@redhat.com> (reviewer:CLANG/LLVM BUILD SUPPORT) > linux-kernel@vger.kernel.org (open list) > linux-pm@vger.kernel.org (open list:POWER SUPPLY CLASS/SUBSYSTEM and DRIVERS) > linux-arm-msm@vger.kernel.org (open list:ARM/QUALCOMM SUPPORT) > llvm@lists.linux.dev (open list:CLANG/LLVM BUILD SUPPORT) > > INFO: ADD: Patch: 0001-power-supply-add-Qualcomm-PMI8998-SMB2-Charger-drive.patch > DEBUG: Cc Lists: > Cc: linux-arm-msm@vger.kernel.org > Cc: llvm@lists.linux.dev > Cc: linux-pm@vger.kernel.org > Cc: linux-kernel@vger.kernel.org > DEBUG: Cc Others: > Cc: Tom Rix <trix@redhat.com> > Cc: Nick Desaulniers <ndesaulniers@google.com> > Cc: Nathan Chancellor <nathan@kernel.org> > DEBUG: Cc Maintainers: > None > DEBUG: To Maintainers: > To: Sebastian Reichel <sre@kernel.org> > To: Andy Gross <agross@kernel.org> > To: Bjorn Andersson <andersson@kernel.org> > To: Konrad Dybcio <konrad.dybcio@linaro.org> > > INFO: Maintainers added to all patch files successfully > > The first "GET:" output prints the output of `get_maintainer.pl` verbatim, and > the "ADD:" output shows what exactly is getting added to that patch. Hope this > is what you were expecting. Please let me know if you'd prefer any other > modifications to this debug output. ups. I tested with --verbosity=info but not with =debug, therefore I missed it. Sorry for the noise. [...] > > While testing, I thought that adding addresses without filtering-out duplicates > > was odd; but as git-send-email does the unique filtering, it doesn't matter. > > Since I'm using `set()` in this script, the uniqueness is guaranteed here as > well - there won't be any duplicates. I thought about patch files that already have 'To/Cc' headers (e.g. 'git format-patch --to=... --cc=...' or by running add-maintainer.py multiple times for updating the lists of recipients. The result is a patch file with possible duplicated lines; but as written: it does matter, effectively. Kind regards, Nicolas
On Aug 25 2023 13:44, Nicolas Schier wrote: > On Thu 24 Aug 2023 14:44:36 GMT, Guru Das Srinagesh wrote: > > > While testing, I thought that adding addresses without filtering-out duplicates > > > was odd; but as git-send-email does the unique filtering, it doesn't matter. > > > > Since I'm using `set()` in this script, the uniqueness is guaranteed here as > > well - there won't be any duplicates. > > I thought about patch files that already have 'To/Cc' headers (e.g. > 'git format-patch --to=... --cc=...' or by running add-maintainer.py > multiple times for updating the lists of recipients. The result is a > patch file with possible duplicated lines; but as written: it does > matter, effectively. Sorry, did you mean "does" or "does *not*"? I'll make sure to test v3 of this script out on patches that have To/Cc already included and also run it multiple times on the same patch (effectively the same thing). Thank you. Guru Das.
On Fri, Aug 25, 2023 at 10:00:01AM -0700 Guru Das Srinagesh wrote: > On Aug 25 2023 13:44, Nicolas Schier wrote: > > On Thu 24 Aug 2023 14:44:36 GMT, Guru Das Srinagesh wrote: > > > > While testing, I thought that adding addresses without filtering-out duplicates > > > > was odd; but as git-send-email does the unique filtering, it doesn't matter. > > > > > > Since I'm using `set()` in this script, the uniqueness is guaranteed here as > > > well - there won't be any duplicates. > > > > I thought about patch files that already have 'To/Cc' headers (e.g. > > 'git format-patch --to=... --cc=...' or by running add-maintainer.py > > multiple times for updating the lists of recipients. The result is a > > patch file with possible duplicated lines; but as written: it does > > matter, effectively. > > Sorry, did you mean "does" or "does *not*"? I'm sorry, "it doe not matter". Nicolas > I'll make sure to test v3 of this script out on patches that have To/Cc already > included and also run it multiple times on the same patch (effectively the same > thing). > > Thank you. > > Guru Das.
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) + + next_line_after_from = from_line[0] + 1 + + for o in cc_others: + lines.insert(next_line_after_from, o + "\n") + for l in cc_lists: + lines.insert(next_line_after_from, l + "\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 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('--verbosity', choices=['debug', 'info', 'error'], default='error', help="Verbosity level of script output") + args = parser.parse_args() + + logging.basicConfig(level=args.verbosity.upper(), format='%(levelname)s: %(message)s') + + entities_per_file = dict() + + for patch in args.patches: + 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 args.patches: + 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 args.patches: + add_maintainers_to_file(patch, entities_per_file, all_entities_union) + + logging.info("Maintainers added to all patch files successfully") + +if __name__ == "__main__": + main()
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: " 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 <quic_gurus@quicinc.com> --- scripts/add-maintainer.py | 113 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 113 insertions(+) create mode 100755 scripts/add-maintainer.py