diff mbox

[U-Boot,5/8] patman: Provide option to ignore bad aliases

Message ID 1363833781-14557-6-git-send-email-sjg@chromium.org
State Superseded, archived
Delegated to: Simon Glass
Headers show

Commit Message

Simon Glass March 21, 2013, 2:42 a.m. UTC
Often it happens that patches include tags which don't have aliases. It
is annoying that patman fails in this case, and provides no option to
continue other than adding empty tags to the .patman file.

Correct this by adding a '-t' option to ignore tags that don't exist.
Print a warning instead.

Since running the tests is not a common operation, move this to --test
instead, to reserve -t for this new option.

Signed-off-by: Simon Glass <sjg@chromium.org>
---
 tools/patman/gitutil.py | 62 +++++++++++++++++++++++++++++++++++--------------
 tools/patman/patman.py  | 10 +++++---
 tools/patman/series.py  |  9 ++++---
 3 files changed, 57 insertions(+), 24 deletions(-)

Comments

Doug Anderson March 21, 2013, 5:09 p.m. UTC | #1
Simon,

Nothing critical and this could go in as-is, but a few nits below.


On Wed, Mar 20, 2013 at 7:42 PM, Simon Glass <sjg@chromium.org> wrote:
> -        raw += LookupEmail(item, alias)
> +        raw += LookupEmail(item, alias, raise_on_error=not ignore_errors)

optional: Change it so functions are consistent about whether it's
"raise_on_error" or "ignore_errors"


> +    >>> EmailPatches(series, 'cover', ['p1', 'p2'], True, False, 'cc-fname', \
> +            False, alias)

Doctest that actually tests the raise_on_error?  See doctests in
gitutil.py for example syntax.


> -def LookupEmail(lookup_name, alias=None, level=0):
> +def LookupEmail(lookup_name, alias=None, raise_on_error=True, level=0):
>      """If an email address is an alias, look it up and return the full name
>
>      TODO: Why not just use git's own alias feature?
>
>      Args:
>          lookup_name: Alias or email address to look up
> +        alias: Dictionary containing aliases (None to use settings default)
> +        raise_on_error: True to raise an error when an alias fails to match

...and what happens if you pass False?


> +    >>> LookupEmail('odd', alias, False)
> +    \033[1;31mAlias 'odd' not found\033[0m
> +    []
> +    >>> # In this case the loop part will effectively be ignored.
> +    >>> LookupEmail('loop', alias, False)
> +    \033[1;31mRecursive email alias at 'other'\033[0m
> +    \033[1;31mRecursive email alias at 'john'\033[0m
> +    \033[1;31mRecursive email alias at 'mary'\033[0m
> +    ['j.bloggs@napier.co.nz', 'm.poppins@cloud.net']

optional nit: for optional args it's nice to specify them with keywords, like
  >>> LookupEmail('loop', alias=alias, raise_on_error=False)

Please test the raise_on_error=True case.


-Doug
Simon Glass March 26, 2013, 10:46 p.m. UTC | #2
Hi Doug,

On Thu, Mar 21, 2013 at 10:09 AM, Doug Anderson <dianders@chromium.org> wrote:
> Simon,
>
> Nothing critical and this could go in as-is, but a few nits below.
>
>
> On Wed, Mar 20, 2013 at 7:42 PM, Simon Glass <sjg@chromium.org> wrote:
>> -        raw += LookupEmail(item, alias)
>> +        raw += LookupEmail(item, alias, raise_on_error=not ignore_errors)
>
> optional: Change it so functions are consistent about whether it's
> "raise_on_error" or "ignore_errors"

Will do.

>
>
>> +    >>> EmailPatches(series, 'cover', ['p1', 'p2'], True, False, 'cc-fname', \
>> +            False, alias)
>
> Doctest that actually tests the raise_on_error?  See doctests in
> gitutil.py for example syntax.

It is tested by LookupEmail() below.

>
>
>> -def LookupEmail(lookup_name, alias=None, level=0):
>> +def LookupEmail(lookup_name, alias=None, raise_on_error=True, level=0):
>>      """If an email address is an alias, look it up and return the full name
>>
>>      TODO: Why not just use git's own alias feature?
>>
>>      Args:
>>          lookup_name: Alias or email address to look up
>> +        alias: Dictionary containing aliases (None to use settings default)
>> +        raise_on_error: True to raise an error when an alias fails to match
>
> ...and what happens if you pass False?

Will add comment.

>
>
>> +    >>> LookupEmail('odd', alias, False)
>> +    \033[1;31mAlias 'odd' not found\033[0m
>> +    []
>> +    >>> # In this case the loop part will effectively be ignored.
>> +    >>> LookupEmail('loop', alias, False)
>> +    \033[1;31mRecursive email alias at 'other'\033[0m
>> +    \033[1;31mRecursive email alias at 'john'\033[0m
>> +    \033[1;31mRecursive email alias at 'mary'\033[0m
>> +    ['j.bloggs@napier.co.nz', 'm.poppins@cloud.net']
>
> optional nit: for optional args it's nice to specify them with keywords, like
>   >>> LookupEmail('loop', alias=alias, raise_on_error=False)
>
> Please test the raise_on_error=True case.

Yes, that's the current ignore_errors=False I think, so I will keep this.

Regards,
Simon
diff mbox

Patch

diff --git a/tools/patman/gitutil.py b/tools/patman/gitutil.py
index c35d209..5c860c4 100644
--- a/tools/patman/gitutil.py
+++ b/tools/patman/gitutil.py
@@ -159,7 +159,7 @@  def ApplyPatches(verbose, args, start_point):
         print stdout, stderr
     return error_count == 0
 
-def BuildEmailList(in_list, tag=None, alias=None):
+def BuildEmailList(in_list, tag=None, alias=None, ignore_errors=False):
     """Build a list of email addresses based on an input list.
 
     Takes a list of email addresses and aliases, and turns this into a list
@@ -172,6 +172,8 @@  def BuildEmailList(in_list, tag=None, alias=None):
     Args:
         in_list:        List of aliases/email addresses
         tag:            Text to put before each address
+        alias:          Alias dictionary
+        ignore_errors:  Don't raise on alias errors
 
     Returns:
         List of email addresses
@@ -193,7 +195,7 @@  def BuildEmailList(in_list, tag=None, alias=None):
     quote = '"' if tag and tag[0] == '-' else ''
     raw = []
     for item in in_list:
-        raw += LookupEmail(item, alias)
+        raw += LookupEmail(item, alias, raise_on_error=not ignore_errors)
     result = []
     for item in raw:
         if not item in result:
@@ -202,7 +204,7 @@  def BuildEmailList(in_list, tag=None, alias=None):
         return ['%s %s%s%s' % (tag, quote, email, quote) for email in result]
     return result
 
-def EmailPatches(series, cover_fname, args, dry_run, cc_fname,
+def EmailPatches(series, cover_fname, args, dry_run, ignore_errors, cc_fname,
         self_only=False, alias=None, in_reply_to=None):
     """Email a patch series.
 
@@ -211,6 +213,7 @@  def EmailPatches(series, cover_fname, args, dry_run, cc_fname,
         cover_fname: filename of cover letter
         args: list of filenames of patch files
         dry_run: Just return the command that would be run
+        ignore_errors: Don't raise on alias errors
         cc_fname: Filename of Cc file for per-commit Cc
         self_only: True to just email to yourself as a test
         in_reply_to: If set we'll pass this to git as --in-reply-to.
@@ -233,20 +236,21 @@  def EmailPatches(series, cover_fname, args, dry_run, cc_fname,
     >>> series = series.Series()
     >>> series.to = ['fred']
     >>> series.cc = ['mary']
-    >>> EmailPatches(series, 'cover', ['p1', 'p2'], True, 'cc-fname', False, \
-            alias)
+    >>> EmailPatches(series, 'cover', ['p1', 'p2'], True, False, 'cc-fname', \
+            False, alias)
     'git send-email --annotate --to "f.bloggs@napier.co.nz" --cc \
 "m.poppins@cloud.net" --cc-cmd "./patman --cc-cmd cc-fname" cover p1 p2'
-    >>> EmailPatches(series, None, ['p1'], True, 'cc-fname', False, alias)
+    >>> EmailPatches(series, None, ['p1'], True, False, 'cc-fname', False, \
+            alias)
     'git send-email --annotate --to "f.bloggs@napier.co.nz" --cc \
 "m.poppins@cloud.net" --cc-cmd "./patman --cc-cmd cc-fname" p1'
     >>> series.cc = ['all']
-    >>> EmailPatches(series, 'cover', ['p1', 'p2'], True, 'cc-fname', True, \
-            alias)
+    >>> EmailPatches(series, 'cover', ['p1', 'p2'], True, False, 'cc-fname', \
+            True, alias)
     'git send-email --annotate --to "this-is-me@me.com" --cc-cmd "./patman \
 --cc-cmd cc-fname" cover p1 p2'
-    >>> EmailPatches(series, 'cover', ['p1', 'p2'], True, 'cc-fname', False, \
-            alias)
+    >>> EmailPatches(series, 'cover', ['p1', 'p2'], True, False, 'cc-fname', \
+            False, alias)
     'git send-email --annotate --to "f.bloggs@napier.co.nz" --cc \
 "f.bloggs@napier.co.nz" --cc "j.bloggs@napier.co.nz" --cc \
 "m.poppins@cloud.net" --cc-cmd "./patman --cc-cmd cc-fname" cover p1 p2'
@@ -254,14 +258,14 @@  def EmailPatches(series, cover_fname, args, dry_run, cc_fname,
     # Restore argv[0] since we clobbered it.
     >>> sys.argv[0] = _old_argv0
     """
-    to = BuildEmailList(series.get('to'), '--to', alias)
+    to = BuildEmailList(series.get('to'), '--to', alias, ignore_errors)
     if not to:
         print ("No recipient, please add something like this to a commit\n"
             "Series-to: Fred Bloggs <f.blogs@napier.co.nz>")
         return
-    cc = BuildEmailList(series.get('cc'), '--cc', alias)
+    cc = BuildEmailList(series.get('cc'), '--cc', alias, ignore_errors)
     if self_only:
-        to = BuildEmailList([os.getenv('USER')], '--to', alias)
+        to = BuildEmailList([os.getenv('USER')], '--to', alias, ignore_errors)
         cc = []
     cmd = ['git', 'send-email', '--annotate']
     if in_reply_to:
@@ -279,13 +283,15 @@  def EmailPatches(series, cover_fname, args, dry_run, cc_fname,
     return str
 
 
-def LookupEmail(lookup_name, alias=None, level=0):
+def LookupEmail(lookup_name, alias=None, raise_on_error=True, level=0):
     """If an email address is an alias, look it up and return the full name
 
     TODO: Why not just use git's own alias feature?
 
     Args:
         lookup_name: Alias or email address to look up
+        alias: Dictionary containing aliases (None to use settings default)
+        raise_on_error: True to raise an error when an alias fails to match
 
     Returns:
         tuple:
@@ -319,6 +325,15 @@  def LookupEmail(lookup_name, alias=None, level=0):
     Traceback (most recent call last):
     ...
     OSError: Recursive email alias at 'other'
+    >>> LookupEmail('odd', alias, False)
+    \033[1;31mAlias 'odd' not found\033[0m
+    []
+    >>> # In this case the loop part will effectively be ignored.
+    >>> LookupEmail('loop', alias, False)
+    \033[1;31mRecursive email alias at 'other'\033[0m
+    \033[1;31mRecursive email alias at 'john'\033[0m
+    \033[1;31mRecursive email alias at 'mary'\033[0m
+    ['j.bloggs@napier.co.nz', 'm.poppins@cloud.net']
     """
     if not alias:
         alias = settings.alias
@@ -327,16 +342,27 @@  def LookupEmail(lookup_name, alias=None, level=0):
         return [lookup_name]
 
     lookup_name = lookup_name.lower()
+    col = terminal.Color()
 
+    out_list = []
     if level > 10:
-        raise OSError, "Recursive email alias at '%s'" % lookup_name
+        msg = "Recursive email alias at '%s'" % lookup_name
+        if raise_on_error:
+            raise OSError, msg
+        else:
+            print col.Color(col.RED, msg)
+            return out_list
 
-    out_list = []
     if lookup_name:
         if not lookup_name in alias:
-            raise ValueError, "Alias '%s' not found" % lookup_name
+            msg = "Alias '%s' not found" % lookup_name
+            if raise_on_error:
+                raise ValueError, msg
+            else:
+                print col.Color(col.RED, msg)
+                return out_list
         for item in alias[lookup_name]:
-            todo = LookupEmail(item, alias, level + 1)
+            todo = LookupEmail(item, alias, raise_on_error, level + 1)
             for new_item in todo:
                 if not new_item in out_list:
                     out_list.append(new_item)
diff --git a/tools/patman/patman.py b/tools/patman/patman.py
index 5000b7d..5768f56 100755
--- a/tools/patman/patman.py
+++ b/tools/patman/patman.py
@@ -57,7 +57,9 @@  parser.add_option('-r', '--in-reply-to', type='string', action='store',
                   help="Message ID that this series is in reply to")
 parser.add_option('-s', '--start', dest='start', type='int',
        default=0, help='Commit to start creating patches from (0 = HEAD)')
-parser.add_option('-t', '--test', action='store_true', dest='test',
+parser.add_option('-t', '--ignore-bad-tags', action='store_true',
+                  default=False, help='Ignore bad tags / aliases')
+parser.add_option('--test', action='store_true', dest='test',
                   default=False, help='run tests')
 parser.add_option('-v', '--verbose', action='store_true', dest='verbose',
        default=False, help='Verbose output of errors and warnings')
@@ -159,13 +161,15 @@  else:
             options.count + options.start):
         ok = False
 
-    cc_file = series.MakeCcFile(options.process_tags, cover_fname)
+    cc_file = series.MakeCcFile(options.process_tags, cover_fname,
+                                options.ignore_bad_tags)
 
     # Email the patches out (giving the user time to check / cancel)
     cmd = ''
     if ok or options.ignore_errors:
         cmd = gitutil.EmailPatches(series, cover_fname, args,
-                options.dry_run, cc_file, in_reply_to=options.in_reply_to)
+                options.dry_run, options.ignore_bad_tags, cc_file,
+                in_reply_to=options.in_reply_to)
 
     # For a dry run, just show our actions as a sanity check
     if options.dry_run:
diff --git a/tools/patman/series.py b/tools/patman/series.py
index 6c5c570..fab17c9 100644
--- a/tools/patman/series.py
+++ b/tools/patman/series.py
@@ -206,7 +206,7 @@  class Series(dict):
             str = 'Change log exists, but no version is set'
             print col.Color(col.RED, str)
 
-    def MakeCcFile(self, process_tags, cover_fname):
+    def MakeCcFile(self, process_tags, cover_fname, ignore_bad_tags):
         """Make a cc file for us to use for per-commit Cc automation
 
         Also stores in self._generated_cc to make ShowActions() faster.
@@ -214,6 +214,7 @@  class Series(dict):
         Args:
             process_tags: Process tags as if they were aliases
             cover_fname: If non-None the name of the cover letter.
+            ignore_bad_tags: Ignore bad tags / aliases
         Return:
             Filename of temp file created
         """
@@ -224,8 +225,10 @@  class Series(dict):
         for commit in self.commits:
             list = []
             if process_tags:
-                list += gitutil.BuildEmailList(commit.tags)
-            list += gitutil.BuildEmailList(commit.cc_list)
+                list += gitutil.BuildEmailList(commit.tags,
+                                               ignore_errors=ignore_bad_tags)
+            list += gitutil.BuildEmailList(commit.cc_list,
+                                           ignore_errors=ignore_bad_tags)
             list += get_maintainer.GetMaintainer(commit.patch)
             all_ccs += list
             print >>fd, commit.patch, ', '.join(list)