Re: [PATCH v6 11/17] git hook run: add an --ignore-missing flag

[Glen Choo <chooglen@google.com>]

Ævar Arnfjörð Bjarmason         <avarab@gmail.com> writes:

> For certain one-shot hooks we'd like to optimistically run them, and
> not complain if they don't exist.
>
> This was already supported by the underlying hook.c library, but had
> not been exposed via "git hook run". The command version of this will
> be used by send-email in a subsequent commit.
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  Documentation/git-hook.txt | 10 +++++++++-
>  builtin/hook.c             |  8 ++++++--
>  t/t1800-hook.sh            |  5 +++++
>  3 files changed, 20 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/git-hook.txt b/Documentation/git-hook.txt
> index e39b1b5d069..77c3a8ad909 100644
> --- a/Documentation/git-hook.txt
> +++ b/Documentation/git-hook.txt
> @@ -8,7 +8,7 @@ git-hook - Run git hooks
>  SYNOPSIS
>  --------
>  [verse]
> -'git hook' run <hook-name> [-- <hook-args>]
> +'git hook' run [--ignore-missing] <hook-name> [-- <hook-args>]
>  
>  DESCRIPTION
>  -----------
> @@ -28,6 +28,14 @@ Any positional arguments to the hook should be passed after a
>  mandatory `--` (or `--end-of-options`, see linkgit:gitcli[7]). See
>  linkgit:githooks[5] for arguments hooks might expect (if any).
>  
> +OPTIONS
> +-------
> +
> +--ignore-missing::
> +	Ignore any missing hook by quietly returning zero. Used for
> +	tools that want to do a blind one-shot run of a hook that may
> +	or may not be present.

The most obvious question whenever a new option is introduced is "Is
this a descriptive and consistent option name?" The answer we came to in
review club is yes, "--ignore-missing" is good :)

However, the description isn't 100% clear about what it means for a hook
to be missing (possible confusion: is a missing hook a hook that isn't
documented in Documentation/githooks.txt?). I suspect that your wording
is intentionally open-ended because Git will eventually look for hooks
in the config as well as `.git/hooks`. Nevertheless, it would help to
get a precise definition in the docs so that readers know what to expect
and we can advise() users consistently.

> +
>  SEE ALSO
>  --------
>  linkgit:githooks[5]
> diff --git a/builtin/hook.c b/builtin/hook.c
> index 9b67ff50cef..54e5c6ec933 100644
> --- a/builtin/hook.c
> +++ b/builtin/hook.c
> @@ -7,7 +7,7 @@
>  #include "strvec.h"
>  
>  #define BUILTIN_HOOK_RUN_USAGE \
> -	N_("git hook run <hook-name> [-- <hook-args>]")
> +	N_("git hook run [--ignore-missing] <hook-name> [-- <hook-args>]")
>  
>  static const char * const builtin_hook_usage[] = {
>  	BUILTIN_HOOK_RUN_USAGE,
> @@ -23,8 +23,11 @@ static int run(int argc, const char **argv, const char *prefix)
>  {
>  	int i;
>  	struct run_hooks_opt opt = RUN_HOOKS_OPT_INIT;
> +	int ignore_missing = 0;
>  	const char *hook_name;
>  	struct option run_options[] = {
> +		OPT_BOOL(0, "ignore-missing", &ignore_missing,
> +			 N_("silently ignore missing requested <hook-name>")),
>  		OPT_END(),
>  	};
>  	int ret;
> @@ -52,7 +55,8 @@ static int run(int argc, const char **argv, const char *prefix)
>  	git_config(git_default_config, NULL);
>  
>  	hook_name = argv[0];
> -	opt.error_if_missing = 1;
> +	if (!ignore_missing)
> +		opt.error_if_missing = 1;

It's not clear why --ignore-missing isn't just the default behavior (and
judging from the lines above, it looks like it used to be?). After some
debate, most of the Google reviewers liked this new default, but it was
difficult to assess the merits of the decision because the thought
process wasn't obvious.

Here's the line of reasoning: if we only consider the hooks in-tree, it
certainly seems like none of the callers actually want to error out if
the hook is missing. This suggests that the default behavior of ignoring
missing hooks is actually what we want, so why change the default
behavior and add another option to worry about?

I can propose at least one reason, which is that "git hook" is also
meant to be invoked by end users in the shell and not just by programs.
I used to use npm quite a lot, and a pattern I am quite fond of is to
have an npm script called "precommit" which runs my linters and
formatters, and I'd make `npm run precommit` part of my pre-commit hook.
But a nice benefit is that I can also run `npm run precommit` at any
time to run all of my tooling, regardless of whether or not I am making
a commit. It would be pretty neat to be able to do away with npm and
just use `git hook run pre-commit'. In this scenario, I would certainly
expect `git hook run pre-commit` to fail if there is no `pre-commit`
hook, so yes, this new default behavior can make sense.

But I can't tell if this is something you had intended all along, or if
you had something else in mind. If anything, the original cover letter
[1] _almost_ seems to suggest otherwise.

  The new "git hook" command is (for now) only for the benefit of
  in-tree non-C programs that need to run hooks, and this series
  converts git-send-email and git-p4 to use it.

It's worth explaining the rationale in the commit message.

>  	ret = run_hooks_opt(hook_name, &opt);
>  	if (ret < 0) /* error() return */
>  		ret = 1;


> diff --git a/t/t1800-hook.sh b/t/t1800-hook.sh
> index 3aea1b105f0..29718aa9913 100755
> --- a/t/t1800-hook.sh
> +++ b/t/t1800-hook.sh
> @@ -21,6 +21,11 @@ test_expect_success 'git hook run: nonexistent hook' '
>  	test_cmp stderr.expect stderr.actual
>  '
>  
> +test_expect_success 'git hook run: nonexistent hook with --ignore-missing' '
> +	git hook run --ignore-missing does-not-exist 2>stderr.actual &&
> +	test_must_be_empty stderr.actual
> +'
> +

For completeness, this should include a test for the non-exceptional
case i.e. when the hook isn't missing.

>  test_expect_success 'git hook run: basic' '
>  	write_script .git/hooks/test-hook <<-EOF &&
>  	echo Test hook
> -- 
> 2.34.1.1146.gb52885e7c44

[1] https://lore.kernel.org/git/cover-00.13-00000000000-20211012T131934Z-avarab@gmail.com/