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

From: Junio C Hamano <[email-masked]>
Date: Thu, 12 Aug 2021 12:08:10 -0700

Emily Shaffer <[email-masked]> writes:

> diff --git a/builtin/hook.c b/builtin/hook.c
> index c36b05376c..3aa65dd791 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 = hook_list(hookname, 1);
>  
>  	if (list_empty(head)) {
>  		printf(_("no commands configured for hook '%s'\n"),
> @@ -108,7 +108,7 @@ static int run(int argc, const char **argv, const char *prefix)
>  	git_config(git_default_config, NULL);
>  
>  	hook_name = argv[0];
> -	hooks = hook_list(hook_name);
> +	hooks = hook_list(hook_name, 1);
>  	if (list_empty(hooks)) {
>  		/* ... act like run_hooks_oneshot() under --ignore-missing */
>  		if (ignore_missing)

This is minor, as I expect that the callers of hook_list() will
always confined in builtin/hook.c, but it probably is easier to read
if you gave two functions, just like you have the pair of helpers
find_hook() and find_hook_gently(), as the literal "1" forces the
readers to remember if that means "die if not found", or "ok if it
is a bogus name".

In addition, it may make more sense to keep hook_list() signal
failure by returning NULL and leave the dying to the caller.
In-code callers (as opposed to "git hook run" that can throw any
random string that came from the user at the API) will never throw a
bogus name unless there is a bug, and they'll have to check for an
error return from hook_list() anyway and the error message they
would give may have to be different from the one that is given
against a hook name randomly thrown at us by the user.