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

> "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?

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