Re: Feature request: hooks directory

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Aug 31, 2018, 13:43 UTC
Message-ID: <874lfad537.fsf@evledraar.gmail.com>
In-Reply-To: <CAEpdsiZFMphQTdJnqFYH03M80W8CcrCbr2Uewktm0jy4D+Fz1A@mail.gmail.com>

On Fri, Aug 31 2018, Wesley Schwengle wrote:

Show 24 quoted lines

> Hop,
>
> 2018-08-30 16:45 GMT+02:00 Ævar Arnfjörð Bjarmason <avarab@gmail.com>:
>
>>> Solution:
>>> We discussed this at work and we thought about making a .d directory
>>> for the hooks, eg.  $GIT_DIR/hooks/post-commit.d, where a user can put
>>> the post-commit hooks in. This allows us to provide post commit hooks
>>> and allows the user to add additional hooks him/herself. We could
>>> implement this in our own code base. But we were wondering if this
>>> approach could be shared with the git community and if this behavior
>>> is wanted in git itself.
>>
>> There is interest in this. This E-Mail of mine gives a good summary of
>> prior discussions about this:
>> https://public-inbox.org/git/877eqqnq22.fsf@evledraar.gmail.com/
>>
>> I.e. it's something I've personally been interested in doing in the
>> past, there's various bolt-on solutions to do it (basically local hook
>> runners) used by various projects.
>
> Thank you for the input. Do you by any chance still have that branch?
> Or would you advise me to start fresh, if so, do you have any pointers
> on where to look as I'm brand new to the git source code?

No, sorry. Start by grepping the hook names found in the githooks manpage in the C code.

One of the things that's hard, well not hard, just tedious about this, is that most of them are implementing their own ad-hoc way of doing stuff. E.g. the *-receive hooks are in receive-pack.c in run_receive_hook().

There is run_hook_le() and friends, but it's not used by everything.

So e.g. for the pre-receive hook in order to run 2 of them instead of 1 you need to untangle the state where we're feeding the hook with the input (and potentially buffer it, not stream it), instead of doing it as a one-off as we're doing now.

Then some hooks get nothing on stdin, some get stuff on stdin, some produce output on stdout/stderr etc.

As a first approximation, just add a e.g. support for a pre-receive.2 hook, that gets run after pre-receive, to see what needs to be done to run it twice.

Show 5 quoted lines

> From the thread I've extracted three stories:
>
> 1) As a developer I want to have 'hooks.multiHooks' to enable
> multi-hook support in git
> Input is welcome for another name.

Yeah maybe we should have a setting, but FWIW I think we should just stat() whether the hooks/$hook_name.d directory exist, and then use it, although maybe we'll need stuff like hooks.multiHooks to give the likes of GitLab (which already do that themselves) an upgrade path...

You can see their implementation here: https://gitlab.com/gitlab-org/gitlab-shell/blob/master/lib/gitlab_custom_hook.rb

> 2) As a developer I want natural sort order executing for my githooks
> so I can predict executions
> See https://public-inbox.org/git/CACBZZX6AYBYeb5S4nEBhYbx1r=icJ81JGYBx5=H4wacPhHjFbQ@mail.gmail.com/
> for more information

Yeah, any sane implementation of this will execute the hooks in hooks/$hook_name.d in glob() order.

> 3) As a developer I want to run $GIT_DIR/hooks/<hook> before
> $GIT_DIR/hooks/<hook>.d/*
> Reference: https://public-inbox.org/git/CACBZZX6j6q2DUN_Z-Pnent1u714dVNPFBrL_PiEQyLmCzLUVxg@mail.gmail.com/

For e.g. GitLab the hook/pre-receive is a runner that'll run all hook/pre-receive.d/*, so this probably makes sense to hide behind a config setting. I think it's sensible as a default to move to to just try to move away from hooks/<hook> and use hook/<hook>.d/* instead.

Show 6 quoted lines

> The following story would be.. nice to have I think. I'm not sure I
> would want to implement this from the get go as I don't have a use
> case for it.
> 4) As a developer I want a way to have a hook report an error and let
> another hook decide if we want to pass or not.
> Reference: https://public-inbox.org/git/xmqq60v4don1.fsf@gitster.mtv.corp.google.com/

I think a default that makes more sense is a while ! ret = glob(<hooks>) loop, i.e. a failure will do early exit. But yeah. Junio seemed to want this to be configurable.

Show 14 quoted lines

> 2018-08-31 5:16 GMT+02:00 Jonathan Nieder <jrnieder@gmail.com>:
>> A few unrelated thoughts, to expand on this.
>>
>> Separately from that, in [1] I mentioned that I want to revamp how
>> hooks work somewhat, to avoid the attack described there (or the more
>> common attack also described there that involves a zip file).  Such a
>> revamp would be likely to also handle this multiple-hook use case.
>>
>> [1] https://public-inbox.org/git/20171002234517.GV19555@aiede.mtv.corp.google.com/
>
> The zip file attack vector doesn't change with adding a hook.d
> directory structure? If I have one file or multiple files, the attack
> stays the same?
> I think I'm asking if this would be a show stopper for the feature.

Yeah I don't see how what Jonathan's talking about there has any relevance to whether we run 1 or 100 hooks.