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

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 12, 2021, 19:08 UTC
Message-ID: <xmqqy29632hx.fsf@gitster.g>
In-Reply-To: <20210812004258.74318-5-emilyshaffer@google.com>

Emily Shaffer <emilyshaffer@google.com> writes:

Show 22 quoted lines

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