hook: add hook.<event>.enabled switch

Add a hook.<event>.enabled config key that disables all hooks for
a given event, when set to false, acting as a high-level switch
above the existing per-hook hook.<friendly-name>.enabled.

Event-disabled hooks are shown in "git hook list" with an
"event-disabled" tab-separated prefix before the name:

$ git hook list test-hook
event-disabled  hook-1
event-disabled  hook-2

With --show-scope:

$ git hook list --show-scope test-hook
local   event-disabled  hook-1

When a hook is both per-hook disabled and event-disabled, only
"event-disabled" is shown: the event-level switch is the more
relevant piece of information, and the per-hook "disabled" status
will surface once the event is re-enabled.

Using an event name as a friendly-name (e.g. hook.<event>.enabled)
can cause ambiguity, so a fatal error is issued when using a known
event name and a warning is issued for unknown event name, since
a collision cannot be detected with certainty for unknown events.

Suggested-by: Patrick Steinhardt <ps@pks.im>
Suggested-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Adrian Ratiu <adrian.ratiu@collabora.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/Documentation/config/hook.adoc b/Documentation/config/hook.adoc
index d4fa29d936d6e2fa0a4d8b637293e8e0c8f373ab..e0db3afa194080978defa1da018c01251b40546f 100644 (file)
--- a/Documentation/config/hook.adoc
+++ b/Documentation/config/hook.adoc
@@ -15,6 +15,12 @@ hook.<friendly-name>.event::
        events, specify the key more than once. An empty value resets
        the list of events, clearing any previously defined events for
        `hook.<friendly-name>`. See linkgit:git-hook[1].
++
+The `<friendly-name>` must not be the same as a known hook event name
+(e.g. do not use `hook.pre-commit.event`). Using a known event name as
+a friendly-name is a fatal error because it creates an ambiguity with
+`hook.<event>.enabled` and `hook.<event>.jobs`. For unknown event names,
+a warning is issued when `<friendly-name>` matches the event value.
 
 hook.<friendly-name>.enabled::
        Whether the hook `hook.<friendly-name>` is enabled. Defaults to `true`.
@@ -33,6 +39,20 @@ hook.<friendly-name>.parallel::
        found in the hooks directory do not need to, and run in parallel when
        the effective job count is greater than 1. See linkgit:git-hook[1].
 
+hook.<event>.enabled::
+       Switch to enable or disable all hooks for the `<event>` hook event.
+       When set to `false`, no hooks fire for that event, regardless of any
+       per-hook `hook.<friendly-name>.enabled` settings. Defaults to `true`.
+       See linkgit:git-hook[1].
++
+Note on naming: `<event>` must be the event name (e.g. `pre-commit`),
+not a hook friendly-name. Since using a known event name as a
+friendly-name is disallowed (see `hook.<friendly-name>.event` above),
+there is no ambiguity between event-level and per-hook `.enabled`
+settings for known events. For unknown events, if a friendly-name
+matches the event name despite the warning, `.enabled` is treated
+as per-hook only.
+
 hook.<event>.jobs::
        Specifies how many hooks can be run simultaneously for the `<event>`
        hook event (e.g. `hook.post-receive.jobs = 4`). Overrides `hook.jobs`

diff --git a/builtin/hook.c b/builtin/hook.c
index 1839412dca3edcffd9adaa957e3174899bd9e227..8e47e22e2a1e5ff073f5ef571e6925931be4922d 100644 (file)
--- a/builtin/hook.c
+++ b/builtin/hook.c
@@ -87,14 +87,22 @@ static int list(int argc, const char **argv, const char *prefix,
                        const char *name = h->u.configured.friendly_name;
                        const char *scope = show_scope ?
                                config_scope_name(h->u.configured.scope) : NULL;
+                       /*
+                        * Show the most relevant disable reason. Event-level
+                        * takes precedence: if the whole event is off, that
+                        * is what the user needs to know. The per-hook
+                        * "disabled" surfaces once the event is re-enabled.
+                        */
+                       const char *disability =
+                               h->u.configured.event_disabled ? "event-disabled\t" :
+                               h->u.configured.disabled       ? "disabled\t"       :
+                                                                "";
                        if (scope)
-                               printf("%s\t%s%s%c", scope,
-                                      h->u.configured.disabled ? "disabled\t" : "",
-                                      name, line_terminator);
+                               printf("%s\t%s%s%c", scope, disability, name,
+                                      line_terminator);
                        else
-                               printf("%s%s%c",
-                                      h->u.configured.disabled ? "disabled\t" : "",
-                                      name, line_terminator);
+                               printf("%s%s%c", disability, name,
+                                      line_terminator);
                        break;
                }
                default:

diff --git a/hook.c b/hook.c
index 19076f8f2baba69617c66e5669aab64948b4802b..bc990d4ed4d7543e4834fe96146b130f3611b8b9 100644 (file)
--- a/hook.c
+++ b/hook.c
@@ -133,7 +133,9 @@ struct hook_config_cache_entry {
  * Callback struct to collect all hook.* keys in a single config pass.
  * commands: friendly-name to command map.
  * event_hooks: event-name to list of friendly-names map.
- * disabled_hooks: set of friendly-names with hook.<friendly-name>.enabled = false.
+ * disabled_hooks: set of all names with hook.<name>.enabled = false; after
+ *                 parsing, names that are not friendly-names become event-level
+ *                 disables stored in r->disabled_events. This collects all.
  * parallel_hooks: friendly-name to parallel flag.
  * event_jobs: event-name to per-event jobs count (stored as uintptr_t, NULL == unset).
  * jobs: value of the global hook.jobs key. Defaults to 0 if unset (stored in r->hook_jobs).
@@ -189,8 +191,21 @@ static int hook_config_lookup_all(const char *key, const char *value,
                        strmap_for_each_entry(&data->event_hooks, &iter, e)
                                unsorted_string_list_remove(e->value, hook_name, 0);
                } else {
-                       struct string_list *hooks =
-                               strmap_get(&data->event_hooks, value);
+                       struct string_list *hooks;
+
+                       if (is_known_hook(hook_name))
+                               die(_("hook friendly-name '%s' collides with "
+                                     "a known event name; please choose a "
+                                     "different friendly-name"),
+                                   hook_name);
+
+                       if (!strcmp(hook_name, value))
+                               warning(_("hook friendly-name '%s' is the "
+                                         "same as its event; this may cause "
+                                         "ambiguity with hook.%s.enabled"),
+                                       hook_name, hook_name);
+
+                       hooks = strmap_get(&data->event_hooks, value);
 
                        if (!hooks) {
                                CALLOC_ARRAY(hooks, 1);
@@ -345,6 +360,22 @@ static void build_hook_config_map(struct repository *r, struct strmap *cache)
 
        warn_jobs_on_friendly_names(&cb_data);
 
+       /*
+        * Populate disabled_events: names in disabled_hooks that are not
+        * friendly-names are event-level switches (hook.<event>.enabled = false).
+        * Names that are friendly-names are already handled per-hook via the
+        * hook_config_cache_entry.disabled flag below.
+        */
+       if (r) {
+               string_list_clear(&r->disabled_events, 0);
+               string_list_init_dup(&r->disabled_events);
+               for (size_t i = 0; i < cb_data.disabled_hooks.nr; i++) {
+                       const char *n = cb_data.disabled_hooks.items[i].string;
+                       if (!is_friendly_name(&cb_data, n))
+                               string_list_append(&r->disabled_events, n);
+               }
+       }
+
        /* Construct the cache from parsed configs. */
        strmap_for_each_entry(&cb_data.event_hooks, &iter, e) {
                struct string_list *hook_names = e->value;
@@ -446,6 +477,8 @@ static void list_hooks_add_configured(struct repository *r,
 {
        struct strmap *cache = get_hook_config_cache(r);
        struct string_list *configured_hooks = strmap_get(cache, hookname);
+       bool event_is_disabled = r ? !!unsorted_string_list_lookup(&r->disabled_events,
+                                                                  hookname) : 0;
 
        /* Iterate through configured hooks and initialize internal states */
        for (size_t i = 0; configured_hooks && i < configured_hooks->nr; i++) {
@@ -472,6 +505,7 @@ static void list_hooks_add_configured(struct repository *r,
                        entry->command ? xstrdup(entry->command) : NULL;
                hook->u.configured.scope = entry->scope;
                hook->u.configured.disabled = entry->disabled;
+               hook->u.configured.event_disabled = event_is_disabled;
                hook->parallel = entry->parallel;
 
                string_list_append(list, friendly_name)->util = hook;
@@ -484,6 +518,8 @@ static void list_hooks_add_configured(struct repository *r,
        if (!r || !r->gitdir) {
                hook_cache_clear(cache);
                free(cache);
+               if (r)
+                       string_list_clear(&r->disabled_events, 0);
        }
 }
 
@@ -515,7 +551,7 @@ int hook_exists(struct repository *r, const char *name)
        for (size_t i = 0; i < hooks->nr; i++) {
                struct hook *h = hooks->items[i].util;
                if (h->kind == HOOK_TRADITIONAL ||
-                   !h->u.configured.disabled) {
+                   (!h->u.configured.disabled && !h->u.configured.event_disabled)) {
                        exists = 1;
                        break;
                }
@@ -538,7 +574,8 @@ static int pick_next_hook(struct child_process *cp,
                if (hook_cb->hook_to_run_index >= hook_list->nr)
                        return 0;
                h = hook_list->items[hook_cb->hook_to_run_index++].util;
-       } while (h->kind == HOOK_CONFIGURED && h->u.configured.disabled);
+       } while (h->kind == HOOK_CONFIGURED &&
+                (h->u.configured.disabled || h->u.configured.event_disabled));
 
        cp->no_stdin = 1;
        strvec_pushv(&cp->env, hook_cb->options->env.v);

diff --git a/hook.h b/hook.h
index 5a93f56618e1236881b0f0203c9bba9e1c9dc7ce..b4372b636ff4de7749734029d07b7b227b7fba0a 100644 (file)
--- a/hook.h
+++ b/hook.h
@@ -32,6 +32,7 @@ struct hook {
                        const char *command;
                        enum config_scope scope;
                        bool disabled;
+                       bool event_disabled;
                } configured;
        } u;
 

diff --git a/repository.c b/repository.c
index 4030db4460714d681ad0c3674b8dcf50deefaf0d..db57b8308b94e71bad934c0ce1b8b7f000cea04b 100644 (file)
--- a/repository.c
+++ b/repository.c
@@ -427,6 +427,7 @@ void repo_clear(struct repository *repo)
                FREE_AND_NULL(repo->hook_config_cache);
        }
        strmap_clear(&repo->event_jobs, 0); /* values are uintptr_t, not heap ptrs */
+       string_list_clear(&repo->disabled_events, 0);
 
        if (repo->promisor_remote_config) {
                promisor_remote_clear(repo->promisor_remote_config);

diff --git a/repository.h b/repository.h
index 6b67ec02e2984c18bb4cea6a81834ab894a1dd7f..4969d8b8ebed607207940e36a7b53e2fef63f2dc 100644 (file)
--- a/repository.h
+++ b/repository.h
@@ -2,6 +2,7 @@
 #define REPOSITORY_H
 
 #include "strmap.h"
+#include "string-list.h"
 #include "repo-settings.h"
 #include "environment.h"
 
@@ -178,6 +179,9 @@ struct repository {
        /* Cached map of event-name -> jobs count (as uintptr_t) from hook.<event>.jobs. */
        struct strmap event_jobs;
 
+       /* Cached list of event names with hook.<event>.enabled = false. */
+       struct string_list disabled_events;
+
        /* Configurations related to promisor remotes. */
        char *repository_format_partial_clone;
        struct promisor_remote_config *promisor_remote_config;

diff --git a/t/t1800-hook.sh b/t/t1800-hook.sh
index 89fedc48ff497fda94d60507dcfc3a738634327a..c4ff25f6b088eaddfc370af0232d6e7eff57639a 100755 (executable)
--- a/t/t1800-hook.sh
+++ b/t/t1800-hook.sh
@@ -1058,4 +1058,87 @@ test_expect_success 'hook.<event>.jobs does not warn for a real event name' '
        test_grep ! "friendly-name" err
 '
 
+test_expect_success 'hook.<event>.enabled=false skips all hooks for event' '
+       test_config hook.hook-1.event test-hook &&
+       test_config hook.hook-1.command "echo ran" &&
+       test_config hook.test-hook.enabled false &&
+       git hook run --allow-unknown-hook-name test-hook >out 2>err &&
+       test_must_be_empty out
+'
+
+test_expect_success 'hook.<event>.enabled=true does not suppress hooks' '
+       test_config hook.hook-1.event test-hook &&
+       test_config hook.hook-1.command "echo ran" &&
+       test_config hook.test-hook.enabled true &&
+       git hook run --allow-unknown-hook-name test-hook >out 2>err &&
+       test_grep "ran" err
+'
+
+test_expect_success 'hook.<event>.enabled=false does not affect other events' '
+       test_config hook.hook-1.event test-hook &&
+       test_config hook.hook-1.command "echo ran" &&
+       test_config hook.other-event.enabled false &&
+       git hook run --allow-unknown-hook-name test-hook >out 2>err &&
+       test_grep "ran" err
+'
+
+test_expect_success 'hook.<friendly-name>.enabled=false still disables that hook' '
+       test_config hook.hook-1.event test-hook &&
+       test_config hook.hook-1.command "echo hook-1" &&
+       test_config hook.hook-2.event test-hook &&
+       test_config hook.hook-2.command "echo hook-2" &&
+       test_config hook.hook-1.enabled false &&
+       git hook run --allow-unknown-hook-name test-hook >out 2>err &&
+       test_grep ! "hook-1" err &&
+       test_grep "hook-2" err
+'
+
+test_expect_success 'git hook list shows event-disabled hooks as event-disabled' '
+       test_config hook.hook-1.event test-hook &&
+       test_config hook.hook-1.command "echo ran" &&
+       test_config hook.hook-2.event test-hook &&
+       test_config hook.hook-2.command "echo ran" &&
+       test_config hook.test-hook.enabled false &&
+       git hook list --allow-unknown-hook-name test-hook >actual &&
+       test_grep "^event-disabled      hook-1$" actual &&
+       test_grep "^event-disabled      hook-2$" actual
+'
+
+test_expect_success 'git hook list shows scope with event-disabled' '
+       test_config hook.hook-1.event test-hook &&
+       test_config hook.hook-1.command "echo ran" &&
+       test_config hook.test-hook.enabled false &&
+       git hook list --allow-unknown-hook-name --show-scope test-hook >actual &&
+       test_grep "^local       event-disabled  hook-1$" actual
+'
+
+test_expect_success 'git hook list still shows hooks when event is disabled' '
+       test_config hook.hook-1.event test-hook &&
+       test_config hook.hook-1.command "echo ran" &&
+       test_config hook.test-hook.enabled false &&
+       git hook list --allow-unknown-hook-name test-hook >actual &&
+       test_grep "event-disabled" actual
+'
+
+test_expect_success 'friendly-name matching known event name is rejected' '
+       test_config hook.pre-commit.event pre-commit &&
+       test_config hook.pre-commit.command "echo oops" &&
+       test_must_fail git hook run pre-commit 2>err &&
+       test_grep "collides with a known event name" err
+'
+
+test_expect_success 'friendly-name matching known event name is rejected even for different event' '
+       test_config hook.pre-commit.event post-commit &&
+       test_config hook.pre-commit.command "echo oops" &&
+       test_must_fail git hook run post-commit 2>err &&
+       test_grep "collides with a known event name" err
+'
+
+test_expect_success 'friendly-name matching unknown event warns' '
+       test_config hook.test-hook.event test-hook &&
+       test_config hook.test-hook.command "echo ran" &&
+       git hook run --allow-unknown-hook-name test-hook >out 2>err &&
+       test_grep "same as its event" err
+'
+
 test_done