On Tue, Aug 24, 2021 at 10:12:04PM +0200, Ævar Arnfjörð Bjarmason wrote:
>
>
> On Wed, Aug 18 2021, Emily Shaffer wrote:
>
> > 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.
>
> > [...]
> > 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
