[PATCH 0/6] BaseTools: Improve PatchCheck and add GitPostCommitHook

[PATCH 0/6] BaseTools: Improve PatchCheck and add GitPostCommitHook

[Philippe Mathieu-Daudé]

Hi,

This series add a script that can be use as git-post-commit
hook to run CheckPatch after each commit.

I avoided shell scripts and use Python, so it should be
usable by Visual Studio, but as I don't use it, I haven't
tested there. However it is helpful for my use case, from
a Linux console, because I won't forget to use CheckPatch
again.

In Patch #3 (GitPostCommitHook) I commented a problem with
adding CRLF scripts on Unix, we can not run them directly.

Patch #1 is actually a minor bug fix.

Regards,

Phil.

Cc: Bob Feng <bob.c.feng@intel.com>
Cc: Liming Gao <liming.gao@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Leif Lindholm <leif.lindholm@linaro.org>

Philippe Mathieu-Daude (6):
  BaseTools/PatchCheck: Stop parsing commit message after --- separator
  BaseTools/PatchCheck: Add a --quiet option
  BaseTools/Scripts: Add GitPostCommitHook.py
  BaseTools/PatchCheck: Extract the run_git() function
  BaseTools/PatchCheck: Add printw() function to print warnings
  BaseTools/PatchCheck: Allow to print colored warnings

 BaseTools/Scripts/GitPostCommitHook.py | 21 ++++++++
 BaseTools/Scripts/PatchCheck.py        | 70 ++++++++++++++++++--------
 2 files changed, 70 insertions(+), 21 deletions(-)
 create mode 100755 BaseTools/Scripts/GitPostCommitHook.py

-- 
2.21.0

[PATCH 1/6] BaseTools/PatchCheck: Stop parsing commit message after --- separator

[Philippe Mathieu-Daudé]

git-format-patch uses the '---' separator to mark the end of the
commit message. Anything after the separator is stripped by git-am,
it is pointless to check this part.

Cc: Bob Feng <bob.c.feng@intel.com>
Cc: Liming Gao <liming.gao@intel.com>
Signed-off-by: Philippe Mathieu-Daude <philmd@redhat.com>
---
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Leif Lindholm <leif.lindholm@linaro.org>
---
 BaseTools/Scripts/PatchCheck.py | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/BaseTools/Scripts/PatchCheck.py b/BaseTools/Scripts/PatchCheck.py
index 2a4e6f603e79..8bcf857a7d15 100755
--- a/BaseTools/Scripts/PatchCheck.py
+++ b/BaseTools/Scripts/PatchCheck.py
@@ -209,6 +209,10 @@ class CommitMessageCheck:
                        'empty.')
 
         for i in range(2, count):
+            if lines[i].strip() == '---':
+                # '---' marks the end of the commit message.
+                count = i
+                break
             if (len(lines[i]) >= 76 and
                 len(lines[i].split()) > 1 and
                 not lines[i].startswith('git-svn-id:')):
-- 
2.21.0

Re: [PATCH 1/6] BaseTools/PatchCheck: Stop parsing commit message after --- separator

[Laszlo Ersek]

On 12/05/19 17:12, Philippe Mathieu-Daude wrote:
> git-format-patch uses the '---' separator to mark the end of the
> commit message. Anything after the separator is stripped by git-am,
> it is pointless to check this part.
> 
> Cc: Bob Feng <bob.c.feng@intel.com>
> Cc: Liming Gao <liming.gao@intel.com>
> Signed-off-by: Philippe Mathieu-Daude <philmd@redhat.com>
> ---
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Leif Lindholm <leif.lindholm@linaro.org>
> ---
>  BaseTools/Scripts/PatchCheck.py | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/BaseTools/Scripts/PatchCheck.py b/BaseTools/Scripts/PatchCheck.py
> index 2a4e6f603e79..8bcf857a7d15 100755
> --- a/BaseTools/Scripts/PatchCheck.py
> +++ b/BaseTools/Scripts/PatchCheck.py
> @@ -209,6 +209,10 @@ class CommitMessageCheck:
>                         'empty.')
>  
>          for i in range(2, count):
> +            if lines[i].strip() == '---':
> +                # '---' marks the end of the commit message.
> +                count = i
> +                break
>              if (len(lines[i]) >= 76 and
>                  len(lines[i].split()) > 1 and
>                  not lines[i].startswith('git-svn-id:')):
> 

This shortcut seems to prevent two checks (on the affected area):
- line length check,
- separation of the signature block by an empty line.

While preventing the second check in the affected area certainly makes
sense, I think preserving the line length check would be nice.

For example, "git-notes" places notes in that area, and if someone uses
a text editor configuration in which hard line breaks are not inserted,
we could end up with a humongous note there. Even though it would not be
applied by git-am, I think we should not get such notes posted to the list.

(This text editor configuration is not theoretical: earlier we used to
get actual commit messages with no hard line breaks.)

So... I don't want to complicate this for you needlessly, but you
mention in the series blurb that this patch is a bugfix. What is the bug?

Hmmm... does the check (mistakenly) fire if the diffstat is very wide?
(It can easily be very wide, because our pathnames are long.) Hmmm.

Okay, I agree that's a much more practical problem than what I describe
above. So:

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Thanks
Laszlo

[PATCH 2/6] BaseTools/PatchCheck: Add a --quiet option

[Philippe Mathieu-Daudé]

Add a --quiet option to only display warnings.
This will be useful when this script is called embedded.

Cc: Bob Feng <bob.c.feng@intel.com>
Cc: Liming Gao <liming.gao@intel.com>
Signed-off-by: Philippe Mathieu-Daude <philmd@redhat.com>
---
 BaseTools/Scripts/PatchCheck.py | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/BaseTools/Scripts/PatchCheck.py b/BaseTools/Scripts/PatchCheck.py
index 8bcf857a7d15..f907e96b9501 100755
--- a/BaseTools/Scripts/PatchCheck.py
+++ b/BaseTools/Scripts/PatchCheck.py
@@ -19,7 +19,7 @@ import subprocess
 import sys
 
 class Verbose:
-    SILENT, ONELINE, NORMAL = range(3)
+    SILENT, QUIET, ONELINE, NORMAL = range(4)
     level = NORMAL
 
 class CommitMessageCheck:
@@ -56,10 +56,10 @@ class CommitMessageCheck:
             print(self.url)
 
     def error(self, *err):
-        if self.ok and Verbose.level > Verbose.ONELINE:
+        if self.ok and Verbose.level >= Verbose.QUIET:
             print('The commit message format is not valid:')
         self.ok = False
-        if Verbose.level < Verbose.NORMAL:
+        if Verbose.level in [Verbose.SILENT, Verbose.ONELINE]:
             return
         count = 0
         for line in err:
@@ -404,10 +404,10 @@ class GitDiffCheck:
         self.error(err, err2)
 
     def error(self, *err):
-        if self.ok and Verbose.level > Verbose.ONELINE:
+        if self.ok and Verbose.level >= Verbose.QUIET:
             print('Code format is not valid:')
         self.ok = False
-        if Verbose.level < Verbose.NORMAL:
+        if Verbose.level in [Verbose.SILENT, Verbose.ONELINE]:
             return
         count = 0
         for line in err:
@@ -530,7 +530,8 @@ class CheckGitCommits:
                     print()
                 else:
                     blank_line = True
-                print('Checking git commit:', commit)
+                if Verbose.level > Verbose.QUIET:
+                    print('Checking git commit:', commit)
             patch = self.read_patch_from_git(commit)
             self.ok &= CheckOnePatch(commit, patch).ok
         if not commits:
@@ -634,12 +635,17 @@ class PatchCheckApp:
         group.add_argument("--oneline",
                            action="store_true",
                            help="Print one result per line")
+        group.add_argument("--quiet",
+                           action="store_true",
+                           help="Only print warnings and errors")
         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.quiet:
+            Verbose.level = Verbose.QUIET
         if self.args.silent:
             Verbose.level = Verbose.SILENT
 
-- 
2.21.0

[PATCH 3/6] BaseTools/Scripts: Add GitPostCommitHook.py

[Philippe Mathieu-Daudé]

Git allows the use of various hooks (see [*]). In particular it
provides the 'post-commit' hook, which is a convenient place to
run the PatchCheck script.

[*] https://git-scm.com/book/en/v2/Customizing-Git-Git-Hooks

Cc: Bob Feng <bob.c.feng@intel.com>
Cc: Liming Gao <liming.gao@intel.com>
Signed-off-by: Philippe Mathieu-Daude <philmd@redhat.com>
---
I'm hitting a egg/chicken problem here.

If I add this script without CRLF, PatchCheck complains:

 Code format is not valid:
  * Line ending ('\n') is not CRLF
    File: BaseTools/Scripts/GitPostCommitHook.py
    Line: #!/usr/bin/python

However if I convert it to CRLF, then Linux does not recognize the
shebang, and if I symlink the script, when the hook is executed I get:

  fatal: cannot run .git/hooks/post-commit: No such file or directory

Because the interpreter expects the shebang line with Unix line ending
(linefeed only).

As a kludge I'm happy using:

  $ echo > .git/hooks/post-commit
  #!/bin/sh
  test -e BaseTools/Scripts/GitPostCommitHook.py && exec python3 BaseTools/Scripts/GitPostCommitHook.py
  EOF
  $ chmod +x .git/hooks/post-commit

(The 'test' allow to checkout to older references where the script
is not available).

Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Leif Lindholm <leif.lindholm@linaro.org>
---
 BaseTools/Scripts/GitPostCommitHook.py | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)
 create mode 100755 BaseTools/Scripts/GitPostCommitHook.py

diff --git a/BaseTools/Scripts/GitPostCommitHook.py b/BaseTools/Scripts/GitPostCommitHook.py
new file mode 100755
index 000000000000..4ea933a7eedf
--- /dev/null
+++ b/BaseTools/Scripts/GitPostCommitHook.py
@@ -0,0 +1,21 @@
+#!/usr/bin/python
+## @file
+#  Run PatchCheck on the last commit.
+#
+#  Copyright (c) 2019, Red Hat, Inc.
+#
+#  SPDX-License-Identifier: BSD-2-Clause-Patent
+#
+# To have git run this script after each commit, create a symbolic
+# link or copy it to .git/hooks/post-commit in your EDK2 repository.
+
+import os
+import sys
+
+if __name__ == '__main__':
+    sys.path.append(os.path.join(os.environ['EDK_TOOLS_PATH'], 'Scripts'))
+    from PatchCheck import CheckGitCommits, Verbose
+    Verbose.level = Verbose.QUIET
+    GIT_REFERENCE = 'HEAD'
+    COMMIT_COUNT = 1
+    sys.exit(CheckGitCommits(GIT_REFERENCE, COMMIT_COUNT).ok)
-- 
2.21.0

Re: [PATCH 3/6] BaseTools/Scripts: Add GitPostCommitHook.py

[Laszlo Ersek]

On 12/05/19 17:12, Philippe Mathieu-Daude wrote:
> Git allows the use of various hooks (see [*]). In particular it
> provides the 'post-commit' hook, which is a convenient place to
> run the PatchCheck script.
> 
> [*] https://git-scm.com/book/en/v2/Customizing-Git-Git-Hooks
> 
> Cc: Bob Feng <bob.c.feng@intel.com>
> Cc: Liming Gao <liming.gao@intel.com>
> Signed-off-by: Philippe Mathieu-Daude <philmd@redhat.com>
> ---
> I'm hitting a egg/chicken problem here.
> 
> If I add this script without CRLF, PatchCheck complains:
> 
>  Code format is not valid:
>   * Line ending ('\n') is not CRLF
>     File: BaseTools/Scripts/GitPostCommitHook.py
>     Line: #!/usr/bin/python
> 
> However if I convert it to CRLF, then Linux does not recognize the
> shebang, and if I symlink the script, when the hook is executed I get:
> 
>   fatal: cannot run .git/hooks/post-commit: No such file or directory
> 
> Because the interpreter expects the shebang line with Unix line ending
> (linefeed only).

(1) To my understanding (and I could be wrong), we don't add shebangs to
python code in edk2 that is supposed to be run on both Windows and
Linux. Removing the shebang altogether would solve your problem.

The new problem is then: how to invoke the new script, from a git hook.
I *feel* (but I'm unsure) that this is going to be OS-specific anyway,
is it not? See for example the python scripts in BaseTools -- we have
OS-specific wrappers for them:

  BaseTools/BinWrappers/PosixLike/build
  BaseTools/BinWrappers/WindowsLike/build.bat

After you source "edksetup.sh", or else run "edksetup.bat", the proper
wrapper will be used, when you subsequently type "build".

So I think something similar would be necessary for a git hook too. (TBH
I don't even know how git hooks are supposed to look & work on Windows
-- but on Linux anyway, it could be a shell script that explicitly
launches the python intepreter, with "GitPostCommitHook.py").

> 
> As a kludge I'm happy using:
> 
>   $ echo > .git/hooks/post-commit
>   #!/bin/sh
>   test -e BaseTools/Scripts/GitPostCommitHook.py && exec python3 BaseTools/Scripts/GitPostCommitHook.py
>   EOF
>   $ chmod +x .git/hooks/post-commit
> 
> (The 'test' allow to checkout to older references where the script
> is not available).

Right, exactly -- this is not even a "kludge", but it approaches what I
think is our existing practice.

Just drop the shebang from the new python script, and locate the python
interpreter more flexibly (like the BinWrappers do). You could even
introduce the necessary BinWrapper(s), and then invoke that from the hook.

(BTW I mean the above only as food for thought & discussion, not as
something that you "must" implement.)


(2) I'd like to mention a scenario that makes me think twice about any
commit-related hook that can fail (i.e., exit with nonzero status):

What if you are in the middle of a complex rebase, stage the new changes
for the currently revised patch, and then run "git rebase --continue"
(expecting an automatic amendment, without explicitly running "git
commit --amend" first)?

I don't know how git-rebase behaves when such a hook fails
mid-processing. And I don't really want to find out :)

Hmmm, wait, let me see githooks(5):

   post-commit
       This hook is invoked by git-commit(1). It takes no parameters,
       and is invoked after a commit is made.

       This hook is meant primarily for notification, and cannot
       affect the outcome of git commit.

That's in fact reassuring! It means that, whatever happens in this hook,
it cannot break git-rebase. That's nice. So there is no concern in that
regard.

I think this (good) behavior should be mentioned in the commit message
(or even in the script).

So...

> 
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Leif Lindholm <leif.lindholm@linaro.org>
> ---
>  BaseTools/Scripts/GitPostCommitHook.py | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
>  create mode 100755 BaseTools/Scripts/GitPostCommitHook.py
> 
> diff --git a/BaseTools/Scripts/GitPostCommitHook.py b/BaseTools/Scripts/GitPostCommitHook.py
> new file mode 100755
> index 000000000000..4ea933a7eedf
> --- /dev/null
> +++ b/BaseTools/Scripts/GitPostCommitHook.py
> @@ -0,0 +1,21 @@
> +#!/usr/bin/python

I think the shebang shoud go, and...

> +## @file
> +#  Run PatchCheck on the last commit.
> +#
> +#  Copyright (c) 2019, Red Hat, Inc.
> +#
> +#  SPDX-License-Identifier: BSD-2-Clause-Patent
> +#
> +# To have git run this script after each commit, create a symbolic
> +# link or copy it to .git/hooks/post-commit in your EDK2 repository.

these instructions should be updated, according to (1).

Thanks,
Laszlo

> +
> +import os
> +import sys
> +
> +if __name__ == '__main__':
> +    sys.path.append(os.path.join(os.environ['EDK_TOOLS_PATH'], 'Scripts'))
> +    from PatchCheck import CheckGitCommits, Verbose
> +    Verbose.level = Verbose.QUIET
> +    GIT_REFERENCE = 'HEAD'
> +    COMMIT_COUNT = 1
> +    sys.exit(CheckGitCommits(GIT_REFERENCE, COMMIT_COUNT).ok)
> 

[PATCH 4/6] BaseTools/PatchCheck: Extract the run_git() function

[Philippe Mathieu-Daudé]

Extract run_git() from the CheckGitCommits class, so we can
reuse it from other places.

Cc: Bob Feng <bob.c.feng@intel.com>
Cc: Liming Gao <liming.gao@intel.com>
Signed-off-by: Philippe Mathieu-Daude <philmd@redhat.com>
---
 BaseTools/Scripts/PatchCheck.py | 23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)

diff --git a/BaseTools/Scripts/PatchCheck.py b/BaseTools/Scripts/PatchCheck.py
index f907e96b9501..fc1b077f3c64 100755
--- a/BaseTools/Scripts/PatchCheck.py
+++ b/BaseTools/Scripts/PatchCheck.py
@@ -18,6 +18,16 @@ import re
 import subprocess
 import sys
 
+def run_git(*args):
+    """Run git in a subprocess, return the command output."""
+    cmd = [ 'git' ]
+    cmd += args
+    p = subprocess.Popen(cmd,
+                         stdout=subprocess.PIPE,
+                         stderr=subprocess.STDOUT)
+    Result = p.communicate()
+    return Result[0].decode('utf-8', 'ignore') if Result[0] and Result[0].find(b"fatal") !=0 else None
+
 class Verbose:
     SILENT, QUIET, ONELINE, NORMAL = range(4)
     level = NORMAL
@@ -543,21 +553,12 @@ class CheckGitCommits:
         if max_count is not None:
             cmd.append('--max-count=' + str(max_count))
         cmd.append(rev_spec)
-        out = self.run_git(*cmd)
+        out = run_git(*cmd)
         return out.split() if out else []
 
     def read_patch_from_git(self, commit):
         # Run git to get the commit patch
-        return self.run_git('show', '--pretty=email', '--no-textconv', commit)
-
-    def run_git(self, *args):
-        cmd = [ 'git' ]
-        cmd += args
-        p = subprocess.Popen(cmd,
-                     stdout=subprocess.PIPE,
-                     stderr=subprocess.STDOUT)
-        Result = p.communicate()
-        return Result[0].decode('utf-8', 'ignore') if Result[0] and Result[0].find(b"fatal")!=0 else None
+        return run_git('show', '--pretty=email', '--no-textconv', commit)
 
 class CheckOnePatchFile:
     """Performs a patch check for a single file.
-- 
2.21.0

[PATCH 5/6] BaseTools/PatchCheck: Add printw() function to print warnings

[Philippe Mathieu-Daudé]

Introduce printw() to print warnings.
For now there are no logical change, we only call print().

Cc: Bob Feng <bob.c.feng@intel.com>
Cc: Liming Gao <liming.gao@intel.com>
Signed-off-by: Philippe Mathieu-Daude <philmd@redhat.com>
---
 BaseTools/Scripts/PatchCheck.py | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/BaseTools/Scripts/PatchCheck.py b/BaseTools/Scripts/PatchCheck.py
index fc1b077f3c64..5692f6eaf8bd 100755
--- a/BaseTools/Scripts/PatchCheck.py
+++ b/BaseTools/Scripts/PatchCheck.py
@@ -28,6 +28,10 @@ def run_git(*args):
     Result = p.communicate()
     return Result[0].decode('utf-8', 'ignore') if Result[0] and Result[0].find(b"fatal") !=0 else None
 
+def printw(line):
+    """Print a warning to the console."""
+    print(line)
+
 class Verbose:
     SILENT, QUIET, ONELINE, NORMAL = range(4)
     level = NORMAL
@@ -67,14 +71,14 @@ class CommitMessageCheck:
 
     def error(self, *err):
         if self.ok and Verbose.level >= Verbose.QUIET:
-            print('The commit message format is not valid:')
+            printw('The commit message format is not valid:')
         self.ok = False
         if Verbose.level in [Verbose.SILENT, Verbose.ONELINE]:
             return
         count = 0
         for line in err:
             prefix = (' *', '  ')[count > 0]
-            print(prefix, line)
+            printw("{}{}".format(prefix, line))
             count += 1
 
     # Find 'contributed-under:' at the start of a line ignoring case and
@@ -415,14 +419,14 @@ class GitDiffCheck:
 
     def error(self, *err):
         if self.ok and Verbose.level >= Verbose.QUIET:
-            print('Code format is not valid:')
+            printw('Code format is not valid:')
         self.ok = False
         if Verbose.level in [Verbose.SILENT, Verbose.ONELINE]:
             return
         count = 0
         for line in err:
             prefix = (' *', '  ')[count > 0]
-            print(prefix, line)
+            printw("{}{}".format(prefix, line))
             count += 1
 
 class CheckOnePatch:
-- 
2.21.0

[PATCH 6/6] BaseTools/PatchCheck: Allow to print colored warnings

[Philippe Mathieu-Daudé]

If the user has the termcolor package installed, and he configured
a 'color.checkpatch.error' color, use it to display colored warnings.

Example to display warnings in red:

  $ git config color.checkpatch.error red

Cc: Bob Feng <bob.c.feng@intel.com>
Cc: Liming Gao <liming.gao@intel.com>
Signed-off-by: Philippe Mathieu-Daude <philmd@redhat.com>
---
 BaseTools/Scripts/PatchCheck.py | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/BaseTools/Scripts/PatchCheck.py b/BaseTools/Scripts/PatchCheck.py
index 5692f6eaf8bd..8e37311bd69a 100755
--- a/BaseTools/Scripts/PatchCheck.py
+++ b/BaseTools/Scripts/PatchCheck.py
@@ -32,6 +32,19 @@ def printw(line):
     """Print a warning to the console."""
     print(line)
 
+if run_git('config', 'color.ui') != 'false':
+    # If the user configure git color.checkpatch.error and the termcolor package
+    # is available, display warnings using the requested color.
+    warning_color = run_git('config', 'color.checkpatch.error')
+    if warning_color:
+        try:
+            from termcolor import colored, cprint
+            _ = colored("check we can parse the color", warning_color.strip())
+            def printw(line):
+                cprint(line, color=warning_color.strip(), attrs=['bold'])
+        except:
+            pass
+
 class Verbose:
     SILENT, QUIET, ONELINE, NORMAL = range(4)
     level = NORMAL
-- 
2.21.0