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

["Laszlo Ersek" <lersek@redhat.com>]

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)
>