From: Ben Keene <seraphire@gmail.com>
Date: Wed, 05 Feb 2020 19:56:19 GMT
Subject: Re: [PATCH v2 2/4] git-p4: create new method gitRunHook
Message-ID: <ac44531e-b02d-5a98-3e25-a305b1250cf6@gmail.com>
In-Reply-To: <xmqqr1za6q83.fsf@gitster-ct.c.googlers.com>


On 2/4/2020 3:40 PM, Junio C Hamano wrote:
> "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.
I'm not sure what you mean by top level of the tree unless you mean
that it is not part of a class, but a "Free standing" function? And
yes, it returns a value so it should be called a function. I'll
correct that.  I chose to not put the function within a class so
that if other hooks should be added, it would not require a refactoring
of the code to use the function in other classes.
>> +    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?
Good point.
>
> 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?

Unfortunately, the original code for running the p4-pre-submit hook
was under-developed and there was no way to run other executable
files in the context of a hook. Nothing ran for me, which is what
prompted this change.  But to your point, the restrictions are
unnecessary. I googled around a little and found these two SO articles:

https://stackoverflow.com/questions/18277429/executing-git-hooks-on-windows

https://stackoverflow.com/questions/22074247/git-hook-under-windows

But I haven't found information on how Git for Windows handles
hooks directly.

>
> 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.
I'm committing a new version of this change that will define 
run_hook_command.
>> +    return True
>>   
>>   def main():
>>       if len(sys.argv[1:]) == 0: