Re: [PATCH v2 1/1] scripts: Add add-maintainer.py

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:

quoted

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.

quoted

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 [off-list ref]' (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 [off-list ref] (maintainer:POWER SUPPLY CLASS/SUBSYSTEM and DRIVERS)
    Andy Gross [off-list ref] (maintainer:ARM/QUALCOMM SUPPORT)
    Bjorn Andersson [off-list ref] (maintainer:ARM/QUALCOMM SUPPORT)
    Konrad Dybcio [off-list ref] (maintainer:ARM/QUALCOMM SUPPORT)
    Nathan Chancellor [off-list ref] (supporter:CLANG/LLVM BUILD SUPPORT)
    Nick Desaulniers [off-list ref] (supporter:CLANG/LLVM BUILD SUPPORT)
    Tom Rix [off-list ref] (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 [off-list ref]
    Cc: Nick Desaulniers [off-list ref]
    Cc: Nathan Chancellor [off-list ref]
    DEBUG: Cc Maintainers:
    None
    DEBUG: To Maintainers:
    To: Sebastian Reichel [off-list ref]
    To: Andy Gross [off-list ref]
    To: Bjorn Andersson [off-list ref]
    To: Konrad Dybcio [off-list ref]
    
    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.

[...]

quoted

+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.

[...]

quoted

+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.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel