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

On Tue, Aug 24, 2021 at 10:12:04PM +0200, Ævar Arnfjörð Bjarmason wrote:

On Wed, Aug 18 2021, Emily Shaffer wrote:

quoted

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.

Hmmm. I personally do not like "a subsequent commit adds this" in commit
messages. But I'm having trouble expressing why :) To me, "I know about
me and everything that came before me" is fine and "I wish in the future
x would happen" is fine, for commit messages, but "x will happen in the
future" feels a little wrong.

Anyway, I think it is a little distracting to include that message in
the earlier commit, since the "doesn't work outside of a repo" aspect of
hooks is not changing at all.

quoted

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

Yeah, I think it is probably fine to do allllll the git-send-email
fixups in this one commit, actually. I think your point is correct that
right now the only command which can benefit is git-send-email. It might
also be reasonable to drop this change from this series, but I won't
bother doing so unless it looks like this one commit is holding up the
rest of the series.

I could see, though, a later world where we might want something like a
"pre-clone" or "post-clone"(-pre-checkout) hook, now that we don't need
to have a .gitdir in order to run any hooks. Will make a very brief
"later it might be nice if..." addition to the commit message.

 - Emily