Re: [PATCH v2 2/4] git-p4: create new method gitRunHook

[Junio C Hamano <gitster@pobox.com>]

"Ben Keene via GitGitGadget" <gitgitgadget@gmail.com> writes:

> diff --git a/git-p4.py b/git-p4.py
> index 7d8a5ee788..4e481b3b55 100755
> --- a/git-p4.py
> +++ b/git-p4.py
> @@ -4125,6 +4125,35 @@ def printUsage(commands):
>      "unshelve" : P4Unshelve,
>  }
>  
> +def gitRunHook(cmd, param=[]):
> +    """Execute a hook if the hook exists."""
> +    if verbose:
> +        sys.stderr.write("Looking for hook: %s\n" % cmd)
> +        sys.stderr.flush()
> +
> +    hooks_path = gitConfig("core.hooksPath")
> +    if len(hooks_path) <= 0:
> +        hooks_path = os.path.join(os.environ.get("GIT_DIR", ".git"), "hooks")

This assumes that the process when his function is called (by the
way, even though the title of the patch uses the word "method", this
is not a method but a function, no?), it is always at the top level
of the working tree.  Is that a good assumption?  I don't know the
code well, so "yes it is good because a very early thing we do is to
go up to the top" is a good answer.

> +    hook_file = os.path.join(hooks_path, cmd)
> +    if isinstance(param,basestring):
> +        param=[param]
> +
> +    if platform.system() == 'Windows':
> +        exepath = os.environ.get("EXEPATH")
> +        if exepath is None:
> +            exepath = ""
> +        shexe = os.path.join(exepath, "bin", "sh.exe")
> +        if os.path.isfile(shexe) \
> +            and os.path.isfile(hook_file) \
> +            and os.access(hook_file, os.X_OK) \
> +            and subprocess.call([shexe, hook_file] + param) != 0:
> +            return False
> +
> +    else:
> +        if os.path.isfile(hook_file) and os.access(hook_file, os.X_OK) and subprocess.call([hook_file] + param) != 0:
> +            return False

Doesn't this mean that on Windows, a hook MUST be written as a shell
script, but on other platforms, a hook can be any executable?

I am not sure if it is necessary to penalize Windows users this way.
How do other parts of the system run hooks on Windows?  E.g. can
"pre-commit" hook be an executable Python script on Windows?

Even if it is needed to have different implementations (and possibly
reduced capabilities) for "we found this file is a hook, now run it
with these parameters" on different platform, the above looks a bit
inverted.  If the code in this function were

    if os.path.isfile(hook_file) and
       os.access(hook_file, os.X_OK) and
       run_hook_command(hook_file, param) != 0:
	return False
    else:
	return True

and a thin helper function whose only task is "now run it with these
parameters" is separately written, e.g.

    def run_hook_command(hook_file, params):
	if Windows:
		... do things in Windows specific way ...
	else:
		return subprocess.call([hook_file] + param)

That would have been 100% better, as it would have made it clear
that logically gitRunHook() does exactly the same thing on all
platforms (i.e. find where the hook is, normalize param, check if
the hook file is actually enabled, and finally execute the hook with
the param), while the details of how the "execute" part (and only
that part) works may be different.

> +    return True
>  
>  def main():
>      if len(sys.argv[1:]) == 0: