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