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

From: Emily Shaffer <emilyshaffer@google.com>
Date: Apr 13, 2020, 19:15 UTC
Message-ID: <20200413191515.GA5478@google.com>
In-Reply-To: <20200410213146.GA2075494@coredump.intra.peff.net>

On Fri, Apr 10, 2020 at 05:31:46PM -0400, Jeff King wrote:

Show 16 quoted lines

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

Show 11 quoted lines

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

Show 8 quoted lines

> 
>   # 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!

Show 6 quoted lines

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

Show 10 quoted lines

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