[v2,2/4] git-p4: create new method gitRunHook
diff mbox series

Message ID f1f9fdc542353196612f8dd6b996d4fbd1f76c73.1580507895.git.gitgitgadget@gmail.com
State New
Headers show
Series
  • git-p4: add hook p4-pre-edit-changelist
Related show

Commit Message

Philippe Blain via GitGitGadget Jan. 31, 2020, 9:58 p.m. UTC
From: Ben Keene <seraphire@gmail.com>

This commit is in preparation of introducing new p4 submit hooks.

The current code in the python script git-p4.py makes the assumption
that the git hooks can be executed by subprocess.call() method. However,
when git is run on Windows, this may not work as expected.

The subprocess.call() does not execute SH.EXE implictly under Windows,
so the scripts may fail. In other words, the hooks do not execute under
windows because the shell interpreter is not automatically loaded.

Add a new function, gitRunHook, that takes 2 parameters:
* the filename of an optionally registered git hook
* an optional list of parameters

The gitRunHook function will honor the existing behavior seen in the
current code for executing the p4-pre-submit hook:

* Hooks are looked for in core.hooksPath directory.
* If core.hooksPath is not set, then the current .git/hooks directory
  is checked.
* If the hook does not exist, the function returns True.
* If the hook file is not accessible, the function returns True.
* If the hook returns a zero exit code when executed, the function
  return True.
* If the hook returns a non-zero exit code, the function returns False.

Add new conditional behavior for Windows:
* Check for an evironment variable 'EXEPATH' which should be set by
  git when git-p4.py is envoked.
* If EXEPATH is None - treat it as an empty string.
* If EXEPATH is set, look for sh.exe in the bin/ directory located
  in EXEPATH.
* If EXEPATH is not set, attempt to resolve against "bin/sh.exe"
* Add a new test for Windows that checks to see of sh.exe can be
  located. If not, return True.

Signed-off-by: Ben Keene <seraphire@gmail.com>
---
 git-p4.py | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

Comments

Junio C Hamano Feb. 4, 2020, 8:40 p.m. UTC | #1
"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:
Ben Keene Feb. 5, 2020, 7:56 p.m. UTC | #2
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:
Junio C Hamano Feb. 5, 2020, 9:42 p.m. UTC | #3
Ben Keene <seraphire@gmail.com> writes:

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

No.  The discussion about function vs method was over immediately
after we left the parentheses ;-)

The "top level of the working tree" is the directory where the files
you see in "git ls-tree $commit^{tree}" would appear in.  In our
project, that is where the primary Makefile, COPYING, Documentation/,
etc. hangs from.

The code in your patch (quoted above) says that "When $GIT_DIR is
not set, '.git/hooks/' is the directory the hooks live in".  That is
true only when your process is at the top level of the working tree.
If you chdir'ed to a subdirectory (e.g. Documentation/ in our
project) and then let the quoted code run, hooks_path is set to
".git/hooks/", but from the point of view of the process running
inside "Documentation/" subdirectory, it should actually be
"../.git/hooks/", right?

> And
> yes, it returns a value so it should be called a function. I'll
> correct that.

This is an irrelevant tangent ;-) but yeah, it is a function, as
opposed to a method, because it is not attached to any class.  I did
not think Python differentiated functions that return a value and
ones that do not (e.g. Pascal call the latter "procedure").

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

I think that is a sensible design decision to have a free-standing
function to execute hooks.

Thanks.
Ben Keene Feb. 6, 2020, 2 p.m. UTC | #4
On 2/5/2020 4:42 PM, Junio C Hamano wrote:
> Ben Keene <seraphire@gmail.com> writes:
>
>>>> +        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?
> No.  The discussion about function vs method was over immediately
> after we left the parentheses ;-)
>
> The "top level of the working tree" is the directory where the files
> you see in "git ls-tree $commit^{tree}" would appear in.  In our
> project, that is where the primary Makefile, COPYING, Documentation/,
> etc. hangs from.
>
> The code in your patch (quoted above) says that "When $GIT_DIR is
> not set, '.git/hooks/' is the directory the hooks live in".  That is
> true only when your process is at the top level of the working tree.
> If you chdir'ed to a subdirectory (e.g. Documentation/ in our
> project) and then let the quoted code run, hooks_path is set to
> ".git/hooks/", but from the point of view of the process running
> inside "Documentation/" subdirectory, it should actually be
> "../.git/hooks/", right?

Okay, NOW I get what you meant.  Thank you for the explanation!
The hook directory resolution is what was originally found in the
git-p4.py script.  I just tried to describe what it is doing
because I moved the code around.

As best as I understand the mechanics, and I haven't examined the
source code of git, this is just experimental, the GIT_DIR environment
value is set (at least in Git for Windows) when the program is
executed, so this always has GIT_DIR set, which may be why this
wasn't an issue in the past?

Does this prompt the need to search the hierarchy if we don't find
the directory?

>> And
>> yes, it returns a value so it should be called a function. I'll
>> correct that.
> This is an irrelevant tangent ;-) but yeah, it is a function, as
> opposed to a method, because it is not attached to any class.  I did
> not think Python differentiated functions that return a value and
> ones that do not (e.g. Pascal call the latter "procedure").
>
>> 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.
> I think that is a sensible design decision to have a free-standing
> function to execute hooks.
>
> Thanks.
Junio C Hamano Feb. 6, 2020, 6:26 p.m. UTC | #5
Ben Keene <seraphire@gmail.com> writes:

> On 2/5/2020 4:42 PM, Junio C Hamano wrote:
>> Ben Keene <seraphire@gmail.com> writes:
>>
>>>>> +        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.
>>> ...
> As best as I understand the mechanics, and I haven't examined the
> source code of git, this is just experimental,...

As I already said that I do not know the code well, it is useless
for you to also speculate.  One of us must read the code before
speaking further ;-)

I just scanned "def main()", and it seems that it always goes to the
top-level of the working tree by doing chdir(cdup), where cdup is
learned from "git rev-parse --show-cdup", i.e. "tell me how to chdir
up to the top-level of the working tree".

I am assuming that nobody runs "git p4" in a bare repository, so
under that assumption, I think it would be safe to say that we can
assume we are always at the top.  Also, GIT_DIR is exported from
there, so it probably is a good idea to make sure that the run-hook
helper just uses os.environ.get("GIT_DIR") and barfs if the environ
is not set (i.e. there is something funny going on) without
pretending that it is prepared to deal with such a case, which is
what the "[, default]" parameter to .get method is doing.

I.e.

	hooks_path = os.path.join(os.environ["GIT_DIR"], "hooks")
    
> Does this prompt the need to search the hierarchy if we don't find
> the directory?

No, we just saw that it is done early in "def main()".  It is done
by "rev-parse --git-dir" and "rev-parse --show-cdup".

Patch
diff mbox series

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")
+
+    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
+    return True
 
 def main():
     if len(sys.argv[1:]) == 0: