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

[Chris Packham]

On Tue, Aug 29, 2017 at 6:37 AM, Philipp Tomsich
<philipp.tomsich at 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 at 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 at 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.