Re: [PATCH v2 2/4] patman: Refactor MakeCcFile() into two functions

Hi,

On Sun, Feb 19, 2023 at 3:50 PM Simon Glass sjg@chromium.org wrote:

@@ -234,6 +234,48 @@ class Series(dict): str = 'Change log exists, but no version is set' print(col.build(col.RED, str))

• def GetCcForCommit(self, commit, process_tags, warn_on_error,

•                   add_maintainers, limit, get_maintainer_script,
  

•                   all_skips):
  

•    """Get the email CCs to use with a particular commit
  

•    Uses subject tags and get_maintainers.pl script to find people to cc
  

•    on a patch
  

•    Args:
  

•        commit (Commit): Commit to process
  

•        process_tags (bool): Process tags as if they were aliases
  

•        warn_on_error (bool): True to print a warning when an alias fails to
  

•            match, False to ignore it.
  

•        add_maintainers (bool or list of str): Either:
  

•            True/False to call the get_maintainers to CC maintainers
  

•            List of maintainers to include (for testing)
  

•        limit (int): Limit the length of the Cc list (None if no limit)
  

•        get_maintainer_script (str): The file name of the get_maintainer.pl
  

•            script (or compatible).
  

•        all_skips (set of str): Set of bouncing email address that were
  

•            dropped from the output
  

It wouldn't hurt to mention that "all_skips" is essentially a return value from this function (this function updates it to include the email addresses that were skipped).

@@ -259,28 +301,18 @@ class Series(dict): fname = '/tmp/patman.%d' % os.getpid() fd = open(fname, 'w', encoding='utf-8') all_ccs = []

•    all_skips = set()
     for commit in self.commits:
  

•        cc = []
  

•        if process_tags:
  

•            cc += gitutil.build_email_list(commit.tags,
  

•                                           warn_on_error=warn_on_error)
  

•        cc += gitutil.build_email_list(commit.cc_list,
  

•                                       warn_on_error=warn_on_error)
  

•        if type(add_maintainers) == type(cc):
  

•            cc += add_maintainers
  

•        elif add_maintainers:
  

•            cc += get_maintainer.get_maintainer(get_maintainer_script,
  

•                                                commit.patch)
  

•        for x in set(cc) & set(settings.bounces):
  

•            print(col.build(col.YELLOW, 'Skipping "%s"' % x))
  

•        cc = list(set(cc) - set(settings.bounces))
  

•        if limit is not None:
  

•            cc = cc[:limit]
  

•        cc = self.GetCcForCommit(commit, process_tags, warn_on_error,
  

•                                 add_maintainers, limit,
  

•                                 get_maintainer_script, all_skips)
         all_ccs += cc
         print(commit.patch, '\0'.join(sorted(set(cc))), file=fd)
         self._generated_cc[commit.patch] = cc
  
  

•    for x in sorted(list(all_skips)):
  

Why "sorted(list(all_skips))" and not just "sorted(all_skips)"?

Both of the above are nits, so I'm OK w/:

Reviewed-by: Douglas Anderson dianders@chromium.org