[PATCH v2] BaseTools/PatchCheck.py: Ignore CR and LF characters in subject length

[PATCH v2] BaseTools/PatchCheck.py: Ignore CR and LF characters in subject length

[Philippe Mathieu-Daudé]

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

Strip the trailing characters before checking the subject line is
less than 72 characters.

Fixes: e61406708c83f
Cc: Liming Gao <liming.gao@intel.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Reviewed-by: Jordan Justen <jordan.l.justen@intel.com>
Signed-off-by: Philippe Mathieu-Daude <philmd@redhat.com>
---
Cc: Yonghong Zhu <yonghong.zhu@intel.com>
Cc: Zhichao Gao <zhichao.gao@intel.com>
v2: Use rstrip() (Jordan Justen)
v1: https://edk2.groups.io/g/devel/message/52425
---
 BaseTools/Scripts/PatchCheck.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/BaseTools/Scripts/PatchCheck.py b/BaseTools/Scripts/PatchCheck.py
index 2a4e6f603e79..9668025798da 100755
--- a/BaseTools/Scripts/PatchCheck.py
+++ b/BaseTools/Scripts/PatchCheck.py
@@ -196,7 +196,7 @@ class CommitMessageCheck:
             self.error('Empty commit message!')
             return
 
-        if count >= 1 and len(lines[0]) >= 72:
+        if count >= 1 and len(lines[0].rstrip()) >= 72:
             self.error('First line of commit message (subject line) ' +
                        'is too long.')
 
-- 
2.21.0

Re: [edk2-devel] [PATCH v2] BaseTools/PatchCheck.py: Ignore CR and LF characters in subject length

[Bob Feng]

Reviewed-by: Bob Feng <bob.c.feng@intel.com>

-----Original Message-----
From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of Philippe Mathieu-Daudé
Sent: Thursday, January 2, 2020 8:17 PM
To: devel@edk2.groups.io
Cc: Philippe Mathieu-Daude <philmd@redhat.com>; Gao, Liming <liming.gao@intel.com>; Justen, Jordan L <jordan.l.justen@intel.com>; Zhu, Yonghong <yonghong.zhu@intel.com>; Gao, Zhichao <zhichao.gao@intel.com>
Subject: [edk2-devel] [PATCH v2] BaseTools/PatchCheck.py: Ignore CR and LF characters in subject length

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

Strip the trailing characters before checking the subject line is less than 72 characters.

Fixes: e61406708c83f
Cc: Liming Gao <liming.gao@intel.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Reviewed-by: Jordan Justen <jordan.l.justen@intel.com>
Signed-off-by: Philippe Mathieu-Daude <philmd@redhat.com>
---
Cc: Yonghong Zhu <yonghong.zhu@intel.com>
Cc: Zhichao Gao <zhichao.gao@intel.com>
v2: Use rstrip() (Jordan Justen)
v1: https://edk2.groups.io/g/devel/message/52425
---
 BaseTools/Scripts/PatchCheck.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/BaseTools/Scripts/PatchCheck.py b/BaseTools/Scripts/PatchCheck.py index 2a4e6f603e79..9668025798da 100755
--- a/BaseTools/Scripts/PatchCheck.py
+++ b/BaseTools/Scripts/PatchCheck.py
@@ -196,7 +196,7 @@ class CommitMessageCheck:
             self.error('Empty commit message!')
             return
 
-        if count >= 1 and len(lines[0]) >= 72:
+        if count >= 1 and len(lines[0].rstrip()) >= 72:
             self.error('First line of commit message (subject line) ' +
                        'is too long.')
 
--
2.21.0

Re: [edk2-devel] [PATCH v2] BaseTools/PatchCheck.py: Ignore CR and LF characters in subject length

[Philippe Mathieu-Daudé]

Hi Liming,

On 1/9/20 6:35 AM, Feng, Bob C wrote:
> Reviewed-by: Bob Feng <bob.c.feng@intel.com>

Is the Review-by tag from Bob enough to get this patch merged?

Thanks,

Phil.

> 
> -----Original Message-----
> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of Philippe Mathieu-Daudé
> Sent: Thursday, January 2, 2020 8:17 PM
> To: devel@edk2.groups.io
> Cc: Philippe Mathieu-Daude <philmd@redhat.com>; Gao, Liming <liming.gao@intel.com>; Justen, Jordan L <jordan.l.justen@intel.com>; Zhu, Yonghong <yonghong.zhu@intel.com>; Gao, Zhichao <zhichao.gao@intel.com>
> Subject: [edk2-devel] [PATCH v2] BaseTools/PatchCheck.py: Ignore CR and LF characters in subject length
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=113
> 
> Strip the trailing characters before checking the subject line is less than 72 characters.
> 
> Fixes: e61406708c83f
> Cc: Liming Gao <liming.gao@intel.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Reviewed-by: Jordan Justen <jordan.l.justen@intel.com>
> Signed-off-by: Philippe Mathieu-Daude <philmd@redhat.com>
> ---
> Cc: Yonghong Zhu <yonghong.zhu@intel.com>
> Cc: Zhichao Gao <zhichao.gao@intel.com>
> v2: Use rstrip() (Jordan Justen)
> v1: https://edk2.groups.io/g/devel/message/52425
> ---
>   BaseTools/Scripts/PatchCheck.py | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/BaseTools/Scripts/PatchCheck.py b/BaseTools/Scripts/PatchCheck.py index 2a4e6f603e79..9668025798da 100755
> --- a/BaseTools/Scripts/PatchCheck.py
> +++ b/BaseTools/Scripts/PatchCheck.py
> @@ -196,7 +196,7 @@ class CommitMessageCheck:
>               self.error('Empty commit message!')
>               return
>   
> -        if count >= 1 and len(lines[0]) >= 72:
> +        if count >= 1 and len(lines[0].rstrip()) >= 72:
>               self.error('First line of commit message (subject line) ' +
>                          'is too long.')
>   
> --
> 2.21.0
> 
> 
> 
> 

Re: [edk2-devel] [PATCH v2] BaseTools/PatchCheck.py: Ignore CR and LF characters in subject length

[Laszlo Ersek]

On 01/20/20 10:11, Philippe Mathieu-Daudé wrote:
> Hi Liming,
> 
> On 1/9/20 6:35 AM, Feng, Bob C wrote:
>> Reviewed-by: Bob Feng <bob.c.feng@intel.com>
> 
> Is the Review-by tag from Bob enough to get this patch merged?

Yes, see the "M" role:

BaseTools
F: BaseTools/
W: https://github.com/tianocore/tianocore.github.io/wiki/BaseTools
M: Bob Feng <bob.c.feng@intel.com>
M: Liming Gao <liming.gao@intel.com>

... In fact, the patch has been committed as 2649a735b249e.

Unfortunately:

- your name has been messed up in the "Author" meta-datum :(

- we don't directly know who committed the patch, becaus "Committer"
only says "mergify[bot]"

Now, based on the subject and the commit date of the patch, I can find
some related notification emails in my "github.com email notifications"
folder. From those, the revelant (merged) GitHub PR seems to be:

https://github.com/tianocore/edk2/pull/283

I agree it's annoying to not have confirmation on the mailing list,
directly in your patch's thread. The only thing I can recommend is,
check the git history periodically, for recently committed patches,
authored by yourself.

Personally, on the maintainer side, I always make sure to confirm a
merge on-list, separately.

Thanks
Laszlo

Re: [edk2-devel] [PATCH v2] BaseTools/PatchCheck.py: Ignore CR and LF characters in subject length

[Philippe Mathieu-Daudé]

On 1/20/20 11:27 AM, Laszlo Ersek wrote:
> On 01/20/20 10:11, Philippe Mathieu-Daudé wrote:
>> Hi Liming,
>>
>> On 1/9/20 6:35 AM, Feng, Bob C wrote:
>>> Reviewed-by: Bob Feng <bob.c.feng@intel.com>
>>
>> Is the Review-by tag from Bob enough to get this patch merged?
> 
> Yes, see the "M" role:
> 
> BaseTools
> F: BaseTools/
> W: https://github.com/tianocore/tianocore.github.io/wiki/BaseTools
> M: Bob Feng <bob.c.feng@intel.com>
> M: Liming Gao <liming.gao@intel.com>
> 
> ... In fact, the patch has been committed as 2649a735b249e.
> 
> Unfortunately:
> 
> - your name has been messed up in the "Author" meta-datum :(

Now I see it, OK. I don't understand charset encoding enough to avoid 
mojibake, but I'll try few things with PatchCheck.

> - we don't directly know who committed the patch, becaus "Committer"
> only says "mergify[bot]"
> 
> Now, based on the subject and the commit date of the patch, I can find
> some related notification emails in my "github.com email notifications"
> folder. From those, the revelant (merged) GitHub PR seems to be:
> 
> https://github.com/tianocore/edk2/pull/283

Thanks Bob for pushing it, sorry for the noise.

Thanks Laszlo for checking and updating me.

> I agree it's annoying to not have confirmation on the mailing list,
> directly in your patch's thread. The only thing I can recommend is,
> check the git history periodically, for recently committed patches,
> authored by yourself.
> 
> Personally, on the maintainer side, I always make sure to confirm a
> merge on-list, separately.
> 
> Thanks
> Laszlo
>