Patchwork [U-Boot,v2,1/7] patman: Fix up checkpatch parsing to deal with 'CHECK' lines

login
register
mail settings
Submitter Simon Glass
Date March 26, 2013, 11:09 p.m.
Message ID <1364339385-10035-2-git-send-email-sjg@chromium.org>
Download mbox | patch
Permalink /patch/231569/
State Accepted, archived
Delegated to: Simon Glass
Headers show

Comments

Simon Glass - March 26, 2013, 11:09 p.m.
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.

At the same time, clean up formatting of the CheckPatches() output,
fix erroneous "internal error" if multiple patches have warnings and
be more robust to new types of problems.

Signed-off-by: Simon Glass <sjg@chromium.org>
---
Changes in v2:
- Update code to remove match_count
- Update commit message to add detail on what is changing
- Update tests to check the 'checks' value, and add a new test
- Use namedtuple to hold the return value from CheckPatch() function

 tools/patman/checkpatch.py | 110 ++++++++++++++++++++++++++++-----------------
 tools/patman/test.py       |  64 +++++++++++++++++---------
 2 files changed, 112 insertions(+), 62 deletions(-)
Doug Anderson - April 1, 2013, 11:16 p.m.
Simon,

On Tue, Mar 26, 2013 at 4:09 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.
>
> At the same time, clean up formatting of the CheckPatches() output,
> fix erroneous "internal error" if multiple patches have warnings and
> be more robust to new types of problems.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
> Changes in v2:
> - Update code to remove match_count
> - Update commit message to add detail on what is changing
> - Update tests to check the 'checks' value, and add a new test
> - Use namedtuple to hold the return value from CheckPatch() function
>
>  tools/patman/checkpatch.py | 110 ++++++++++++++++++++++++++++-----------------
>  tools/patman/test.py       |  64 +++++++++++++++++---------
>  2 files changed, 112 insertions(+), 62 deletions(-)

Thanks for fixing and for fixing up the tests as well.

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

Patch

diff --git a/tools/patman/checkpatch.py b/tools/patman/checkpatch.py
index d3a0477..83aaf71 100644
--- a/tools/patman/checkpatch.py
+++ b/tools/patman/checkpatch.py
@@ -19,6 +19,7 @@ 
 # MA 02111-1307 USA
 #
 
+import collections
 import command
 import gitutil
 import os
@@ -57,63 +58,86 @@  def CheckPatch(fname, verbose=False):
     """Run checkpatch.pl on a file.
 
     Returns:
-        4-tuple containing:
-            result: False=failure, True=ok
+        namedtuple containing:
+            ok: False=failure, True=ok
             problems: List of problems, each a dict:
                 'type'; error or warning
                 'msg': text message
                 'file' : filename
                 'line': line number
+            errors: Number of errors
+            warnings: Number of warnings
+            checks: Number of checks
             lines: Number of lines
+            stdout: Full output of checkpatch
     """
-    result = False
-    error_count, warning_count, lines = 0, 0, 0
-    problems = []
+    fields = ['ok', 'problems', 'errors', 'warnings', 'checks', 'lines',
+              'stdout']
+    result = collections.namedtuple('CheckPatchResult', fields)
+    result.ok = False
+    result.errors, result.warning, result.checks = 0, 0, 0
+    result.lines = 0
+    result.problems = []
     chk = FindCheckPatch()
     item = {}
-    stdout = command.Output(chk, '--no-tree', fname)
+    result.stdout = command.Output(chk, '--no-tree', fname)
     #pipe = subprocess.Popen(cmd, stdout=subprocess.PIPE)
     #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():
+    for line in result.stdout.splitlines():
         if verbose:
             print line
 
         # A blank line indicates the end of a message
         if not line and item:
-            problems.append(item)
+            result.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))
+            result.errors = int(match.group(1))
+            result.warnings = int(match.group(2))
+            if len(match.groups()) == 4:
+                result.checks = int(match.group(3))
+                result.lines = int(match.group(4))
+            else:
+                result.lines = int(match.group(3))
         elif re_ok.match(line):
-            result = True
+            result.ok = True
         elif re_bad.match(line):
-            result = False
-        match = re_error.match(line)
-        if match:
-            item['msg'] = match.group(1)
+            result.ok = False
+        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
 
 def GetWarningMsg(col, msg_type, fname, line, msg):
     '''Create a message for a given file/line
@@ -128,37 +152,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)
-        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:
+        result = CheckPatch(fname, verbose)
+        if not result.ok:
+            error_count += result.errors
+            warning_count += result.warnings
+            check_count += result.checks
+            print '%d errors, %d warnings, %d checks for %s:' % (result.errors,
+                    result.warnings, result.checks, col.Color(col.BLUE, fname))
+            if (len(result.problems) != result.errors + result.warnings +
+                    result.checks):
                 print "Internal error: some problems lost"
-            for item in problems:
-                print GetWarningMsg(col, item['type'],
+            for item in result.problems:
+                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..8cd2647 100644
--- a/tools/patman/test.py
+++ b/tools/patman/test.py
@@ -190,6 +190,11 @@  index 0000000..2234c87
 +		rec->time_us = (uint32_t)timer_get_us();
 +		rec->name = name;
 +	}
++	if (!rec->name &&
++	%ssomething_else) {
++		rec->time_us = (uint32_t)timer_get_us();
++		rec->name = name;
++	}
 +%sreturn rec->time_us;
 +}
 --
@@ -197,15 +202,18 @@  index 0000000..2234c87
 '''
         signoff = 'Signed-off-by: Simon Glass <sjg@chromium.org>\n'
         tab = '	'
+        indent = '    '
         if data_type == 'good':
             pass
         elif data_type == 'no-signoff':
             signoff = ''
         elif data_type == 'spaces':
             tab = '   '
+        elif data_type == 'indent':
+            indent = tab
         else:
             print 'not implemented'
-        return data % (signoff, tab, tab)
+        return data % (signoff, tab, indent, tab)
 
     def SetupData(self, data_type):
         inhandle, inname = tempfile.mkstemp()
@@ -215,33 +223,49 @@  index 0000000..2234c87
         infd.close()
         return inname
 
-    def testCheckpatch(self):
+    def testGood(self):
         """Test checkpatch operation"""
         inf = self.SetupData('good')
-        result, problems, err, warn, lines, stdout = checkpatch.CheckPatch(inf)
-        self.assertEqual(result, True)
-        self.assertEqual(problems, [])
-        self.assertEqual(err, 0)
-        self.assertEqual(warn, 0)
-        self.assertEqual(lines, 67)
+        result = checkpatch.CheckPatch(inf)
+        self.assertEqual(result.ok, True)
+        self.assertEqual(result.problems, [])
+        self.assertEqual(result.errors, 0)
+        self.assertEqual(result.warnings, 0)
+        self.assertEqual(result.checks, 0)
+        self.assertEqual(result.lines, 67)
         os.remove(inf)
 
+    def testNoSignoff(self):
         inf = self.SetupData('no-signoff')
-        result, problems, err, warn, lines, stdout = checkpatch.CheckPatch(inf)
-        self.assertEqual(result, False)
-        self.assertEqual(len(problems), 1)
-        self.assertEqual(err, 1)
-        self.assertEqual(warn, 0)
-        self.assertEqual(lines, 67)
+        result = checkpatch.CheckPatch(inf)
+        self.assertEqual(result.ok, False)
+        self.assertEqual(len(result.problems), 1)
+        self.assertEqual(result.errors, 1)
+        self.assertEqual(result.warnings, 0)
+        self.assertEqual(result.checks, 0)
+        self.assertEqual(result.lines, 67)
         os.remove(inf)
 
+    def testSpaces(self):
         inf = self.SetupData('spaces')
-        result, problems, err, warn, lines, stdout = checkpatch.CheckPatch(inf)
-        self.assertEqual(result, False)
-        self.assertEqual(len(problems), 2)
-        self.assertEqual(err, 0)
-        self.assertEqual(warn, 2)
-        self.assertEqual(lines, 67)
+        result = checkpatch.CheckPatch(inf)
+        self.assertEqual(result.ok, False)
+        self.assertEqual(len(result.problems), 1)
+        self.assertEqual(result.errors, 0)
+        self.assertEqual(result.warnings, 1)
+        self.assertEqual(result.checks, 0)
+        self.assertEqual(result.lines, 67)
+        os.remove(inf)
+
+    def testIndent(self):
+        inf = self.SetupData('indent')
+        result = checkpatch.CheckPatch(inf)
+        self.assertEqual(result.ok, False)
+        self.assertEqual(len(result.problems), 1)
+        self.assertEqual(result.errors, 0)
+        self.assertEqual(result.warnings, 0)
+        self.assertEqual(result.checks, 1)
+        self.assertEqual(result.lines, 67)
         os.remove(inf)