[U-Boot] Patman avoiding bounces

[U-Boot] Patman avoiding bounces

[Chris Packham]

Hi,

I recently sent a few patches using patman and ran into two problems.

The first is a set of undeliverable addresses. This isn't a big problem,
more of an annoyance but it also means that anyone replying to my patches
also gets the same set of bounces. Would adding a blacklist feature be a
good idea? Should such a list be stored in-tree? It might also be worth
integrating with git's mailmap to handle those who are still active under a
new address, that's probably something that would be useful in-tree for
other uses.

The other issue is the number of addresses hitting the mailing list limit
requiring moderator approval. I'm not sure what the exact limit is but
presumably it's something in the mailman config. It would be great if
patman had some (configurable) knowledge of this limit an only added Ccs up
to this. I'm less certain of a heuristic for which addresses to drop,
obviously the known inactive ones mentioned above are easy to drop. After
that I'm not sure maybe base it on number of commits to the affected files
(i think get_maintainer.pl already gathers this info).

Thoughts? If i get some time I'll try and make a proof of concept for the
blacklist.

Thanks,
Chris

[U-Boot] Patman avoiding bounces

[Bin Meng]

+Simon, Tom, Wolfgang,

On Sun, Aug 27, 2017 at 7:59 AM, Chris Packham <judge.packham@gmail.com> wrote:
> Hi,
>
> I recently sent a few patches using patman and ran into two problems.
>
> The first is a set of undeliverable addresses. This isn't a big problem,
> more of an annoyance but it also means that anyone replying to my patches
> also gets the same set of bounces. Would adding a blacklist feature be a
> good idea? Should such a list be stored in-tree? It might also be worth
> integrating with git's mailmap to handle those who are still active under a
> new address, that's probably something that would be useful in-tree for
> other uses.
>

I've encountered this for several times. Yes, it's quite annoying.

> The other issue is the number of addresses hitting the mailing list limit
> requiring moderator approval. I'm not sure what the exact limit is but
> presumably it's something in the mailman config. It would be great if
> patman had some (configurable) knowledge of this limit an only added Ccs up
> to this. I'm less certain of a heuristic for which addresses to drop,
> obviously the known inactive ones mentioned above are easy to drop. After
> that I'm not sure maybe base it on number of commits to the affected files
> (i think get_maintainer.pl already gathers this info).
>

I think the patch cc idea is to make sure everyone that is probably
affected by this patch will get direct email (not from ML) sent to
him/herself. If we cut down the cc list to match the ML limit, that's
probably not a good idea. Maybe we can increase the ML limit? Tom or
Wolfgang could comment on this.

> Thoughts? If i get some time I'll try and make a proof of concept for the
> blacklist.

Regards,
Bin

[U-Boot] Patman avoiding bounces

[Simon Glass]

Hi Chris,

On 26 August 2017 at 18:17, Bin Meng <bmeng.cn@gmail.com> wrote:
> +Simon, Tom, Wolfgang,
>
> On Sun, Aug 27, 2017 at 7:59 AM, Chris Packham <judge.packham@gmail.com> wrote:
>> Hi,
>>
>> I recently sent a few patches using patman and ran into two problems.
>>
>> The first is a set of undeliverable addresses. This isn't a big problem,
>> more of an annoyance but it also means that anyone replying to my patches
>> also gets the same set of bounces. Would adding a blacklist feature be a
>> good idea? Should such a list be stored in-tree? It might also be worth
>> integrating with git's mailmap to handle those who are still active under a
>> new address, that's probably something that would be useful in-tree for
>> other uses.
>>
>
> I've encountered this for several times. Yes, it's quite annoying.

Yes that sounds like a nice feature. I think it should be stored in-tree.

Also some people have email addresses with commas, e.g. "Bloggs, F.
<f.bloggs@napier.co.nz>' and this current confuses patman.

>
>> The other issue is the number of addresses hitting the mailing list limit
>> requiring moderator approval. I'm not sure what the exact limit is but
>> presumably it's something in the mailman config. It would be great if
>> patman had some (configurable) knowledge of this limit an only added Ccs up
>> to this. I'm less certain of a heuristic for which addresses to drop,
>> obviously the known inactive ones mentioned above are easy to drop. After
>> that I'm not sure maybe base it on number of commits to the affected files
>> (i think get_maintainer.pl already gathers this info).
>>
>
> I think the patch cc idea is to make sure everyone that is probably
> affected by this patch will get direct email (not from ML) sent to
> him/herself. If we cut down the cc list to match the ML limit, that's
> probably not a good idea. Maybe we can increase the ML limit? Tom or
> Wolfgang could comment on this.

Well the messages get manually reviewed so perhaps this doesn't matter
too much. It might be nice for patman to warn to expect an email from
the list but that no action should be needed.

>
>> Thoughts? If i get some time I'll try and make a proof of concept for the
>> blacklist.
>
> Regards,
> Bin

Regards,
Simon

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

[Chris Packham]

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

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',
+                  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 []
-- 
2.14.1

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

[Philipp Tomsich]

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.

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

[Chris Packham]

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.