Re: [PATCH v7] BaseTools/Scripts/PatchCheck.py: Extended patch style check for c code

[Jordan Justen <jordan.l.justen@intel.com>]

On 2016-12-19 00:31:13, Gao, Liming wrote:
> Daniil:
>   This version has addressed my comments. I see Jordan also provides
>   some comments. Are they handled in this patch?
>

No, they were not addressed. Here are my replies:

v5: https://lists.01.org/pipermail/edk2-devel/2016-December/005779.html

v6: https://lists.01.org/pipermail/edk2-devel/2016-December/005866.html

>
> > -----Original Message-----
> > From: Daniil Egranov [mailto:daniil.egranov@arm.com]
> > Sent: Saturday, December 17, 2016 7:11 AM
> > To: edk2-devel@lists.01.org
> > Cc: leif.lindholm@linaro.org; Gao, Liming <liming.gao@intel.com>; Daniil
> > Egranov <daniil.egranov@arm.com>
> > Subject: [PATCH v7] BaseTools/Scripts/PatchCheck.py: Extended patch style
> > check for c code
> > 
> > Changed output format to make it more comapct.
> > Disabled colored output by default. For terminals which support ANSI
> > escape codes, colored output can be enabled with the "--color" command
> > line parameter.
> > 
> > Contributed-under: TianoCore Contribution Agreement 1.0
> > Signed-off-by: Daniil Egranov <daniil.egranov@arm.com>
> > ---
> > Changelog:
> > 
> > v6
> > Added reporting a source code line number in case of error when
> > checking a patch file or using a git sha1 commit string. In case of
> > a patch file, both patch and source code line numbers will be provided.
> > Corrected handling the case with #define foo(..) macro. The absence of
> > the space before open parenthesis is not reported as the error anymore.
> > Added checking for lower case "void" and "static" words.
> > Added colors for error messages (experimental?)
> > 
> > v5
> > Corrected code checking for multi-line and commented lines. Both
> > multi-line and open comment flags will be reset when leaving
> > diff "+" area of the patch.
> > Changed version of the tool to 0.2.
> > 
> > v4
> > Corrected maximum code line size to 120 characters.
> > 
> > v3
> > Corrected space detection before parentheses.
> > 
> > v2:
> > Fixed several indentation cases
> > 
> > v1:
> > Fixed reporting signature error for a cover letter.
> > Fixed reporting line size error for a file change information
> > included in the commit message.
> > Fixed line number reported in PatchCheck error messages. It
> > points to the correct line in the diff file.
> > The patch extends style checking for c code:
> >  Added check for code indentation.
> >  Added report line size greater than 80 characters.
> >  Added checking for space before '('.
> >  Added checking for space before '{'.
> >  Added checking for '}' to be on a new line and have spaces
> >  for "} else {" or "} while ()" cases.
> > 
> >  BaseTools/Scripts/PatchCheck.py | 379
> > ++++++++++++++++++++++++++++++++++++----
> >  1 file changed, 341 insertions(+), 38 deletions(-)
> > 
> > diff --git a/BaseTools/Scripts/PatchCheck.py
> > b/BaseTools/Scripts/PatchCheck.py
> > index 7c30082..eae084c 100755
> > --- a/BaseTools/Scripts/PatchCheck.py
> > +++ b/BaseTools/Scripts/PatchCheck.py
> > @@ -15,7 +15,7 @@
> > 
> >  from __future__ import print_function
> > 
> > -VersionNumber = '0.1'
> > +VersionNumber = '0.2'
> >  __copyright__ = "Copyright (c) 2015 - 2016, Intel Corporation  All rights
> > reserved."
> > 
> >  import email
> > @@ -25,6 +25,44 @@ import re
> >  import subprocess
> >  import sys
> > 
> > +class MsgFormat:
> > +    ERROR, WARNING, REPORT, LINK, NORMAL = range(5)
> > +    color_on = False
> > +
> > +class ColorTypes:
> > +    RED = '\033[91m'
> > +    GREEN = '\033[92m'
> > +    BLUE = '\033[94m'
> > +    CYAN = '\033[96m'
> > +    WHITE = '\033[97m'
> > +    YELLOW = '\033[93m'
> > +    MAGENTA = '\033[95m'
> > +    GREY = '\033[90m'
> > +    BLACK = '\033[90m'
> > +    DEFAULT = '\033[0m'
> > +
> > +class TextColor:
> > +    def __init__(self, color_on, msg_format):
> > +        self.color = ''
> > +        if color_on == True:
> > +            if msg_format == MsgFormat.ERROR:
> > +                self.color = ColorTypes.RED
> > +            elif msg_format == MsgFormat.WARNING:
> > +                self.color = ColorTypes.YELLOW
> > +            elif msg_format == MsgFormat.REPORT:
> > +                self.color = ColorTypes.CYAN
> > +            elif msg_format == MsgFormat.LINK:
> > +                self.color = ColorTypes.BLUE
> > +            elif msg_format == MsgFormat.NORMAL:
> > +                self.color = ColorTypes.DEFAULT
> > +            else:
> > +                self.color = ColorTypes.DEFAULT
> > +        else:
> > +            self.color = ''
> > +
> > +    def __str__(self):
> > +        return self.color
> > +
> >  class Verbose:
> >      SILENT, ONELINE, NORMAL = range(3)
> >      level = NORMAL
> > @@ -32,7 +70,7 @@ class Verbose:
> >  class CommitMessageCheck:
> >      """Checks the contents of a git commit message."""
> > 
> > -    def __init__(self, subject, message):
> > +    def __init__(self, subject, message, message_offset, cover):
> >          self.ok = True
> > 
> >          if subject is None and  message is None:
> > @@ -41,9 +79,15 @@ class CommitMessageCheck:
> > 
> >          self.subject = subject
> >          self.msg = message
> > +        self.msg_offset = message_offset
> > +        self.cover = cover
> > +
> > +        if not cover:
> > +            self.check_contributed_under()
> > +            self.check_signed_off_by()
> > +        else:
> > +            print('The commit message is cover letter.')
> > 
> > -        self.check_contributed_under()
> > -        self.check_signed_off_by()
> >          self.check_misc_signatures()
> >          self.check_overall_format()
> >          self.report_message_result()
> > @@ -60,9 +104,11 @@ class CommitMessageCheck:
> >          else:
> >              return_code = 1
> >          if not self.ok:
> > -            print(self.url)
> > +            print('\n' + self.url)
> > 
> >      def error(self, *err):
> > +        start = TextColor(MsgFormat.color_on, MsgFormat.ERROR)
> > +        end = TextColor(MsgFormat.color_on, MsgFormat.NORMAL)
> >          if self.ok and Verbose.level > Verbose.ONELINE:
> >              print('The commit message format is not valid:')
> >          self.ok = False
> > @@ -71,7 +117,7 @@ class CommitMessageCheck:
> >          count = 0
> >          for line in err:
> >              prefix = (' *', '  ')[count > 0]
> > -            print(prefix, line)
> > +            print(start, prefix, line, end)
> >              count += 1
> > 
> >      def check_contributed_under(self):
> > @@ -180,7 +226,12 @@ class CommitMessageCheck:
> >          for sig in self.sig_types:
> >              self.find_signatures(sig)
> > 
> > +    diff_change_info_re = \
> > +        re.compile(r'.*\|\s+(\d|Bin)*\s.*[\+\-]')
> > +
> >      def check_overall_format(self):
> > +        start = TextColor(MsgFormat.color_on, MsgFormat.REPORT)
> > +        end = TextColor(MsgFormat.color_on, MsgFormat.NORMAL)
> >          lines = self.msg.splitlines()
> > 
> >          if len(lines) >= 1 and lines[0].endswith('\r\n'):
> > @@ -197,9 +248,10 @@ class CommitMessageCheck:
> >              self.error('Empty commit message!')
> >              return
> > 
> > -        if count >= 1 and len(lines[0]) >= 72:
> > +        if count >= 1 and len(lines[0]) > 72:
> >              self.error('First line of commit message (subject line) ' +
> > -                       'is too long.')
> > +                       'is too long (%d) (max 72 characters): ' %(len(lines[0])) +
> > +                       str(start) + lines[0] + str(end))
> > 
> >          if count >= 1 and len(lines[0].strip()) == 0:
> >              self.error('First line of commit message (subject line) ' +
> > @@ -210,10 +262,13 @@ class CommitMessageCheck:
> >                         'empty.')
> > 
> >          for i in range(2, count):
> > -            if (len(lines[i]) >= 76 and
> > +            if (len(lines[i]) > 76 and
> >                  len(lines[i].split()) > 1 and
> > -                not lines[i].startswith('git-svn-id:')):
> > -                self.error('Line %d of commit message is too long.' % (i + 1))
> > +                not lines[i].startswith('git-svn-id:') and
> > +                self.diff_change_info_re.search(lines[i]) is None):
> > +                self.error('Line %d of commit message is too long (%d) (max 76
> > characters): ' \
> > +                           % (i + self.msg_offset - 1, len(lines[i])) +
> > +                           str(start) + lines[i] + str(end))
> > 
> >          last_sig_line = None
> >          for i in range(count - 1, 0, -1):
> > @@ -236,13 +291,21 @@ class CommitMessageCheck:
> >  class GitDiffCheck:
> >      """Checks the contents of a git diff."""
> > 
> > -    def __init__(self, diff):
> > +    def __init__(self, diff, offset, patch_type):
> >          self.ok = True
> >          self.format_ok = True
> >          self.lines = diff.splitlines(True)
> >          self.count = len(self.lines)
> >          self.line_num = 0
> >          self.state = START
> > +        self.comments_open = False
> > +        self.multiline_string = False
> > +        self.current_indent_size = 0
> > +        self.parentheses_count = 0
> > +        self.offset = offset
> > +        self.file_type = "other_type"
> > +        self.patch_type = patch_type
> > +        self.src_line_num = 0;
> >          while self.line_num < self.count and self.format_ok:
> >              line_num = self.line_num
> >              self.run()
> > @@ -255,6 +318,14 @@ class GitDiffCheck:
> >          if self.ok:
> >              print('The code passed all checks.')
> > 
> > +    def clean_counts(self):
> > +        self.current_indent_size = -1
> > +        self.parentheses_count = 0
> > +        self.comments_open = False
> > +        self.multiline_string = False
> > +
> > +    diff_src_line_start_re = re.compile(r'(\+\d+)')
> > +
> >      def run(self):
> >          line = self.lines[self.line_num]
> > 
> > @@ -281,9 +352,28 @@ class GitDiffCheck:
> >                  self.format_error("didn't find diff command")
> >              self.line_num += 1
> >          elif self.state == PRE_PATCH:
> > -            if line.startswith('+++ b/'):
> > +            if line.startswith('+++ '):
> >                  self.set_filename(line[6:].rstrip())
> > +                print("\nChecking patch for %s ..." %(self.hunk_filename))
> > +                if self.hunk_filename.endswith(".c") or \
> > +                   self.hunk_filename.endswith(".h"):
> > +                    self.file_type = "c_type"
> > +                elif  self.hunk_filename.endswith(".dec") or \
> > +                      self.hunk_filename.endswith(".dsc") or \
> > +                      self.hunk_filename.endswith(".fdf"):
> > +                    self.file_type = "edk2_type"
> > +                elif  self.hunk_filename.endswith(".S"):
> > +                    self.file_type = "asm_arm_type"
> > +                elif  self.hunk_filename.endswith(".asm"):
> > +                    self.file_type = "asm_type"
> > +                else:
> > +                    self.file_type = "other_type"
> >              if line.startswith('@@ '):
> > +                sls = self.diff_src_line_start_re.search(line)
> > +                if sls is not None:
> > +                    self.src_line_num = int(sls.group(0))
> > +                else:
> > +                    self.src_line_num = 0
> >                  self.state = PATCH
> >                  self.binary = False
> >              elif line.startswith('GIT binary patch'):
> > @@ -299,15 +389,25 @@ class GitDiffCheck:
> >              self.line_num += 1
> >          elif self.state == PATCH:
> >              if self.binary:
> > +                self.clean_counts()
> >                  pass
> >              if line.startswith('-'):
> > +                self.clean_counts()
> >                  pass
> >              elif line.startswith('+'):
> > -                self.check_added_line(line[1:])
> > +                # indentation resets every time we leave "+" diff area
> > +                self.check_added_line(line[1:], self.line_num + self.offset,
> > +                                      self.src_line_num)
> > +                self.src_line_num +=1
> >              elif line.startswith(r'\ No newline '):
> > +                self.clean_counts()
> >                  pass
> >              elif not line.startswith(' '):
> > +                self.clean_counts()
> >                  self.format_error("unexpected patch line")
> > +            else:
> > +                self.src_line_num +=1
> > +                self.clean_counts()
> >              self.line_num += 1
> > 
> >      pre_patch_prefixes = (
> > @@ -332,13 +432,19 @@ class GitDiffCheck:
> >          else:
> >              self.force_crlf = True
> > 
> > -    def added_line_error(self, msg, line):
> > +    def added_line_error(self, msg, line, diff_line_num, src_line_num,
> > patch_type):
> > +        start = TextColor(MsgFormat.color_on, MsgFormat.REPORT)
> > +        end = TextColor(MsgFormat.color_on, MsgFormat.NORMAL)
> >          lines = [ msg ]
> > -        if self.hunk_filename is not None:
> > -            lines.append('File: ' + self.hunk_filename)
> > -        lines.append('Line: ' + line)
> > -
> >          self.error(*lines)
> > +        if self.hunk_filename is not None:
> > +            print('File: ' + self.hunk_filename)
> > +        line_info = 'Line '
> > +        if patch_type == 'patch_file_type':
> > +            line_info += ('%d(patch) ' %(diff_line_num))
> > +        line_info += ('%d(source) ' %(src_line_num))
> > +        line_info += (': ' +str(start) + line.lstrip() + str(end))
> > +        print(line_info)
> > 
> >      old_debug_re = \
> >          re.compile(r'''
> > @@ -348,28 +454,195 @@ class GitDiffCheck:
> >                     ''',
> >                     re.VERBOSE)
> > 
> > -    def check_added_line(self, line):
> > +    space_check_parentheses_re = re.compile(r'#define\s*[a-zA-Z0-
> > 9_]\S*\(|[a-zA-Z0-9_]\S*\((?<!\)\()(?<!\(\()')
> > +    space_check_brace_open_re = re.compile(r'[a-zA-Z0-9_]\S*\{')
> > +    space_check_brace_close_re = re.compile(r'[\}]\S.*')
> > +    brace_open_check_re = re.compile(r'[a-zA-Z0-9_]*\{')
> > +    brace_close_check_re = re.compile(r'.*\}.*')
> > +    multiline_check_re = re.compile(r'[a-zA-Z0-9_]*(\\|\&|\,|\+|\-
> > |\*|\||\=)\Z')
> > +    void_cap_check_re = re.compile(r'(\s|\b|\()void(\s+|\*|\,|\))')
> > +    static_cap_check_re = re.compile(r'(\s|\b)static\s+')
> > +
> > +    def check_added_line(self, line, diff_line_num, src_line_num):
> >          eol = ''
> >          for an_eol in self.line_endings:
> >              if line.endswith(an_eol):
> >                  eol = an_eol
> >                  line = line[:-len(eol)]
> > 
> > -        stripped = line.rstrip()
> > +        rstripped = line.rstrip()
> > +        lstripped = line.lstrip()
> > 
> >          if self.force_crlf and eol != '\r\n':
> >              self.added_line_error('Line ending (%s) is not CRLF' % repr(eol),
> > -                                  line)
> > +                                  line, diff_line_num, src_line_num, self.patch_type)
> >          if '    ' in line:
> > -            self.added_line_error('Tab character used', line)
> > -        if len(stripped) < len(line):
> > -            self.added_line_error('Trailing whitespace found', line)
> > +            self.added_line_error('Tab character used',
> > +                                  line, diff_line_num, src_line_num, self.patch_type)
> > +        if len(rstripped) < len(line):
> > +            self.added_line_error('Trailing whitespace found',
> > +                                  line, diff_line_num, src_line_num, self.patch_type)
> > +
> > +        # empty "+" line, skip it
> > +        if len(line) == 0:
> > +            return
> > 
> > -        mo = self.old_debug_re.search(line)
> > -        if mo is not None:
> > -            self.added_line_error('EFI_D_' + mo.group(1) + ' was used, '
> > -                                  'but DEBUG_' + mo.group(1) +
> > -                                  ' is now recommended', line)
> > +        # the file type specific checks
> > +        if self.file_type == "c_type":
> > +
> > +            # set initial indentation as the number of spaces after the first "+"
> > +            # indentation does not apply on lines with 0 characters
> > +            # adjust the indentation if first "+" has "}"
> > +            if self.current_indent_size == -1:
> > +                if len(lstripped) != 0:
> > +                    self.current_indent_size = len(line) - len(lstripped)
> > +                else:
> > +                    self.clean_counts()
> > +
> > +                if lstripped.startswith("}"):
> > +                     self.current_indent_size = line.find("}") + 2
> > +
> > +            indent_size = len(line) - len(lstripped)
> > +            indent_size_adjust = 0
> > +            force_brace_check = False
> > +
> > +            if len(line) > 120:
> > +                self.added_line_error('Line is too long (%d) (max 120 characters)',
> > +                                      line, diff_line_num, src_line_num, self.patch_type)
> > +
> > +            # skip comments for code checking
> > +            if lstripped.startswith("//"):
> > +                return
> > +            if lstripped.startswith("/*") and not self.comments_open:
> > +                self.comments_open = True
> > +
> > +            if rstripped.endswith("*/") and self.comments_open:
> > +                self.comments_open = False
> > +                return
> > +
> > +            if rstripped.endswith("*/") and not self.comments_open:
> > +                # found unknown close comment, reset the indentation as
> > +                # no idea where it started
> > +                self.current_indent_size = -1
> > +                return
> > +
> > +            if not self.comments_open:
> > +                mo = self.old_debug_re.search(line)
> > +                if mo is not None:
> > +                    self.added_line_error('EFI_D_' + mo.group(1) + ' was used, '
> > +                                          'but DEBUG_' + mo.group(1) +
> > +                                          ' is now recommended',
> > +                                          line, diff_line_num, src_line_num, self.patch_type)
> > +
> > +                # track down multiple braces in the same line
> > +                braces_count = line.count('{') - line.count('}')
> > +
> > +                # check if line has text in double quotes as we do not
> > +                # need to check text messages
> > +                text_str_start = line.find('\"')
> > +
> > +                # track down a special case with "} else ..{" braces
> > +                if line.find("else") != -1:
> > +                    force_brace_check = True
> > +
> > +                # check for closing brace which affects indentation
> > +                if braces_count < 0 and not force_brace_check:
> > +                    self.current_indent_size -= 2 * abs(braces_count)
> > +                elif force_brace_check:
> > +                    if line.find("}") != -1:
> > +                        self.current_indent_size -= 2
> > +                        braces_count = 1
> > +
> > +                # for the switch()/case:, adjust "case:" and "default:" indentation
> > +                # as they aligned with the "switch()" indentation
> > +                if lstripped.startswith("case ") or \
> > +                    lstripped.startswith("default:"):
> > +                    indent_size_adjust = -2
> > +
> > +                # indentation code check related to the diff structure
> > +                # do not check code indentation between parentheses
> > +                if indent_size != self.current_indent_size + indent_size_adjust \
> > +                    and len(line) != 0 and self.parentheses_count == 0 \
> > +                    and not self.multiline_string:
> > +                        self.added_line_error('Invalid indentation. Expecting %d but
> > found %d spaces' \
> > +                                              %(self.current_indent_size + indent_size_adjust, \
> > +                                              indent_size),
> > +                                              line, diff_line_num, src_line_num, self.patch_type)
> > +
> > +                # check for missing space before open brace
> > +                if braces_count > 0 or force_brace_check:
> > +                    if line.find("{") != -1:
> > +                        self.current_indent_size += 2 * braces_count
> > +                    if not line.startswith("{"):
> > +                        spb_check = self.space_check_brace_open_re.search(line)
> > +                        if spb_check is not None:
> > +                            self.added_line_error('Missing space before \'{\'',
> > +                                                  line, diff_line_num, src_line_num, self.patch_type)
> > +
> > +                # check for missing space before open parenthesis
> > +                spo = self.space_check_parentheses_re.finditer(line)
> > +                if spo is not None and not self.multiline_string:
> > +                    # check if it's not inside of the double quoted string or
> > +                    # (aa)(bb)fff case or parenthesis belong to #define macro
> > +                    for parstr in spo:
> > +                        if not (text_str_start != -1 and text_str_start < parstr.start())
> > and \
> > +                           not parstr.group(0).startswith("#define"):
> > +                            self.added_line_error('Missing space before \'(\'',
> > +                                                  line, diff_line_num, src_line_num, self.patch_type)
> > +                            break;
> > +
> > +                # check for closing brace cases
> > +                if braces_count < 0 or \
> > +                    (force_brace_check and line.find("}") != -1):
> > +                    if not lstripped.startswith("}") and rstripped.endswith("}"):
> > +                        self.added_line_error('The \'}\' is not on its own line',
> > +                                              line, diff_line_num, src_line_num, self.patch_type)
> > +                    if not lstripped.endswith("}"):
> > +                        scb = self.space_check_brace_close_re.search(lstripped)
> > +                        if scb is not None \
> > +                            and lstripped[scb.start() + 1] != ';' \
> > +                            and lstripped[scb.start() + 1] != ',' \
> > +                            and lstripped[scb.start() + 1] != '}':
> > +                            self.added_line_error('Missing space after \'}\'',
> > +                                                  line, diff_line_num, src_line_num, self.patch_type)
> > +
> > +                # check for using "void" instead of "VOID"
> > +                vcc = self.void_cap_check_re.search(line)
> > +                if vcc is not None and \
> > +                    not (text_str_start != -1 and text_str_start < vcc.start()):
> > +                    self.added_line_error('Using \"void\" instead of \"VOID\"',
> > +                                          line, diff_line_num, src_line_num, self.patch_type)
> > +
> > +                #check for using "static" instead of "STATIC"
> > +                scc = self.static_cap_check_re.search(line)
> > +                if scc is not None and \
> > +                    not (text_str_start != -1 and text_str_start < scc.start()):
> > +                    self.added_line_error('Using \"static\" instead of \"STATIC\"',
> > +                                          line, diff_line_num, src_line_num, self.patch_type)
> > +
> > +                # track down parentheses, no indentation check between them
> > +                if not (self.parentheses_count == 0 and lstripped.startswith(')') \
> > +                        and not self.multiline_string and not self.comments_open):
> > +                    self.parentheses_count += line.count('(') - line.count(')')
> > +
> > +                # multiline strings indentation is unknown
> > +                if not self.comments_open:
> > +                    mlc = self.multiline_check_re.search(line)
> > +                    if mlc is not None:
> > +                        self.multiline_string = True
> > +                    else:
> > +                        self.multiline_string = False
> > +
> > +        elif self.file_type == "edk2_type":
> > +            pass
> > +        elif self.file_type == "asm_arm_type":
> > +            pass
> > +        elif self.file_type == "asm_type":
> > +            pass
> > +        elif self.file_type == "other_type":
> > +            pass
> > +        else:
> > +            pass
> > 
> >      split_diff_re = re.compile(r'''
> >                                     (?P<cmd>
> > @@ -388,6 +661,8 @@ class GitDiffCheck:
> >          self.error(err, err2)
> > 
> >      def error(self, *err):
> > +        start = TextColor(MsgFormat.color_on, MsgFormat.ERROR)
> > +        end = TextColor(MsgFormat.color_on, MsgFormat.NORMAL)
> >          if self.ok and Verbose.level > Verbose.ONELINE:
> >              print('Code format is not valid:')
> >          self.ok = False
> > @@ -396,7 +671,7 @@ class GitDiffCheck:
> >          count = 0
> >          for line in err:
> >              prefix = (' *', '  ')[count > 0]
> > -            print(prefix, line)
> > +            print(start, prefix, line, end)
> >              count += 1
> > 
> >  class CheckOnePatch:
> > @@ -406,16 +681,17 @@ class CheckOnePatch:
> >      patch content.
> >      """
> > 
> > -    def __init__(self, name, patch):
> > +    def __init__(self, name, patch, patch_type):
> >          self.patch = patch
> >          self.find_patch_pieces()
> > 
> > -        msg_check = CommitMessageCheck(self.commit_subject,
> > self.commit_msg)
> > +        msg_check = CommitMessageCheck(self.commit_subject,
> > self.commit_msg, \
> > +                                       self.commit_msg_offset, self.cover)
> >          msg_ok = msg_check.ok
> > 
> >          diff_ok = True
> >          if self.diff is not None:
> > -            diff_check = GitDiffCheck(self.diff)
> > +            diff_check = GitDiffCheck(self.diff, self.diff_offset, patch_type)
> >              diff_ok = diff_check.ok
> > 
> >          self.ok = msg_ok and diff_ok
> > @@ -458,6 +734,16 @@ class CheckOnePatch:
> >                     ''',
> >                     re.VERBOSE)
> > 
> > +    cover_letter_re = re.compile(r'Subject:\s*\[PATCH.*.0/.*',
> > +                                  re.IGNORECASE | re.MULTILINE)
> > +
> > +    def is_cover_letter(self, patch):
> > +        cl = self.cover_letter_re.search(patch)
> > +        if cl is None:
> > +            return False
> > +
> > +        return True
> > +
> >      def find_patch_pieces(self):
> >          if sys.version_info < (3, 0):
> >              patch = self.patch.encode('ascii', 'ignore')
> > @@ -465,10 +751,13 @@ class CheckOnePatch:
> >              patch = self.patch
> > 
> >          self.commit_msg = None
> > +        self.commit_msg_offset = 0
> >          self.stat = None
> >          self.commit_subject = None
> >          self.commit_prefix = None
> >          self.diff = None
> > +        self.diff_offset = 0
> > +        self.cover = False
> > 
> >          if patch.startswith('diff --git'):
> >              self.diff = patch
> > @@ -480,6 +769,14 @@ class CheckOnePatch:
> >          assert(parts[0].get_content_type() == 'text/plain')
> >          content = parts[0].get_payload(decode=True).decode('utf-8', 'ignore')
> > 
> > +        # find offset of content
> > +        self.commit_msg_offset = len(patch.splitlines()) -
> > len(content.splitlines())
> > +
> > +        # find offset of first diff section
> > +        mo = self.git_diff_re.search(patch)
> > +        if mo is not None:
> > +            self.diff_offset = len(patch[:mo.start()].splitlines()) + 1
> > +
> >          mo = self.git_diff_re.search(content)
> >          if mo is not None:
> >              self.diff = content[mo.start():]
> > @@ -492,6 +789,7 @@ class CheckOnePatch:
> >              self.stat = mo.group('stat')
> >              self.commit_msg = mo.group('commit_message')
> > 
> > +        self.cover = self.is_cover_letter(patch)
> >          self.commit_subject = pmail['subject'].replace('\r\n', '')
> >          self.commit_subject = self.commit_subject.replace('\n', '')
> >          self.commit_subject = self.subject_prefix_re.sub('',
> > self.commit_subject, 1)
> > @@ -514,9 +812,9 @@ class CheckGitCommits:
> >                      print()
> >                  else:
> >                      blank_line = True
> > -                print('Checking git commit:', commit)
> > +                print('Checking git commit:', commit, '\n')
> >              patch = self.read_patch_from_git(commit)
> > -            self.ok &= CheckOnePatch(commit, patch).ok
> > +            self.ok &= CheckOnePatch(commit, patch, "git_commit_type").ok
> > 
> >      def read_commit_list_from_git(self, rev_spec, max_count):
> >          # Run git to get the commit patch
> > @@ -554,8 +852,8 @@ class CheckOnePatchFile:
> >              patch = f.read().decode('utf-8', 'ignore')
> >              f.close()
> >          if Verbose.level > Verbose.ONELINE:
> > -            print('Checking patch file:', patch_filename)
> > -        self.ok = CheckOnePatch(patch_filename, patch).ok
> > +            print('Checking patch file:', patch_filename, '\n')
> > +        self.ok = CheckOnePatch(patch_filename, patch, "patch_file_type").ok
> > 
> >  class CheckOneArg:
> >      """Performs a patch check for a single command line argument.
> > @@ -618,11 +916,16 @@ class PatchCheckApp:
> >          group.add_argument("--silent",
> >                             action="store_true",
> >                             help="Print nothing")
> > +        group.add_argument("--color",
> > +                           action="store_true",
> > +                           help="Enable color messages")
> >          self.args = parser.parse_args()
> >          if self.args.oneline:
> >              Verbose.level = Verbose.ONELINE
> >          if self.args.silent:
> >              Verbose.level = Verbose.SILENT
> > +        if self.args.color:
> > +            MsgFormat.color_on = True
> > 
> >  if __name__ == "__main__":
> >      sys.exit(PatchCheckApp().retval)
> > --
> > 2.7.4
>