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

From: Emily Shaffer <emilyshaffer@google.com>
Date: Aug 18, 2021, 20:51 UTC
Message-ID: <YR1y7jxhGmnTJKyH@google.com>
In-Reply-To: <xmqqy29632hx.fsf@gitster.g>

On Thu, Aug 12, 2021 at 12:08:10PM -0700, Junio C Hamano wrote:

Show 32 quoted lines

> 
> Emily Shaffer <emilyshaffer@google.com> 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".

Yes, I see what you mean. Ok. I have been wanting to change the naming anyways - most functions in hook.h are verb-y ("find hook", "run hooks", so on) but hook_list stands out as the only noun-y function.

So I considered changing it to "list_hooks" and "list_hooks_gently", to align with find_hook(_gently)....

Show 9 quoted lines

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

Sure, that makes sense enough... but then I wonder if it would be better to let the caller check whether the name is allowed at all, first, separately from the hook_list() call.

On the one hand, having hook_list() do the validation of the hook name makes it harder for a hook doing something very unusual to neglect to add documentation. (I'm thinking of, for example, a hook doing something equally weird to the proc-receive hook, which cannot use the hook library because it needs to be able to do this weird two-way communication thing. (https://lore.kernel.org/git/20210527000856.695702-31-emilyshaffer%40google.com)) It would be pretty bad for a hook which is already complicated to also forget to include documentation.

On the other hand, as it is now - builtin/hook.c hardcodes "I don't care if the hook is unknown" and hook.c hardcodes "reject if the hook is unknown" and nobody else calls hook_list at all - it isn't so bad to bail early, before even calling hook_list() in the first place, if the hook is unknown.

I also think that approach would make a callsite easier to understand than checking for null from hook_list().

  const char *hookname = "my-new-hook";

  /* Here it's pretty clear what the reason for the error was... */
  if (!known_hook(hookname))
    BUG("is hook '%s' in Documentation/githooks.txt?", hookname);

  hooks = hook_list(hookname);
  ...

vs.

  const char *hookname = "my-new-hook";
  hooks = hook_list(hookname);
  /*
   * But here, I have to go and look at the hook_list() source to
   * understand why null 'hooks' means I missed some doc step.
   */
  if (!hookname)
    BUG("is hook '%s' in Documentation/githooks.txt?", hookname);
  ...

Maybe others disagree with me, but I would guess the first example is more easily understandable to someone unfamiliar with the hook code. So I think I will go with that approach, and include some notice in the doc comment over hook_list().

 - Emily