[Patch] BaseTools: PatchCheck to align with wiki and report error for EFI_D_*

[Patch] BaseTools: PatchCheck to align with wiki and report error for EFI_D_*

[Yonghong Zhu]

This patch updates PatchCheck.py：
1.Update max line length to 70 that describe in wiki
2.remove the two [] when do the char count calculation
3.report error for EFI_D_* macro if it is used, recommend to use
DEBUG_* format.

Cc: Liming Gao <liming.gao@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Yonghong Zhu <yonghong.zhu@intel.com>
---
 BaseTools/Scripts/PatchCheck.py | 21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)

diff --git a/BaseTools/Scripts/PatchCheck.py b/BaseTools/Scripts/PatchCheck.py
index 455c130..d423220 100755
--- a/BaseTools/Scripts/PatchCheck.py
+++ b/BaseTools/Scripts/PatchCheck.py
@@ -1,9 +1,9 @@
 ## @file
 #  Check a patch for various format issues
 #
-#  Copyright (c) 2015, Intel Corporation. All rights reserved.<BR>
+#  Copyright (c) 2015 - 2016, 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
@@ -14,11 +14,11 @@
 #
 
 from __future__ import print_function
 
 VersionNumber = '0.1'
-__copyright__ = "Copyright (c) 2015, Intel Corporation  All rights reserved."
+__copyright__ = "Copyright (c) 2015 - 2016, Intel Corporation  All rights reserved."
 
 import email
 import argparse
 import os
 import re
@@ -195,11 +195,11 @@ class CommitMessageCheck:
 
         if count <= 0:
             self.error('Empty commit message!')
             return
 
-        if count >= 1 and len(lines[0]) > 76:
+        if count >= 1 and len(lines[0]) > 70:
             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) ' +
@@ -208,11 +208,11 @@ class CommitMessageCheck:
         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
+            if (len(lines[i]) > 70 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
@@ -354,10 +354,12 @@ class GitDiffCheck:
                                   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)
+        if 'EFI_D_' in line:
+            self.added_line_error('EFI_D_* used, Please use DEBUG_* format', line)
 
     split_diff_re = re.compile(r'''
                                    (?P<cmd>
                                        ^ diff \s+ --git \s+ a/.+ \s+ b/.+ $
                                    )
@@ -471,11 +473,20 @@ class CheckOnePatch:
             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('[')
+        pfx_start_tmp = self.commit_subject.find('[')
+        pfx_end = self.commit_subject.find(']', pfx_start_tmp)
+        pfx_start = pfx_start_tmp
+        while (pfx_end > pfx_start_tmp):
+            pfx_start_tmp = self.commit_subject.find('[', pfx_end)
+            if pfx_start_tmp >= 0:
+                pfx_end = self.commit_subject.find(']',pfx_start_tmp)
+            else:
+                break
+
         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()
-- 
2.6.1.windows.1

[Patch] BaseTools: support PCD value to use expression in the DEC file

[Yonghong Zhu]

This patch add the support for Pcd value to use expression in the DEC file.

Cc: Liming Gao <liming.gao@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Yonghong Zhu <yonghong.zhu@intel.com>
---
 BaseTools/Source/Python/Common/Misc.py             | 57 ++++++++++++----------
 .../Source/Python/Workspace/MetaFileParser.py      | 21 ++++++--
 2 files changed, 49 insertions(+), 29 deletions(-)

diff --git a/BaseTools/Source/Python/Common/Misc.py b/BaseTools/Source/Python/Common/Misc.py
index c99716d..3be1f0f 100644
--- a/BaseTools/Source/Python/Common/Misc.py
+++ b/BaseTools/Source/Python/Common/Misc.py
@@ -1410,36 +1410,11 @@ def ParseConsoleLog(Filename):
             Opw.write('%s\n' % Line)
 
     Opr.close()
     Opw.close()
 
-## AnalyzeDscPcd
-#
-#  Analyze DSC PCD value, since there is no data type info in DSC
-#  This fuction is used to match functions (AnalyzePcdData, AnalyzeHiiPcdData, AnalyzeVpdPcdData) used for retrieving PCD value from database
-#  1. Feature flag: TokenSpace.PcdCName|PcdValue
-#  2. Fix and Patch:TokenSpace.PcdCName|PcdValue[|MaxSize]
-#  3. Dynamic default:
-#     TokenSpace.PcdCName|PcdValue[|VOID*[|MaxSize]]
-#     TokenSpace.PcdCName|PcdValue
-#  4. Dynamic VPD:
-#     TokenSpace.PcdCName|VpdOffset[|VpdValue]
-#     TokenSpace.PcdCName|VpdOffset[|MaxSize[|VpdValue]]
-#  5. Dynamic HII:
-#     TokenSpace.PcdCName|HiiString|VaiableGuid|VariableOffset[|HiiValue]
-#  PCD value needs to be located in such kind of string, and the PCD value might be an expression in which
-#    there might have "|" operator, also in string value.
-#
-#  @param Setting: String contain information described above with "TokenSpace.PcdCName|" stripped
-#  @param PcdType: PCD type: feature, fixed, dynamic default VPD HII
-#  @param DataType: The datum type of PCD: VOID*, UNIT, BOOL
-#  @retval:
-#    ValueList: A List contain fields described above
-#    IsValid:   True if conforming EBNF, otherwise False
-#    Index:     The index where PcdValue is in ValueList
-#
-def AnalyzeDscPcd(Setting, PcdType, DataType=''):
+def AnalyzePcdExpression(Setting):
     Setting = Setting.strip()
     # There might be escaped quote in a string: \", \\\"
     Data = Setting.replace('\\\\', '//').replace('\\\"', '\\\'')
     # There might be '|' in string and in ( ... | ... ), replace it with '-'
     NewStr = ''
@@ -1465,10 +1440,40 @@ def AnalyzeDscPcd(Setting, PcdType, DataType=''):
             FieldList.append(Setting[StartPos:].strip())
             break
         FieldList.append(Setting[StartPos:Pos].strip())
         StartPos = Pos + 1
 
+    return FieldList
+
+## AnalyzeDscPcd
+#
+#  Analyze DSC PCD value, since there is no data type info in DSC
+#  This fuction is used to match functions (AnalyzePcdData, AnalyzeHiiPcdData, AnalyzeVpdPcdData) used for retrieving PCD value from database
+#  1. Feature flag: TokenSpace.PcdCName|PcdValue
+#  2. Fix and Patch:TokenSpace.PcdCName|PcdValue[|MaxSize]
+#  3. Dynamic default:
+#     TokenSpace.PcdCName|PcdValue[|VOID*[|MaxSize]]
+#     TokenSpace.PcdCName|PcdValue
+#  4. Dynamic VPD:
+#     TokenSpace.PcdCName|VpdOffset[|VpdValue]
+#     TokenSpace.PcdCName|VpdOffset[|MaxSize[|VpdValue]]
+#  5. Dynamic HII:
+#     TokenSpace.PcdCName|HiiString|VaiableGuid|VariableOffset[|HiiValue]
+#  PCD value needs to be located in such kind of string, and the PCD value might be an expression in which
+#    there might have "|" operator, also in string value.
+#
+#  @param Setting: String contain information described above with "TokenSpace.PcdCName|" stripped
+#  @param PcdType: PCD type: feature, fixed, dynamic default VPD HII
+#  @param DataType: The datum type of PCD: VOID*, UNIT, BOOL
+#  @retval:
+#    ValueList: A List contain fields described above
+#    IsValid:   True if conforming EBNF, otherwise False
+#    Index:     The index where PcdValue is in ValueList
+#
+def AnalyzeDscPcd(Setting, PcdType, DataType=''):
+    FieldList = AnalyzePcdExpression(Setting)
+
     IsValid = True
     if PcdType in (MODEL_PCD_FIXED_AT_BUILD, MODEL_PCD_PATCHABLE_IN_MODULE, MODEL_PCD_FEATURE_FLAG):
         Value = FieldList[0]
         Size = ''
         if len(FieldList) > 1:
diff --git a/BaseTools/Source/Python/Workspace/MetaFileParser.py b/BaseTools/Source/Python/Workspace/MetaFileParser.py
index 82d874f..1a5fdf5 100644
--- a/BaseTools/Source/Python/Workspace/MetaFileParser.py
+++ b/BaseTools/Source/Python/Workspace/MetaFileParser.py
@@ -24,11 +24,11 @@ import Common.EdkLogger as EdkLogger
 import Common.GlobalData as GlobalData
 
 from CommonDataClass.DataClass import *
 from Common.DataType import *
 from Common.String import *
-from Common.Misc import GuidStructureStringToGuidString, CheckPcdDatum, PathClass, AnalyzePcdData, AnalyzeDscPcd
+from Common.Misc import GuidStructureStringToGuidString, CheckPcdDatum, PathClass, AnalyzePcdData, AnalyzeDscPcd, AnalyzePcdExpression
 from Common.Expression import *
 from CommonDataClass.Exceptions import *
 from Common.LongFilePathSupport import OpenLongFilePath as open
 
 from MetaFileTable import MetaFileStorage
@@ -1633,10 +1633,11 @@ class DecParser(MetaFileParser):
             return
         MetaFileParser.__init__(self, FilePath, FileType, Arch, Table, -1)
         self._Comments = []
         self._Version = 0x00010005  # Only EDK2 dec file is supported
         self._AllPCDs = [] # Only for check duplicate PCD
+        self._AllPcdDict = {}
 
     ## Parser starter
     def Start(self):
         Content = ''
         try:
@@ -1846,14 +1847,14 @@ class DecParser(MetaFileParser):
         PtrValue = ValueRe.findall(TokenList[1])
 
         # Has VOID* type string, may contain "|" character in the string. 
         if len(PtrValue) != 0:
             ptrValueList = re.sub(ValueRe, '', TokenList[1])
-            ValueList = GetSplitValueList(ptrValueList)
+            ValueList = AnalyzePcdExpression(ptrValueList)
             ValueList[0] = PtrValue[0]
         else:
-            ValueList = GetSplitValueList(TokenList[1])
+            ValueList = AnalyzePcdExpression(TokenList[1])
 
 
         # check if there's enough datum information given
         if len(ValueList) != 3:
             EdkLogger.error('Parser', FORMAT_INVALID, "Invalid PCD Datum information given",
@@ -1876,10 +1877,23 @@ class DecParser(MetaFileParser):
         if ValueList[2] == '':
             EdkLogger.error('Parser', FORMAT_INVALID, "Missing Token in PCD Datum information",
                             ExtraData=self._CurrentLine + \
                                       " (<TokenSpaceGuidCName>.<PcdCName>|<DefaultValue>|<DatumType>|<Token>)",
                             File=self.MetaFile, Line=self._LineIndex + 1)
+
+        PcdValue = ValueList[0]
+        if PcdValue:
+            try:
+                ValueList[0] = ValueExpression(PcdValue, self._AllPcdDict)(True)
+            except WrnExpression, Value:
+                ValueList[0] = Value.result
+
+        if ValueList[0] == 'True':
+            ValueList[0] = '1'
+        if ValueList[0] == 'False':
+            ValueList[0] = '0'
+
         # check format of default value against the datum type
         IsValid, Cause = CheckPcdDatum(ValueList[1], ValueList[0])
         if not IsValid:
             EdkLogger.error('Parser', FORMAT_INVALID, Cause, ExtraData=self._CurrentLine,
                             File=self.MetaFile, Line=self._LineIndex + 1)
@@ -1894,10 +1908,11 @@ class DecParser(MetaFileParser):
             EdkLogger.error('Parser', FORMAT_INVALID,
                             "The same PCD name and GUID have been already defined",
                             ExtraData=self._CurrentLine, File=self.MetaFile, Line=self._LineIndex + 1)
         else:
             self._AllPCDs.append((self._Scope[0], self._ValueList[0], self._ValueList[1]))
+            self._AllPcdDict[TAB_SPLIT.join(self._ValueList[0:2])] = ValueList[0]
 
         self._ValueList[2] = ValueList[0].strip() + '|' + ValueList[1].strip() + '|' + ValueList[2].strip()
 
     _SectionParser = {
         MODEL_META_DATA_HEADER          :   MetaFileParser._DefineParser,
-- 
2.6.1.windows.1

Re: [Patch] BaseTools: support PCD value to use expression in the DEC file

[Gao, Liming]

Reviewed-by: Liming Gao <liming.gao@intel.com>

> -----Original Message-----
> From: Zhu, Yonghong
> Sent: Thursday, October 13, 2016 5:30 PM
> To: edk2-devel@lists.01.org
> Cc: Gao, Liming <liming.gao@intel.com>
> Subject: [Patch] BaseTools: support PCD value to use expression in the DEC
> file
> 
> This patch add the support for Pcd value to use expression in the DEC file.
> 
> Cc: Liming Gao <liming.gao@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Yonghong Zhu <yonghong.zhu@intel.com>
> ---
>  BaseTools/Source/Python/Common/Misc.py             | 57 ++++++++++++------
> ----
>  .../Source/Python/Workspace/MetaFileParser.py      | 21 ++++++--
>  2 files changed, 49 insertions(+), 29 deletions(-)
> 
> diff --git a/BaseTools/Source/Python/Common/Misc.py
> b/BaseTools/Source/Python/Common/Misc.py
> index c99716d..3be1f0f 100644
> --- a/BaseTools/Source/Python/Common/Misc.py
> +++ b/BaseTools/Source/Python/Common/Misc.py
> @@ -1410,36 +1410,11 @@ def ParseConsoleLog(Filename):
>              Opw.write('%s\n' % Line)
> 
>      Opr.close()
>      Opw.close()
> 
> -## AnalyzeDscPcd
> -#
> -#  Analyze DSC PCD value, since there is no data type info in DSC
> -#  This fuction is used to match functions (AnalyzePcdData,
> AnalyzeHiiPcdData, AnalyzeVpdPcdData) used for retrieving PCD value from
> database
> -#  1. Feature flag: TokenSpace.PcdCName|PcdValue
> -#  2. Fix and Patch:TokenSpace.PcdCName|PcdValue[|MaxSize]
> -#  3. Dynamic default:
> -#     TokenSpace.PcdCName|PcdValue[|VOID*[|MaxSize]]
> -#     TokenSpace.PcdCName|PcdValue
> -#  4. Dynamic VPD:
> -#     TokenSpace.PcdCName|VpdOffset[|VpdValue]
> -#     TokenSpace.PcdCName|VpdOffset[|MaxSize[|VpdValue]]
> -#  5. Dynamic HII:
> -#     TokenSpace.PcdCName|HiiString|VaiableGuid|VariableOffset[|HiiValue]
> -#  PCD value needs to be located in such kind of string, and the PCD value
> might be an expression in which
> -#    there might have "|" operator, also in string value.
> -#
> -#  @param Setting: String contain information described above with
> "TokenSpace.PcdCName|" stripped
> -#  @param PcdType: PCD type: feature, fixed, dynamic default VPD HII
> -#  @param DataType: The datum type of PCD: VOID*, UNIT, BOOL
> -#  @retval:
> -#    ValueList: A List contain fields described above
> -#    IsValid:   True if conforming EBNF, otherwise False
> -#    Index:     The index where PcdValue is in ValueList
> -#
> -def AnalyzeDscPcd(Setting, PcdType, DataType=''):
> +def AnalyzePcdExpression(Setting):
>      Setting = Setting.strip()
>      # There might be escaped quote in a string: \", \\\"
>      Data = Setting.replace('\\\\', '//').replace('\\\"', '\\\'')
>      # There might be '|' in string and in ( ... | ... ), replace it with '-'
>      NewStr = ''
> @@ -1465,10 +1440,40 @@ def AnalyzeDscPcd(Setting, PcdType,
> DataType=''):
>              FieldList.append(Setting[StartPos:].strip())
>              break
>          FieldList.append(Setting[StartPos:Pos].strip())
>          StartPos = Pos + 1
> 
> +    return FieldList
> +
> +## AnalyzeDscPcd
> +#
> +#  Analyze DSC PCD value, since there is no data type info in DSC
> +#  This fuction is used to match functions (AnalyzePcdData,
> AnalyzeHiiPcdData, AnalyzeVpdPcdData) used for retrieving PCD value from
> database
> +#  1. Feature flag: TokenSpace.PcdCName|PcdValue
> +#  2. Fix and Patch:TokenSpace.PcdCName|PcdValue[|MaxSize]
> +#  3. Dynamic default:
> +#     TokenSpace.PcdCName|PcdValue[|VOID*[|MaxSize]]
> +#     TokenSpace.PcdCName|PcdValue
> +#  4. Dynamic VPD:
> +#     TokenSpace.PcdCName|VpdOffset[|VpdValue]
> +#     TokenSpace.PcdCName|VpdOffset[|MaxSize[|VpdValue]]
> +#  5. Dynamic HII:
> +#
> TokenSpace.PcdCName|HiiString|VaiableGuid|VariableOffset[|HiiValue]
> +#  PCD value needs to be located in such kind of string, and the PCD value
> might be an expression in which
> +#    there might have "|" operator, also in string value.
> +#
> +#  @param Setting: String contain information described above with
> "TokenSpace.PcdCName|" stripped
> +#  @param PcdType: PCD type: feature, fixed, dynamic default VPD HII
> +#  @param DataType: The datum type of PCD: VOID*, UNIT, BOOL
> +#  @retval:
> +#    ValueList: A List contain fields described above
> +#    IsValid:   True if conforming EBNF, otherwise False
> +#    Index:     The index where PcdValue is in ValueList
> +#
> +def AnalyzeDscPcd(Setting, PcdType, DataType=''):
> +    FieldList = AnalyzePcdExpression(Setting)
> +
>      IsValid = True
>      if PcdType in (MODEL_PCD_FIXED_AT_BUILD,
> MODEL_PCD_PATCHABLE_IN_MODULE, MODEL_PCD_FEATURE_FLAG):
>          Value = FieldList[0]
>          Size = ''
>          if len(FieldList) > 1:
> diff --git a/BaseTools/Source/Python/Workspace/MetaFileParser.py
> b/BaseTools/Source/Python/Workspace/MetaFileParser.py
> index 82d874f..1a5fdf5 100644
> --- a/BaseTools/Source/Python/Workspace/MetaFileParser.py
> +++ b/BaseTools/Source/Python/Workspace/MetaFileParser.py
> @@ -24,11 +24,11 @@ import Common.EdkLogger as EdkLogger
>  import Common.GlobalData as GlobalData
> 
>  from CommonDataClass.DataClass import *
>  from Common.DataType import *
>  from Common.String import *
> -from Common.Misc import GuidStructureStringToGuidString,
> CheckPcdDatum, PathClass, AnalyzePcdData, AnalyzeDscPcd
> +from Common.Misc import GuidStructureStringToGuidString,
> CheckPcdDatum, PathClass, AnalyzePcdData, AnalyzeDscPcd,
> AnalyzePcdExpression
>  from Common.Expression import *
>  from CommonDataClass.Exceptions import *
>  from Common.LongFilePathSupport import OpenLongFilePath as open
> 
>  from MetaFileTable import MetaFileStorage
> @@ -1633,10 +1633,11 @@ class DecParser(MetaFileParser):
>              return
>          MetaFileParser.__init__(self, FilePath, FileType, Arch, Table, -1)
>          self._Comments = []
>          self._Version = 0x00010005  # Only EDK2 dec file is supported
>          self._AllPCDs = [] # Only for check duplicate PCD
> +        self._AllPcdDict = {}
> 
>      ## Parser starter
>      def Start(self):
>          Content = ''
>          try:
> @@ -1846,14 +1847,14 @@ class DecParser(MetaFileParser):
>          PtrValue = ValueRe.findall(TokenList[1])
> 
>          # Has VOID* type string, may contain "|" character in the string.
>          if len(PtrValue) != 0:
>              ptrValueList = re.sub(ValueRe, '', TokenList[1])
> -            ValueList = GetSplitValueList(ptrValueList)
> +            ValueList = AnalyzePcdExpression(ptrValueList)
>              ValueList[0] = PtrValue[0]
>          else:
> -            ValueList = GetSplitValueList(TokenList[1])
> +            ValueList = AnalyzePcdExpression(TokenList[1])
> 
> 
>          # check if there's enough datum information given
>          if len(ValueList) != 3:
>              EdkLogger.error('Parser', FORMAT_INVALID, "Invalid PCD Datum
> information given",
> @@ -1876,10 +1877,23 @@ class DecParser(MetaFileParser):
>          if ValueList[2] == '':
>              EdkLogger.error('Parser', FORMAT_INVALID, "Missing Token in PCD
> Datum information",
>                              ExtraData=self._CurrentLine + \
>                                        "
> (<TokenSpaceGuidCName>.<PcdCName>|<DefaultValue>|<DatumType>|<
> Token>)",
>                              File=self.MetaFile, Line=self._LineIndex + 1)
> +
> +        PcdValue = ValueList[0]
> +        if PcdValue:
> +            try:
> +                ValueList[0] = ValueExpression(PcdValue, self._AllPcdDict)(True)
> +            except WrnExpression, Value:
> +                ValueList[0] = Value.result
> +
> +        if ValueList[0] == 'True':
> +            ValueList[0] = '1'
> +        if ValueList[0] == 'False':
> +            ValueList[0] = '0'
> +
>          # check format of default value against the datum type
>          IsValid, Cause = CheckPcdDatum(ValueList[1], ValueList[0])
>          if not IsValid:
>              EdkLogger.error('Parser', FORMAT_INVALID, Cause,
> ExtraData=self._CurrentLine,
>                              File=self.MetaFile, Line=self._LineIndex + 1)
> @@ -1894,10 +1908,11 @@ class DecParser(MetaFileParser):
>              EdkLogger.error('Parser', FORMAT_INVALID,
>                              "The same PCD name and GUID have been already defined",
>                              ExtraData=self._CurrentLine, File=self.MetaFile,
> Line=self._LineIndex + 1)
>          else:
>              self._AllPCDs.append((self._Scope[0], self._ValueList[0],
> self._ValueList[1]))
> +            self._AllPcdDict[TAB_SPLIT.join(self._ValueList[0:2])] = ValueList[0]
> 
>          self._ValueList[2] = ValueList[0].strip() + '|' + ValueList[1].strip() + '|' +
> ValueList[2].strip()
> 
>      _SectionParser = {
>          MODEL_META_DATA_HEADER          :   MetaFileParser._DefineParser,
> --
> 2.6.1.windows.1

Re: [Patch] BaseTools: PatchCheck to align with wiki and report error for EFI_D_*

[Jordan Justen]

On 2016-10-13 02:29:46, Yonghong Zhu wrote:
> This patch updates PatchCheck.py：

Can you split this into 3 separate patches?

> 1.Update max line length to 70 that describe in wiki

I think we should update PatchCheck.py and the wiki to say that:

1. The subject line of the commit message should be < 72 characters.

This works well with git log --oneline to still produce output less
than 80 characters.

2. The other lines of the commit message should be < 76 characters.

When using git log, these lines are indented 4 characters, so once
again this will keep the log message readable and under 80 columns.

> 2.remove the two [] when do the char count calculation

How about instead we add a new regex to the class:

    subject_prefix_re = \
        re.compile(r'''^
                           \s* (\[
                               [^\[\]]* # Allow all non-brackets
                           \])? \s*
                   ''',
                   re.VERBOSE)


Then you can use it line this:

          self.commit_subject = \
	      self.subject_prefix_re.sub('', self.commit_subject, 1)

This will change:

  ' a' => 'a'
  ' [patch 1] a' => 'a'

But, it will ignore this, because I don't think we should try to match
brackets here:

  ' [a [b]] a'

> 3.report error for EFI_D_* macro if it is used, recommend to use
> DEBUG_* format.

This should definitely be a separate patch since we are starting to
look at code.

-Jordan

> 
> Cc: Liming Gao <liming.gao@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Yonghong Zhu <yonghong.zhu@intel.com>
> ---
>  BaseTools/Scripts/PatchCheck.py | 21 ++++++++++++++++-----
>  1 file changed, 16 insertions(+), 5 deletions(-)
> 
> diff --git a/BaseTools/Scripts/PatchCheck.py b/BaseTools/Scripts/PatchCheck.py
> index 455c130..d423220 100755
> --- a/BaseTools/Scripts/PatchCheck.py
> +++ b/BaseTools/Scripts/PatchCheck.py
> @@ -1,9 +1,9 @@
>  ## @file
>  #  Check a patch for various format issues
>  #
> -#  Copyright (c) 2015, Intel Corporation. All rights reserved.<BR>
> +#  Copyright (c) 2015 - 2016, 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
> @@ -14,11 +14,11 @@
>  #
>  
>  from __future__ import print_function
>  
>  VersionNumber = '0.1'
> -__copyright__ = "Copyright (c) 2015, Intel Corporation  All rights reserved."
> +__copyright__ = "Copyright (c) 2015 - 2016, Intel Corporation  All rights reserved."
>  
>  import email
>  import argparse
>  import os
>  import re
> @@ -195,11 +195,11 @@ class CommitMessageCheck:
>  
>          if count <= 0:
>              self.error('Empty commit message!')
>              return
>  
> -        if count >= 1 and len(lines[0]) > 76:
> +        if count >= 1 and len(lines[0]) > 70:
>              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) ' +
> @@ -208,11 +208,11 @@ class CommitMessageCheck:
>          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
> +            if (len(lines[i]) > 70 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
> @@ -354,10 +354,12 @@ class GitDiffCheck:
>                                    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)
> +        if 'EFI_D_' in line:
> +            self.added_line_error('EFI_D_* used, Please use DEBUG_* format', line)
>  
>      split_diff_re = re.compile(r'''
>                                     (?P<cmd>
>                                         ^ diff \s+ --git \s+ a/.+ \s+ b/.+ $
>                                     )
> @@ -471,11 +473,20 @@ class CheckOnePatch:
>              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('[')
> +        pfx_start_tmp = self.commit_subject.find('[')
> +        pfx_end = self.commit_subject.find(']', pfx_start_tmp)
> +        pfx_start = pfx_start_tmp
> +        while (pfx_end > pfx_start_tmp):
> +            pfx_start_tmp = self.commit_subject.find('[', pfx_end)
> +            if pfx_start_tmp >= 0:
> +                pfx_end = self.commit_subject.find(']',pfx_start_tmp)
> +            else:
> +                break
> +
>          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()
> -- 
> 2.6.1.windows.1
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel

Re: [Patch] BaseTools: PatchCheck to align with wiki and report error for EFI_D_*

[Zhu, Yonghong]

Thanks Jordan. I will separate the patches and send out the updated version.

Besides can you help to update the wiki ? 

Best Regards,
Zhu Yonghong


-----Original Message-----
From: Justen, Jordan L 
Sent: Sunday, October 16, 2016 4:14 AM
To: Zhu, Yonghong <yonghong.zhu@intel.com>; edk2-devel@lists.01.org
Cc: Gao, Liming <liming.gao@intel.com>
Subject: Re: [edk2] [Patch] BaseTools: PatchCheck to align with wiki and report error for EFI_D_*

On 2016-10-13 02:29:46, Yonghong Zhu wrote:
> This patch updates PatchCheck.py：

Can you split this into 3 separate patches?

> 1.Update max line length to 70 that describe in wiki

I think we should update PatchCheck.py and the wiki to say that:

1. The subject line of the commit message should be < 72 characters.

This works well with git log --oneline to still produce output less than 80 characters.

2. The other lines of the commit message should be < 76 characters.

When using git log, these lines are indented 4 characters, so once again this will keep the log message readable and under 80 columns.

> 2.remove the two [] when do the char count calculation

How about instead we add a new regex to the class:

    subject_prefix_re = \
        re.compile(r'''^
                           \s* (\[
                               [^\[\]]* # Allow all non-brackets
                           \])? \s*
                   ''',
                   re.VERBOSE)


Then you can use it line this:

          self.commit_subject = \
	      self.subject_prefix_re.sub('', self.commit_subject, 1)

This will change:

  ' a' => 'a'
  ' [patch 1] a' => 'a'

But, it will ignore this, because I don't think we should try to match brackets here:

  ' [a [b]] a'

> 3.report error for EFI_D_* macro if it is used, recommend to use
> DEBUG_* format.

This should definitely be a separate patch since we are starting to look at code.

-Jordan

> 
> Cc: Liming Gao <liming.gao@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Yonghong Zhu <yonghong.zhu@intel.com>
> ---
>  BaseTools/Scripts/PatchCheck.py | 21 ++++++++++++++++-----
>  1 file changed, 16 insertions(+), 5 deletions(-)
> 
> diff --git a/BaseTools/Scripts/PatchCheck.py 
> b/BaseTools/Scripts/PatchCheck.py index 455c130..d423220 100755
> --- a/BaseTools/Scripts/PatchCheck.py
> +++ b/BaseTools/Scripts/PatchCheck.py
> @@ -1,9 +1,9 @@
>  ## @file
>  #  Check a patch for various format issues  # -#  Copyright (c) 2015, 
> Intel Corporation. All rights reserved.<BR>
> +#  Copyright (c) 2015 - 2016, 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
> @@ -14,11 +14,11 @@
>  #
>  
>  from __future__ import print_function
>  
>  VersionNumber = '0.1'
> -__copyright__ = "Copyright (c) 2015, Intel Corporation  All rights reserved."
> +__copyright__ = "Copyright (c) 2015 - 2016, Intel Corporation  All rights reserved."
>  
>  import email
>  import argparse
>  import os
>  import re
> @@ -195,11 +195,11 @@ class CommitMessageCheck:
>  
>          if count <= 0:
>              self.error('Empty commit message!')
>              return
>  
> -        if count >= 1 and len(lines[0]) > 76:
> +        if count >= 1 and len(lines[0]) > 70:
>              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) ' 
> + @@ -208,11 +208,11 @@ class CommitMessageCheck:
>          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
> +            if (len(lines[i]) > 70 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
> @@ -354,10 +354,12 @@ class GitDiffCheck:
>                                    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)
> +        if 'EFI_D_' in line:
> +            self.added_line_error('EFI_D_* used, Please use DEBUG_* 
> + format', line)
>  
>      split_diff_re = re.compile(r'''
>                                     (?P<cmd>
>                                         ^ diff \s+ --git \s+ a/.+ \s+ b/.+ $
>                                     )
> @@ -471,11 +473,20 @@ class CheckOnePatch:
>              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('[')
> +        pfx_start_tmp = self.commit_subject.find('[')
> +        pfx_end = self.commit_subject.find(']', pfx_start_tmp)
> +        pfx_start = pfx_start_tmp
> +        while (pfx_end > pfx_start_tmp):
> +            pfx_start_tmp = self.commit_subject.find('[', pfx_end)
> +            if pfx_start_tmp >= 0:
> +                pfx_end = self.commit_subject.find(']',pfx_start_tmp)
> +            else:
> +                break
> +
>          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()
> --
> 2.6.1.windows.1
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel

Re: [Patch] BaseTools: PatchCheck to align with wiki and report error for EFI_D_*

[Jordan Justen]

On 2016-10-16 06:32:32, Zhu, Yonghong wrote:
> Thanks Jordan. I will separate the patches and send out the updated version.
> 
> Besides can you help to update the wiki ? 
> 

How does it look?

https://github.com/tianocore/tianocore.github.io/wiki/Commit-Message-Format

-Jordan

> 
> -----Original Message-----
> From: Justen, Jordan L 
> Sent: Sunday, October 16, 2016 4:14 AM
> To: Zhu, Yonghong <yonghong.zhu@intel.com>; edk2-devel@lists.01.org
> Cc: Gao, Liming <liming.gao@intel.com>
> Subject: Re: [edk2] [Patch] BaseTools: PatchCheck to align with wiki and report error for EFI_D_*
> 
> On 2016-10-13 02:29:46, Yonghong Zhu wrote:
> > This patch updates PatchCheck.py：
> 
> Can you split this into 3 separate patches?
> 
> > 1.Update max line length to 70 that describe in wiki
> 
> I think we should update PatchCheck.py and the wiki to say that:
> 
> 1. The subject line of the commit message should be < 72 characters.
> 
> This works well with git log --oneline to still produce output less than 80 characters.
> 
> 2. The other lines of the commit message should be < 76 characters.
> 
> When using git log, these lines are indented 4 characters, so once again this will keep the log message readable and under 80 columns.
> 
> > 2.remove the two [] when do the char count calculation
> 
> How about instead we add a new regex to the class:
> 
>     subject_prefix_re = \
>         re.compile(r'''^
>                            \s* (\[
>                                [^\[\]]* # Allow all non-brackets
>                            \])? \s*
>                    ''',
>                    re.VERBOSE)
> 
> 
> Then you can use it line this:
> 
>           self.commit_subject = \
>               self.subject_prefix_re.sub('', self.commit_subject, 1)
> 
> This will change:
> 
>   ' a' => 'a'
>   ' [patch 1] a' => 'a'
> 
> But, it will ignore this, because I don't think we should try to match brackets here:
> 
>   ' [a [b]] a'
> 
> > 3.report error for EFI_D_* macro if it is used, recommend to use
> > DEBUG_* format.
> 
> This should definitely be a separate patch since we are starting to look at code.
> 
> -Jordan
> 
> > 
> > Cc: Liming Gao <liming.gao@intel.com>
> > Contributed-under: TianoCore Contribution Agreement 1.0
> > Signed-off-by: Yonghong Zhu <yonghong.zhu@intel.com>
> > ---
> >  BaseTools/Scripts/PatchCheck.py | 21 ++++++++++++++++-----
> >  1 file changed, 16 insertions(+), 5 deletions(-)
> > 
> > diff --git a/BaseTools/Scripts/PatchCheck.py 
> > b/BaseTools/Scripts/PatchCheck.py index 455c130..d423220 100755
> > --- a/BaseTools/Scripts/PatchCheck.py
> > +++ b/BaseTools/Scripts/PatchCheck.py
> > @@ -1,9 +1,9 @@
> >  ## @file
> >  #  Check a patch for various format issues  # -#  Copyright (c) 2015, 
> > Intel Corporation. All rights reserved.<BR>
> > +#  Copyright (c) 2015 - 2016, 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
> > @@ -14,11 +14,11 @@
> >  #
> >  
> >  from __future__ import print_function
> >  
> >  VersionNumber = '0.1'
> > -__copyright__ = "Copyright (c) 2015, Intel Corporation  All rights reserved."
> > +__copyright__ = "Copyright (c) 2015 - 2016, Intel Corporation  All rights reserved."
> >  
> >  import email
> >  import argparse
> >  import os
> >  import re
> > @@ -195,11 +195,11 @@ class CommitMessageCheck:
> >  
> >          if count <= 0:
> >              self.error('Empty commit message!')
> >              return
> >  
> > -        if count >= 1 and len(lines[0]) > 76:
> > +        if count >= 1 and len(lines[0]) > 70:
> >              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) ' 
> > + @@ -208,11 +208,11 @@ class CommitMessageCheck:
> >          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
> > +            if (len(lines[i]) > 70 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
> > @@ -354,10 +354,12 @@ class GitDiffCheck:
> >                                    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)
> > +        if 'EFI_D_' in line:
> > +            self.added_line_error('EFI_D_* used, Please use DEBUG_* 
> > + format', line)
> >  
> >      split_diff_re = re.compile(r'''
> >                                     (?P<cmd>
> >                                         ^ diff \s+ --git \s+ a/.+ \s+ b/.+ $
> >                                     )
> > @@ -471,11 +473,20 @@ class CheckOnePatch:
> >              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('[')
> > +        pfx_start_tmp = self.commit_subject.find('[')
> > +        pfx_end = self.commit_subject.find(']', pfx_start_tmp)
> > +        pfx_start = pfx_start_tmp
> > +        while (pfx_end > pfx_start_tmp):
> > +            pfx_start_tmp = self.commit_subject.find('[', pfx_end)
> > +            if pfx_start_tmp >= 0:
> > +                pfx_end = self.commit_subject.find(']',pfx_start_tmp)
> > +            else:
> > +                break
> > +
> >          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()
> > --
> > 2.6.1.windows.1
> > 
> > _______________________________________________
> > edk2-devel mailing list
> > edk2-devel@lists.01.org
> > https://lists.01.org/mailman/listinfo/edk2-devel

Re: [Patch] BaseTools: PatchCheck to align with wiki and report error for EFI_D_*

[Zhu, Yonghong]

It's good to me. I will send patch to align with this wiki.

Best Regards,
Zhu Yonghong


-----Original Message-----
From: Justen, Jordan L 
Sent: Monday, October 17, 2016 1:50 PM
To: Zhu, Yonghong <yonghong.zhu@intel.com>; edk2-devel@lists.01.org
Cc: Gao, Liming <liming.gao@intel.com>; Zhu, Yonghong <yonghong.zhu@intel.com>
Subject: RE: [edk2] [Patch] BaseTools: PatchCheck to align with wiki and report error for EFI_D_*

On 2016-10-16 06:32:32, Zhu, Yonghong wrote:
> Thanks Jordan. I will separate the patches and send out the updated version.
> 
> Besides can you help to update the wiki ? 
> 

How does it look?

https://github.com/tianocore/tianocore.github.io/wiki/Commit-Message-Format

-Jordan

> 
> -----Original Message-----
> From: Justen, Jordan L
> Sent: Sunday, October 16, 2016 4:14 AM
> To: Zhu, Yonghong <yonghong.zhu@intel.com>; edk2-devel@lists.01.org
> Cc: Gao, Liming <liming.gao@intel.com>
> Subject: Re: [edk2] [Patch] BaseTools: PatchCheck to align with wiki 
> and report error for EFI_D_*
> 
> On 2016-10-13 02:29:46, Yonghong Zhu wrote:
> > This patch updates PatchCheck.py：
> 
> Can you split this into 3 separate patches?
> 
> > 1.Update max line length to 70 that describe in wiki
> 
> I think we should update PatchCheck.py and the wiki to say that:
> 
> 1. The subject line of the commit message should be < 72 characters.
> 
> This works well with git log --oneline to still produce output less than 80 characters.
> 
> 2. The other lines of the commit message should be < 76 characters.
> 
> When using git log, these lines are indented 4 characters, so once again this will keep the log message readable and under 80 columns.
> 
> > 2.remove the two [] when do the char count calculation
> 
> How about instead we add a new regex to the class:
> 
>     subject_prefix_re = \
>         re.compile(r'''^
>                            \s* (\[
>                                [^\[\]]* # Allow all non-brackets
>                            \])? \s*
>                    ''',
>                    re.VERBOSE)
> 
> 
> Then you can use it line this:
> 
>           self.commit_subject = \
>               self.subject_prefix_re.sub('', self.commit_subject, 1)
> 
> This will change:
> 
>   ' a' => 'a'
>   ' [patch 1] a' => 'a'
> 
> But, it will ignore this, because I don't think we should try to match brackets here:
> 
>   ' [a [b]] a'
> 
> > 3.report error for EFI_D_* macro if it is used, recommend to use
> > DEBUG_* format.
> 
> This should definitely be a separate patch since we are starting to look at code.
> 
> -Jordan
> 
> > 
> > Cc: Liming Gao <liming.gao@intel.com>
> > Contributed-under: TianoCore Contribution Agreement 1.0
> > Signed-off-by: Yonghong Zhu <yonghong.zhu@intel.com>
> > ---
> >  BaseTools/Scripts/PatchCheck.py | 21 ++++++++++++++++-----
> >  1 file changed, 16 insertions(+), 5 deletions(-)
> > 
> > diff --git a/BaseTools/Scripts/PatchCheck.py 
> > b/BaseTools/Scripts/PatchCheck.py index 455c130..d423220 100755
> > --- a/BaseTools/Scripts/PatchCheck.py
> > +++ b/BaseTools/Scripts/PatchCheck.py
> > @@ -1,9 +1,9 @@
> >  ## @file
> >  #  Check a patch for various format issues  # -#  Copyright (c) 
> > 2015, Intel Corporation. All rights reserved.<BR>
> > +#  Copyright (c) 2015 - 2016, 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
> > @@ -14,11 +14,11 @@
> >  #
> >  
> >  from __future__ import print_function
> >  
> >  VersionNumber = '0.1'
> > -__copyright__ = "Copyright (c) 2015, Intel Corporation  All rights reserved."
> > +__copyright__ = "Copyright (c) 2015 - 2016, Intel Corporation  All rights reserved."
> >  
> >  import email
> >  import argparse
> >  import os
> >  import re
> > @@ -195,11 +195,11 @@ class CommitMessageCheck:
> >  
> >          if count <= 0:
> >              self.error('Empty commit message!')
> >              return
> >  
> > -        if count >= 1 and len(lines[0]) > 76:
> > +        if count >= 1 and len(lines[0]) > 70:
> >              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) ' 
> > + @@ -208,11 +208,11 @@ class CommitMessageCheck:
> >          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
> > +            if (len(lines[i]) > 70 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
> > @@ -354,10 +354,12 @@ class GitDiffCheck:
> >                                    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)
> > +        if 'EFI_D_' in line:
> > +            self.added_line_error('EFI_D_* used, Please use DEBUG_* 
> > + format', line)
> >  
> >      split_diff_re = re.compile(r'''
> >                                     (?P<cmd>
> >                                         ^ diff \s+ --git \s+ a/.+ \s+ b/.+ $
> >                                     ) @@ -471,11 +473,20 @@ class 
> > CheckOnePatch:
> >              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('[')
> > +        pfx_start_tmp = self.commit_subject.find('[')
> > +        pfx_end = self.commit_subject.find(']', pfx_start_tmp)
> > +        pfx_start = pfx_start_tmp
> > +        while (pfx_end > pfx_start_tmp):
> > +            pfx_start_tmp = self.commit_subject.find('[', pfx_end)
> > +            if pfx_start_tmp >= 0:
> > +                pfx_end = self.commit_subject.find(']',pfx_start_tmp)
> > +            else:
> > +                break
> > +
> >          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()
> > --
> > 2.6.1.windows.1
> > 
> > _______________________________________________
> > edk2-devel mailing list
> > edk2-devel@lists.01.org
> > https://lists.01.org/mailman/listinfo/edk2-devel

Re: [Patch] BaseTools: PatchCheck to align with wiki and report error for EFI_D_*

[Jordan Justen]

On 2016-10-16 06:32:32, Zhu, Yonghong wrote:
> Thanks Jordan. I will separate the patches and send out the updated version.
> 

One other suggestion for the commit message on your patch. Can you add
this at the bottom before the Contributed-under?

Fixes: https://bugzilla.tianocore.org/show_bug.cgi?id=113

What do you think?

Maybe we should update the commit message wiki page to suggest this as
well?

-Jordan

>
> -----Original Message-----
> From: Justen, Jordan L 
> Sent: Sunday, October 16, 2016 4:14 AM
> To: Zhu, Yonghong <yonghong.zhu@intel.com>; edk2-devel@lists.01.org
> Cc: Gao, Liming <liming.gao@intel.com>
> Subject: Re: [edk2] [Patch] BaseTools: PatchCheck to align with wiki and report error for EFI_D_*
> 
> On 2016-10-13 02:29:46, Yonghong Zhu wrote:
> > This patch updates PatchCheck.py：
> 
> Can you split this into 3 separate patches?
> 
> > 1.Update max line length to 70 that describe in wiki
> 
> I think we should update PatchCheck.py and the wiki to say that:
> 
> 1. The subject line of the commit message should be < 72 characters.
> 
> This works well with git log --oneline to still produce output less than 80 characters.
> 
> 2. The other lines of the commit message should be < 76 characters.
> 
> When using git log, these lines are indented 4 characters, so once again this will keep the log message readable and under 80 columns.
> 
> > 2.remove the two [] when do the char count calculation
> 
> How about instead we add a new regex to the class:
> 
>     subject_prefix_re = \
>         re.compile(r'''^
>                            \s* (\[
>                                [^\[\]]* # Allow all non-brackets
>                            \])? \s*
>                    ''',
>                    re.VERBOSE)
> 
> 
> Then you can use it line this:
> 
>           self.commit_subject = \
>               self.subject_prefix_re.sub('', self.commit_subject, 1)
> 
> This will change:
> 
>   ' a' => 'a'
>   ' [patch 1] a' => 'a'
> 
> But, it will ignore this, because I don't think we should try to match brackets here:
> 
>   ' [a [b]] a'
> 
> > 3.report error for EFI_D_* macro if it is used, recommend to use
> > DEBUG_* format.
> 
> This should definitely be a separate patch since we are starting to look at code.
> 
> -Jordan
> 
> > 
> > Cc: Liming Gao <liming.gao@intel.com>
> > Contributed-under: TianoCore Contribution Agreement 1.0
> > Signed-off-by: Yonghong Zhu <yonghong.zhu@intel.com>
> > ---
> >  BaseTools/Scripts/PatchCheck.py | 21 ++++++++++++++++-----
> >  1 file changed, 16 insertions(+), 5 deletions(-)
> > 
> > diff --git a/BaseTools/Scripts/PatchCheck.py 
> > b/BaseTools/Scripts/PatchCheck.py index 455c130..d423220 100755
> > --- a/BaseTools/Scripts/PatchCheck.py
> > +++ b/BaseTools/Scripts/PatchCheck.py
> > @@ -1,9 +1,9 @@
> >  ## @file
> >  #  Check a patch for various format issues  # -#  Copyright (c) 2015, 
> > Intel Corporation. All rights reserved.<BR>
> > +#  Copyright (c) 2015 - 2016, 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
> > @@ -14,11 +14,11 @@
> >  #
> >  
> >  from __future__ import print_function
> >  
> >  VersionNumber = '0.1'
> > -__copyright__ = "Copyright (c) 2015, Intel Corporation  All rights reserved."
> > +__copyright__ = "Copyright (c) 2015 - 2016, Intel Corporation  All rights reserved."
> >  
> >  import email
> >  import argparse
> >  import os
> >  import re
> > @@ -195,11 +195,11 @@ class CommitMessageCheck:
> >  
> >          if count <= 0:
> >              self.error('Empty commit message!')
> >              return
> >  
> > -        if count >= 1 and len(lines[0]) > 76:
> > +        if count >= 1 and len(lines[0]) > 70:
> >              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) ' 
> > + @@ -208,11 +208,11 @@ class CommitMessageCheck:
> >          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
> > +            if (len(lines[i]) > 70 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
> > @@ -354,10 +354,12 @@ class GitDiffCheck:
> >                                    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)
> > +        if 'EFI_D_' in line:
> > +            self.added_line_error('EFI_D_* used, Please use DEBUG_* 
> > + format', line)
> >  
> >      split_diff_re = re.compile(r'''
> >                                     (?P<cmd>
> >                                         ^ diff \s+ --git \s+ a/.+ \s+ b/.+ $
> >                                     )
> > @@ -471,11 +473,20 @@ class CheckOnePatch:
> >              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('[')
> > +        pfx_start_tmp = self.commit_subject.find('[')
> > +        pfx_end = self.commit_subject.find(']', pfx_start_tmp)
> > +        pfx_start = pfx_start_tmp
> > +        while (pfx_end > pfx_start_tmp):
> > +            pfx_start_tmp = self.commit_subject.find('[', pfx_end)
> > +            if pfx_start_tmp >= 0:
> > +                pfx_end = self.commit_subject.find(']',pfx_start_tmp)
> > +            else:
> > +                break
> > +
> >          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()
> > --
> > 2.6.1.windows.1
> > 
> > _______________________________________________
> > edk2-devel mailing list
> > edk2-devel@lists.01.org
> > https://lists.01.org/mailman/listinfo/edk2-devel

Re: [Patch] BaseTools: PatchCheck to align with wiki and report error for EFI_D_*

[Zhu, Yonghong]

Sure, I will add this info.

Yes, add this suggestion is good.

Best Regards,
Zhu Yonghong

-----Original Message-----
From: Justen, Jordan L 
Sent: Monday, October 17, 2016 1:56 PM
To: Zhu, Yonghong <yonghong.zhu@intel.com>; edk2-devel@lists.01.org
Cc: Gao, Liming <liming.gao@intel.com>; Zhu, Yonghong <yonghong.zhu@intel.com>
Subject: RE: [edk2] [Patch] BaseTools: PatchCheck to align with wiki and report error for EFI_D_*

On 2016-10-16 06:32:32, Zhu, Yonghong wrote:
> Thanks Jordan. I will separate the patches and send out the updated version.
> 

One other suggestion for the commit message on your patch. Can you add this at the bottom before the Contributed-under?

Fixes: https://bugzilla.tianocore.org/show_bug.cgi?id=113

What do you think?

Maybe we should update the commit message wiki page to suggest this as well?

-Jordan

>
> -----Original Message-----
> From: Justen, Jordan L
> Sent: Sunday, October 16, 2016 4:14 AM
> To: Zhu, Yonghong <yonghong.zhu@intel.com>; edk2-devel@lists.01.org
> Cc: Gao, Liming <liming.gao@intel.com>
> Subject: Re: [edk2] [Patch] BaseTools: PatchCheck to align with wiki 
> and report error for EFI_D_*
> 
> On 2016-10-13 02:29:46, Yonghong Zhu wrote:
> > This patch updates PatchCheck.py：
> 
> Can you split this into 3 separate patches?
> 
> > 1.Update max line length to 70 that describe in wiki
> 
> I think we should update PatchCheck.py and the wiki to say that:
> 
> 1. The subject line of the commit message should be < 72 characters.
> 
> This works well with git log --oneline to still produce output less than 80 characters.
> 
> 2. The other lines of the commit message should be < 76 characters.
> 
> When using git log, these lines are indented 4 characters, so once again this will keep the log message readable and under 80 columns.
> 
> > 2.remove the two [] when do the char count calculation
> 
> How about instead we add a new regex to the class:
> 
>     subject_prefix_re = \
>         re.compile(r'''^
>                            \s* (\[
>                                [^\[\]]* # Allow all non-brackets
>                            \])? \s*
>                    ''',
>                    re.VERBOSE)
> 
> 
> Then you can use it line this:
> 
>           self.commit_subject = \
>               self.subject_prefix_re.sub('', self.commit_subject, 1)
> 
> This will change:
> 
>   ' a' => 'a'
>   ' [patch 1] a' => 'a'
> 
> But, it will ignore this, because I don't think we should try to match brackets here:
> 
>   ' [a [b]] a'
> 
> > 3.report error for EFI_D_* macro if it is used, recommend to use
> > DEBUG_* format.
> 
> This should definitely be a separate patch since we are starting to look at code.
> 
> -Jordan
> 
> > 
> > Cc: Liming Gao <liming.gao@intel.com>
> > Contributed-under: TianoCore Contribution Agreement 1.0
> > Signed-off-by: Yonghong Zhu <yonghong.zhu@intel.com>
> > ---
> >  BaseTools/Scripts/PatchCheck.py | 21 ++++++++++++++++-----
> >  1 file changed, 16 insertions(+), 5 deletions(-)
> > 
> > diff --git a/BaseTools/Scripts/PatchCheck.py 
> > b/BaseTools/Scripts/PatchCheck.py index 455c130..d423220 100755
> > --- a/BaseTools/Scripts/PatchCheck.py
> > +++ b/BaseTools/Scripts/PatchCheck.py
> > @@ -1,9 +1,9 @@
> >  ## @file
> >  #  Check a patch for various format issues  # -#  Copyright (c) 
> > 2015, Intel Corporation. All rights reserved.<BR>
> > +#  Copyright (c) 2015 - 2016, 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
> > @@ -14,11 +14,11 @@
> >  #
> >  
> >  from __future__ import print_function
> >  
> >  VersionNumber = '0.1'
> > -__copyright__ = "Copyright (c) 2015, Intel Corporation  All rights reserved."
> > +__copyright__ = "Copyright (c) 2015 - 2016, Intel Corporation  All rights reserved."
> >  
> >  import email
> >  import argparse
> >  import os
> >  import re
> > @@ -195,11 +195,11 @@ class CommitMessageCheck:
> >  
> >          if count <= 0:
> >              self.error('Empty commit message!')
> >              return
> >  
> > -        if count >= 1 and len(lines[0]) > 76:
> > +        if count >= 1 and len(lines[0]) > 70:
> >              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) ' 
> > + @@ -208,11 +208,11 @@ class CommitMessageCheck:
> >          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
> > +            if (len(lines[i]) > 70 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
> > @@ -354,10 +354,12 @@ class GitDiffCheck:
> >                                    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)
> > +        if 'EFI_D_' in line:
> > +            self.added_line_error('EFI_D_* used, Please use DEBUG_* 
> > + format', line)
> >  
> >      split_diff_re = re.compile(r'''
> >                                     (?P<cmd>
> >                                         ^ diff \s+ --git \s+ a/.+ \s+ b/.+ $
> >                                     ) @@ -471,11 +473,20 @@ class 
> > CheckOnePatch:
> >              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('[')
> > +        pfx_start_tmp = self.commit_subject.find('[')
> > +        pfx_end = self.commit_subject.find(']', pfx_start_tmp)
> > +        pfx_start = pfx_start_tmp
> > +        while (pfx_end > pfx_start_tmp):
> > +            pfx_start_tmp = self.commit_subject.find('[', pfx_end)
> > +            if pfx_start_tmp >= 0:
> > +                pfx_end = self.commit_subject.find(']',pfx_start_tmp)
> > +            else:
> > +                break
> > +
> >          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()
> > --
> > 2.6.1.windows.1
> > 
> > _______________________________________________
> > edk2-devel mailing list
> > edk2-devel@lists.01.org
> > https://lists.01.org/mailman/listinfo/edk2-devel