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

Emily Shaffer [off-list ref] writes:

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

Thanks.

On Thu, Aug 19, 2021 at 03:39:06PM -0700, Junio C Hamano wrote:

Emily Shaffer [off-list ref] writes:

quoted

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