On 2/4/2020 3:40 PM, Junio C Hamano wrote:
"Ben Keene via GitGitGadget" <gitgitgadget@xxxxxxxxx> 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:
