[PATCH] git: move non-kallithea hooks and execute other hooks

Sat Oct 24 11:05:18 UTC 2020

On 10/23/20 9:30 AM, Tim Ooms wrote:
>>> # HG changeset patch
>>> # User Tim Ooms <tatankat at users.noreply.github.com>
>>> # Date 1603201903 -7200
>>> #      Tue Oct 20 15:51:43 2020 +0200
>>> # Node ID 672e57b165d0c1774b692b5706a174bf98f42e4c
>>> # Parent  b9b53e25a08d3714c54d82641b419e6d01820e12
>>> git: move non-kallithea hooks and execute other hooks
>
>> It might have rough edges to bypass Kallithea when accessing 
>> Kallithea-managed repos. It is perhaps better to go with the 
>> assumption that Kallithea fully owns the repositories it manage.
>
> I tried to not bypass Kallithea by letting it execute the other hooks. 
> I know the mercurial hooks are more integrated, but as the git hooks 
> don't work the same way I think this is the next best thing. Although 
> there may be better ways, I'm not sure.

If you only access the repos through Kallithea, I think it would be 
better to run your hooks as Kallithea extensions. I'm not sure that's 
possible right now, but we can perhaps fix that.

> Can you say more about your use case? What other hooks are installed? 
> What precautions do they use to avoid clobbering the Kallithea hooks?
>
> Use cases I think of:
> post:
> * trigger CI build (like jenkins)
> * notify bug tracker (like the bug genie)
> pre:
> * check for commit message
> * verify syntax/codestyle
> Other examples: 
> https://scriptrunner.adaptavist.com/stash/3.0.14-beta1/docs/pre_receive_hooks/
> These hooks are really independent and don't touch the repository. If 
> they fail, it has no effect on the other hooks. A hook can mess up if 
> it just hangs and does not return. But you will always have this if 
> people are using custom hooks, even in mercurial.

Thanks. Some good examples of "hooks" into the default process.

I understand you install these hooks yourself - it is not some 3rd party 
system doing it. Your purpose with installing the hooks is to customize 
the Kallithea process.

In that case, would it perhaps be better to hook into Kallithea 
"extensions" instead of hooking in as Git hooks? If we don't have the 
right kind of extension hooks, perhaps we should introduce that?

> It must be a general challenge with Git to fit multiple hooks into the 
> single hook slot. Is there any prior art for solving / working around 
> that problem?
>
> Yes, there is:
> * solution using "pee": 
> https://git.seveas.net/using-multiple-post-receive-hooks.html

Nice. But depends on perl. It would be nice to have it as Python. But I 
guess Git already depends on perl ...

> * using bash built-ins: 
> https://serverfault.com/questions/909153/how-to-add-multiple-post-receive-hooks-to-a-git-repository

It seems like the hooks can have multiple lines on stdin. This batch 
script will invoke the hooks multiple times in that case. That might be 
unfortunate. And it also doesn't detect failures.

And unfortunately, it seems like they both just describe how it *can* be 
done. It is not a standard way of doing it.

>> It would be nice if there was a "standard" dispatcher (perhaps a 
>> simple shell script) that could be installed in The Hook Location - 
>> something that picked up multiple hook files. Instead of having our 
>> hook call others, it would perhaps be better to install a generic-ish 
>> hook and let it dispatch to both our hook and the existing one.
>
> I would indeed like a shell solution (I used the bash solution till 
> now, but till now that causes problems when doing an update), but that 
> ain't portable and I think Kallithea can run on Windows. It can have 
> advantages to let Kallithea run the other hooks: they can be managed 
> (in a later stage) by Kallithea, can be logged by Kallithea and admins 
> can be notified if a hook fails to run.
> Also, running directly the Kallithea hook, Kallithea -being the one 
> managing the repository- is sure that its hook is correctly called and 
> it's shebang is correctly set and executable.
>
> This can also be done using something generic-ish (and I like the 
> idea), but you add another layer that can fail and another file to 
> keep up to date like the existing hook.

As a step in the right direction, we can perhaps let Kallithea 
create/overwrite post-receive.d/kallithea.hook *if* post-receive already 
exists *and* isn't a kallithea hook, *and* if post-receive.d exists.

That will leave it to you to install the dispatcher when installing your 
hooks.

>> The solution proposed here also touch upon lessons learned from 
>> general /etc/xxx.rc/ configuration systems: People will often want to 
>> rename configuration files to something like '.bu' or '.old' or 
>> '.disabled'. It is thus a very good idea to only match on one file 
>> extension - and perhaps also a prefix. The "real" hooks do not have 
>> any extension, but the globbed ones could perhaps match 
>> 'pre-receive.*.hook'? Also, we should check that they are executable 
>> for the current user before trying to run them.
>
> Nice catches! That would indeed improve the robustness. A 
> try-catch-log would probably also be in place, so we always try all 
> hooks.
> For pre-receive hooks, we should also check the return code of all 
> hooks and return non-zero if one of them does not return 0. For 
> post-receive, only the Kallithea return code matters.
>
>> For the chmod changes, I think we only should care about the hooks we 
>> install. If the existing hook isn't executable, git will ignore it 
>> and we should do the same - not try to clean things up we don't know 
>> about. We just wrote our own hook file, so we know it isn't a 
>> symlink, and 0755 should be fine? Ideally, we should probably use 
>> umask and set X bit ... but I doubt that ever will make any difference.
>
> I only change the permissions of the Kallithea hook. The check for the 
> symlink is because the Kallithea hook could have been changed by an 
> admin to a symlink. Kallithea then logs an error about not being able 
> to write the hook, while in fact the hook was updated, only the mode 
> could not be changed.

It seems like a Kallithea bug: If "reinstalling" a Git hook, and the 
existing hook is a symlink, it will rewrite the symlink target. That is 
most certainly the wrong thing to do. It should let the file replace the 
symlink. The best way to fix that bug would probably be to always write 
hook files atomically: first write to non-existing file with an 
additional random suffix, then rename that into the right filename (and 
thus replace the symlink with an actual file).

First of all, we should fix that bug. Then, let's revisit remaining 
problems regarding chmod (if any).

> The umask can make a difference if you let your web administrators not 
> run under the web server's user, but they should be able to manage the 
> hooks. That is why you would like to have g+w set and not be removed 
> by the software.

Ok, we can read the old mode from the new file and set the X bit.

/Mads