From: "Gao, Liming" <liming.gao@intel.com>
To: Daniil Egranov <daniil.egranov@arm.com>,
"edk2-devel@lists.01.org" <edk2-devel@lists.01.org>
Cc: "leif.lindholm@linaro.org" <leif.lindholm@linaro.org>
Subject: Re: [PATCH] BaseTools/Scripts/PatchCheck.py: Extended patch style check for c code
Date: Mon, 12 Dec 2016 03:09:06 +0000 [thread overview]
Message-ID: <4A89E2EF3DFEDB4C8BFDE51014F606A14D6A00D3@SHSMSX104.ccr.corp.intel.com> (raw)
In-Reply-To: <1481261704-107192-1-git-send-email-daniil.egranov@arm.com>
Per coding style document, 5.1.1 Lines shall be 120 columns, or less. So, I suggest to add report line size for line with 120+ characters.
> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> Daniil Egranov
> Sent: Friday, December 09, 2016 1:35 PM
> To: edk2-devel@lists.01.org
> Cc: leif.lindholm@linaro.org
> Subject: [edk2] [PATCH] BaseTools/Scripts/PatchCheck.py: Extended patch
> style check for c code
>
> 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
>
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
prev parent reply other threads:[~2016-12-12 3:09 UTC|newest]
Thread overview: 2+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-12-09 5:35 [PATCH] BaseTools/Scripts/PatchCheck.py: Extended patch style check for c code Daniil Egranov
2016-12-12 3:09 ` Gao, Liming [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=4A89E2EF3DFEDB4C8BFDE51014F606A14D6A00D3@SHSMSX104.ccr.corp.intel.com \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox