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

* [PATCH] BaseTools/Scripts/PatchCheck.py: Extended patch style check for c code
@ 2016-12-09  5:35 Daniil Egranov
  2016-12-12  3:09 ` Gao, Liming
  0 siblings, 1 reply; 2+ messages in thread
From: Daniil Egranov @ 2016-12-09  5:35 UTC (permalink / raw)
  To: edk2-devel; +Cc: leif.lindholm, Daniil Egranov

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.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Daniil Egranov <daniil.egranov@arm.com>
---
 BaseTools/Scripts/PatchCheck.py | 245 ++++++++++++++++++++++++++++++++++++----
 1 file changed, 221 insertions(+), 24 deletions(-)

diff --git a/BaseTools/Scripts/PatchCheck.py b/BaseTools/Scripts/PatchCheck.py
index 7c30082..2074bea 100755
--- a/BaseTools/Scripts/PatchCheck.py
+++ b/BaseTools/Scripts/PatchCheck.py
@@ -32,7 +32,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 +41,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()
@@ -180,6 +186,9 @@ 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):
         lines = self.msg.splitlines()
 
@@ -197,9 +206,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])))
+            print(lines[0], '\n')
 
         if count >= 1 and len(lines[0].strip()) == 0:
             self.error('First line of commit message (subject line) ' +
@@ -210,10 +220,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])))
+                print(lines[i], '\n')
 
         last_sig_line = None
         for i in range(count - 1, 0, -1):
@@ -236,13 +249,19 @@ class CommitMessageCheck:
 class GitDiffCheck:
     """Checks the contents of a git diff."""
 
-    def __init__(self, diff):
+    def __init__(self, diff, offset):
         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.dquote_open = False
+        self.multiline_string = False
+        self.current_indent_size = 0
+        self.parentheses_count = 0
+        self.offset = offset
         while self.line_num < self.count and self.format_ok:
             line_num = self.line_num
             self.run()
@@ -255,6 +274,10 @@ class GitDiffCheck:
         if self.ok:
             print('The code passed all checks.')
 
+    def clean_counts(self):
+        self.current_indent_size = -1
+        self.parentheses_count = 0
+
     def run(self):
         line = self.lines[self.line_num]
 
@@ -283,6 +306,20 @@ class GitDiffCheck:
         elif self.state == PRE_PATCH:
             if line.startswith('+++ b/'):
                 self.set_filename(line[6:].rstrip())
+                print("Checking patch for %s ...\n" %(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('@@ '):
                 self.state = PATCH
                 self.binary = False
@@ -296,18 +333,27 @@ class GitDiffCheck:
                         ok = True
                 if not ok:
                     self.format_error("didn't find diff hunk marker (@@)")
+
             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)
             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.clean_counts()
+
             self.line_num += 1
 
     pre_patch_prefixes = (
@@ -336,7 +382,7 @@ class GitDiffCheck:
         lines = [ msg ]
         if self.hunk_filename is not None:
             lines.append('File: ' + self.hunk_filename)
-        lines.append('Line: ' + line)
+        lines.append('Line: ' + line + '\n')
 
         self.error(*lines)
 
@@ -348,28 +394,156 @@ class GitDiffCheck:
                    ''',
                    re.VERBOSE)
 
-    def check_added_line(self, line):
+    space_check_parentheses_re = re.compile(r'[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'[a-zA-Z0-9_]*\}')
+    multiline_check_re = re.compile(r'[a-zA-Z0-9_]*\\\Z')
+
+    def check_added_line(self, line, 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()
+        # empty "+" line, skip it
+        if len(line) == 0:
+            return
+
+        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),
+            self.added_line_error('Line ending (%s) is not CRLF:' % repr(eol),
                                   line)
         if '\t' 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 at line %d:'\
+                                  %(line_num), line)
+        if len(rstripped) < len(line):
+            self.added_line_error('Trailing whitespace found at line %d:'\
+                                  %(line_num), 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()
 
-        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)
+                if lstripped.startswith("}"):
+                     self.current_indent_size = line.find("}") + 2
+
+            if lstripped.startswith("{"):
+                self.clean_counts()
+                self.current_indent_size = 0;
+
+            indent_size = len(line) - len(lstripped)
+
+            if len(line) > 80:
+                self.added_line_error('Line is too long (%d) (max 80 characters):' \
+                                  %(len(line)), line)
+
+            # skip comments for code checking
+            if lstripped.startswith("//"):
+                return
+            if lstripped.startswith("/*") and not self.comments_open:
+                self.comments_open = True
+                return
+            if rstripped.endswith("*/") and self.comments_open:
+                self.comments_open = False
+                return
+            elif rstripped.endswith("*/"):
+                # 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 at line %d' \
+                                      %(line_num), line)
+
+                # check for closing brace which affects indentation
+                bcc = self.brace_close_check_re.search(line)
+                if bcc is not None and not self.multiline_string \
+                    and not self.comments_open:
+                    self.current_indent_size -= 2
+
+                # indentation code check related to the diff structure
+                # do not check code indentation between parentheses
+                if indent_size != self.current_indent_size and len(line) != 0 \
+                    and self.parentheses_count == 0 and not self.multiline_string \
+                    and not self.comments_open:
+                     self.added_line_error('Invalid indentation at line %d. Expecting %d but found %d spaces:' \
+                        % (line_num, self.current_indent_size, indent_size), line)
+
+                # check for missing space before open parenthesis
+                spo = self.space_check_parentheses_re.search(line)
+                if spo is not None and not self.multiline_string \
+                    and not self.comments_open:
+                    # check if it's not inside of the double quoted string or
+                    # (aa)(bb)fff case
+                    if line.find('\"') > spo.start() and line[:spo.start()] != ')':
+                        self.added_line_error('Missing space before \'(\' at line %d:'\
+                                          %(line_num), line)
+
+                # check for missing space before open brace
+                boc = self.brace_open_check_re.search(line)
+                if boc is not None and not self.multiline_string \
+                    and not self.comments_open:
+                    self.current_indent_size += 2
+                    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 \'{\' at line %d:'\
+                                                  %(line_num), line)
+
+                # check for closing brace cases
+                bcc = self.brace_close_check_re.search(line)
+                if bcc is not None and not self.multiline_string \
+                    and not self.comments_open:
+                    if not lstripped.startswith("}"):
+                        self.added_line_error('The \'}\' is not on its own line at line %d:'\
+                                                %(line_num), line)
+                    if not lstripped.endswith("}"):
+                        scb = self.space_check_brace_close_re.search(line)
+                        if scb is not None \
+                            and lstripped[scb.start() + 1] != ';':
+                             self.added_line_error('Missing space after \'}\' at line %d:'\
+                                                  %(line_num), line)
+
+                # 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>
@@ -410,12 +584,13 @@ class CheckOnePatch:
         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)
             diff_ok = diff_check.ok
 
         self.ok = msg_ok and diff_ok
@@ -458,6 +633,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 +650,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 +668,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 +688,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)
-- 
2.7.4



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