| Message ID | 20210715232603.3415111-8-emilyshaffer@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | config-based hooks restarted | expand |
On Thu, Jul 15 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. > > It still doesn't make sense to look for hooks in the hookdir in nongit > repos, though, as there is no hookdir. Hrm, I haven't tested but re the discussion we had about RUN_SETUP_GENTLY on my re-rolled base topic is this really just a regression in my changes there? I.e. I assumed we could do RUN_SETUP for the bug-for-bug compatibility step, but send-email runs out of repo hooks and we just didn't have tests for it, or am I missing something?
On Fri, Jul 16, 2021 at 10:33:25AM +0200, Ævar Arnfjörð Bjarmason wrote: > > > On Thu, Jul 15 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. > > > > It still doesn't make sense to look for hooks in the hookdir in nongit > > repos, though, as there is no hookdir. > > Hrm, I haven't tested but re the discussion we had about > RUN_SETUP_GENTLY on my re-rolled base topic is this really just a > regression in my changes there? > > I.e. I assumed we could do RUN_SETUP for the bug-for-bug compatibility > step, but send-email runs out of repo hooks and we just didn't have > tests for it, or am I missing something? I'm not sure. I could see a case for you including RUN_SETUP_GENTLY on your series and adding a test for sendemail-validate + core.hooksPath in global config. I think I also don't have support for that case here, actually.... Anyway, it looks like right now git-send-email.perl:validate_patch() doesn't bother if it's out-of-repo, so this wouldn't have worked before (and still won't work even after this change). So either I can add a patch to my series to allow that, or you can modify your patch converting sendemail-validate to 'git hook run' to drop the 'if $repo' line and teach your series to behave correctly with nongit+hooksPath. It looks like in my earlier attempt at the series, I did drop that check and run 'git hook run' no matter what kind of directory we're in. - Emily
On Thu, Jul 22 2021, Emily Shaffer wrote: > On Fri, Jul 16, 2021 at 10:33:25AM +0200, Ævar Arnfjörð Bjarmason wrote: >> >> >> On Thu, Jul 15 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. >> > >> > It still doesn't make sense to look for hooks in the hookdir in nongit >> > repos, though, as there is no hookdir. >> >> Hrm, I haven't tested but re the discussion we had about >> RUN_SETUP_GENTLY on my re-rolled base topic is this really just a >> regression in my changes there? >> >> I.e. I assumed we could do RUN_SETUP for the bug-for-bug compatibility >> step, but send-email runs out of repo hooks and we just didn't have >> tests for it, or am I missing something? > > I'm not sure. I could see a case for you including RUN_SETUP_GENTLY on > your series and adding a test for sendemail-validate + core.hooksPath in > global config. I think I also don't have support for that case here, > actually.... The narrow goal of the base topic is to be a bug-for-bug compatible version of what we have now on "master", just via a dispatch command/API. So yeah, that git-send-email.perl behavior looks bizarre, but let's fix it in a separate set of behavior changing patches on top, no? > Anyway, it looks like right now git-send-email.perl:validate_patch() > doesn't bother if it's out-of-repo, so this wouldn't have worked before (and > still won't work even after this change). So either I can add a patch to > my series to allow that, or you can modify your patch converting > sendemail-validate to 'git hook run' to drop the 'if $repo' line and > teach your series to behave correctly with nongit+hooksPath. It looks > like in my earlier attempt at the series, I did drop that check and run > 'git hook run' no matter what kind of directory we're in. I think this was one of the things that either needed a test change in your series, or I saw tests for changes to existing but untested behavior, I think this was the latter. I completely agree that the behavior is weird, probably undesired and should be changed. I'm just saying that we should split up refactorings & behavior changes.
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 ed90edcad7..b08b876d5d 100644 --- a/hook.c +++ b/hook.c @@ -202,7 +202,6 @@ static int hook_config_lookup(const char *key, const char *value, void *cb_data) struct list_head* hook_list(const char* hookname, int allow_unknown) { struct list_head *hook_head = xmalloc(sizeof(struct list_head)); - const char *hook_path; struct strbuf hook_key = STRBUF_INIT; struct hook_config_cb cb_data = { &hook_key, hook_head }; @@ -216,14 +215,17 @@ struct list_head* hook_list(const char* hookname, int allow_unknown) git_config(hook_config_lookup, &cb_data); - if (allow_unknown) - hook_path = find_hook_gently(hookname); - else - hook_path = find_hook(hookname); + if (have_git_dir()) { + const char *hook_path; + if (allow_unknown) + hook_path = find_hook_gently(hookname); + else + hook_path = find_hook(hookname); - /* Add the hook from the hookdir */ - if (hook_path) - append_or_move_hook(hook_head, hook_path)->from_hookdir = 1; + /* Add the hook from the hookdir */ + if (hook_path) + append_or_move_hook(hook_head, hook_path)->from_hookdir = 1; + } return hook_head; } diff --git a/t/t1360-config-based-hooks.sh b/t/t1360-config-based-hooks.sh index 12fca516ec..e4a7b06ad1 100755 --- a/t/t1360-config-based-hooks.sh +++ b/t/t1360-config-based-hooks.sh @@ -34,6 +34,19 @@ test_expect_success 'git hook rejects commands without a hookname' ' test_must_fail git hook list ' +test_expect_success 'git hook runs outside of a repo' ' + setup_hooks && + + cat >expected <<-EOF && + $ROOT/path/def + EOF + + nongit git config --list --global && + + nongit git hook list pre-commit >actual && + test_cmp expected actual +' + test_expect_success 'git hook list orders by config order' ' setup_hooks &&
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 | 18 ++++++++++-------- t/t1360-config-based-hooks.sh | 13 +++++++++++++ 3 files changed, 24 insertions(+), 9 deletions(-)