| Message ID | f1f9fdc542353196612f8dd6b996d4fbd1f76c73.1580507895.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | git-p4: add hook p4-pre-edit-changelist | expand |
"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:
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:
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.
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.
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".
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: