| Message ID | 20210819033450.3382652-7-emilyshaffer@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | config-based hooks restarted | expand |
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. > [...] > 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.
On August 24, 2021 4:12 PM, Æ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: To clarify the requirements here, if running without a gitdir (and thus without a repository?) how will front-ends know what to supply? Will this just be "some shell script" that runs? > > 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. So we cannot assume anything about the repository, correct? Similar to running git version but not git status? >> [...] >> 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? So this is complete consent to run outside of git? I wonder whether there needs to be an attribute associated with the hook that enables this edge capability. That way we can validate whether the hook should be run or not (from front-end scripts).
On Tue, Aug 24 2021, Randall S. Becker wrote: > On August 24, 2021 4:12 PM, Æ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: > > To clarify the requirements here, if running without a gitdir (and > thus without a repository?) how will front-ends know what to supply? > Will this just be "some shell script" that runs? Emily's also aiming to have "git hook run" running "some shell script", but in this case we're talking about any git program that runs hooks, but doesn't require a repo. I think the only one is git-send-email's sendemail-validate hook. >> >> 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. > > So we cannot assume anything about the repository, correct? Similar to running git version but not git status? Right, with RUN_SETUP_GENTLY we may not have a repo at all, so like commands such as "git bundle", "git config", "git ls-remote" etc. >>> [...] >>> 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? > > So this is complete consent to run outside of git? I wonder whether > there needs to be an attribute associated with the hook that enables > this edge capability. That way we can validate whether the hook should > be run or not (from front-end scripts). If I understand you correctly you're pointing out that anyone with a sendemail-validate script could previously have assumed a repo, but not anymore. So e.g. someone who always has other config the hook requires when they run it in repository might unexpectedly have that hook fail if they naïvely set the config for that sendemail-validate hook via "git config --global". I think that's a good point, and one I hadn't really considered. I think it's probably best to just document this edge case for the one (or few?) hooks that have this caveat than to not support it. I.e. it seems useful to have a sendemail-validate without a repo (just to parse/validate the about-to-be-sent-email), that we needed a repo before was really only an emergent effect. Also, I think it's not new with this config-based hook mechanism, but something that I introduced in back when I added core.hooksPath (but I don't think I realized I was adding this edge case at the time).
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
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
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. It still doesn't make sense to look for hooks in the hookdir in nongit repos, though, as there is no hookdir. Signed-off-by: Emily Shaffer <emilyshaffer@google.com> --- 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(-)