Subject: Re: [PATCH v3 5/6] hook: include hooks from the config

From: Emily Shaffer <[email-masked]>
Date: Thu, 19 Aug 2021 16:43:14 -0700

On Thu, Aug 19, 2021 at 03:39:06PM -0700, Junio C Hamano wrote:
> 
> Emily Shaffer <[email-masked]> writes:
> 
> > diff --git a/Documentation/config/hook.txt b/Documentation/config/hook.txt
> > index 96d3d6572c..c394756328 100644
> > --- a/Documentation/config/hook.txt
> > +++ b/Documentation/config/hook.txt
> > @@ -1,3 +1,21 @@
> > +hook.<name>.command::
> > +	A command to execute whenever `hook.<name>` is invoked. `<name>` should
> > +	be a unique "friendly" name which you can use to identify this hook
> > +	command. (You can specify when to invoke this command with
> > +	`hook.<name>.event`.) The value can be an executable on your device or a
> > +	oneliner for your shell. If more than one value is specified for the
> > +	same `<name>`, the last value parsed will be the only command executed.
> > +	See linkgit:git-hook[1].
> > +
> > +hook.<name>.event::
> > +	The hook events which should invoke `hook.<name>`. `<name>` should be a
> > +	unique "friendly" name which you can use to identify this hook. The
> > +	value should be the name of a hook event, like "pre-commit" or "update".
> > +	(See linkgit:githooks[5] for a complete list of hooks Git knows about.)
> > +	On the specified event, the associated `hook.<name>.command` will be
> > +	executed. More than one event can be specified if you wish for
> > +	`hook.<name>` to execute on multiple events. See linkgit:git-hook[1].
> 
> Looking much better.  It now gives enough information to readers to
> understand (if not enough to convince that it is a good idea) why an
> indirection with "friendly name" between the event and command is
> there.  In short, <name> names the command to be run and without
> indirection, you'd end up having to write:
> 
>     [hook "check-whitespace && spellcheck-log-message"]
> 	event = pre-commit
>     [hook "check-whitespace && spellcheck-log-message"]
> 	event = another-hookable-event
> 
> which may give the same expressiveness (and may even be workable if
> the configuration were machine generated) but it is typo-prone, and
> a single typo, or even an insignificant whitespace change in the
> command, would destroy the grouping of "this command fires upon
> these events".
> 
> It becomes much less typo prone with the indirection, i.e.
> 
>     [hook "logcheck"]
> 	command = check-whitespace && spellcheck-log-message
> 
>     [hook "logcheck"]
> 	event = pre-commit
> 
>     [hook "logcheck"]
> 	event = another-hookable-event
> 
> using the "friendly name", especially if these entries are spread
> across different configuration files.
> 
> My original question was primarily because I thought the
> second-level <name> corresponded to <event>.  If that were the case,
> it can trivially be made simpler without making it typo-prone, i.e.
> 
>     [hook "pre-commit"]
> 	command = check-whitespace && spellcheck-log-message
> 
>     [hook "another-hookable-event"]
> 	command = check-whitespace && spellcheck-log-message
> 
> since the name of the event would be much shorter than the command
> line.  But since we are not grouping per hookable event (to apply
> the "last one wins" rule to determine which single command is the
> one that gets to run).
> 

To be clear, the config schema did work the way you describe until this
revision. Ævar suggested the change and it seemed like a good idea to me
(and the rest of the Google folks) so I changed between v2 and v3 of the
restart.

 - Emily