Re: [PATCH v2 5/6] hook: include hooks from the config
From: Junio C Hamano <hidden>
Date: 2021-08-12 20:48:09
Emily Shaffer [off-list ref] writes:
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:
$ 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.
quoted hunk
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. 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.
+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?
+Conmmands are run in the order Git encounters their associated
"Conmmands -> Commands", I would think.
+`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.
+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.
quoted hunk
diff --git a/builtin/hook.c b/builtin/hook.c index 3aa65dd791..ea49dc4ef6 100644 --- a/builtin/hook.c +++ b/builtin/hook.c@@ -49,7 +49,7 @@ static int list(int argc, const char **argv, const char *prefix) head = hook_list(hookname, 1); if (list_empty(head)) { - printf(_("no commands configured for hook '%s'\n"), + printf(_("no hooks configured for event '%s'\n"), hookname);...@@ -58,7 +58,8 @@ static int list(int argc, const char **argv, const char *prefix) struct hook *item = list_entry(pos, struct hook, list); item = list_entry(pos, struct hook, list); if (item) - printf("%s\n", item->hook_path); + printf("%s\n", item->name ? item->name + : _("hook from hookdir")); }
I won't comment on this part as my comments on earlier patches would probably have butchered the preimage already for this change to survive intact ;-)