Re: [RFC PATCH v2 0/2] configuration-based hook management

From: Emily Shaffer <emilyshaffer@google.com>
Date: Apr 14, 2020, 19:24 UTC
Message-ID: <20200414192418.GB5478@google.com>
In-Reply-To: <efad3927-1d8f-5545-48e9-9a58c2308273@gmail.com>

On Tue, Apr 14, 2020 at 04:15:11PM +0100, Phillip Wood wrote:

Show 21 quoted lines

> Hi Emily
> 
> Thanks for working on this, having a way to manage multiple commands per
> hook without using an external framework would be really useful
> 
> On 14/04/2020 01:54, Emily Shaffer wrote:
> > Not much to look at compared to the original RFC I sent some months ago.
> > This implements Peff's suggestion of using the "hookcmd" section as a
> > layer of indirection.
> 
> I'm not really clear what the advantage of this indirection is. It seems
> unlikely to me that different hooks will share exactly the same command line
> or other options. In the 'git secrets' example earlier in this thread each
> hook needs to use a different command line. In general a command cannot tell
> which hook it is being invoked as without a flag of some kind. (In some
> cases it can use the number of arguments if that is different for each hook
> that it handles but that is not true in general)
> 
> Without the redirection one could have
>   hook.pre-commit.linter.command = my-command
>   hook.pre-commit.check-whitespace.command = 'git diff --check --cached'

I think this isn't supported by the config semantics. Have a look at config.h:parse_config_key:

  /*
   * Match and parse a config key of the form:
   *
   *   section.(subsection.)?key
   *
   * (i.e., what gets handed to a config_fn_t). The caller provides the section;
   * we return -1 if it does not match, 0 otherwise. The subsection and key
   * out-parameters are filled by the function (and *subsection is NULL if it is
   * missing).
   *
   * If the subsection pointer-to-pointer passed in is NULL, returns 0 only if
   * there is no subsection at all.
   */
  int parse_config_key(const char *var,
                       const char *section,
                       const char **subsection, int *subsection_len,
                       const char **key);

We'd need to fudge one of these fields to include the extra section, I think. Unfortunate, because I find your example very tidy, but in practice maybe not very neat. The closest thing I can find to a nice way of writing it might be:

  [hook.pre-commit "linter"]
    command = my-command
    before = check-whitespace
  [hook.pre-commit "check-whitespace"]
    command = 'git diff --check --cached'

But this is kind of a lie; the sections aren't "hook", "pre-commit", and "linter" as you'd expect. Whether it's OK to lie like this, though, I don't know - I suspect it might make it awkward for others trying to parse the config. (my Vim syntax highlighter had kind of a hard time.)

Show 16 quoted lines

> 
> and other keys can be added for ordering etc. e.g.
>   hook.pre-commit.linter.before = check-whitespace
> 
> With the indirection one needs to set
>   hook.pre-commit.command = linter
>   hook.pre-commit.check-whitespace = 'git diff --check --cached'
>   hookcmd.linter.command = my-command
>   hookcmd.linter.pre-commit-before = check-whitespace
> 
> which involves setting an extra key and checking it each time the hook is
> invoked without any benefit that I can see. I suspect which one seems more
> logical depends on how one thinks of setting hooks - I tend to think "I want
> to set a pre-commit hook" not "I want to set a git-secrets hook". If you've
> got an example where this indirection is helpful or necessary that would be
> really useful to see.

Thanks for sharing your workflow; as always, it's hard to understand the ways others work differently from yourself, so I'm glad to hear from you. Let me think some more on it and reply back again.

 - Emily