[PATCH 3 of 3] hooks: fix potentially invalid interpreter in git hooks (Issue #333)

[Mads Kiilerich]

On 4/1/19 10:25 PM, Thomas De Schampheleire wrote:
> # HG changeset patch
> # User Thomas De Schampheleire <thomas.de_schampheleire at nokia.com>
> # Date 1554150088 -7200
> #      Mon Apr 01 22:21:28 2019 +0200
> # Branch stable
> # Node ID 83678cb1c0fa84473f686d94867c777c2671632b
> # Parent  953047e8c88a9c5ccaa4ddc1bb417622de37f01f
> hooks: fix potentially invalid interpreter in git hooks (Issue #333)
>
> Commit 5e501b6ee639c2cf25080ba8c3d53f4e4b6ad32e introduced the use of


(Generally, I suggest using short 12 char hash prefixes for Mercurial.)


> 'sys.executable' as interpreter for git hooks instead of 'python2' with the
> following argument:
>
>      "Windows doesn't necessarily have "python2" available in $PATH, but we
>      still want to make sure we don't end up invoking a python3. Using the
>      absolute path seems more safe."
>
> But, sys.executable does not necessarily point to Python. When Kallithea is
> started under uWSGI, sys.executable points to the uwsgi executable. As a
> result, the git hooks did not work in this case:
>
>      $ git push
>      Password for 'http://user@localhost:5050':
>      Enumerating objects: 3, done.
>      Counting objects: 100% (3/3), done.
>      Writing objects: 100% (3/3), 241 bytes | 241.00 KiB/s, done.
>      Total 3 (delta 0), reused 0 (delta 0)
>      remote: unable to load configuration from hooks/pre-receive
>      To http://localhost:5050/gitrepo-new
>       ! [remote rejected] master -> master (pre-receive hook declined)
>      error: failed to push some refs to 'http://user@localhost:5050/gitrepo-new'


Perhaps more interesting than this high level failure, mention what 
hash-bang line would be inserted into the hook scripts.


> Try to determine a better interpreter for the git hooks by covering multiple
> cases.
>
> In the case of Windows without virtualenv, we are not 100% sure that the
> value will be valid. More heuristics could be added there, but if we can
> assume that most installations on Windows will effectively use a virtualenv,
> which have their own handling in the new code, it may not be worth the
> complexity.


The heuristics can probably never be perfect. Should we have a .ini 
configure option or environment variable (PYTHON_SYS_EXECUTABLE?) for 
overriding it?


> diff --git a/kallithea/model/scm.py b/kallithea/model/scm.py
> --- a/kallithea/model/scm.py
> +++ b/kallithea/model/scm.py
> @@ -720,6 +720,30 @@ class ScmModel(object):
>   
>           return choices, hist_l
>   
> +    def _get_python_executable(self):
> +        """Return a Python executable for use in hooks
> +
> +        This is not simply sys.executable as that points to uwsgi if using
> +        uwsgi.
> +
> +        The returned string is valid when run in the current PATH, it is not
> +        necessarily a full path by itself.
> +        """
> +        # If we are running in a virtualenv, the python in the PATH is the
> +        # right one
> +        if 'VIRTUAL_ENV' in os.environ:
> +            return 'python'
> +
> +        # On Windows, there is no 'python2' executable. We'll use 'python' and
> +        # hope that the user has set their PATH correctly so it points to
> +        # Python 2, not Python 3.
> +        if kallithea.is_windows:
> +            return 'python'
> +
> +        # On Unix, outside of a virtualenv, there should be a 'python2'
> +        # executable in the PATH
> +        return 'python2'


Perhaps also see Mercurial `_usecorrectpython` and `hgcmd` and 
`sysexecutable` and `PYTHON_SYS_EXECUTABLE`. But I guess they don't add 
much info.

This is something that will have to be revisited when switching to 
Python3, but probably hard to be forward-compatible already.


>       def install_git_hooks(self, repo, force_create=False):
>           """
>           Creates a kallithea hook inside a git repository
> @@ -734,11 +758,11 @@ class ScmModel(object):
>           if not os.path.isdir(loc):
>               os.makedirs(loc)
>   
> -        tmpl_post = "#!/usr/bin/env %s\n" % sys.executable or 'python2'
> +        tmpl_post = "#!/usr/bin/env %s\n" % self._get_python_executable()


It seems a bit brutal to just ignore sys.executable completely while 
moving code out. (Perhaps first add the wrapper function without 
changing the heuristics - then change the heuristics with a simple, 
explicit and readable change.

This change should probably also be tested on Windows - I am not sure it 
worked correctly before in most common Windows setups. (Where we 
probably must assume that there is no .cmd wrapper script ...)

Other than that (and without any blockers here), this series looks good 
to me.

/Mads