Subject: Re: [TOPIC 2/17] Hooks in the future

From: Emily Shaffer <[email-masked]>
Date: Thu, 12 Mar 2020 07:16:28 -0700

On Thu, Mar 12, 2020 at 02:56:53PM +1100, James Ramsay wrote:
> 1. Emily: hooks in the config file. Sent a read only patch, but didn’t get
> much traction. Add a new header in the config file, then have prefix number
> so that security scan could be configured at system level that would run
> last, and then hook could also be configured at the project level.
> 
> 2. Peff: Having hooks in the config would be nice. But don’t do it at
> `hooks.prereceive`, but use a subconfig like `hooks.prereceive.command` so
> it’s possible to add options later on.
> 
> 3. Brian: sometimes the need to overridden, ordering works for me. For Git
> LFS it would be helpful to have a pre-push hook for LFS, and a different one
> for something else. Want flexibility about finding and discovering hooks.
> 
> 4. Emily: if you want to specify a hook that is in the work tree, then it
> has to be configured after cloning.
> 
> 5. Jonathan: It’s better to start with something low complexity as long as
> it can be extended/changed later. If there's room to tweak it over time then
> I'm not too worried about the initial version being perfect — we can make
> mistakes and learn from them. A challenge will be how hooks interact.
> Analogy to the challenges of stacked union filesystems and security modules
> in Linux. Analogy to sequence number allocation for unit scripts
> 
> 6. CB: Declare dependencies instead of a sequence number? In theory
> independent hooks can also run in parallel.
> 
> 7. Peff: Maybe that’s something to not worry about from the start. Like, how
> many hooks do you expect to run anyway.
> 
> 8. Christian: At booking.com they use a lot of hooks, and they also sent
> patches to the mailing list to improve that.
> 
> 9. Emily: In-tree hooks?
> 
> 10. Brian: You can do `git cat-file <ref> | sh` to run a hook.
> 
> 11. Brandon: Is it possible to globally to disable all hooks locally? It
> might be a security concern. Or is it something we might want to add?
> 
> 12. Peff: No it’s not.

Thanks for the notes, James.

I came away with the understanding that we want the config hook to look
something like this (barring misunderstanding of config file syntax,
plus or minus naming quibbles):

[hook "/path/to/executable.sh"]
	event = pre-commit

The idea being that by using a subsection, we can extend the format
later much more easily, but by starting simply, we can start using it
and see what we need or don't want. We can use config order to begin
with.

This means that we could do something like this:

[hook "/path/to/executable.sh"]
	event = pre-commit
	order = 123
	mustSucceed = false
	parallelizable = true

etc, etc as needed.

But I wonder if we also want to be able to do something like this:

[hook "/etc/git-secrets/git-secrets"]
	event = pre-commit
	event = prepare-commit-msg
	...

I guess the point is that we can choose to allow this, or not. I could
see there being some trouble if you wanted the execution order to work
differently (e.g. run it first for pre-commit but last for
prepare-commit-msg)...

I think, though, that something like
hook.pre-commit."path/to/executable.sh" won't work. It doesn't seem like
multiple subsections are OK in config syntax, as far as I can see. I'd
be interested to know I'm wrong :)

Will try and get some work on this soon, but honestly my hope is to get
bugreport squared away first.

 - Emily