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

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