[PATCH 3 of 3 v2] hooks: fix potentially invalid interpreter in git hooks (Issue #333)

[Mads Kiilerich]

On 4/7/19 9:23 AM, Thomas De Schampheleire wrote:
>
>>         Can we somehow make it show a more useful error message in
>>         this case? I
>>         guess not - none of our code is executed ...
>>
>>
>>     Actually, there is some of our code executed: SimpleGit. But, at
>>     that point you don't know that the git hook call is going to fail.
>
>
>     First: Since we install Git hooks in the file system, they will
>     also be invoked if someone push directly. That may or may not be a
>     feature, but we have to handle it.
>
>
> I don't think this is a valid argument: for mercurial repos the hooks 
> are not installed on the file system. Direct pushing is not supported.


I would say that direct pushing for Mercurial repos is supported, in the 
sense that Kallithea doesn't modify repos at all and doesn't prevent it. 
After direct pushes, you might want to run repo-update-metadata to let 
Kallithea know about it.

With Git hooks installed into the repos, I think we kind of have a 
responsibility to make sure they work in all cases, without error 
messages. If not, users might get the impression that Kallithea 
"destroyed" their repositories. I don't know if I would prefer that the 
hook did nothing if not invoked from Kallithea, or if it would be fine 
if it did logging etc, even though we don't have that with Mercurial.

We could perhaps just decide that it is no big deal and ignore that aspect.


>     We kind of *have* to run something inside the hook. That is the
>     only way to know exactly what is pushed (or pulled). If we want
>     branch access control, it will(!?) also have to run inside hooks.
>
>
> Knowing what was pushed and what not is a more convincing argument. 
> But how do we know this in mercurial? Does mercurial tell us, or do we 
> find out? Is it impossible/fragile to do the same in git?


Yes, the Mercurial hook tells us when the log_push_action hook is invoked.

Git does the same, but it is a bit more work, both because the data 
model and hooks are different, and because it is a separate process. 
Thus we have the git hook which calls into handle_git_post_receive .

(I think it will be more readable in the future as
https://kallithea-scm.org/repos/kallithea-incoming/files/d9c3498964118a7fe6e484572d8c88f85118398f/kallithea/lib/hooks.py#L102
https://kallithea-scm.org/repos/kallithea-incoming/files/d9c3498964118a7fe6e484572d8c88f85118398f/kallithea/lib/hooks.py#L344 
)


>     Summarizing: I don't think that is feasible. But if it is, it will
>     not be simple. Not suitable for the stable branch.
>
>
> Ok. But when deciding what to do on the stable branch, I think it 
> makes sense to have an idea what we'll do for default. I think we 
> should avoid introducing user visible changes like ini settings if 
> they will not apply on default.


Agreed. But I don't think it is feasible to stop using hooks, so in 
either case, we have to make sure they work. This fix is thus important 
and sound: We have to make sure the Git hooks use the right interpreter.

/Mads

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.sfconservancy.org/pipermail/kallithea-general/attachments/20190407/380e82df/attachment.html>