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

Wed Nov 4 09:14:31 UTC 2020

Hello Mads,

On 24/10/2020 13:05, Mads Kiilerich wrote:
> 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?

If installing an extension is as easy as installing the git hook 
(meaning: simply put the provided hook code somewhere) this can indeed 
be a good solution (together with something to allow custom hooks).

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

This would indeed be a step in the right direction. The admin can always 
search in the docs how to create the dispatcher if she/he wants to keep 
a single custom hook.
The globbing with pre-&postfix or the subdirectory (cleaner, but can 
break the script) both have there (dis)advantages.

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

If someone put a symbolic link to another hook and ask Kallithea to 
force overwrite the hook, then Kallithea should indeed only overwrite 
the symbolic link.
But when Kallithea verified that the link's destination is a Kallithea 
hook, Kallithea should keep the symbolic link and update the destination.

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

I also added:
a+r as you need read access to execute too
u+w to be able to update the hook

-- 
kind regards,
Tim
-------------- next part --------------
A non-text attachment was scrubbed...
Name: tim_ooms.vcf
Type: text/x-vcard
Size: 4 bytes
Desc: not available
URL: <http://lists.sfconservancy.org/pipermail/kallithea-general/attachments/20201104/9985d224/attachment.vcf>