Re: [PATCH v3 4/6] hook: allow running non-native hooks

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Aug 24, 2021, 15:55 UTC
Message-ID: <87o89mej3y.fsf@evledraar.gmail.com>
In-Reply-To: <20210819033450.3382652-5-emilyshaffer@google.com>

On Wed, Aug 18 2021, Emily Shaffer wrote:

Show 23 quoted lines

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

Show 22 quoted lines

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