On Tue, Apr 14, 2020 at 04:15:11PM +0100, Phillip Wood wrote:
> 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.)
>
> 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
