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

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 4, 2020, 20:40 UTC
Message-ID: <xmqqr1za6q83.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <f1f9fdc542353196612f8dd6b996d4fbd1f76c73.1580507895.git.gitgitgadget@gmail.com>

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

Show 17 quoted lines

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

Show 18 quoted lines

> +    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: