All of lore.kernel.org
 help / color / mirror / Atom feed
* [U-Boot] [PATCH 0/8] Various patman fixes
@ 2013-03-21  2:42 Simon Glass
  2013-03-21  2:42 ` [U-Boot] [PATCH 1/8] patman: Fix up checkpatch parsing to deal with 'CHECK' lines Simon Glass
                   ` (7 more replies)
  0 siblings, 8 replies; 21+ messages in thread
From: Simon Glass @ 2013-03-21  2:42 UTC (permalink / raw)
  To: u-boot

Since checkpatch has changed in U-Boot, patman no longer detects errors
properly. It seems like a good oppotunity to fix some of the bugs and
annoying habits of patman that people have mentioned on the list.

I believe the first five patches should be considered bug fixes. The
ones after that are new features.

If I have missed any bugs / problems in this sweep, please let me know.


Simon Glass (8):
  patman: Fix up checkpatch parsing to deal with 'CHECK' lines
  patman: Don't look for tags inside quotes
  patman: Minor help message/README fixes
  patman: Fix the comment in CheckTags to mention multiple tags
  patman: Provide option to ignore bad aliases
  patman: Add -a option to refrain from test-applying the patches
  patman: Add Cover-letter-cc tag to Cc cover letter to people
  patman: Add Series-process-log tag to sort/uniq change logs

 tools/patman/README         | 22 +++++++++++--
 tools/patman/checkpatch.py  | 75 +++++++++++++++++++++++++++++----------------
 tools/patman/commit.py      |  7 +++--
 tools/patman/gitutil.py     | 62 ++++++++++++++++++++++++++-----------
 tools/patman/patchstream.py | 10 +++++-
 tools/patman/patman.py      | 24 ++++++++++-----
 tools/patman/series.py      | 26 +++++++++++-----
 tools/patman/test.py        |  9 ++++--
 8 files changed, 165 insertions(+), 70 deletions(-)

-- 
1.8.1.3

^ permalink raw reply	[flat|nested] 21+ messages in thread

* [U-Boot] [PATCH 1/8] patman: Fix up checkpatch parsing to deal with 'CHECK' lines
  2013-03-21  2:42 [U-Boot] [PATCH 0/8] Various patman fixes Simon Glass
@ 2013-03-21  2:42 ` Simon Glass
  2013-03-21 16:35   ` Doug Anderson
  2013-03-21  2:42 ` [U-Boot] [PATCH 2/8] patman: Don't look for tags inside quotes Simon Glass
                   ` (6 subsequent siblings)
  7 siblings, 1 reply; 21+ messages in thread
From: Simon Glass @ 2013-03-21  2:42 UTC (permalink / raw)
  To: u-boot

checkpatch has a new type of warning, a 'CHECK'. At present patman fails
with these, which makes it less than useful.

Add support for checks, making it backwards compatible with the old
checkpatch.

Signed-off-by: Simon Glass <sjg@chromium.org>
---
 tools/patman/checkpatch.py | 75 +++++++++++++++++++++++++++++-----------------
 tools/patman/test.py       |  9 ++++--
 2 files changed, 54 insertions(+), 30 deletions(-)

diff --git a/tools/patman/checkpatch.py b/tools/patman/checkpatch.py
index d3a0477..d1743ae 100644
--- a/tools/patman/checkpatch.py
+++ b/tools/patman/checkpatch.py
@@ -64,10 +64,14 @@ def CheckPatch(fname, verbose=False):
                 'msg': text message
                 'file' : filename
                 'line': line number
+            error_count: Number of errors
+            warning_count: Number of warnings
+            check_count: Number of checks
             lines: Number of lines
+            stdout: Full output of checkpatch
     """
     result = False
-    error_count, warning_count, lines = 0, 0, 0
+    error_count, warning_count, check_count, lines = 0, 0, 0, 0
     problems = []
     chk = FindCheckPatch()
     item = {}
@@ -76,11 +80,16 @@ def CheckPatch(fname, verbose=False):
     #stdout, stderr = pipe.communicate()
 
     # total: 0 errors, 0 warnings, 159 lines checked
+    # or:
+    # total: 0 errors, 2 warnings, 7 checks, 473 lines checked
     re_stats = re.compile('total: (\\d+) errors, (\d+) warnings, (\d+)')
+    re_stats_full = re.compile('total: (\\d+) errors, (\d+) warnings, (\d+)'
+                               ' checks, (\d+)')
     re_ok = re.compile('.*has no obvious style problems')
     re_bad = re.compile('.*has style problems, please review')
     re_error = re.compile('ERROR: (.*)')
     re_warning = re.compile('WARNING: (.*)')
+    re_check = re.compile('CHECK: (.*)')
     re_file = re.compile('#\d+: FILE: ([^:]*):(\d+):')
 
     for line in stdout.splitlines():
@@ -91,29 +100,39 @@ def CheckPatch(fname, verbose=False):
         if not line and item:
             problems.append(item)
             item = {}
-        match = re_stats.match(line)
+        match = re_stats_full.match(line)
+        if not match:
+            match = re_stats.match(line)
         if match:
             error_count = int(match.group(1))
             warning_count = int(match.group(2))
-            lines = int(match.group(3))
+            match_count = int(match.group(3))
+            if len(match.groups()) == 4:
+               check_count = match_count
+               lines = int(match.group(4))
         elif re_ok.match(line):
             result = True
         elif re_bad.match(line):
             result = False
-        match = re_error.match(line)
-        if match:
-            item['msg'] = match.group(1)
+        err_match = re_error.match(line)
+        warn_match = re_warning.match(line)
+        file_match = re_file.match(line)
+        check_match = re_check.match(line)
+        if err_match:
+            item['msg'] = err_match.group(1)
             item['type'] = 'error'
-        match = re_warning.match(line)
-        if match:
-            item['msg'] = match.group(1)
+        elif warn_match:
+            item['msg'] = warn_match.group(1)
             item['type'] = 'warning'
-        match = re_file.match(line)
-        if match:
-            item['file'] = match.group(1)
-            item['line'] = int(match.group(2))
+        elif check_match:
+            item['msg'] = check_match.group(1)
+            item['type'] = 'check'
+        elif file_match:
+            item['file'] = file_match.group(1)
+            item['line'] = int(file_match.group(2))
 
-    return result, problems, error_count, warning_count, lines, stdout
+    return (result, problems, error_count, warning_count, check_count,
+            lines, stdout)
 
 def GetWarningMsg(col, msg_type, fname, line, msg):
     '''Create a message for a given file/line
@@ -128,37 +147,39 @@ def GetWarningMsg(col, msg_type, fname, line, msg):
         msg_type = col.Color(col.YELLOW, msg_type)
     elif msg_type == 'error':
         msg_type = col.Color(col.RED, msg_type)
+    elif msg_type == 'check':
+        msg_type = col.Color(col.MAGENTA, msg_type)
     return '%s: %s,%d: %s' % (msg_type, fname, line, msg)
 
 def CheckPatches(verbose, args):
     '''Run the checkpatch.pl script on each patch'''
-    error_count = 0
-    warning_count = 0
+    error_count, warning_count, check_count = 0, 0, 0
     col = terminal.Color()
 
     for fname in args:
-        ok, problems, errors, warnings, lines, stdout = CheckPatch(fname,
-                verbose)
+        ok, problems, errors, warnings, checks, lines, stdout = CheckPatch(
+                fname, verbose)
         if not ok:
             error_count += errors
             warning_count += warnings
-            print '%d errors, %d warnings for %s:' % (errors,
-                    warnings, fname)
-            if len(problems) != error_count + warning_count:
+            check_count += checks
+            print '%d errors, %d warnings, %d checks for %s:' % (errors,
+                    warnings, checks, col.Color(col.BLUE, fname))
+            if len(problems) != errors + warnings + checks:
                 print "Internal error: some problems lost"
             for item in problems:
-                print GetWarningMsg(col, item['type'],
+                print GetWarningMsg(col, item.get('type', '<unknown>'),
                         item.get('file', '<unknown>'),
-                        item.get('line', 0), item['msg'])
+                        item.get('line', 0), item.get('msg', 'message'))
+            print
             #print stdout
-    if error_count != 0 or warning_count != 0:
-        str = 'checkpatch.pl found %d error(s), %d warning(s)' % (
-            error_count, warning_count)
+    if error_count or warning_count or check_count:
+        str = 'checkpatch.pl found %d error(s), %d warning(s), %d checks(s)'
         color = col.GREEN
         if warning_count:
             color = col.YELLOW
         if error_count:
             color = col.RED
-        print col.Color(color, str)
+        print col.Color(color, str % (error_count, warning_count, check_count))
         return False
     return True
diff --git a/tools/patman/test.py b/tools/patman/test.py
index f801ced..5a41ef5 100644
--- a/tools/patman/test.py
+++ b/tools/patman/test.py
@@ -218,7 +218,8 @@ index 0000000..2234c87
     def testCheckpatch(self):
         """Test checkpatch operation"""
         inf = self.SetupData('good')
-        result, problems, err, warn, lines, stdout = checkpatch.CheckPatch(inf)
+        result, problems, err, warn, check, lines, stdout = (
+            checkpatch.CheckPatch(inf))
         self.assertEqual(result, True)
         self.assertEqual(problems, [])
         self.assertEqual(err, 0)
@@ -227,7 +228,8 @@ index 0000000..2234c87
         os.remove(inf)
 
         inf = self.SetupData('no-signoff')
-        result, problems, err, warn, lines, stdout = checkpatch.CheckPatch(inf)
+        result, problems, err, warn, check, lines, stdout = (
+            checkpatch.CheckPatch(inf))
         self.assertEqual(result, False)
         self.assertEqual(len(problems), 1)
         self.assertEqual(err, 1)
@@ -236,7 +238,8 @@ index 0000000..2234c87
         os.remove(inf)
 
         inf = self.SetupData('spaces')
-        result, problems, err, warn, lines, stdout = checkpatch.CheckPatch(inf)
+        result, problems, err, warn, check, lines, stdout = (
+            checkpatch.CheckPatch(inf))
         self.assertEqual(result, False)
         self.assertEqual(len(problems), 2)
         self.assertEqual(err, 0)
-- 
1.8.1.3

^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [U-Boot] [PATCH 2/8] patman: Don't look for tags inside quotes
  2013-03-21  2:42 [U-Boot] [PATCH 0/8] Various patman fixes Simon Glass
  2013-03-21  2:42 ` [U-Boot] [PATCH 1/8] patman: Fix up checkpatch parsing to deal with 'CHECK' lines Simon Glass
@ 2013-03-21  2:42 ` Simon Glass
  2013-03-21 16:50   ` Doug Anderson
  2013-03-21  2:42 ` [U-Boot] [PATCH 3/8] patman: Minor help message/README fixes Simon Glass
                   ` (5 subsequent siblings)
  7 siblings, 1 reply; 21+ messages in thread
From: Simon Glass @ 2013-03-21  2:42 UTC (permalink / raw)
  To: u-boot

At present something like:

   Revert "arm: Add cache operations"

will try to use

   Revert "arm

as a tag. Clearly this is wrong, so fix it.

If the revert is intended to be tagged, then the tag can come before
the revert, perhaps. Alternatively the 'Cc' tag can be used in the commit
messages.

Signed-off-by: Simon Glass <sjg@chromium.org>
---
 tools/patman/commit.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/patman/commit.py b/tools/patman/commit.py
index 7144e54..64baf52 100644
--- a/tools/patman/commit.py
+++ b/tools/patman/commit.py
@@ -22,7 +22,7 @@
 import re
 
 # Separates a tag: at the beginning of the subject from the rest of it
-re_subject_tag = re.compile('([^:]*):\s*(.*)')
+re_subject_tag = re.compile('([^:"]*):\s*(.*)')
 
 class Commit:
     """Holds information about a single commit/patch in the series.
-- 
1.8.1.3

^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [U-Boot] [PATCH 3/8] patman: Minor help message/README fixes
  2013-03-21  2:42 [U-Boot] [PATCH 0/8] Various patman fixes Simon Glass
  2013-03-21  2:42 ` [U-Boot] [PATCH 1/8] patman: Fix up checkpatch parsing to deal with 'CHECK' lines Simon Glass
  2013-03-21  2:42 ` [U-Boot] [PATCH 2/8] patman: Don't look for tags inside quotes Simon Glass
@ 2013-03-21  2:42 ` Simon Glass
  2013-03-21 16:52   ` Doug Anderson
  2013-03-21  2:42 ` [U-Boot] [PATCH 4/8] patman: Fix the comment in CheckTags to mention multiple tags Simon Glass
                   ` (4 subsequent siblings)
  7 siblings, 1 reply; 21+ messages in thread
From: Simon Glass @ 2013-03-21  2:42 UTC (permalink / raw)
  To: u-boot

A few of the help messages are not quite right, and there is a typo
in the README. Fix these.

Signed-off-by: Simon Glass <sjg@chromium.org>
---
 tools/patman/README    | 2 +-
 tools/patman/patman.py | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/tools/patman/README b/tools/patman/README
index 86d366f..c0657bc 100644
--- a/tools/patman/README
+++ b/tools/patman/README
@@ -68,7 +68,7 @@ will get a consistent result each time.
 How to configure it
 ===================
 
-For most cases of using patman for U-Boot developement patman will
+For most cases of using patman for U-Boot development, patman will
 locate and use the file 'doc/git-mailrc' in your U-Boot directory.
 This contains most of the aliases you will need.
 
diff --git a/tools/patman/patman.py b/tools/patman/patman.py
index 377408d..5000b7d 100755
--- a/tools/patman/patman.py
+++ b/tools/patman/patman.py
@@ -49,7 +49,7 @@ parser.add_option('-i', '--ignore-errors', action='store_true',
        dest='ignore_errors', default=False,
        help='Send patches email even if patch errors are found')
 parser.add_option('-n', '--dry-run', action='store_true', dest='dry_run',
-       default=False, help="Do a try run (create but don't email patches)")
+       default=False, help="Do a dry run (create but don't email patches)")
 parser.add_option('-p', '--project', default=project.DetectProject(),
                   help="Project name; affects default option values and "
                   "aliases [default: %default]")
@@ -72,7 +72,7 @@ parser.add_option('--no-tags', action='store_false', dest='process_tags',
 parser.usage = """patman [options]
 
 Create patches from commits in a branch, check them and email them as
-specified by tags you place in the commits. Use -n to """
+specified by tags you place in the commits. Use -n to do a dry run first."""
 
 
 # Parse options twice: first to get the project and second to handle
-- 
1.8.1.3

^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [U-Boot] [PATCH 4/8] patman: Fix the comment in CheckTags to mention multiple tags
  2013-03-21  2:42 [U-Boot] [PATCH 0/8] Various patman fixes Simon Glass
                   ` (2 preceding siblings ...)
  2013-03-21  2:42 ` [U-Boot] [PATCH 3/8] patman: Minor help message/README fixes Simon Glass
@ 2013-03-21  2:42 ` Simon Glass
  2013-03-21 16:53   ` Doug Anderson
  2013-03-21  2:42 ` [U-Boot] [PATCH 5/8] patman: Provide option to ignore bad aliases Simon Glass
                   ` (3 subsequent siblings)
  7 siblings, 1 reply; 21+ messages in thread
From: Simon Glass @ 2013-03-21  2:42 UTC (permalink / raw)
  To: u-boot

This comment is less than helpful. Since multiple tags are supported, add
an example of how multiple tags work.

Signed-off-by: Simon Glass <sjg@chromium.org>
---
 tools/patman/commit.py | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/tools/patman/commit.py b/tools/patman/commit.py
index 64baf52..eb3c023 100644
--- a/tools/patman/commit.py
+++ b/tools/patman/commit.py
@@ -61,9 +61,10 @@ class Commit:
 
         Subject tags look like this:
 
-            propounder: Change the widget to propound correctly
+            propounder: fort: Change the widget to propound correctly
 
-        Multiple tags are supported. The list is updated in self.tag
+        Here the tags are propounder and fort. Multiple tags are supported.
+        The list is updated in self.tag.
 
         Returns:
             None if ok, else the name of a tag with no email alias
-- 
1.8.1.3

^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [U-Boot] [PATCH 5/8] patman: Provide option to ignore bad aliases
  2013-03-21  2:42 [U-Boot] [PATCH 0/8] Various patman fixes Simon Glass
                   ` (3 preceding siblings ...)
  2013-03-21  2:42 ` [U-Boot] [PATCH 4/8] patman: Fix the comment in CheckTags to mention multiple tags Simon Glass
@ 2013-03-21  2:42 ` Simon Glass
  2013-03-21 17:09   ` Doug Anderson
  2013-03-21  2:42 ` [U-Boot] [PATCH 6/8] patman: Add -a option to refrain from test-applying the patches Simon Glass
                   ` (2 subsequent siblings)
  7 siblings, 1 reply; 21+ messages in thread
From: Simon Glass @ 2013-03-21  2:42 UTC (permalink / raw)
  To: u-boot

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

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 at napier.co.nz" --cc \
 "m.poppins at 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 at napier.co.nz" --cc \
 "m.poppins at 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 at 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 at napier.co.nz" --cc \
 "f.bloggs at napier.co.nz" --cc "j.bloggs at napier.co.nz" --cc \
 "m.poppins at 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 at napier.co.nz', 'm.poppins at 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)
-- 
1.8.1.3

^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [U-Boot] [PATCH 6/8] patman: Add -a option to refrain from test-applying the patches
  2013-03-21  2:42 [U-Boot] [PATCH 0/8] Various patman fixes Simon Glass
                   ` (4 preceding siblings ...)
  2013-03-21  2:42 ` [U-Boot] [PATCH 5/8] patman: Provide option to ignore bad aliases Simon Glass
@ 2013-03-21  2:42 ` Simon Glass
  2013-03-21 17:12   ` Doug Anderson
  2013-03-21  2:43 ` [U-Boot] [PATCH 7/8] patman: Add Cover-letter-cc tag to Cc cover letter to people Simon Glass
  2013-03-21  2:43 ` [U-Boot] [PATCH 8/8] patman: Add Series-process-log tag to sort/uniq change logs Simon Glass
  7 siblings, 1 reply; 21+ messages in thread
From: Simon Glass @ 2013-03-21  2:42 UTC (permalink / raw)
  To: u-boot

Especially with the Linux kernel, it takes a long time (a minute or more)
to test-apply the patches, so patman becomes significantly less useful.
The only real problem that is found with this apply step is trailing spaces.
Provide a -a option to skip this step, for those working with clean patches.

Signed-off-by: Simon Glass <sjg@chromium.org>
---
 tools/patman/patman.py | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/tools/patman/patman.py b/tools/patman/patman.py
index 5768f56..b92a393 100755
--- a/tools/patman/patman.py
+++ b/tools/patman/patman.py
@@ -41,6 +41,9 @@ import test
 
 
 parser = OptionParser()
+parser.add_option('-a', '--no-apply', action='store_false',
+                  dest='apply_patches', default=True,
+                  help="Don't test-apply patches with git am")
 parser.add_option('-H', '--full-help', action='store_true', dest='full_help',
        default=False, help='Display the README file')
 parser.add_option('-c', '--count', dest='count', type='int',
@@ -157,9 +160,10 @@ else:
         ok = checkpatch.CheckPatches(options.verbose, args)
     else:
         ok = True
-    if not gitutil.ApplyPatches(options.verbose, args,
-            options.count + options.start):
-        ok = False
+    if options.apply_patches:
+        if not gitutil.ApplyPatches(options.verbose, args,
+                                    options.count + options.start):
+            ok = False
 
     cc_file = series.MakeCcFile(options.process_tags, cover_fname,
                                 options.ignore_bad_tags)
-- 
1.8.1.3

^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [U-Boot] [PATCH 7/8] patman: Add Cover-letter-cc tag to Cc cover letter to people
  2013-03-21  2:42 [U-Boot] [PATCH 0/8] Various patman fixes Simon Glass
                   ` (5 preceding siblings ...)
  2013-03-21  2:42 ` [U-Boot] [PATCH 6/8] patman: Add -a option to refrain from test-applying the patches Simon Glass
@ 2013-03-21  2:43 ` Simon Glass
  2013-03-21  3:46   ` Doug Anderson
  2013-03-21  2:43 ` [U-Boot] [PATCH 8/8] patman: Add Series-process-log tag to sort/uniq change logs Simon Glass
  7 siblings, 1 reply; 21+ messages in thread
From: Simon Glass @ 2013-03-21  2:43 UTC (permalink / raw)
  To: u-boot

The cover letter is sent to everyone who is on the Cc list for any of
the patches in the series. Sometimes it is useful to send just the cover
letter to additional people, so that they are aware of the series, but
don't need to wade through all the individual patches.

Add a new Cover-letter-cc tag for this purpose.

Signed-off-by: Simon Glass <sjg@chromium.org>
---
 tools/patman/README         | 12 +++++++++++-
 tools/patman/patchstream.py |  8 ++++++++
 tools/patman/series.py      | 11 ++++++++---
 3 files changed, 27 insertions(+), 4 deletions(-)

diff --git a/tools/patman/README b/tools/patman/README
index c0657bc..d4e7f36 100644
--- a/tools/patman/README
+++ b/tools/patman/README
@@ -182,6 +182,10 @@ END
 	Sets the cover letter contents for the series. The first line
 	will become the subject of the cover letter
 
+Cover-letter-cc: email / alias
+	Additional email addresses / aliases to send cover letter to (you
+	can add this multiple times)
+
 Series-notes:
 blah blah
 blah blah
@@ -263,7 +267,13 @@ will create a patch which is copied to x86, arm, sandbox, mikef, ag and
 afleming.
 
 If you have a cover letter it will get sent to the union of the CC lists of
-all of the other patches.
+all of the other patches. If you want to sent it to additional people you
+can add a tag:
+
+Cover-letter-cc: <list of addresses>
+
+These people will get the cover letter even if they are not on the To/Cc
+list for any of the patches.
 
 
 Example Work Flow
diff --git a/tools/patman/patchstream.py b/tools/patman/patchstream.py
index 76f815b..a4f2f31 100644
--- a/tools/patman/patchstream.py
+++ b/tools/patman/patchstream.py
@@ -42,6 +42,9 @@ re_signoff = re.compile('^Signed-off-by:')
 # The start of the cover letter
 re_cover = re.compile('^Cover-letter:')
 
+# A cover letter Cc
+re_cover_cc = re.compile('^Cover-letter-cc: *(.*)')
+
 # Patch series tag
 re_series = re.compile('^Series-(\w*): *(.*)')
 
@@ -153,6 +156,7 @@ class PatchStream:
         # Handle state transition and skipping blank lines
         series_match = re_series.match(line)
         commit_match = re_commit.match(line) if self.is_log else None
+        cover_cc_match = re_cover_cc.match(line)
         tag_match = None
         if self.state == STATE_PATCH_HEADER:
             tag_match = re_tag.match(line)
@@ -205,6 +209,10 @@ class PatchStream:
             self.in_section = 'cover'
             self.skip_blank = False
 
+        elif cover_cc_match:
+            value = cover_cc_match.group(1)
+            self.AddToSeries(line, 'cover-cc', value)
+
         # If we are in a change list, key collected lines until a blank one
         elif self.in_change:
             if is_blank:
diff --git a/tools/patman/series.py b/tools/patman/series.py
index fab17c9..fe919be 100644
--- a/tools/patman/series.py
+++ b/tools/patman/series.py
@@ -27,7 +27,8 @@ import gitutil
 import terminal
 
 # Series-xxx tags that we understand
-valid_series = ['to', 'cc', 'version', 'changes', 'prefix', 'notes', 'name'];
+valid_series = ['to', 'cc', 'version', 'changes', 'prefix', 'notes', 'name',
+                'cover-cc']
 
 class Series(dict):
     """Holds information about a patch series, including all tags.
@@ -43,6 +44,7 @@ class Series(dict):
     def __init__(self):
         self.cc = []
         self.to = []
+        self.cover_cc = []
         self.commits = []
         self.cover = None
         self.notes = []
@@ -69,6 +71,7 @@ class Series(dict):
             value: Tag value (part after 'Series-xxx: ')
         """
         # If we already have it, then add to our list
+        name = name.replace('-', '_')
         if name in self:
             values = value.split(',')
             values = [str.strip() for str in values]
@@ -140,7 +143,8 @@ class Series(dict):
         print 'Prefix:\t ', self.get('prefix')
         if self.cover:
             print 'Cover: %d lines' % len(self.cover)
-            all_ccs = itertools.chain(*self._generated_cc.values())
+            cover_cc = gitutil.BuildEmailList(self.get('cover_cc', ''))
+            all_ccs = itertools.chain(cover_cc, *self._generated_cc.values())
             for email in set(all_ccs):
                     print '      Cc: ',email
         if cmd:
@@ -235,7 +239,8 @@ class Series(dict):
             self._generated_cc[commit.patch] = list
 
         if cover_fname:
-            print >>fd, cover_fname, ', '.join(set(all_ccs))
+            cover_cc = gitutil.BuildEmailList(self.get('cover_cc', ''))
+            print >>fd, cover_fname, ', '.join(set(cover_cc + all_ccs))
 
         fd.close()
         return fname
-- 
1.8.1.3

^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [U-Boot] [PATCH 8/8] patman: Add Series-process-log tag to sort/uniq change logs
  2013-03-21  2:42 [U-Boot] [PATCH 0/8] Various patman fixes Simon Glass
                   ` (6 preceding siblings ...)
  2013-03-21  2:43 ` [U-Boot] [PATCH 7/8] patman: Add Cover-letter-cc tag to Cc cover letter to people Simon Glass
@ 2013-03-21  2:43 ` Simon Glass
  2013-03-21 17:51   ` Doug Anderson
  7 siblings, 1 reply; 21+ messages in thread
From: Simon Glass @ 2013-03-21  2:43 UTC (permalink / raw)
  To: u-boot

For some series with lots of changes it is annoying that duplicate change
log items are not caught. It is also helpful sometimes to sort the change
logs.

Add a Series-process-log tag to enable this, which can be placed in a
commit to control this.

The change to the Cc: line is to fix a checkpatch warning.

Signed-off-by: Simon Glass <sjg@chromium.org>
---
 tools/patman/README         | 8 +++++++-
 tools/patman/patchstream.py | 2 +-
 tools/patman/series.py      | 8 ++++++--
 3 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/tools/patman/README b/tools/patman/README
index d4e7f36..566f54d 100644
--- a/tools/patman/README
+++ b/tools/patman/README
@@ -225,9 +225,15 @@ Series-changes: n
 	to update the log there and then, knowing that the script will
 	do the rest.
 
-Cc: Their Name <email>
+ Cc: Their Name <email>
 	This copies a single patch to another email address.
 
+Series-process-log: sort, uniq
+	This tells patman to sort and/or uniq the change logs. It is
+	assumed that each change log entry is only a single line long.
+	Use 'sort' to sort the entries, and 'uniq' to include only
+	unique entries. If omitted, no change log processing is done.
+
 Various other tags are silently removed, like these Chrome OS and
 Gerrit tags:
 
diff --git a/tools/patman/patchstream.py b/tools/patman/patchstream.py
index a4f2f31..9d8a918 100644
--- a/tools/patman/patchstream.py
+++ b/tools/patman/patchstream.py
@@ -46,7 +46,7 @@ re_cover = re.compile('^Cover-letter:')
 re_cover_cc = re.compile('^Cover-letter-cc: *(.*)')
 
 # Patch series tag
-re_series = re.compile('^Series-(\w*): *(.*)')
+re_series = re.compile('^Series-([a-z-]*): *(.*)')
 
 # Commit tags that we want to collect and keep
 re_tag = re.compile('^(Tested-by|Acked-by|Reviewed-by|Cc): (.*)')
diff --git a/tools/patman/series.py b/tools/patman/series.py
index fe919be..7080a55 100644
--- a/tools/patman/series.py
+++ b/tools/patman/series.py
@@ -28,7 +28,7 @@ import terminal
 
 # Series-xxx tags that we understand
 valid_series = ['to', 'cc', 'version', 'changes', 'prefix', 'notes', 'name',
-                'cover-cc']
+                'cover-cc', 'process_log']
 
 class Series(dict):
     """Holds information about a patch series, including all tags.
@@ -167,15 +167,19 @@ class Series(dict):
             etc.
         """
         final = []
+        process_it = self.get('process_log', '')
         need_blank = False
         for change in sorted(self.changes, reverse=True):
             out = []
             for this_commit, text in self.changes[change]:
                 if commit and this_commit != commit:
                     continue
-                out.append(text)
+                if not ('uniq' in process_it and text in out):
+                    out.append(text)
             line = 'Changes in v%d:' % change
             have_changes = len(out) > 0
+            if 'sort' in process_it:
+                out = sorted(out)
             if have_changes:
                 out.insert(0, line)
             else:
-- 
1.8.1.3

^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [U-Boot] [PATCH 7/8] patman: Add Cover-letter-cc tag to Cc cover letter to people
  2013-03-21  2:43 ` [U-Boot] [PATCH 7/8] patman: Add Cover-letter-cc tag to Cc cover letter to people Simon Glass
@ 2013-03-21  3:46   ` Doug Anderson
  0 siblings, 0 replies; 21+ messages in thread
From: Doug Anderson @ 2013-03-21  3:46 UTC (permalink / raw)
  To: u-boot

Simon,

On Wed, Mar 20, 2013 at 7:43 PM, Simon Glass <sjg@chromium.org> wrote:
> The cover letter is sent to everyone who is on the Cc list for any of
> the patches in the series. Sometimes it is useful to send just the cover
> letter to additional people, so that they are aware of the series, but
> don't need to wade through all the individual patches.
>
> Add a new Cover-letter-cc tag for this purpose.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
>  tools/patman/README         | 12 +++++++++++-
>  tools/patman/patchstream.py |  8 ++++++++
>  tools/patman/series.py      | 11 ++++++++---
>  3 files changed, 27 insertions(+), 4 deletions(-)

Identical to the already posted
<http://patchwork.ozlabs.org/patch/222755/> so I'll repeat my existing
review.  ;)

Reviewed-by: Doug Anderson <dianders@chromium.org>

^ permalink raw reply	[flat|nested] 21+ messages in thread

* [U-Boot] [PATCH 1/8] patman: Fix up checkpatch parsing to deal with 'CHECK' lines
  2013-03-21  2:42 ` [U-Boot] [PATCH 1/8] patman: Fix up checkpatch parsing to deal with 'CHECK' lines Simon Glass
@ 2013-03-21 16:35   ` Doug Anderson
  2013-03-26 22:12     ` Simon Glass
  0 siblings, 1 reply; 21+ messages in thread
From: Doug Anderson @ 2013-03-21 16:35 UTC (permalink / raw)
  To: u-boot

Simon,

On Wed, Mar 20, 2013 at 7:42 PM, Simon Glass <sjg@chromium.org> wrote:
> checkpatch has a new type of warning, a 'CHECK'. At present patman fails
> with these, which makes it less than useful.
>
> Add support for checks, making it backwards compatible with the old
> checkpatch.

There are also a few other minor fixups in this:
* Cleanup formatting of the CheckPatches() output.
* Fix erroneous "internal error" if multiple patches have warnings.
* Be more robust to new types of problems.


> @@ -64,10 +64,14 @@ def CheckPatch(fname, verbose=False):
>                  'msg': text message
>                  'file' : filename
>                  'line': line number
> +            error_count: Number of errors
> +            warning_count: Number of warnings
> +            check_count: Number of checks
>              lines: Number of lines
> +            stdout: Full output of checkpatch

nit: Right above this it says you're returning a 4-tuple.  That's no
longer true.  Could just remove the "4-".

optional: I've found that returning big tuples like this is
problematic.  Whenever you change the number of things returned you've
got to modify any callers that call like:

a, b, c, d = CheckPatch(...)

to now be:

a, b, c, d, e = CheckPatch(...)

...or never use the above syntax and use:

result = CheckPatch(...)
blah = result[0]

Maybe use a namedtuple so that callers can use the result more cleanly?


>          if match:
>              error_count = int(match.group(1))
>              warning_count = int(match.group(2))
> -            lines = int(match.group(3))
> +            match_count = int(match.group(3))
> +            if len(match.groups()) == 4:
> +               check_count = match_count
> +               lines = int(match.group(4))

I'm confused about match_count here.  What is it supposed to contain?
I can't tell from the name of it.  It looks like a temporary variable
holding either check_count or lines.  ...but you forget to assign
"lines = match_count" in an "else" case so things are broken with old
versions of checkpatch, right?


-Doug

^ permalink raw reply	[flat|nested] 21+ messages in thread

* [U-Boot] [PATCH 2/8] patman: Don't look for tags inside quotes
  2013-03-21  2:42 ` [U-Boot] [PATCH 2/8] patman: Don't look for tags inside quotes Simon Glass
@ 2013-03-21 16:50   ` Doug Anderson
  2013-03-26 22:14     ` Simon Glass
  0 siblings, 1 reply; 21+ messages in thread
From: Doug Anderson @ 2013-03-21 16:50 UTC (permalink / raw)
  To: u-boot

Simon,

On Wed, Mar 20, 2013 at 7:42 PM, Simon Glass <sjg@chromium.org> wrote:
>  # Separates a tag: at the beginning of the subject from the rest of it
> -re_subject_tag = re.compile('([^:]*):\s*(.*)')
> +re_subject_tag = re.compile('([^:"]*):\s*(.*)')

I'd go further and prevent all spaces.
  re_subject_tag = re.compile('([^:\s]*):\s*(.*)')

I ran into a similar problem with my patch I sent up earlier:

  patman: Add Cover-letter-cc: tag to Cc cover letter to people

I "fixed" that by removing the extra colon, but that was sorta lame.
We shouldn't have tags with spaces in them anyway...

  patman: Add Cover-letter-cc tag to Cc cover letter to people


-Doug

^ permalink raw reply	[flat|nested] 21+ messages in thread

* [U-Boot] [PATCH 3/8] patman: Minor help message/README fixes
  2013-03-21  2:42 ` [U-Boot] [PATCH 3/8] patman: Minor help message/README fixes Simon Glass
@ 2013-03-21 16:52   ` Doug Anderson
  0 siblings, 0 replies; 21+ messages in thread
From: Doug Anderson @ 2013-03-21 16:52 UTC (permalink / raw)
  To: u-boot

Simon,

On Wed, Mar 20, 2013 at 7:42 PM, Simon Glass <sjg@chromium.org> wrote:
> A few of the help messages are not quite right, and there is a typo
> in the README. Fix these.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
>  tools/patman/README    | 2 +-
>  tools/patman/patman.py | 4 ++--
>  2 files changed, 3 insertions(+), 3 deletions(-)

Reviewed-by: Doug Anderson <dianders@chromium.org>

^ permalink raw reply	[flat|nested] 21+ messages in thread

* [U-Boot] [PATCH 4/8] patman: Fix the comment in CheckTags to mention multiple tags
  2013-03-21  2:42 ` [U-Boot] [PATCH 4/8] patman: Fix the comment in CheckTags to mention multiple tags Simon Glass
@ 2013-03-21 16:53   ` Doug Anderson
  0 siblings, 0 replies; 21+ messages in thread
From: Doug Anderson @ 2013-03-21 16:53 UTC (permalink / raw)
  To: u-boot

Simon,

On Wed, Mar 20, 2013 at 7:42 PM, Simon Glass <sjg@chromium.org> wrote:
> This comment is less than helpful. Since multiple tags are supported, add
> an example of how multiple tags work.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
>  tools/patman/commit.py | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)

Reviewed-by: Doug Anderson <dianders@chromium.org>

^ permalink raw reply	[flat|nested] 21+ messages in thread

* [U-Boot] [PATCH 5/8] patman: Provide option to ignore bad aliases
  2013-03-21  2:42 ` [U-Boot] [PATCH 5/8] patman: Provide option to ignore bad aliases Simon Glass
@ 2013-03-21 17:09   ` Doug Anderson
  2013-03-26 22:46     ` Simon Glass
  0 siblings, 1 reply; 21+ messages in thread
From: Doug Anderson @ 2013-03-21 17:09 UTC (permalink / raw)
  To: u-boot

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 at napier.co.nz', 'm.poppins at 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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* [U-Boot] [PATCH 6/8] patman: Add -a option to refrain from test-applying the patches
  2013-03-21  2:42 ` [U-Boot] [PATCH 6/8] patman: Add -a option to refrain from test-applying the patches Simon Glass
@ 2013-03-21 17:12   ` Doug Anderson
  0 siblings, 0 replies; 21+ messages in thread
From: Doug Anderson @ 2013-03-21 17:12 UTC (permalink / raw)
  To: u-boot

Simon,

On Wed, Mar 20, 2013 at 7:42 PM, Simon Glass <sjg@chromium.org> wrote:
> Especially with the Linux kernel, it takes a long time (a minute or more)
> to test-apply the patches, so patman becomes significantly less useful.
> The only real problem that is found with this apply step is trailing spaces.
> Provide a -a option to skip this step, for those working with clean patches.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
>  tools/patman/patman.py | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)

Reviewed-by: Doug Anderson <dianders@chromium.org>

^ permalink raw reply	[flat|nested] 21+ messages in thread

* [U-Boot] [PATCH 8/8] patman: Add Series-process-log tag to sort/uniq change logs
  2013-03-21  2:43 ` [U-Boot] [PATCH 8/8] patman: Add Series-process-log tag to sort/uniq change logs Simon Glass
@ 2013-03-21 17:51   ` Doug Anderson
  2013-03-26 22:15     ` Simon Glass
  0 siblings, 1 reply; 21+ messages in thread
From: Doug Anderson @ 2013-03-21 17:51 UTC (permalink / raw)
  To: u-boot

Simon,

On Wed, Mar 20, 2013 at 7:43 PM, Simon Glass <sjg@chromium.org> wrote:
> For some series with lots of changes it is annoying that duplicate change
> log items are not caught. It is also helpful sometimes to sort the change
> logs.
>
> Add a Series-process-log tag to enable this, which can be placed in a
> commit to control this.
>
> The change to the Cc: line is to fix a checkpatch warning.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
>  tools/patman/README         | 8 +++++++-
>  tools/patman/patchstream.py | 2 +-
>  tools/patman/series.py      | 8 ++++++--
>  3 files changed, 14 insertions(+), 4 deletions(-)

Not sure I'd find this terribly useful myself, but I don't see
anything wrong with it.  I think my change log items tend to be more
than one line long for one...

> +                if not ('uniq' in process_it and text in out):

optional: My brain had a hard time processing this.  I did the logic
transformation myself:

  if 'uniq' not in process_it or text not in out:


Also: Do you really want the "process_it" to be so free-form?  That
seems like it might be asking for disaster.  Why not specify that it's
comma-separated and be done.

-Doug

^ permalink raw reply	[flat|nested] 21+ messages in thread

* [U-Boot] [PATCH 1/8] patman: Fix up checkpatch parsing to deal with 'CHECK' lines
  2013-03-21 16:35   ` Doug Anderson
@ 2013-03-26 22:12     ` Simon Glass
  0 siblings, 0 replies; 21+ messages in thread
From: Simon Glass @ 2013-03-26 22:12 UTC (permalink / raw)
  To: u-boot

Hi Doug,

On Thu, Mar 21, 2013 at 9:35 AM, Doug Anderson <dianders@chromium.org> wrote:
> Simon,
>
> On Wed, Mar 20, 2013 at 7:42 PM, Simon Glass <sjg@chromium.org> wrote:
>> checkpatch has a new type of warning, a 'CHECK'. At present patman fails
>> with these, which makes it less than useful.
>>
>> Add support for checks, making it backwards compatible with the old
>> checkpatch.
>
> There are also a few other minor fixups in this:
> * Cleanup formatting of the CheckPatches() output.
> * Fix erroneous "internal error" if multiple patches have warnings.
> * Be more robust to new types of problems.

Yes - I'll add them to the commit message.

>
>
>> @@ -64,10 +64,14 @@ def CheckPatch(fname, verbose=False):
>>                  'msg': text message
>>                  'file' : filename
>>                  'line': line number
>> +            error_count: Number of errors
>> +            warning_count: Number of warnings
>> +            check_count: Number of checks
>>              lines: Number of lines
>> +            stdout: Full output of checkpatch
>
> nit: Right above this it says you're returning a 4-tuple.  That's no
> longer true.  Could just remove the "4-".
>
> optional: I've found that returning big tuples like this is
> problematic.  Whenever you change the number of things returned you've
> got to modify any callers that call like:
>
> a, b, c, d = CheckPatch(...)
>
> to now be:
>
> a, b, c, d, e = CheckPatch(...)
>
> ...or never use the above syntax and use:
>
> result = CheckPatch(...)
> blah = result[0]
>
> Maybe use a namedtuple so that callers can use the result more cleanly?

Yes, we are well past the point of needing something like that, so will add it.

>
>
>>          if match:
>>              error_count = int(match.group(1))
>>              warning_count = int(match.group(2))
>> -            lines = int(match.group(3))
>> +            match_count = int(match.group(3))
>> +            if len(match.groups()) == 4:
>> +               check_count = match_count
>> +               lines = int(match.group(4))
>
> I'm confused about match_count here.  What is it supposed to contain?
> I can't tell from the name of it.  It looks like a temporary variable
> holding either check_count or lines.  ...but you forget to assign
> "lines = match_count" in an "else" case so things are broken with old
> versions of checkpatch, right?

Yes - we don't currently use the return value. But I will fix it.

Regards,
Simon

>
>
> -Doug

^ permalink raw reply	[flat|nested] 21+ messages in thread

* [U-Boot] [PATCH 2/8] patman: Don't look for tags inside quotes
  2013-03-21 16:50   ` Doug Anderson
@ 2013-03-26 22:14     ` Simon Glass
  0 siblings, 0 replies; 21+ messages in thread
From: Simon Glass @ 2013-03-26 22:14 UTC (permalink / raw)
  To: u-boot

Hi Doug,

On Thu, Mar 21, 2013 at 9:50 AM, Doug Anderson <dianders@chromium.org> wrote:
> Simon,
>
> On Wed, Mar 20, 2013 at 7:42 PM, Simon Glass <sjg@chromium.org> wrote:
>>  # Separates a tag: at the beginning of the subject from the rest of it
>> -re_subject_tag = re.compile('([^:]*):\s*(.*)')
>> +re_subject_tag = re.compile('([^:"]*):\s*(.*)')
>
> I'd go further and prevent all spaces.
>   re_subject_tag = re.compile('([^:\s]*):\s*(.*)')
>
> I ran into a similar problem with my patch I sent up earlier:
>
>   patman: Add Cover-letter-cc: tag to Cc cover letter to people
>
> I "fixed" that by removing the extra colon, but that was sorta lame.
> We shouldn't have tags with spaces in them anyway...
>
>   patman: Add Cover-letter-cc tag to Cc cover letter to people

Yes - will update this, thanks.

Regards,
Simon

^ permalink raw reply	[flat|nested] 21+ messages in thread

* [U-Boot] [PATCH 8/8] patman: Add Series-process-log tag to sort/uniq change logs
  2013-03-21 17:51   ` Doug Anderson
@ 2013-03-26 22:15     ` Simon Glass
  0 siblings, 0 replies; 21+ messages in thread
From: Simon Glass @ 2013-03-26 22:15 UTC (permalink / raw)
  To: u-boot

Hi Doug,

On Thu, Mar 21, 2013 at 10:51 AM, Doug Anderson <dianders@chromium.org> wrote:
> Simon,
>
> On Wed, Mar 20, 2013 at 7:43 PM, Simon Glass <sjg@chromium.org> wrote:
>> For some series with lots of changes it is annoying that duplicate change
>> log items are not caught. It is also helpful sometimes to sort the change
>> logs.
>>
>> Add a Series-process-log tag to enable this, which can be placed in a
>> commit to control this.
>>
>> The change to the Cc: line is to fix a checkpatch warning.
>>
>> Signed-off-by: Simon Glass <sjg@chromium.org>
>> ---
>>  tools/patman/README         | 8 +++++++-
>>  tools/patman/patchstream.py | 2 +-
>>  tools/patman/series.py      | 8 ++++++--
>>  3 files changed, 14 insertions(+), 4 deletions(-)
>
> Not sure I'd find this terribly useful myself, but I don't see
> anything wrong with it.  I think my change log items tend to be more
> than one line long for one...

I use it a lot. Brevity is a virtue :-)

>
>> +                if not ('uniq' in process_it and text in out):
>
> optional: My brain had a hard time processing this.  I did the logic
> transformation myself:
>
>   if 'uniq' not in process_it or text not in out:

Will update.

>
>
> Also: Do you really want the "process_it" to be so free-form?  That
> seems like it might be asking for disaster.  Why not specify that it's
> comma-separated and be done.

OK I'll add processing to check that.

Regards,
Simon

^ permalink raw reply	[flat|nested] 21+ messages in thread

* [U-Boot] [PATCH 5/8] patman: Provide option to ignore bad aliases
  2013-03-21 17:09   ` Doug Anderson
@ 2013-03-26 22:46     ` Simon Glass
  0 siblings, 0 replies; 21+ messages in thread
From: Simon Glass @ 2013-03-26 22:46 UTC (permalink / raw)
  To: u-boot

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 at napier.co.nz', 'm.poppins at 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

^ permalink raw reply	[flat|nested] 21+ messages in thread

end of thread, other threads:[~2013-03-26 22:46 UTC | newest]

Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-03-21  2:42 [U-Boot] [PATCH 0/8] Various patman fixes Simon Glass
2013-03-21  2:42 ` [U-Boot] [PATCH 1/8] patman: Fix up checkpatch parsing to deal with 'CHECK' lines Simon Glass
2013-03-21 16:35   ` Doug Anderson
2013-03-26 22:12     ` Simon Glass
2013-03-21  2:42 ` [U-Boot] [PATCH 2/8] patman: Don't look for tags inside quotes Simon Glass
2013-03-21 16:50   ` Doug Anderson
2013-03-26 22:14     ` Simon Glass
2013-03-21  2:42 ` [U-Boot] [PATCH 3/8] patman: Minor help message/README fixes Simon Glass
2013-03-21 16:52   ` Doug Anderson
2013-03-21  2:42 ` [U-Boot] [PATCH 4/8] patman: Fix the comment in CheckTags to mention multiple tags Simon Glass
2013-03-21 16:53   ` Doug Anderson
2013-03-21  2:42 ` [U-Boot] [PATCH 5/8] patman: Provide option to ignore bad aliases Simon Glass
2013-03-21 17:09   ` Doug Anderson
2013-03-26 22:46     ` Simon Glass
2013-03-21  2:42 ` [U-Boot] [PATCH 6/8] patman: Add -a option to refrain from test-applying the patches Simon Glass
2013-03-21 17:12   ` Doug Anderson
2013-03-21  2:43 ` [U-Boot] [PATCH 7/8] patman: Add Cover-letter-cc tag to Cc cover letter to people Simon Glass
2013-03-21  3:46   ` Doug Anderson
2013-03-21  2:43 ` [U-Boot] [PATCH 8/8] patman: Add Series-process-log tag to sort/uniq change logs Simon Glass
2013-03-21 17:51   ` Doug Anderson
2013-03-26 22:15     ` Simon Glass

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.