From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-1.mimecast.com (us-smtp-delivery-1.mimecast.com [205.139.110.61]) by mx.groups.io with SMTP id smtpd.web10.2350.1575573883652613145 for ; Thu, 05 Dec 2019 11:24:43 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=YTHcX2bh; spf=pass (domain: redhat.com, ip: 205.139.110.61, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1575573882; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=6Qx+Y1I3bjqJxIaCswb/caMFhkzYCy/rejYWzbI5a6A=; b=YTHcX2bhHmGC1/1ZjGswLeEXFmo+qF+fWcHphTI/hMLgMFxxVICTQ7TGLZyAWCGh0/8h8h 0IPaxjthKtAMVsQsJlz9JYSoCOLEnS05UUtkenFQiZTTGGWDvtHs6BP6EfiTsT8DIG62nC +LVjcvoSAopApllTs+ETocSz58HILXY= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-402-5FPUZOwdNge0GqDPYNBpZQ-1; Thu, 05 Dec 2019 14:24:39 -0500 Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 3ACA563CC0; Thu, 5 Dec 2019 19:24:38 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-62.ams2.redhat.com [10.36.116.62]) by smtp.corp.redhat.com (Postfix) with ESMTP id DEFED19488; Thu, 5 Dec 2019 19:24:33 +0000 (UTC) Subject: Re: [PATCH 3/6] BaseTools/Scripts: Add GitPostCommitHook.py To: Philippe Mathieu-Daude , devel@edk2.groups.io Cc: Bob Feng , Liming Gao , Ard Biesheuvel , Leif Lindholm References: <20191205161234.25071-1-philmd@redhat.com> <20191205161234.25071-4-philmd@redhat.com> From: "Laszlo Ersek" Message-ID: <1059bc3f-2016-2bac-4e7a-43bb6ee0a723@redhat.com> Date: Thu, 5 Dec 2019 20:24:32 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <20191205161234.25071-4-philmd@redhat.com> X-Scanned-By: MIMEDefang 2.84 on 10.5.11.23 X-MC-Unique: 5FPUZOwdNge0GqDPYNBpZQ-1 X-Mimecast-Spam-Score: 0 Content-Language: en-US Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit 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 > Cc: Liming Gao > Signed-off-by: Philippe Mathieu-Daude > --- > 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 > Cc: Ard Biesheuvel > Cc: Leif Lindholm > --- > 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) >