Issue #196: refactoring hook handling
Reported by: | domruf |
State: | new |
Created on: | 2016-02-17 12:24 |
Updated on: | 2016-02-19 16:30 |
Description
The hook handling is scattered around multiple places incl. git hook files that are run out side of the kalithea process.
This causes problems like #22 and lets multiple test fail on my windows machine.
I'd like clean this up and get rid of the git hook files. @kiilerix is there a reason why we should keep the hook files? Will they be needed for future ssh support?
Attachments
Comments
Comment by Mads Kiilerich, on 2016-02-17 17:01
I don't know much about this and has barely touched it.
It seemed like the git hooks files are needed because there not is any other simple way to see what actually has been pushed. On Mercurial we use Mercurial push hooks which will run in the same process/thread for this and we rely on (thread)global variables, IIRC.
I guess we instead perhaps could compare the repo state before and after a push ...
This probably also ties into the nice-to-have feature of branch access control.
Comment by domruf, on 2016-02-17 20:55
OK I will look into it and see if I can get what has been push without the hook files.
What do you mean by branch access control? Allowing to push only on certain branches?
Comment by Mads Kiilerich, on 2016-02-18 02:44
Yes, branch access control could be something like that.
But are the hooks really that bad? I really don't know. But it seems like we could have a similar "call out to the right virtualenv" issue for ssh access.
Comment by domruf, on 2016-02-18 12:04
My main problem is that the hook files fail in my windows test environment. (meaning I can't push to git repos in this test instance) I'm not sure if it is windows specific yet. But non the less it is a tricky problem to debug because of the fact that they don't run in the kallithea context. And there is a big risk that other users run in the same problem with their setup.
Comment by domruf, on 2016-02-18 12:07
@kiilerix what is that current status of ssh support? How will hooks be handled when ssh is ready? Can you point me to the current development branch?
Comment by Mads Kiilerich, on 2016-02-18 17:43
ssh support "lives" in a PR you can find here
Comment by Teemu Vesala, on 2016-02-19 05:41
Make sure that hooks are "single line" at pre- or post-receive files. That way it is easier to use multiple hooks at single git-project. The valid use case are e.g. using gitolite for access control or adding lint check to the push.
Comment by domruf, on 2016-02-19 14:25
@teemu_vesala I'm not sure what you mean. Why do hooks have to be single lines? I mean currently they are, but why is that important? I see no reason why (in the future) it should not be possible to configure multi line hooks.
Comment by domruf, on 2016-02-19 14:41
@kiilerix regarding your question "But are the hooks really that bad?" here is a list of things that I think are bad:
-
hooks don't work with git (in my setup for multiple reasons)
a) the shebang is "#!/usr/bin/env python2" and python2 does not exist on windows
b) when "from kallithea.lib.hooks import handle_git_pre_receive as _handler" is called the script terminates. It does not simply raises an exception. It terminates the whole script.
c) even if the above would work AFAIK lib.hooks.handle_git_receive does not call any hook. Instead it directly calls log_push_action and pre_push.
-
similar to 1c log_push_action is call directly in ScmModel._handle_push instead of calling all hooks
- when edited via the web interface, log_push_action is called directly instead of triggering all hooks.
Comment by Mads Kiilerich, on 2016-02-19 16:30
Sure, bugs are bad. Bugs should be fixed. They do not necessarily prove that hooks are bad.
I really don't know, but while I agree they are problematic and fragile, I am not convinced that there are any better options.
I think we will find the best solution by trying to improve the existing solution to its best ... and then see if something else can be either better. But feel free to jump directly to "something else" ;-)