Re: [PATCH v3 4/6] hook: allow running non-native hooks
From: Ævar Arnfjörð Bjarmason <hidden>
Date: 2021-08-24 19:30:14
On Wed, Aug 18 2021, Emily Shaffer wrote:
As the hook architecture and 'git hook run' become more featureful, we
may find wrappers wanting to use the hook architecture to run their own
hooks, thereby getting nice things like parallelism and idiomatic Git
configuration for free. Enable this by letting 'git hook run' bypass the
known_hooks() check.
We do still want to keep known_hooks() around, though - by die()ing when
an internal Git call asks for run_hooks("my-new-hook"), we can remind
Git developers to update Documentation/githooks.txt with their new hook,
which in turn helps Git users discover this new hook.
[...]
+It's possible to use this command to refer to hooks which are not native to Git,
+for example if a wrapper around Git wishes to expose hooks into its own
+operation in a way which is already familiar to Git users. However, wrappers
+invoking such hooks should be careful to name their hook events something which
+Git is unlikely to use for a native hook later on. For example, Git is much less
+likely to create a `mytool-validate-commit` hook than it is to create a
+`validate-commit` hook.
+
SUBCOMMANDS
-----------The goal here makes sense, but...
quoted hunk ↗ jump to hunk
diff --git a/builtin/hook.c b/builtin/hook.c index d21f303eca..80397d39f5 100644 --- a/builtin/hook.c +++ b/builtin/hook.c@@ -46,7 +46,7 @@ static int list(int argc, const char **argv, const char *prefix) hookname = argv[0]; - head = hook_list(hookname); + head = list_hooks_gently(hookname); if (list_empty(head)) return 1;@@ -105,7 +105,7 @@ static int run(int argc, const char **argv, const char *prefix) git_config(git_default_config, NULL); hook_name = argv[0]; - hooks = list_hooks(hook_name); + hooks = list_hooks_gently(hook_name); if (list_empty(hooks)) { /* ... act like run_hooks_oneshot() under --ignore-missing */ if (ignore_missing)
This introduces a bug v.s. the previous state, e.g. before:
$ git hook run --ignore-missing foobar
fatal: the hook 'foobar' is not known to git, should be in hook-list.h via githooks(5)
But after we'll silently ignore it. I.e. we've conflated
--ignore-missing with a new and hypothetical (and this is now a synonym
of) --ignore-missing-and-allow-unknown-hook-names.
So we've conflated the user's one-shot "foobar" script with wanting to
catch a typo in e.g. git-send-email.perl.
Also instead of the user's typos being caught with a die (here using
your BUG(...) version):
$ git hook list pre-recive
BUG: hook.c:115: Don't recognize hook event 'pre-recive'! Is it documented in Documentation/githooks.txt?
Aborted
We'll now silently return 1, so indistinguishabl from typing it properly
as pre-receive.
All that being said I think it's arguable that if we're going to allow
"git hook run blahblah" that the die() in the base topic in my
"hook-list.h: add a generated list of hooks, like config-list.h" is more
trouble than it's worth.
I.e. do we really need to be concerned about new hooks being added and
someone forgetting a githooks.txt update, or a typo in the git.git code
that nobody notices? Probably not.
But I think the change here is clearly broken vis-a-vis the stated goals
of its commit message as it stands, i.e. "[...]we do still want to keep
known_hooks() around, though[...]". Should we fix it by adding a new
internal-only flag to the command, or just saying we shouldn't have the
behavior at all? What do you think.
Aside from that, this change seems to be untested, I tried making this
non-gentle for testing, and all tests still passed. I.e. we don't have
any tests for running such a hook like mytool-validate-commit, but
should as part of this change.