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

On Tue, Aug 24, 2021 at 05:55:13PM +0200, Ævar Arnfjörð Bjarmason wrote:

On Wed, Aug 18 2021, Emily Shaffer wrote:

quoted

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

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.

I think it's A) pretty important to make it easy for users to run
whatever not-necessarily-git-native hook they want, and B) useful for
script Git commands to take advantage of the typo check. So, I'll add a
`--enforce-known-hookname` (or maybe a better named one, this isn't my
strong suit) and switch git-send-email and friends to use it. Like we
discussed off-list, I think it's a good idea to drop the envvar for
exceptional test names from the codebase entirely.

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.

Sure.


Actually, I was in the middle of typing about how I wouldn't change your
'test-hook' and so on tests, and it occurs to me that it might actually
be a better fit for your series to add this --reject-unknown (or
whatever) flag, instead of the envvar magics. So I'll hold off on making
any changes unless I hear from you.

 - Emily