I think it may have been outlook messing with me when applying the patch. The whitespace notice was about some of the copyright lines. I will pull from your public repo/branch and see if I have the same issue.
Thanks, -Erik -----Original Message----- From: Justen, Jordan L Sent: Friday, October 9, 2015 11:11 AM To: Laszlo Ersek <ler...@redhat.com>; Bjorge, Erik C <erik.c.bjo...@intel.com>; edk2-devel@lists.01.org Cc: Gao, Liming <liming....@intel.com> Subject: Re: [edk2] [PATCH] BaseTools/Scripts: Add PatchCheck.py script On 2015-10-09 10:57:40, Laszlo Ersek wrote: > On 10/09/15 19:13, Bjorge, Erik C wrote: > > The patch has some trailing white spaces > > Apparently, Jordan forgot to run the script on the patch that adds the > script! ;) I ran the script on the patch, and it didn't flag any trailing whitespace. So, if there is trailing whitespace in the patch, then there is a bug in the script. I also tried using my editor to cleanup any tailing whitespace, and it didn't seem to locate any. Can either of you point out a line with trailing whitespace in the patch? -Jordan > > but other than that it works fine. > > > > Reviewed-by: Erik Bjorge <erik.c.bjo...@intel.com> > > > > -----Original Message----- > > From: Justen, Jordan L > > Sent: Wednesday, October 7, 2015 7:53 PM > > To: edk2-devel@lists.01.org > > Cc: Justen, Jordan L <jordan.l.jus...@intel.com>; Bjorge, Erik C > > <erik.c.bjo...@intel.com>; Zhu, Yonghong <yonghong....@intel.com>; > > Gao, Liming <liming....@intel.com> > > Subject: [PATCH] BaseTools/Scripts: Add PatchCheck.py script > > > > This script can be used to check some expected rules for EDK II patches. It > > only works on git formatted patches. > > > > It checks both the commit message and the lines that are added in the patch > > diff. > > > > In the commit message it verifies line lengths, signature formats, and the > > Contributed-under tag. > > > > In the patch, it checks that line endings are CRLF for all files that don't > > have a .sh extension. It verifies that no trailing whitespace is present > > and that tab characters are not used. > > > > Patch contributors should use this script prior to submitting their > > patches. Package maintainers can also use it to verify incoming patches. > > > > It can also be run by specifying a git revision list, so actual patch files > > are not always required. > > > > For example, to checkout this last 5 patches in your git branch you can run: > > > > python PatchCheck.py HEAD~5.. > > > > Or, a shortcut (like git log): > > > > python PatchCheck.py -5 > > > > The --oneline option works similar to git log --oneline. > > > > The --silent option enables silent operation. > > > > The script supports python 2.7 and python 3. > > > > Contributed-under: TianoCore Contribution Agreement 1.0 > > Signed-off-by: Jordan Justen <jordan.l.jus...@intel.com> > > Cc: Erik Bjorge <erik.c.bjo...@intel.com> > > Cc: Yonghong Zhu <yonghong....@intel.com> > > Cc: Liming Gao <liming....@intel.com> > > --- > > BaseTools/Scripts/PatchCheck.py | 607 > > ++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 607 insertions(+) > > create mode 100644 BaseTools/Scripts/PatchCheck.py > > > > diff --git a/BaseTools/Scripts/PatchCheck.py > > b/BaseTools/Scripts/PatchCheck.py new file mode 100644 index > > 0000000..340a997 > > --- /dev/null > > +++ b/BaseTools/Scripts/PatchCheck.py > > @@ -0,0 +1,607 @@ > > +## @file > > +# Check a patch for various format issues # # Copyright (c) 2015, > > +Intel Corporation. All rights reserved.<BR> # # This program and > > +the accompanying materials are licensed and made # available under > > +the terms and conditions of the BSD License which # accompanies > > +this distribution. The full text of the license may be # found at > > +http://opensource.org/licenses/bsd-license.php > > +# > > +# THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" > > +# BASIS, WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER > > +# EXPRESS OR IMPLIED. > > +# > > + > > +from __future__ import print_function > > + > > +VersionNumber = '0.1' > > +__copyright__ = "Copyright (c) 2015, Intel Corporation All rights > > reserved." > > + > > +import email > > +import argparse > > +import os > > +import re > > +import subprocess > > +import sys > > + > > +class Verbose: > > + SILENT, ONELINE, NORMAL = range(3) > > + level = NORMAL > > + > > +class CommitMessageCheck: > > + """Checks the contents of a git commit message.""" > > + > > + def __init__(self, subject, message): > > + self.ok = True > > + > > + if subject is None and message is None: > > + self.error('Commit message is missing!') > > + return > > + > > + self.subject = subject > > + self.msg = message > > + > > + self.check_contributed_under() > > + self.check_signed_off_by() > > + self.check_misc_signatures() > > + self.check_overall_format() > > + self.report_message_result() > > + > > + url = > > 'https://github.com/tianocore/tianocore.github.io/wiki/Commit-Message-Format' > > + > > + def report_message_result(self): > > + if Verbose.level < Verbose.NORMAL: > > + return > > + if self.ok: > > + # All checks passed > > + return_code = 0 > > + print('The commit message format passed all checks.') > > + else: > > + return_code = 1 > > + if not self.ok: > > + print(self.url) > > + > > + def error(self, *err): > > + if self.ok and Verbose.level > Verbose.ONELINE: > > + print('The commit message format is not valid:') > > + self.ok = False > > + if Verbose.level < Verbose.NORMAL: > > + return > > + count = 0 > > + for line in err: > > + prefix = (' *', ' ')[count > 0] > > + print(prefix, line) > > + count += 1 > > + > > + def check_contributed_under(self): > > + cu_msg='Contributed-under: TianoCore Contribution Agreement 1.0' > > + if self.msg.find(cu_msg) < 0: > > + self.error('Missing Contributed-under! (Note: this must be ' + > > + 'added by the code contributor!)') > > + > > + @staticmethod > > + def make_signature_re(sig, re_input=False): > > + if re_input: > > + sub_re = sig > > + else: > > + sub_re = sig.replace('-', r'[-\s]+') > > + re_str = (r'^(?P<tag>' + sub_re + > > + r')(\s*):(\s*)(?P<value>\S.*?)(?:\s*)$') > > + try: > > + return re.compile(re_str, re.MULTILINE|re.IGNORECASE) > > + except Exception: > > + print("Tried to compile re:", re_str) > > + raise > > + > > + sig_block_re = \ > > + re.compile(r'''^ > > + (?: (?P<tag>[^:]+) \s* : \s* > > + (?P<value>\S.*?) ) > > + | > > + (?: \[ (?P<updater>[^:]+) \s* : \s* > > + (?P<note>.+?) \s* \] ) > > + \s* $''', > > + re.VERBOSE | re.MULTILINE) > > + > > + def find_signatures(self, sig): > > + if not sig.endswith('-by') and sig != 'Cc': > > + sig += '-by' > > + regex = self.make_signature_re(sig) > > + > > + sigs = regex.findall(self.msg) > > + > > + bad_case_sigs = filter(lambda m: m[0] != sig, sigs) > > + for s in bad_case_sigs: > > + self.error("'" +s[0] + "' should be '" + sig + "'") > > + > > + for s in sigs: > > + if s[1] != '': > > + self.error('There should be no spaces between ' + sig + > > + " and the ':'") > > + if s[2] != ' ': > > + self.error("There should be a space after '" + sig > > + + > > + ":'") > > + > > + self.check_email_address(s[3]) > > + > > + return sigs > > + > > + email_re1 = re.compile(r'(?:\s*)(.*?)(\s*)<(.+)>\s*$', > > + re.MULTILINE|re.IGNORECASE) > > + > > + def check_email_address(self, email): > > + email = email.strip() > > + mo = self.email_re1.match(email) > > + if mo is None: > > + self.error("Email format is invalid: " + email.strip()) > > + return > > + > > + name = mo.group(1).strip() > > + if name == '': > > + self.error("Name is not provided with email address: " + > > + email) > > + else: > > + quoted = len(name) > 2 and name[0] == '"' and name[-1] == '"' > > + if name.find(',') >= 0 and not quoted: > > + self.error('Add quotes (") around name with a comma: ' + > > + name) > > + > > + if mo.group(2) == '': > > + self.error("There should be a space between the name and " + > > + "email address: " + email) > > + > > + if mo.group(3).find(' ') >= 0: > > + self.error("The email address cannot contain a space: " + > > + mo.group(3)) > > + > > + def check_signed_off_by(self): > > + sob='Signed-off-by' > > + if self.msg.find(sob) < 0: > > + self.error('Missing Signed-off-by! (Note: this must be ' + > > + 'added by the code contributor!)') > > + return > > + > > + sobs = self.find_signatures('Signed-off') > > + > > + if len(sobs) == 0: > > + self.error('Invalid Signed-off-by format!') > > + return > > + > > + sig_types = ( > > + 'Reviewed', > > + 'Reported', > > + 'Tested', > > + 'Suggested', > > + 'Acked', > > + 'Cc' > > + ) > > + > > + def check_misc_signatures(self): > > + for sig in self.sig_types: > > + self.find_signatures(sig) > > + > > + def check_overall_format(self): > > + lines = self.msg.splitlines() > > + > > + if len(lines) >= 1 and lines[0].endswith('\r\n'): > > + empty_line = '\r\n' > > + else: > > + empty_line = '\n' > > + > > + lines.insert(0, empty_line) > > + lines.insert(0, self.subject + empty_line) > > + > > + count = len(lines) > > + > > + if count <= 0: > > + self.error('Empty commit message!') > > + return > > + > > + if count >= 1 and len(lines[0]) > 76: > > + self.error('First line of commit message (subject line) ' + > > + 'is too long.') > > + > > + if count >= 1 and len(lines[0].strip()) == 0: > > + self.error('First line of commit message (subject line) ' + > > + 'is empty.') > > + > > + if count >= 2 and lines[1].strip() != '': > > + self.error('Second line of commit message should be ' + > > + 'empty.') > > + > > + for i in range(2, count): > > + 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)) > > + > > + last_sig_line = None > > + for i in range(count - 1, 0, -1): > > + line = lines[i] > > + mo = self.sig_block_re.match(line) > > + if mo is None: > > + if line.strip() == '': > > + break > > + elif last_sig_line is not None: > > + err2 = 'Add empty line before "%s"?' % last_sig_line > > + self.error('The line before the signature block ' + > > + 'should be empty', err2) > > + else: > > + self.error('The signature block was not found') > > + break > > + last_sig_line = line.strip() > > + > > +(START, PRE_PATCH, PATCH) = range(3) > > + > > +class GitDiffCheck: > > + """Checks the contents of a git diff.""" > > + > > + def __init__(self, diff): > > + self.ok = True > > + self.format_ok = True > > + self.lines = diff.splitlines(True) > > + self.count = len(self.lines) > > + self.line_num = 0 > > + self.state = START > > + while self.line_num < self.count and self.format_ok: > > + line_num = self.line_num > > + self.run() > > + assert(self.line_num > line_num) > > + self.report_message_result() > > + > > + def report_message_result(self): > > + if Verbose.level < Verbose.NORMAL: > > + return > > + if self.ok: > > + print('The code passed all checks.') > > + > > + def run(self): > > + line = self.lines[self.line_num] > > + > > + if self.state in (PRE_PATCH, PATCH): > > + if line.startswith('diff --git'): > > + self.state = START > > + if self.state == PATCH: > > + if line.startswith('@@ '): > > + self.state = PRE_PATCH > > + elif len(line) >= 1 and line[0] not in ' -+' and \ > > + not line.startswith(r'\ No newline '): > > + for line in self.lines[self.line_num + 1:]: > > + if line.startswith('diff --git'): > > + self.format_error('diff found after end of patch') > > + break > > + self.line_num = self.count > > + return > > + > > + if self.state == START: > > + if line.startswith('diff --git'): > > + self.state = PRE_PATCH > > + self.set_filename(None) > > + elif len(line.rstrip()) != 0: > > + self.format_error("didn't find diff command") > > + self.line_num += 1 > > + elif self.state == PRE_PATCH: > > + if line.startswith('+++ b/'): > > + self.set_filename(line[6:].rstrip()) > > + if line.startswith('@@ '): > > + self.state = PATCH > > + else: > > + ok = False > > + for pfx in self.pre_patch_prefixes: > > + if line.startswith(pfx): > > + ok = True > > + if not ok: > > + self.format_error("didn't find diff hunk marker (@@)") > > + self.line_num += 1 > > + elif self.state == PATCH: > > + if line.startswith('-'): > > + pass > > + elif line.startswith('+'): > > + self.check_added_line(line[1:]) > > + elif line.startswith(r'\ No newline '): > > + pass > > + elif not line.startswith(' '): > > + self.format_error("unexpected patch line") > > + self.line_num += 1 > > + > > + pre_patch_prefixes = ( > > + '--- ', > > + '+++ ', > > + 'index ', > > + 'new file ', > > + 'deleted file ', > > + 'old mode ', > > + 'new mode ', > > + 'similarity index ', > > + 'rename ', > > + 'Binary files ', > > + ) > > + > > + line_endings = ('\r\n', '\n\r', '\n', '\r') > > + > > + def set_filename(self, filename): > > + self.hunk_filename = filename > > + if filename: > > + self.force_crlf = not filename.endswith('.sh') > > + else: > > + self.force_crlf = True > > + > > + def added_line_error(self, msg, line): > > + lines = [ msg ] > > + if self.hunk_filename is not None: > > + lines.append('File: ' + self.hunk_filename) > > + lines.append('Line: ' + line) > > + > > + self.error(*lines) > > + > > + def check_added_line(self, line): > > + eol = '' > > + for an_eol in self.line_endings: > > + if line.endswith(an_eol): > > + eol = an_eol > > + line = line[:-len(eol)] > > + > > + stripped = line.rstrip() > > + > > + if self.force_crlf and eol != '\r\n': > > + self.added_line_error('Line ending (%s) is not CRLF' % > > repr(eol), > > + line) > > + if ' ' in line: > > + self.added_line_error('Tab character used', line) > > + if len(stripped) < len(line): > > + self.added_line_error('Trailing whitespace found', > > + line) > > + > > + split_diff_re = re.compile(r''' > > + (?P<cmd> > > + ^ diff \s+ --git \s+ a/.+ \s+ b/.+ $ > > + ) > > + (?P<index> > > + ^ index \s+ .+ $ > > + ) > > + ''', > > + re.IGNORECASE | re.VERBOSE | > > + re.MULTILINE) > > + > > + def format_error(self, err): > > + self.format_ok = False > > + err = 'Patch format error: ' + err > > + err2 = 'Line: ' + self.lines[self.line_num].rstrip() > > + self.error(err, err2) > > + > > + def error(self, *err): > > + if self.ok and Verbose.level > Verbose.ONELINE: > > + print('Code format is not valid:') > > + self.ok = False > > + if Verbose.level < Verbose.NORMAL: > > + return > > + count = 0 > > + for line in err: > > + prefix = (' *', ' ')[count > 0] > > + print(prefix, line) > > + count += 1 > > + > > +class CheckOnePatch: > > + """Checks the contents of a git email formatted patch. > > + > > + Various checks are performed on both the commit message and the > > + patch content. > > + """ > > + > > + def __init__(self, name, patch): > > + self.patch = patch > > + self.find_patch_pieces() > > + > > + msg_check = CommitMessageCheck(self.commit_subject, > > self.commit_msg) > > + msg_ok = msg_check.ok > > + > > + diff_ok = True > > + if self.diff is not None: > > + diff_check = GitDiffCheck(self.diff) > > + diff_ok = diff_check.ok > > + > > + self.ok = msg_ok and diff_ok > > + > > + if Verbose.level == Verbose.ONELINE: > > + if self.ok: > > + result = 'ok' > > + else: > > + result = list() > > + if not msg_ok: > > + result.append('commit message') > > + if not diff_ok: > > + result.append('diff content') > > + result = 'bad ' + ' and '.join(result) > > + print(name, result) > > + > > + > > + git_diff_re = re.compile(r''' > > + ^ diff \s+ --git \s+ a/.+ \s+ b/.+ $ > > + ''', > > + re.IGNORECASE | re.VERBOSE | > > + re.MULTILINE) > > + > > + stat_re = \ > > + re.compile(r''' > > + (?P<commit_message> [\s\S\r\n]* ) > > + (?P<stat> > > + ^ --- $ [\r\n]+ > > + (?: ^ \s+ .+ \s+ \| \s+ \d+ \s+ \+* \-* > > + $ [\r\n]+ )+ > > + [\s\S\r\n]+ > > + ) > > + ''', > > + re.IGNORECASE | re.VERBOSE | re.MULTILINE) > > + > > + def find_patch_pieces(self): > > + if sys.version_info < (3, 0): > > + patch = self.patch.encode('ascii', 'ignore') > > + else: > > + patch = self.patch > > + > > + self.commit_msg = None > > + self.stat = None > > + self.commit_subject = None > > + self.commit_prefix = None > > + self.diff = None > > + > > + if patch.startswith('diff --git'): > > + self.diff = patch > > + return > > + > > + pmail = email.message_from_string(patch) > > + parts = list(pmail.walk()) > > + assert(len(parts) == 1) > > + assert(parts[0].get_content_type() == 'text/plain') > > + content = parts[0].get_payload(decode=True).decode('utf-8', > > + 'ignore') > > + > > + mo = self.git_diff_re.search(content) > > + if mo is not None: > > + self.diff = content[mo.start():] > > + content = content[:mo.start()] > > + > > + mo = self.stat_re.search(content) > > + if mo is None: > > + self.commit_msg = content > > + else: > > + self.stat = mo.group('stat') > > + self.commit_msg = mo.group('commit_message') > > + > > + self.commit_subject = pmail['subject'].replace('\r\n', '') > > + self.commit_subject = self.commit_subject.replace('\n', '') > > + > > + pfx_start = self.commit_subject.find('[') > > + if pfx_start >= 0: > > + pfx_end = self.commit_subject.find(']') > > + if pfx_end > pfx_start: > > + self.commit_prefix = self.commit_subject[pfx_start + 1 : > > pfx_end] > > + self.commit_subject = self.commit_subject[pfx_end + > > + 1 > > + :].lstrip() > > + > > + > > +class CheckGitCommits: > > + """Reads patches from git based on the specified git revision range. > > + > > + The patches are read from git, and then checked. > > + """ > > + > > + def __init__(self, rev_spec, max_count): > > + commits = self.read_commit_list_from_git(rev_spec, max_count) > > + if len(commits) == 1 and Verbose.level > Verbose.ONELINE: > > + commits = [ rev_spec ] > > + self.ok = True > > + blank_line = False > > + for commit in commits: > > + if Verbose.level > Verbose.ONELINE: > > + if blank_line: > > + print() > > + else: > > + blank_line = True > > + print('Checking git commit:', commit) > > + patch = self.read_patch_from_git(commit) > > + self.ok &= CheckOnePatch(commit, patch).ok > > + > > + def read_commit_list_from_git(self, rev_spec, max_count): > > + # Run git to get the commit patch > > + cmd = [ 'rev-list', '--abbrev-commit', '--no-walk' ] > > + if max_count is not None: > > + cmd.append('--max-count=' + str(max_count)) > > + cmd.append(rev_spec) > > + out = self.run_git(*cmd) > > + return out.split() > > + > > + def read_patch_from_git(self, commit): > > + # Run git to get the commit patch > > + return self.run_git('show', '--pretty=email', commit) > > + > > + def run_git(self, *args): > > + cmd = [ 'git' ] > > + cmd += args > > + p = subprocess.Popen(cmd, > > + stdout=subprocess.PIPE, > > + stderr=subprocess.STDOUT) > > + return p.communicate()[0].decode('utf-8', 'ignore') > > + > > +class CheckOnePatchFile: > > + """Performs a patch check for a single file. > > + > > + stdin is used when the filename is '-'. > > + """ > > + > > + def __init__(self, patch_filename): > > + if patch_filename == '-': > > + patch = sys.stdin.read() > > + patch_filename = 'stdin' > > + else: > > + f = open(patch_filename, 'rb') > > + 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 > > + > > +class CheckOneArg: > > + """Performs a patch check for a single command line argument. > > + > > + The argument will be handed off to a file or git-commit based > > + checker. > > + """ > > + > > + def __init__(self, param, max_count=None): > > + self.ok = True > > + if param == '-' or os.path.exists(param): > > + checker = CheckOnePatchFile(param) > > + else: > > + checker = CheckGitCommits(param, max_count) > > + self.ok = checker.ok > > + > > +class PatchCheckApp: > > + """Checks patches based on the command line arguments.""" > > + > > + def __init__(self): > > + self.parse_options() > > + patches = self.args.patches > > + > > + if len(patches) == 0: > > + patches = [ 'HEAD' ] > > + > > + self.ok = True > > + self.count = None > > + for patch in patches: > > + self.process_one_arg(patch) > > + > > + if self.count is not None: > > + self.process_one_arg('HEAD') > > + > > + if self.ok: > > + self.retval = 0 > > + else: > > + self.retval = -1 > > + > > + def process_one_arg(self, arg): > > + if len(arg) >= 2 and arg[0] == '-': > > + try: > > + self.count = int(arg[1:]) > > + return > > + except ValueError: > > + pass > > + self.ok &= CheckOneArg(arg, self.count).ok > > + self.count = None > > + > > + def parse_options(self): > > + parser = argparse.ArgumentParser(description=__copyright__) > > + parser.add_argument('--version', action='version', > > + version='%(prog)s ' + VersionNumber) > > + parser.add_argument('patches', nargs='*', > > + help='[patch file | git rev list]') > > + group = parser.add_mutually_exclusive_group() > > + group.add_argument("--oneline", > > + action="store_true", > > + help="Print one result per line") > > + group.add_argument("--silent", > > + action="store_true", > > + help="Print nothing") > > + self.args = parser.parse_args() > > + if self.args.oneline: > > + Verbose.level = Verbose.ONELINE > > + if self.args.silent: > > + Verbose.level = Verbose.SILENT > > + > > +if __name__ == "__main__": > > + sys.exit(PatchCheckApp().retval) > > -- > > 2.5.1 > > > > _______________________________________________ > > edk2-devel mailing list > > edk2-devel@lists.01.org > > https://lists.01.org/mailman/listinfo/edk2-devel > > > _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
