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

From: Jeff King <peff@peff.net>
Date: Apr 13, 2020, 21:52 UTC
Message-ID: <20200413215256.GA18990@coredump.intra.peff.net>
In-Reply-To: <20200413191515.GA5478@google.com>

On Mon, Apr 13, 2020 at 12:15:15PM -0700, Emily Shaffer wrote:

Show 9 quoted lines

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

Yeah, I do see how that use case makes sense. I wonder how common it is versus having separate one-off hooks. And whether setting the order priority for all hooks at once is that useful (e.g., I can easily imagine a case where the pre-commit hook for program A must go before B, but it's the other way around for another hook).

I'm just speculating, but my instinct is that it's worth trying to make the simple things as simple as possible, while still allowing the more complex things.

Show 9 quoted lines

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

No, what I meant was just that if we had a hook "foo/bar", then the natural option to control its hook-specific order would be:

  [hook "whatever"]
  order-foo/bar = 123

which isn't allowed ("/" is not valid in a key name). But that should be OK since we control the names of hooks and can decide not to make one with an invalid character in it. We might also support hooks for third-party programs (e.g., if a porcelain wrapper wanted to have its own "pre-switch-branches" hook or something), but it's not too much of an imposition to say that the hook name should be a valid config key.

Show 10 quoted lines

> 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

Yeah, that overriding system makes sense to me. Any other option "frotz" would have the same constraint, though I don't think that's too big a deal.

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

Right that makes sense to me.

Show 8 quoted lines

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

Yes. Though I had imagined "fail" as semantics for operating on the whole list of "pre-receive" hooks, you could define it in a per-command way, too. I was thinking of it as "this is the strategy when a command fails". But you could also think of it as "what to do when this particular command fails".

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

If there's a per-command version of "fail", then presumably it would override any per-hook. I.e., I'd expect code to resolve this at run-time, like:

  struct hook *hook = get_hook("pre-receive");
  for (i = 0; i < hook->nr; i++) {
          struct hookcmd *cmd = hook->cmds[i];

          if (run_hook(cmd->prog) != 0) {
                  enum failure_strategy f = cmd->failure_strategy;
                  if (f == FAILURE_STRATEGY_UNSET)
                          f = hook->failure_strategy;
                  switch (f) {
                  ...do whatever...
                  }
          }
  }

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

Yeah, the latter. Or it might even be that an option is sometimes hook-specific and sometimes command-specific.

Show 11 quoted lines

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

I think all of this is _possible_ in either scheme. We're encoding potentially tabular data into a hierarchical config structure. In either case I can set hook->cmd->option or cmd->hook->option. The question is just which arrangement makes it simplest to do the most common things.

Show 7 quoted lines

> 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

Yep, config order makes sense as a default (though I think you could make an argument for lexical order by command-name, which allows naming things "000foo" if the user really wants to).

Show 12 quoted lines

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

I'm not sure what I'd expect a repeated mention of "a.sh" to do, but as long as it's well-defined I don't really care. :)

Show 14 quoted lines

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

Yes, exactly. The config parsing procedure is really just filling in a "struct hookcmd" as it goes, so we don't care that they're in two separate files.

Show 9 quoted lines

> 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

I think your commands would be:

  command = "no-hookcmd-entry.sh pre-receive"

etc in that case, so they'd have different hookcmd blocks. You _wouldn't_ be able to just turn off all of them with one config command, though.

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

Yeah. I don't mind a little over-engineering as long as the easy things remain simple, and the hard things remain possible. But that also means we might be able to grow the hard things later (or never) as long as we have a reasonable plan for them.

-Peff