[PATCH v2 0/1] Add add-maintainer.py script

[PATCH v2 0/1] Add add-maintainer.py script

[Guru Das Srinagesh]

When pushing patches to upstream, the `get_maintainer.pl` script is used to
determine whom to send the patches to. Instead of having to manually process
the output of the script, add a wrapper script to do that for you.

The add-maintainer.py script adds maintainers (and mailing lists) to a patch,
editing it in-place.

Thanks to Bjorn for being a sounding board to this idea and for his valuable
suggestions.

Please try out this script with `--verbosity debug` for verifying that it's
doing "the right thing". I've tested this with a patch series from various
subsystems to ensure variety of maintainers and lists output and found it to be
doing what it is supposed to do.

I referred to the following links during development of this script:
- https://stackoverflow.com/questions/4427542/how-to-do-sed-like-text-replace-with-python
- https://stackoverflow.com/questions/4146009/python-get-list-indexes-using-regular-expression
- https://stackoverflow.com/questions/10507230/insert-line-at-middle-of-file-with-python

v1 -> v2:
- Added set-union logic based on Pavan's comments [1] and Bjorn's early suggestion
- Expanded audience and added more mailing lists to get more review comments and feedback

[1] https://lore.kernel.org/lkml/63764b84-3ebd-4081-836f-4863af196228@quicinc.com/

Guru Das Srinagesh (1):
  scripts: Add add-maintainer.py

 scripts/add-maintainer.py | 113 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 113 insertions(+)
 create mode 100755 scripts/add-maintainer.py

-- 
2.40.0

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

[Guru Das Srinagesh]

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

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()
-- 
2.40.0

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

[Pavan Kondeti]

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 <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?

> 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

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

[Guru Das Srinagesh]

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.

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

[Nicolas Schier]

[-- Attachment #1: Type: text/plain, Size: 8822 bytes --]

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?

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

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

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.

> 
> 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
> 
> 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)]

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

> +    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")

nargs='+' is one or more
nargs='*' is zero, one or more

> +    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()
> -- 
> 2.40.0

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.

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
.

Kind regards,
Nicolas

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Guru Das Srinagesh]

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.

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

[Nicolas Schier]

[-- Attachment #1: Type: text/plain, Size: 3794 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Guru Das Srinagesh]

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.

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

[Nicolas Schier]

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.

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

[Neil Armstrong]

Hi,

On 03/08/2023 10:23, Guru Das Srinagesh wrote:
> When pushing patches to upstream, the `get_maintainer.pl` script is used to
> determine whom to send the patches to. Instead of having to manually process
> the output of the script, add a wrapper script to do that for you.
> 
> The add-maintainer.py script adds maintainers (and mailing lists) to a patch,
> editing it in-place.

FYI the b4 prep command does this:
https://github.com/mricon/b4/blob/e8045d1353165cc065b2f1b180bf1b0846af510e/b4/ez.py#L2055

Perhaps it could be useful to make sure the output is similar ?

So far I've been very satisfied by the output of b4 auto_to_cc.

Thanks,
Neil

> 
> Thanks to Bjorn for being a sounding board to this idea and for his valuable
> suggestions.
> 
> Please try out this script with `--verbosity debug` for verifying that it's
> doing "the right thing". I've tested this with a patch series from various
> subsystems to ensure variety of maintainers and lists output and found it to be
> doing what it is supposed to do.
> 
> I referred to the following links during development of this script:
> - https://stackoverflow.com/questions/4427542/how-to-do-sed-like-text-replace-with-python
> - https://stackoverflow.com/questions/4146009/python-get-list-indexes-using-regular-expression
> - https://stackoverflow.com/questions/10507230/insert-line-at-middle-of-file-with-python
> 
> v1 -> v2:
> - Added set-union logic based on Pavan's comments [1] and Bjorn's early suggestion
> - Expanded audience and added more mailing lists to get more review comments and feedback
> 
> [1] https://lore.kernel.org/lkml/63764b84-3ebd-4081-836f-4863af196228@quicinc.com/
> 
> Guru Das Srinagesh (1):
>    scripts: Add add-maintainer.py
> 
>   scripts/add-maintainer.py | 113 ++++++++++++++++++++++++++++++++++++++
>   1 file changed, 113 insertions(+)
>   create mode 100755 scripts/add-maintainer.py
> 

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

[Guru Das Srinagesh]

On Aug 03 2023 11:16, Neil Armstrong wrote:
> Hi,
> 
> On 03/08/2023 10:23, Guru Das Srinagesh wrote:
> >When pushing patches to upstream, the `get_maintainer.pl` script is used to
> >determine whom to send the patches to. Instead of having to manually process
> >the output of the script, add a wrapper script to do that for you.
> >
> >The add-maintainer.py script adds maintainers (and mailing lists) to a patch,
> >editing it in-place.
> 
> FYI the b4 prep command does this:
> https://github.com/mricon/b4/blob/e8045d1353165cc065b2f1b180bf1b0846af510e/b4/ez.py#L2055
> 
> Perhaps it could be useful to make sure the output is similar ?
> 
> So far I've been very satisfied by the output of b4 auto_to_cc.

Thank you - let me check this tool out.

Guru Das.

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

[Guru Das Srinagesh]

On Aug 10 2023 11:49, Guru Das Srinagesh wrote:
> On Aug 03 2023 11:16, Neil Armstrong wrote:
> > Hi,
> > 
> > On 03/08/2023 10:23, Guru Das Srinagesh wrote:
> > >When pushing patches to upstream, the `get_maintainer.pl` script is used to
> > >determine whom to send the patches to. Instead of having to manually process
> > >the output of the script, add a wrapper script to do that for you.
> > >
> > >The add-maintainer.py script adds maintainers (and mailing lists) to a patch,
> > >editing it in-place.
> > 
> > FYI the b4 prep command does this:
> > https://github.com/mricon/b4/blob/e8045d1353165cc065b2f1b180bf1b0846af510e/b4/ez.py#L2055
> > 
> > Perhaps it could be useful to make sure the output is similar ?
> > 
> > So far I've been very satisfied by the output of b4 auto_to_cc.
> 
> Thank you - let me check this tool out.

I gave `b4` a spin and specifically compared the results of `b4 prep
--auto-to-cc` vs this script. I notice from the code that b4 calls
`get_maintainer.pl` with the following flags:

    --nogit --nogit-fallback --nogit-chief-penguins

I can add these to this script too.

And it collects maintainers and lists by passing, respectively, `--nol` and
`--nom` in two separate calls whereas this script parses the actual roles by
making only one call. b4's approach seems cleaner.

The perl script also can provide just the reviewers by passing `--nol --nom`.

b4 and this script both do:

    - Create set-union of all maintainers and all lists from all patches in a
      series.
    - Apply the same addresses to all patches in a series.

The main thing b4 doesn't do (which this script does) is:

    - add only that specific patch's maintainers as "To: ", and
    - the other maintainers from the other patches as "Cc: " 

Overall, b4 seems like a fantastic feature-rich tool for managing and sending
patches.

Thank you for sharing the link to the actual code - it was very instructive.

Guru Das.

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

[Guru Das Srinagesh]

On Aug 03 2023 01:23, Guru Das Srinagesh wrote:
> When pushing patches to upstream, the `get_maintainer.pl` script is used to
> determine whom to send the patches to. Instead of having to manually process
> the output of the script, add a wrapper script to do that for you.
> 
> The add-maintainer.py script adds maintainers (and mailing lists) to a patch,
> editing it in-place.

Could I request reviews from the other maintainers as well, please? Just to see
if I should continue working on this script or if the `b4` tool obviates the
need for such a script.

Thank you.

Guru Das.

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

[Krzysztof Kozlowski]

On 10/08/2023 20:55, Guru Das Srinagesh wrote:
> On Aug 03 2023 01:23, Guru Das Srinagesh wrote:
>> When pushing patches to upstream, the `get_maintainer.pl` script is used to
>> determine whom to send the patches to. Instead of having to manually process
>> the output of the script, add a wrapper script to do that for you.
>>
>> The add-maintainer.py script adds maintainers (and mailing lists) to a patch,
>> editing it in-place.
> 
> Could I request reviews from the other maintainers as well, please? Just to see
> if I should continue working on this script or if the `b4` tool obviates the
> need for such a script.

I send a bit of patches but I use very simple workflow. It is really
simple, so simple, that I was always surprised how people can make their
life difficult with some complicated process to send patches... and then
obviously skip some maintainers, because of that process.

I almost always feed git send-email with addresses from
scripts/get_maintainers.pl. This tool would not bring any benefits to my
simple workflow.

For newcomers, OTOH, I would either recommend simple workflow or just
use b4. Why? Because if you cannot use git-send-email, then it means
your email setup will make your life difficult and adding maintainers to
existing patch won't help you.

This tool depends on the command line and shell interface of
scripts/get_maintainers.pl which is another reason why it might not be a
good idea.

Best regards,
Krzysztof

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

[Guru Das Srinagesh]

Thanks for the comments, Krzysztof.

On Aug 15 2023 23:06, Krzysztof Kozlowski wrote:
> On 10/08/2023 20:55, Guru Das Srinagesh wrote:
> > On Aug 03 2023 01:23, Guru Das Srinagesh wrote:
> >> When pushing patches to upstream, the `get_maintainer.pl` script is used to
> >> determine whom to send the patches to. Instead of having to manually process
> >> the output of the script, add a wrapper script to do that for you.
> >>
> >> The add-maintainer.py script adds maintainers (and mailing lists) to a patch,
> >> editing it in-place.
> > 
> > Could I request reviews from the other maintainers as well, please? Just to see
> > if I should continue working on this script or if the `b4` tool obviates the
> > need for such a script.
> 
> I send a bit of patches but I use very simple workflow. It is really
> simple, so simple, that I was always surprised how people can make their
> life difficult with some complicated process to send patches... and then
> obviously skip some maintainers, because of that process.

Exactly - this script aims to solve precisely that problem. It fills the gap
between running `get_maintainers.pl` and having to manually edit its output to
add "To: " and "Cc: " and somehow incorporate it in the body of the patch(es).

With this script, the workflow would be as simple as:

  1. Generate patches using `git format-patch`
  2. Run `add-maintainer.py` on the above patches
  3. `git send-email` the patches.

That's it - no need to manually work with email addresses.
  
> I almost always feed git send-email with addresses from
> scripts/get_maintainers.pl. This tool would not bring any benefits to my
> simple workflow.

In the light of the 3-step workflow I've envisioned above, could you please
elaborate why not? If anything, it will only save a developer's time.

> For newcomers, OTOH, I would either recommend simple workflow or just
> use b4. Why? Because if you cannot use git-send-email, then it means
> your email setup will make your life difficult and adding maintainers to
> existing patch won't help you.

You've mentioned a "simple workflow" many times - could you please share more
details on the steps you follow in your workflow for sending patches?

> This tool depends on the command line and shell interface of
> scripts/get_maintainers.pl which is another reason why it might not be a
> good idea.

Could you please elaborate on why depending on the output of
`get_maintainer.pl` is a bad idea? It's what everyone uses, no?

Thank you.

Guru Das.

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

[Neil Armstrong]

On 16/08/2023 19:15, Guru Das Srinagesh wrote:
> Thanks for the comments, Krzysztof.
> 
> On Aug 15 2023 23:06, Krzysztof Kozlowski wrote:
>> On 10/08/2023 20:55, Guru Das Srinagesh wrote:
>>> On Aug 03 2023 01:23, Guru Das Srinagesh wrote:
>>>> When pushing patches to upstream, the `get_maintainer.pl` script is used to
>>>> determine whom to send the patches to. Instead of having to manually process
>>>> the output of the script, add a wrapper script to do that for you.
>>>>
>>>> The add-maintainer.py script adds maintainers (and mailing lists) to a patch,
>>>> editing it in-place.
>>>
>>> Could I request reviews from the other maintainers as well, please? Just to see
>>> if I should continue working on this script or if the `b4` tool obviates the
>>> need for such a script.
>>
>> I send a bit of patches but I use very simple workflow. It is really
>> simple, so simple, that I was always surprised how people can make their
>> life difficult with some complicated process to send patches... and then
>> obviously skip some maintainers, because of that process.
> 
> Exactly - this script aims to solve precisely that problem. It fills the gap
> between running `get_maintainers.pl` and having to manually edit its output to
> add "To: " and "Cc: " and somehow incorporate it in the body of the patch(es).
> 
> With this script, the workflow would be as simple as:
> 
>    1. Generate patches using `git format-patch`
>    2. Run `add-maintainer.py` on the above patches
>    3. `git send-email` the patches.
> 
> That's it - no need to manually work with email addresses.
>    
>> I almost always feed git send-email with addresses from
>> scripts/get_maintainers.pl. This tool would not bring any benefits to my
>> simple workflow.
> 
> In the light of the 3-step workflow I've envisioned above, could you please
> elaborate why not? If anything, it will only save a developer's time.
> 
>> For newcomers, OTOH, I would either recommend simple workflow or just
>> use b4. Why? Because if you cannot use git-send-email, then it means
>> your email setup will make your life difficult and adding maintainers to
>> existing patch won't help you.
> 
> You've mentioned a "simple workflow" many times - could you please share more
> details on the steps you follow in your workflow for sending patches?
> 
>> This tool depends on the command line and shell interface of
>> scripts/get_maintainers.pl which is another reason why it might not be a
>> good idea.
> 
> Could you please elaborate on why depending on the output of
> `get_maintainer.pl` is a bad idea? It's what everyone uses, no?

My opinion is that it would be a better idea to add a new output mode
to scripts/get_maintainer.pl than adding another script on top of it.

Or document somewhere how to use get_maintainer.pl with git-format-patch
without any additional scripts.

Neil

> 
> Thank you.
> 
> Guru Das.

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

[Guru Das Srinagesh]

On Aug 18 2023 10:33, Neil Armstrong wrote:
> My opinion is that it would be a better idea to add a new output mode
> to scripts/get_maintainer.pl than adding another script on top of it.

Sorry, I don't follow. The problem that this script is solving is getting the
output of `get_maintainer.pl` neatly into a patch according to this scheme:

  1. Generate patches using `git format-patch`
  2. Run `add-maintainer.py` on the above patches
  3. `git send-email` the patches.

Not sure how adding a new output mode to `get_maintainer.pl` fits in this
problem space.

Unless you mean to add a switch to it so that it automatically
edits the patch in-place (like `add-maintainer.py` does) and adds all the
addresses directly to the patch - in which case, that would be feature creep.

> Or document somewhere how to use get_maintainer.pl with git-format-patch
> without any additional scripts.

IMHO, `Documentation/process/submitting-patches.rst` should be updated with at
least one or two options addressing how to get from the aforementioned
step #1 -> step #3 addressing the problem that is being solved by step #2. In
this vacuum, every developer and maintainer appears to have their own solution
that works for them.

Thank you.

Guru Das.

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

[Krzysztof Kozlowski]

On 16/08/2023 19:15, Guru Das Srinagesh wrote:
> Thanks for the comments, Krzysztof.
> 
> On Aug 15 2023 23:06, Krzysztof Kozlowski wrote:
>> On 10/08/2023 20:55, Guru Das Srinagesh wrote:
>>> On Aug 03 2023 01:23, Guru Das Srinagesh wrote:
>>>> When pushing patches to upstream, the `get_maintainer.pl` script is used to
>>>> determine whom to send the patches to. Instead of having to manually process
>>>> the output of the script, add a wrapper script to do that for you.
>>>>
>>>> The add-maintainer.py script adds maintainers (and mailing lists) to a patch,
>>>> editing it in-place.
>>>
>>> Could I request reviews from the other maintainers as well, please? Just to see
>>> if I should continue working on this script or if the `b4` tool obviates the
>>> need for such a script.
>>
>> I send a bit of patches but I use very simple workflow. It is really
>> simple, so simple, that I was always surprised how people can make their
>> life difficult with some complicated process to send patches... and then
>> obviously skip some maintainers, because of that process.
> 
> Exactly - this script aims to solve precisely that problem. It fills the gap
> between running `get_maintainers.pl` and having to manually edit its output to
> add "To: " and "Cc: " and somehow incorporate it in the body of the patch(es).

Why would anyone need to manually update it? Just some simple bash
function or git send-email identity.

> 
> With this script, the workflow would be as simple as:
> 
>   1. Generate patches using `git format-patch`
>   2. Run `add-maintainer.py` on the above patches
>   3. `git send-email` the patches.

So one more unnecessary step (2). I don't think it is easier than my
workflow.

I just do only 1 and 3 and that's it. The simplest way ever.

> 
> That's it - no need to manually work with email addresses.

No one suggested it...

>   
>> I almost always feed git send-email with addresses from
>> scripts/get_maintainers.pl. This tool would not bring any benefits to my
>> simple workflow.
> 
> In the light of the 3-step workflow I've envisioned above, could you please
> elaborate why not? If anything, it will only save a developer's time.

Because of unnecessary step 2? One more tool to remember to run?

> 
>> For newcomers, OTOH, I would either recommend simple workflow or just
>> use b4. Why? Because if you cannot use git-send-email, then it means
>> your email setup will make your life difficult and adding maintainers to
>> existing patch won't help you.
> 
> You've mentioned a "simple workflow" many times - could you please share more
> details on the steps you follow in your workflow for sending patches?

I shared it on LKML few times already (and Rob's git send-email identity
is also on LKML), so one more time:

https://github.com/krzk/tools/blob/master/linux/.bash_aliases_linux#L91


> 
>> This tool depends on the command line and shell interface of
>> scripts/get_maintainers.pl which is another reason why it might not be a
>> good idea.
> 
> Could you please elaborate on why depending on the output of
> `get_maintainer.pl` is a bad idea? It's what everyone uses, no?

No, because if interface changes you need to update two tools.

Best regards,
Krzysztof

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

[Bjorn Andersson]

On Fri, Aug 18, 2023 at 10:43:31AM +0200, Krzysztof Kozlowski wrote:
> On 16/08/2023 19:15, Guru Das Srinagesh wrote:
> > Thanks for the comments, Krzysztof.
> > 
> > On Aug 15 2023 23:06, Krzysztof Kozlowski wrote:
> >> On 10/08/2023 20:55, Guru Das Srinagesh wrote:
> >>> On Aug 03 2023 01:23, Guru Das Srinagesh wrote:
> >>>> When pushing patches to upstream, the `get_maintainer.pl` script is used to
> >>>> determine whom to send the patches to. Instead of having to manually process
> >>>> the output of the script, add a wrapper script to do that for you.
> >>>>
> >>>> The add-maintainer.py script adds maintainers (and mailing lists) to a patch,
> >>>> editing it in-place.
> >>>
> >>> Could I request reviews from the other maintainers as well, please? Just to see
> >>> if I should continue working on this script or if the `b4` tool obviates the
> >>> need for such a script.
> >>
> >> I send a bit of patches but I use very simple workflow. It is really
> >> simple, so simple, that I was always surprised how people can make their
> >> life difficult with some complicated process to send patches... and then
> >> obviously skip some maintainers, because of that process.
> > 
> > Exactly - this script aims to solve precisely that problem. It fills the gap
> > between running `get_maintainers.pl` and having to manually edit its output to
> > add "To: " and "Cc: " and somehow incorporate it in the body of the patch(es).
> 
> Why would anyone need to manually update it? Just some simple bash
> function or git send-email identity.
> 

I do this all the time, either to add additional, or remove unnecessary,
recipients from what's provided by get_maintainers.pl.

> > 
> > With this script, the workflow would be as simple as:
> > 
> >   1. Generate patches using `git format-patch`
> >   2. Run `add-maintainer.py` on the above patches
> >   3. `git send-email` the patches.
> 
> So one more unnecessary step (2). I don't think it is easier than my
> workflow.
> 
> I just do only 1 and 3 and that's it. The simplest way ever.
> 

There's no get_maintainer.pl in either 1, or 3, so obviously this isn't
the only thing you do.

Thanks for the link to your alias below, it's now clear that you don't
need an extra step in the procedure, if you only have your extra wrapper
around step 3.


I now also understand why you never ever have a cover-letter, something
Guru's proposed flow handles quite nicely.


That said, b4 prep and b4 send seems like a better suggestion to those
who doesn't already have a workflow in place.

[..]
> > 
> >> This tool depends on the command line and shell interface of
> >> scripts/get_maintainers.pl which is another reason why it might not be a
> >> good idea.
> > 
> > Could you please elaborate on why depending on the output of
> > `get_maintainer.pl` is a bad idea? It's what everyone uses, no?
> 
> No, because if interface changes you need to update two tools.
> 

This is a valid objection, but I've heard that "the simplest way ever"
also depends on exactly this output...

Regards,
Bjorn

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

[Krzysztof Kozlowski]

On 18/08/2023 21:46, Bjorn Andersson wrote:
>>>
>>> With this script, the workflow would be as simple as:
>>>
>>>   1. Generate patches using `git format-patch`
>>>   2. Run `add-maintainer.py` on the above patches
>>>   3. `git send-email` the patches.
>>
>> So one more unnecessary step (2). I don't think it is easier than my
>> workflow.
>>
>> I just do only 1 and 3 and that's it. The simplest way ever.
>>
> 
> There's no get_maintainer.pl in either 1, or 3, so obviously this isn't
> the only thing you do.
> 
> Thanks for the link to your alias below, it's now clear that you don't
> need an extra step in the procedure, if you only have your extra wrapper
> around step 3.
> 
> 
> I now also understand why you never ever have a cover-letter, something
> Guru's proposed flow handles quite nicely.

It's not related. I usually don't create cover letter from laziness, but
pretty often I create them as well and my script/alias works there
perfectly. Cover letter is just one more step:
1. git branch --edit-description
2. git format-patch --cover-letter (with format.coverFromDescription =
subject in gitconfig)
3. git_send_email 0*

No need to run any other tool, no need to add any maintainer entries
(unless touching defconfig and specific soc@ stuff, but this is always
the case regardless of tools).

Really, that script proposed here is the unnecessary step.

Rob's approach with git send-email identity required some work for
cover-letter, but it was also running get_maintainer.pl per each patch,
so recipients did not receive everything. Unless patchset is big, I
prefer to send everything to everyone.

> 
> 
> That said, b4 prep and b4 send seems like a better suggestion to those
> who doesn't already have a workflow in place.

Yes.


Best regards,
Krzysztof

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

[Guru Das Srinagesh]

On Aug 18 2023 10:43, Krzysztof Kozlowski wrote:
> >> For newcomers, OTOH, I would either recommend simple workflow or just
> >> use b4. Why? Because if you cannot use git-send-email, then it means
> >> your email setup will make your life difficult and adding maintainers to
> >> existing patch won't help you.
> > 
> > You've mentioned a "simple workflow" many times - could you please share more
> > details on the steps you follow in your workflow for sending patches?
> 
> I shared it on LKML few times already (and Rob's git send-email identity
> is also on LKML), so one more time:
> 
> https://github.com/krzk/tools/blob/master/linux/.bash_aliases_linux#L91

Thank you for sharing this - it is really neat indeed and you certainly don't
need a step #2 with this method.

However, I see areas for improvement in the alias:
- Subsystem-specific mailing lists, maintainers, reviewers, and other roles are
  all marked as "To: " sans distinction via the alias whereas
  `add-maintainer.py` and `b4` both provide marking of lists as "Cc: " which
  seems aesthetically and semantically more pleasing.
- Only `add-maintainer.py` allows for maintainers to be selectively in "To: "
  and "Cc: " for patches in a series depending on whether they are the
  maintainers for that particular patch or not.

> >> This tool depends on the command line and shell interface of
> >> scripts/get_maintainers.pl which is another reason why it might not be a
> >> good idea.
> > 
> > Could you please elaborate on why depending on the output of
> > `get_maintainer.pl` is a bad idea? It's what everyone uses, no?
> 
> No, because if interface changes you need to update two tools.

But `b4 prep --auto-to-cc` also uses `get_maintainer.pl`!

Also, in your previous email you said to "just use b4", which also depends on
the command line and shell interface of `get_maintainers.pl`. Depending on
`get_maintainer.pl` is therefore perfectly okay - there is no need to reinvent
it or disregard it for whatever reasons.

Thank you.

Guru Das.

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

[Krzysztof Kozlowski]

On 19/08/2023 03:33, Guru Das Srinagesh wrote:
> On Aug 18 2023 10:43, Krzysztof Kozlowski wrote:
>>>> For newcomers, OTOH, I would either recommend simple workflow or just
>>>> use b4. Why? Because if you cannot use git-send-email, then it means
>>>> your email setup will make your life difficult and adding maintainers to
>>>> existing patch won't help you.
>>>
>>> You've mentioned a "simple workflow" many times - could you please share more
>>> details on the steps you follow in your workflow for sending patches?
>>
>> I shared it on LKML few times already (and Rob's git send-email identity
>> is also on LKML), so one more time:
>>
>> https://github.com/krzk/tools/blob/master/linux/.bash_aliases_linux#L91
> 
> Thank you for sharing this - it is really neat indeed and you certainly don't
> need a step #2 with this method.
> 
> However, I see areas for improvement in the alias:
> - Subsystem-specific mailing lists, maintainers, reviewers, and other roles are
>   all marked as "To: " sans distinction via the alias whereas
>   `add-maintainer.py` and `b4` both provide marking of lists as "Cc: " which
>   seems aesthetically and semantically more pleasing.

To or Cc does not matter.

> - Only `add-maintainer.py` allows for maintainers to be selectively in "To: "
>   and "Cc: " for patches in a series depending on whether they are the
>   maintainers for that particular patch or not.

It's intentional to CC everyone. If I wanted to Cc/To
maintainer-per-patch, I would use Rob's send-email identity.

> 
>>>> This tool depends on the command line and shell interface of
>>>> scripts/get_maintainers.pl which is another reason why it might not be a
>>>> good idea.
>>>
>>> Could you please elaborate on why depending on the output of
>>> `get_maintainer.pl` is a bad idea? It's what everyone uses, no?
>>
>> No, because if interface changes you need to update two tools.
> 
> But `b4 prep --auto-to-cc` also uses `get_maintainer.pl`!

Yep, and it's Konstantin's headache to keep it updated. :)

> 
> Also, in your previous email you said to "just use b4", which also depends on
> the command line and shell interface of `get_maintainers.pl`. Depending on
> `get_maintainer.pl` is therefore perfectly okay - there is no need to reinvent
> it or disregard it for whatever reasons.

True, it is okay, but adding more tools to depend on it is more work. b4
is awesome tool, thus I feel it is justified to depend on that
interface. I don't see the need for more tools doing exactly the same.

Best regards,
Krzysztof