[U-Boot] patman: add support for omitting bouncing addresses

Add a --bounce-file option which can be used to omit addresses that are
known to bounce. The option specifies a file which lists addresses (one
per line) that are stripped from the Cc list.

Signed-off-by: Chris Packham <judge.packham@gmail.com>
---
Here's a quick proof of concept. Right now the .bounces file has to
match exactly the addresses that are extracted by patman. This makes it
easy to do set(a) - set(b) to remove all the bounces in one hit.

 tools/patman/patman.py | 6 +++++-
 tools/patman/series.py | 5 ++++-
 tools/patman/tools.py  | 8 ++++++++
 3 files changed, 17 insertions(+), 2 deletions(-)

On Mon, 28 Aug 2017, Chris Packham wrote:

> Add a --bounce-file option which can be used to omit addresses that are
> known to bounce. The option specifies a file which lists addresses (one
> per line) that are stripped from the Cc list.

Reviewed-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>

Absolutely love this (in fact, I had something like this on my to-do list 
as the number of bounces seemed to grow over time).

A few comments below.

>
> Signed-off-by: Chris Packham <judge.packham@gmail.com>
> ---
> Here's a quick proof of concept. Right now the .bounces file has to
> match exactly the addresses that are extracted by patman. This makes it
> easy to do set(a) - set(b) to remove all the bounces in one hit.
>
> tools/patman/patman.py | 6 +++++-
> tools/patman/series.py | 5 ++++-
> tools/patman/tools.py  | 8 ++++++++
> 3 files changed, 17 insertions(+), 2 deletions(-)
>
> diff --git a/tools/patman/patman.py b/tools/patman/patman.py
> index 4b3bc787453e..567226a4d3ce 100755
> --- a/tools/patman/patman.py
> +++ b/tools/patman/patman.py
> @@ -56,6 +56,9 @@ parser.add_option('-v', '--verbose', action='store_true', dest='verbose',
>        default=False, help='Verbose output of errors and warnings')
> parser.add_option('--cc-cmd', dest='cc_cmd', type='string', action='store',
>        default=None, help='Output cc list for patch file (used by git)')
> +parser.add_option('--bounce-file', dest='bounce_fname', type='string',
> +                  default='.bounces',

The '.bounces' default doesn't really fit with how such settings are 
handled throughout patman.

My gut feeling is that
  (a) we want to have a project-wide bounces config (doc/git-bounces ???)
      that complements doc/git-mailrc
  (b) that any overrides/user-local additional settings should be picked up
      via ~/.patman (either by referencing a local overrides file or by
      having some additional overrides directly there)
  (c) this entire mechanism needs to be tied into Setup(...) in settings.py

> +                  help='File containing addresses to skip [default: %default]')
> parser.add_option('--no-check', action='store_false', dest='check_patch',
>                   default=True,
>                   help="Don't check for patch compliance")
> @@ -158,7 +161,8 @@ else:
>
>     cc_file = series.MakeCcFile(options.process_tags, cover_fname,
>                                 not options.ignore_bad_tags,
> -                                options.add_maintainers)
> +                                options.add_maintainers,
> +                                options.bounce_fname)
>
>     # Email the patches out (giving the user time to check / cancel)
>     cmd = ''
> diff --git a/tools/patman/series.py b/tools/patman/series.py
> index d3947a7c2ac5..b6533ab5ee58 100644
> --- a/tools/patman/series.py
> +++ b/tools/patman/series.py
> @@ -11,6 +11,7 @@ import os
> import get_maintainer
> import gitutil
> import terminal
> +from tools import FindBounces
>
> # Series-xxx tags that we understand
> valid_series = ['to', 'cc', 'version', 'changes', 'prefix', 'notes', 'name',
> @@ -202,7 +203,7 @@ class Series(dict):
>             print(col.Color(col.RED, str))
>
>     def MakeCcFile(self, process_tags, cover_fname, raise_on_error,
> -                   add_maintainers):
> +                   add_maintainers, bounce_fname):
>         """Make a cc file for us to use for per-commit Cc automation
>
>         Also stores in self._generated_cc to make ShowActions() faster.
> @@ -222,6 +223,7 @@ class Series(dict):
>         fname = '/tmp/patman.%d' % os.getpid()
>         fd = open(fname, 'w')
>         all_ccs = []
> +        bounces = FindBounces(bounce_fname)
>         for commit in self.commits:
>             cc = []
>             if process_tags:
> @@ -234,6 +236,7 @@ class Series(dict):
>             elif add_maintainers:
>                 cc += get_maintainer.GetMaintainer(commit.patch)
>             cc = [m.encode('utf-8') if type(m) != str else m for m in cc]
> +            cc = set(cc) - set(bounces)
>             all_ccs += cc
>             print(commit.patch, ', '.join(set(cc)), file=fd)
>             self._generated_cc[commit.patch] = cc
> diff --git a/tools/patman/tools.py b/tools/patman/tools.py
> index ba2485303037..9d206a390b02 100644
> --- a/tools/patman/tools.py
> +++ b/tools/patman/tools.py
> @@ -118,3 +118,11 @@ def Align(pos, align):
>
> def NotPowerOfTwo(num):
>     return num and (num & (num - 1))
> +
> +def FindBounces(bounce_fname):
> +    """generate a list of bouncing addresses from a file"""
> +    try:
> +        with open(bounce_fname) as fd:
> +            return [line.strip() for line in fd]
> +    except IOError:
> +        return []
>

Shouldn't this be merged into gitutil.BuildEmailList or into 
gitutil.LookupEmail? It would also be nice if the bounces would be 
handled in analogy to aliases and referenced via settings.alias

Regards,
Philipp.

On Tue, Aug 29, 2017 at 6:37 AM, Philipp Tomsich
<philipp.tomsich@theobroma-systems.com> wrote:
>
>
> On Mon, 28 Aug 2017, Chris Packham wrote:
>
>> Add a --bounce-file option which can be used to omit addresses that are
>> known to bounce. The option specifies a file which lists addresses (one
>> per line) that are stripped from the Cc list.
>
>
> Reviewed-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
>
> Absolutely love this (in fact, I had something like this on my to-do list as
> the number of bounces seemed to grow over time).
>
> A few comments below.
>
>>
>> Signed-off-by: Chris Packham <judge.packham@gmail.com>
>> ---
>> Here's a quick proof of concept. Right now the .bounces file has to
>> match exactly the addresses that are extracted by patman. This makes it
>> easy to do set(a) - set(b) to remove all the bounces in one hit.
>>
>> tools/patman/patman.py | 6 +++++-
>> tools/patman/series.py | 5 ++++-
>> tools/patman/tools.py  | 8 ++++++++
>> 3 files changed, 17 insertions(+), 2 deletions(-)
>>
>> diff --git a/tools/patman/patman.py b/tools/patman/patman.py
>> index 4b3bc787453e..567226a4d3ce 100755
>> --- a/tools/patman/patman.py
>> +++ b/tools/patman/patman.py
>> @@ -56,6 +56,9 @@ parser.add_option('-v', '--verbose',
>> action='store_true', dest='verbose',
>>        default=False, help='Verbose output of errors and warnings')
>> parser.add_option('--cc-cmd', dest='cc_cmd', type='string',
>> action='store',
>>        default=None, help='Output cc list for patch file (used by git)')
>> +parser.add_option('--bounce-file', dest='bounce_fname', type='string',
>> +                  default='.bounces',
>
>
> The '.bounces' default doesn't really fit with how such settings are handled
> throughout patman.
>
> My gut feeling is that
>  (a) we want to have a project-wide bounces config (doc/git-bounces ???)
>      that complements doc/git-mailrc
>  (b) that any overrides/user-local additional settings should be picked up
>      via ~/.patman (either by referencing a local overrides file or by
>      having some additional overrides directly there)
>  (c) this entire mechanism needs to be tied into Setup(...) in settings.py
>

Makes sense. The .bounces file was just convenient. But yeah you are
likely to have project-wide settings that are stored in-tree and user
additions that aren't shared.

>
>> +                  help='File containing addresses to skip [default:
>> %default]')
>> parser.add_option('--no-check', action='store_false', dest='check_patch',
>>                   default=True,
>>                   help="Don't check for patch compliance")
>> @@ -158,7 +161,8 @@ else:
>>
>>     cc_file = series.MakeCcFile(options.process_tags, cover_fname,
>>                                 not options.ignore_bad_tags,
>> -                                options.add_maintainers)
>> +                                options.add_maintainers,
>> +                                options.bounce_fname)
>>
>>     # Email the patches out (giving the user time to check / cancel)
>>     cmd = ''
>> diff --git a/tools/patman/series.py b/tools/patman/series.py
>> index d3947a7c2ac5..b6533ab5ee58 100644
>> --- a/tools/patman/series.py
>> +++ b/tools/patman/series.py
>> @@ -11,6 +11,7 @@ import os
>> import get_maintainer
>> import gitutil
>> import terminal
>> +from tools import FindBounces
>>
>> # Series-xxx tags that we understand
>> valid_series = ['to', 'cc', 'version', 'changes', 'prefix', 'notes',
>> 'name',
>> @@ -202,7 +203,7 @@ class Series(dict):
>>             print(col.Color(col.RED, str))
>>
>>     def MakeCcFile(self, process_tags, cover_fname, raise_on_error,
>> -                   add_maintainers):
>> +                   add_maintainers, bounce_fname):
>>         """Make a cc file for us to use for per-commit Cc automation
>>
>>         Also stores in self._generated_cc to make ShowActions() faster.
>> @@ -222,6 +223,7 @@ class Series(dict):
>>         fname = '/tmp/patman.%d' % os.getpid()
>>         fd = open(fname, 'w')
>>         all_ccs = []
>> +        bounces = FindBounces(bounce_fname)
>>         for commit in self.commits:
>>             cc = []
>>             if process_tags:
>> @@ -234,6 +236,7 @@ class Series(dict):
>>             elif add_maintainers:
>>                 cc += get_maintainer.GetMaintainer(commit.patch)
>>             cc = [m.encode('utf-8') if type(m) != str else m for m in cc]
>> +            cc = set(cc) - set(bounces)
>>             all_ccs += cc
>>             print(commit.patch, ', '.join(set(cc)), file=fd)
>>             self._generated_cc[commit.patch] = cc
>> diff --git a/tools/patman/tools.py b/tools/patman/tools.py
>> index ba2485303037..9d206a390b02 100644
>> --- a/tools/patman/tools.py
>> +++ b/tools/patman/tools.py
>> @@ -118,3 +118,11 @@ def Align(pos, align):
>>
>> def NotPowerOfTwo(num):
>>     return num and (num & (num - 1))
>> +
>> +def FindBounces(bounce_fname):
>> +    """generate a list of bouncing addresses from a file"""
>> +    try:
>> +        with open(bounce_fname) as fd:
>> +            return [line.strip() for line in fd]
>> +    except IOError:
>> +        return []
>>
>
> Shouldn't this be merged into gitutil.BuildEmailList or into
> gitutil.LookupEmail? It would also be nice if the bounces would be handled
> in analogy to aliases and referenced via settings.alias

I struggled to find a home for this. 'tools.py' was just the first
place I found for a reasonably generic helper function. If this code
follows the way aliases work it makes sense for it to live there.

>
> Regards,
> Philipp.