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

On Thu, Aug 12, 2021 at 01:48:00PM -0700, Junio C Hamano wrote:

Emily Shaffer [off-list ref] writes:

quoted

Teach the hook.[hc] library to parse configs to populare the list of
hooks to run for a given event.

Multiple commands can be specified for a given hook by providing
multiple "hook.<friendly-name>.command = <path-to-hook>" and
"hook.<friendly-name>.event = <hook-event>" lines. Hooks will be run in
config order of the "hook.<name>.event" lines.

For example:

  $ git config --list | grep ^hook
  hook.bar.command=~/bar.sh
  hook.bar.event=pre-commit

Your answer might be "read the design doc", but it is unclear to me
why "bar" (friendly-name) is needed in this picture at all.  Is it
because you may want to fire more than one command for pre-commit
event?  IOW,

	[hook "bar"]
		command = bar1.sh
		command = bar2.sh
		event = pre-commit

is easier to manage with an extra level of redirection?  I doubt it
as 

	[hook "pre-commit"]
		command = bar1.sh
		command = bar2.sh

would be equally expressive and shorter.  Or would it help use case
for multiple "friendly-name" to refer to the same "event", e.g.

	[hook "xyzzy"]
		event = pre-commit
		command = xyzzy1

	[hook "frotz"]
		event = pre-commit
                command = frotz1
                command = frotz2

or something?  I am not sure if this gives us useful extra
flexibility, and if so, the extra flexibility helps us more than it
confuses us.

And moving the "event" to the second level in the configuration
hierarchy, getting rid of "friendly-name" from the design, would not
make this example unworkable, either:

quoted

  $ git hook run
  # Runs ~/bar.sh
  # Runs .git/hooks/pre-commit

Again, this is not an objection wrapped in a rhetorical question.
It just is that I do not see how the extra level of redirection
helps us.

Please have a look at
https://lore.kernel.org/git/87fswey5wd.fsf%40evledraar.gmail.com and
replies, where Ævar and I discussed the schema change at length. I know
it is a lot of back and forth but I think it is useful to understand why
I ended up changing the schema this way.

quoted

diff --git a/Documentation/config/hook.txt b/Documentation/config/hook.txt
index 96d3d6572c..a97b980cca 100644
--- a/Documentation/config/hook.txt
+++ b/Documentation/config/hook.txt

@@ -1,3 +1,8 @@
+hook.<command>.command::
+	A command to execute during the <command> hook event. This can be an
+	executable on your device, a oneliner for your shell, or the name of a
+	hookcmd. See linkgit:git-hook[1].

Please make sure you use the terminology consistently.  If the
second level is "friendly name", hook.<name>.command should be
described, instead of hook.<command>.command.

Thanks, this is an oversight. Will update the config/hook.txt doc in
next reroll.

Also, to help those who are familiar with the current Git from their
use in the past 10 years or so, giving an example name from the
current system may help, e.g. when describing hook.<name>.event,
you may want to say the values are things like "pre-commit",
"receive", etc.

Sure.

quoted

+This command parses the default configuration files for pairs of configs like
+so:
+
+  [hook "linter"]
+    event = pre-commit
+    command = ~/bin/linter --c

The above addition of .command should also have hook.<name>.event
next to it, no?

I don't understand the question. Doesn't this config snippet equate to
"""
  hook.linter.event=pre-commit
  hook.linter.command=~/bin/linter --c
"""
? So in this case, '<name>' is 'linter', as that's not a native Git hook.

quoted

+Conmmands are run in the order Git encounters their associated

"Conmmands -> Commands", I would think.

ACK

quoted

+`hook.<name>.event` configs during the configuration parse (see
+linkgit:git-config[1]).

Here you use <name>, which should be matched by the description in
the first hunk of the patch to this file.

Yep.

quoted

+In general, when instructions suggest adding a script to
+`.git/hooks/<hook-event>`, you can specify it in the config instead by running
+`git config --add hook.<some-name>.command <path-to-script> && git config --add
+hook.<some-name>.event <hook-event>` - this way you can share the script between
+multiple repos. That is, `cp ~/my-script.sh ~/project/.git/hooks/pre-commit`
+would become `git config --add hook.my-script.command ~/my-script.sh && git
+config --add hook.my-script.event pre-commit`.

One repository may use a friendly name "xyzzy" while the other may
use "frotz" to group the hooks that trigger upon "pre-commit" event,
but unless one of the repositories change the friendly name to
match, they cannot share these configurations, no?  It seems that an
extra level of indirection is hindering sharing, rather than
helping.

Ah, I think this means the documentation isn't sufficient, if you are
asking that. Instead of explaining it in ephemeral email, I think it is
better for me to explain it in documentation reroll, and for you to then
tell me how you interpret it. I expect to send the reroll before I go
home today, since I didn't receive comments from anybody besides you so
far.

Thanks very much for the feedback on the doc - this is very useful.

 - Emily