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

[Doug Anderson]

Hi,

On Sun, Feb 19, 2023 at 3:50 PM Simon Glass <sjg at 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 at chromium.org>