Re: [PATCH v3 6/6] hook: allow out-of-repo 'git hook' invocations

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Aug 24, 2021, 20:12 UTC
Message-ID: <87ilzueh1g.fsf@evledraar.gmail.com>
In-Reply-To: <20210819033450.3382652-7-emilyshaffer@google.com>

On Wed, Aug 18 2021, Emily Shaffer wrote:

Show 5 quoted lines

> Since hooks can now be supplied via the config, and a config can be
> present without a gitdir via the global and system configs, we can start
> to allow 'git hook run' to occur without a gitdir. This enables us to do
> things like run sendemail-validate hooks when running 'git send-email'
> from a nongit directory.

Sensible goal. Perhaps we should note in an earlier commit when config-based hooks are introduced something like:

    Even though we've added config-based hooks, they currently only work
    if we can find a .git directory, even though certain commands such
    as "git send-email" (or only that command?) can be run outside of a
    git directory. A subsequent commit will address that edge-case.

Show 72 quoted lines

> [...]
> Notes:
>     For hookdir hooks, do we want to run them in nongit dir when core.hooksPath
>     is set? For example, if someone set core.hooksPath in their global config and
>     then ran 'git hook run sendemail-validate' in a nongit dir?
> [...]
>  git.c           |  2 +-
>  hook.c          |  2 +-
>  t/t1800-hook.sh | 20 +++++++++++++++-----
>  3 files changed, 17 insertions(+), 7 deletions(-)
>
> diff --git a/git.c b/git.c
> index 540909c391..39988ee3b0 100644
> --- a/git.c
> +++ b/git.c
> @@ -538,7 +538,7 @@ static struct cmd_struct commands[] = {
>  	{ "grep", cmd_grep, RUN_SETUP_GENTLY },
>  	{ "hash-object", cmd_hash_object },
>  	{ "help", cmd_help },
> -	{ "hook", cmd_hook, RUN_SETUP },
> +	{ "hook", cmd_hook, RUN_SETUP_GENTLY },
>  	{ "index-pack", cmd_index_pack, RUN_SETUP_GENTLY | NO_PARSEOPT },
>  	{ "init", cmd_init_db },
>  	{ "init-db", cmd_init_db },
> diff --git a/hook.c b/hook.c
> index 581d87cbbd..2e08156546 100644
> --- a/hook.c
> +++ b/hook.c
> @@ -218,7 +218,7 @@ struct list_head *list_hooks_gently(const char *hookname)
>  
>  	/* Add the hook from the hookdir. The placeholder makes it easier to
>  	 * allocate work in pick_next_hook. */
> -	if (find_hook_gently(hookname))
> +	if (have_git_dir() && find_hook_gently(hookname))
>  		append_or_move_hook(hook_head, NULL);
>  
>  	return hook_head;
> diff --git a/t/t1800-hook.sh b/t/t1800-hook.sh
> index ef2432f53a..a7e45c0d16 100755
> --- a/t/t1800-hook.sh
> +++ b/t/t1800-hook.sh
> @@ -118,15 +118,25 @@ test_expect_success 'git hook run -- pass arguments' '
>  	test_cmp expect actual
>  '
>  
> -test_expect_success 'git hook run -- out-of-repo runs excluded' '
> -	write_script .git/hooks/test-hook <<-EOF &&
> -	echo Test hook
> -	EOF
> +test_expect_success 'git hook run: out-of-repo runs execute global hooks' '
> +	test_config_global hook.global-hook.event test-hook --add &&
> +	test_config_global hook.global-hook.command "echo no repo no problems" --add &&
> +
> +	echo "global-hook" >expect &&
> +	nongit git hook list test-hook >actual &&
> +	test_cmp expect actual &&
> +
> +	echo "no repo no problems" >expect &&
>  
> -	nongit test_must_fail git hook run test-hook
> +	nongit git hook run test-hook 2>actual &&
> +	test_cmp expect actual
>  '
>  
>  test_expect_success 'git -c core.hooksPath=<PATH> hook run' '
> +	write_script .git/hooks/test-hook <<-EOF &&
> +	echo Test hook
> +	EOF
> +
>  	mkdir my-hooks &&
>  	write_script my-hooks/test-hook <<-\EOF &&
>  	echo Hook ran $1 >>actual

If the only user of this is git-send-email, let's have tests for this in t/t9001-send-email.sh. That should also address your "Notes" above, i.e. let's just test it with core.hooksPath and see what the interaction looks like.