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

[Emily Shaffer <emilyshaffer@google.com>]

On Fri, Apr 10, 2020 at 05:31:46PM -0400, Jeff King wrote:
> On Tue, Apr 07, 2020 at 04:51:16PM -0700, Emily Shaffer wrote:
> 
> > On Tue, Apr 07, 2020 at 04:01:32PM -0700, Emily Shaffer wrote:
> > > Thoughts?
> > 
> > Jonathan Nieder and I discussed this a little bit offline, and he
> > suggested another thought:
> > 
> > [hook "unique-name"]
> >   pre-commit = ~/path-to-hook.sh args-for-precommit
> >   pre-push = ~/path-to-hook.sh
> >   order = 001
> 
> Yeah, giving each block a unique name lets you give them each an order.
> It seems kind of weird to me that you'd define multiple hook types for a
> given name.

Not so odd - git-secrets configures itself for pre-commit,
prepare-commit-msg, and commit-msg-hook. The invocation is slightly
different ('git-secrets pre-commit', 'git-secrets prepare-commit-msg',
etc) but to me it still makes some sense to treat it as a single logical
unit.

> And it doesn't leave a lot of room for defining
> per-hook-type options; you have to make new keys like pre-push-order
> (though that does work because the hook names are a finite set that
> conforms to our config key names).

Oh, interesting. I think you're saying "what if option 'frotz' only
makes sense for prepare-commit-msg; then there's no reason to allow
'frotz' and 'prepare-commit-msg-frotz' and 'post-commit-frotz' and so
on? I think I didn't do a great job explaining myself in that mail, but
my idea was to let an unqualified option name in a hook block set the
default, and then allow it to be overridden by qualifying it with the
name of the hook in question:

[hook "unique-name"]
  option = "some default"
  post-commit-option = "post-commit specific version"
  pre-push = ~/foo.sh pre-push
  post-commit = ~/foo.sh post-commit

Then when post-commit is invoked, option = "post-commit specific
version"; when pre-push is invoked, option = "some default". My
intention was to generate the hook-specific option key on the fly during
setup.

> 
> What if we added a layer of indirection: have a section for each type of
> hook, defining keys for that type. And then for each hook command we
> define there, it can have its own section, too. Maybe better explained
> with an example:
> 
>   [hook "pre-receive"]
>   # put any pre-receive related options here; e.g., a rule for what to
>   # do with hook exit codes (e.g., stop running, run all but return exit
>   # code, ignore failures, etc)
>   fail = stop

Interesting - so this is a default for all pre-receive hooks, that I can
set at whichever scope I wish.

> 
>   # And we can define actual hook commands. This one refers to the
>   # hookcmd block below.
>   command = foo
> 
>   # But if there's no such hookcmd block, we could just do something
>   # sensible, like defaulting hookcmd.X.command to "X"
>   command = /path/to/some-hook.sh

I like this idea a lot!

> 
>   [hookcmd "foo"]
>   # the actual hook command to run
>   command = /path/to/another-hook
>   # other hook options, like order priority
>   order = 123

Looks familiar enough. Now I worry - what if I specify 'fail' here too?

It seems like I may be saying "let's set a default per hookcmd" and you
may be saying "let's set a default per hook". Maybe you're saying "some
options are hook-specific and some options are command-specific." You
might be saying "we shouldn't need to set multiple option values for a
single command," and I think I disagree with that based on the
git-secrets value alone; if I'm getting ready to commit, I want
git-secrets to run last so it can look at changes other hooks made to my
commit, but if I'm getting ready to push, I want git-secrets to run
first so I don't wait around for a test suite just to find that my
commit is invalid anyways. Although, I guess with your schema the former
would be in [hookcmd "git-secrets-committing"] and the latter
would be in [hookcmd "git-secrets-pushing"], so I can set the ordering
how I wish.

(This might be an OK problem to punt on. I don't think there are any
options we have in mind just yet - even "order", we aren't sure whether
to prefer config order or an explicit number. I think if we make no
decision on how to treat per-hook options today, it doesn't stop us from
deciding on some schema tomorrow. Once we do decide, then we put it in
documentation and need to stick to it, but for now I think it's OK to
leave it undefined. We might not even need it.)

This schema also means it's easy to reorder or remove hooks later on,
which I like. A single line in my worktree config is clear:

  hookcmd.git-secrets-committing.skip = true
  hookcmd.git-secrets-pushing.order = 001
> 
> I think both this schema and the one you wrote above can express the
> same set of things. But you don't _have_ to pick a unique name if you
> don't want to. Just doing:
> 
>   [hook "pre-receive"]
>   command = /some/script
> 
> would be valid and useful (and that's as far as 99% of use cases would
> need to go).

Yeah, I see what you mean, and again I really like that. That lets us
run multiples in config order easily:

[hook "pre-receive"]
  command = /some/script
  command = /some/other-script
  command = some-hookcmd-header

If we add a little repeated-name detection then we can also reorder
easily this way if that's the direction we want for ordering:

{global}
hook.pre-receive.command = a.sh
hook.pre-receive.command = b.sh

{local}
hook.pre-receive.command = c.sh
hook.pre-receive.command = a.sh

for a final order of {b.sh, c.sh, a.sh}.

Very nice, IMO.

I wonder - I think even something like this would work:

{global}
[hook "pre-receive"]
  command = no-hookcmd-entry.sh

{local for repo "zork"}
[hookcmd "no-hookcmd-entry.sh"]
  skip = true

For most repos, now I simply invoke no-hookcmd-entry.sh on pre-receive,
but when I'm parsing the config in "zork", now I see a populated hookcmd
entry, and when I look it up with the key I found in the global config,
I see that it's supposed to be skipped.

Although I might need to do something hacky if I have multiple hooks
pointing to the same simple invocation:

{global}
[hook "pre-receive"]
  command = no-hookcmd-entry.sh

[hook "post-commit"]
  command = no-hookcmd-entry.sh

{local}
hookcmd.no-hookcmd-entry.sh.skip = true

[hook "pre-receive"]
  command = modified-no-hookcmd.sh

[hookcmd "modified-no-hookcmd-entry"]
  command = no-hookcmd-entry.sh

That is, I think this makes it kind of tricky to shut off one invocation
for only one hook.  Maybe it makes sense to honor something like:

hookcmd.foo.skip-pre-receive = true

?

I wonder if I'm getting buried in the weeds of stuff we won't ever have
to worry about ;)

 - Emily