hook: show disabled hooks in "git hook list"

Disabled hooks were filtered out of the cache entirely, making them
invisible to "git hook list". Keep them in the cache with a new
"disabled" flag which is propagated to the respective struct hook.

"git hook list" now shows disabled hooks as tab-separated columns,
with the status as a prefix before the name (like scope with
--show-scope). With --show-scope it looks like:

$ git hook list --show-scope pre-commit
global	linter
local	disabled	no-leaks
hook from hookdir

A disabled hook without a command issues a warning instead of the
fatal "hook.X.command must be configured" error. We could also throw
an error, however it seemd a bit excessive to me in this case.

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

diff --git a/builtin/hook.c b/builtin/hook.c
index 4cc65a0dc5970c0a0a4cb7db7a2a1caa3c8e79e9..f671e7f91a033b87f9484a76745190c2410086a9 100644 (file)
--- a/builtin/hook.c
+++ b/builtin/hook.c
@@ -72,16 +72,20 @@ static int list(int argc, const char **argv, const char *prefix,
                case HOOK_TRADITIONAL:
                        printf("%s%c", _("hook from hookdir"), line_terminator);
                        break;
-               case HOOK_CONFIGURED:
-                       if (show_scope)
-                               printf("%s\t%s%c",
-                                      config_scope_name(h->u.configured.scope),
-                                      h->u.configured.friendly_name,
-                                      line_terminator);
+               case HOOK_CONFIGURED: {
+                       const char *name = h->u.configured.friendly_name;
+                       const char *scope = show_scope ?
+                               config_scope_name(h->u.configured.scope) : NULL;
+                       if (scope)
+                               printf("%s\t%s%s%c", scope,
+                                      h->u.configured.disabled ? "disabled\t" : "",
+                                      name, line_terminator);
                        else
-                               printf("%s%c", h->u.configured.friendly_name,
-                                      line_terminator);
+                               printf("%s%s%c",
+                                      h->u.configured.disabled ? "disabled\t" : "",
+                                      name, line_terminator);
                        break;
+               }
                default:
                        BUG("unknown hook kind");
                }

diff --git a/hook.c b/hook.c
index 74f5a1df35ace378a27060ad96f9912906ef39bc..cc23276d27f035b65914df53b8f367aabc29a0be 100644 (file)
--- a/hook.c
+++ b/hook.c
@@ -115,6 +115,7 @@ static void list_hooks_add_default(struct repository *r, const char *hookname,
 struct hook_config_cache_entry {
        char *command;
        enum config_scope scope;
+       bool disabled;
 };
 
 /*
@@ -213,8 +214,10 @@ static int hook_config_lookup_all(const char *key, const char *value,
  * every item's string is the hook's friendly-name and its util pointer is
  * the corresponding command string. Both strings are owned by the map.
  *
- * Disabled hooks and hooks missing a command are already filtered out at
- * parse time, so callers can iterate the list directly.
+ * Disabled hooks are kept in the cache with entry->disabled set, so that
+ * "git hook list" can display them. A non-disabled hook missing a command
+ * is fatal; a disabled hook missing a command emits a warning and is kept
+ * in the cache with entry->command = NULL.
  */
 void hook_cache_clear(struct strmap *cache)
 {
@@ -263,21 +266,26 @@ static void build_hook_config_map(struct repository *r, struct strmap *cache)
                        struct hook_config_cache_entry *entry;
                        char *command;
 
-                       /* filter out disabled hooks */
-                       if (unsorted_string_list_lookup(&cb_data.disabled_hooks,
-                                                       hname))
-                               continue;
+                       bool is_disabled =
+                               !!unsorted_string_list_lookup(
+                                       &cb_data.disabled_hooks, hname);
 
                        command = strmap_get(&cb_data.commands, hname);
-                       if (!command)
-                               die(_("'hook.%s.command' must be configured or "
-                                     "'hook.%s.event' must be removed;"
-                                     " aborting."), hname, hname);
+                       if (!command) {
+                               if (is_disabled)
+                                       warning(_("disabled hook '%s' has no "
+                                                 "command configured"), hname);
+                               else
+                                       die(_("'hook.%s.command' must be configured or "
+                                             "'hook.%s.event' must be removed;"
+                                             " aborting."), hname, hname);
+                       }
 
                        /* util stores a cache entry; owned by the cache. */
                        CALLOC_ARRAY(entry, 1);
-                       entry->command = xstrdup(command);
+                       entry->command = xstrdup_or_null(command);
                        entry->scope = scope;
+                       entry->disabled = is_disabled;
                        string_list_append(hooks, hname)->util = entry;
                }
 
@@ -358,8 +366,10 @@ static void list_hooks_add_configured(struct repository *r,
 
                hook->kind = HOOK_CONFIGURED;
                hook->u.configured.friendly_name = xstrdup(friendly_name);
-               hook->u.configured.command = xstrdup(entry->command);
+               hook->u.configured.command =
+                       entry->command ? xstrdup(entry->command) : NULL;
                hook->u.configured.scope = entry->scope;
+               hook->u.configured.disabled = entry->disabled;
 
                string_list_append(list, friendly_name)->util = hook;
        }
@@ -397,7 +407,16 @@ struct string_list *list_hooks(struct repository *r, const char *hookname,
 int hook_exists(struct repository *r, const char *name)
 {
        struct string_list *hooks = list_hooks(r, name, NULL);
-       int exists = hooks->nr > 0;
+       int exists = 0;
+
+       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) {
+                       exists = 1;
+                       break;
+               }
+       }
        string_list_clear_func(hooks, hook_free);
        free(hooks);
        return exists;
@@ -412,10 +431,11 @@ static int pick_next_hook(struct child_process *cp,
        struct string_list *hook_list = hook_cb->hook_command_list;
        struct hook *h;
 
-       if (hook_cb->hook_to_run_index >= hook_list->nr)
-               return 0;
-
-       h = hook_list->items[hook_cb->hook_to_run_index++].util;
+       do {
+               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);
 
        cp->no_stdin = 1;
        strvec_pushv(&cp->env, hook_cb->options->env.v);

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

diff --git a/t/t1800-hook.sh b/t/t1800-hook.sh
index 6fc6603da820869ec0fd2edfc87aedfaa3bba704..8c5237449dc416e4a9d8f8d7514fd19be67f5a8f 100755 (executable)
--- a/t/t1800-hook.sh
+++ b/t/t1800-hook.sh
@@ -357,7 +357,15 @@ test_expect_success 'disabled hook is not run' '
        test_must_be_empty actual
 '
 
-test_expect_success 'disabled hook does not appear in git hook list' '
+test_expect_success 'disabled hook with no command warns' '
+       test_config hook.nocommand.event "pre-commit" &&
+       test_config hook.nocommand.enabled false &&
+
+       git hook list pre-commit 2>actual &&
+       test_grep "disabled hook.*nocommand.*no command configured" actual
+'
+
+test_expect_success 'disabled hook appears as disabled in git hook list' '
        test_config hook.active.event "pre-commit" &&
        test_config hook.active.command "echo active" &&
        test_config hook.inactive.event "pre-commit" &&
@@ -365,8 +373,27 @@ test_expect_success 'disabled hook does not appear in git hook list' '
        test_config hook.inactive.enabled false &&
 
        git hook list pre-commit >actual &&
-       test_grep "active" actual &&
-       test_grep ! "inactive" actual
+       test_grep "^active$" actual &&
+       test_grep "^disabled    inactive$" actual
+'
+
+test_expect_success 'disabled hook shows scope with --show-scope' '
+       test_config hook.myhook.event "pre-commit" &&
+       test_config hook.myhook.command "echo hi" &&
+       test_config hook.myhook.enabled false &&
+
+       git hook list --show-scope pre-commit >actual &&
+       test_grep "^local       disabled        myhook$" actual
+'
+
+test_expect_success 'disabled configured hook is not reported as existing by hook_exists' '
+       test_when_finished "rm -f git-bugreport-hook-exists-test.txt" &&
+       test_config hook.linter.event "pre-commit" &&
+       test_config hook.linter.command "echo lint" &&
+       test_config hook.linter.enabled false &&
+
+       git bugreport -s hook-exists-test &&
+       test_grep ! "pre-commit" git-bugreport-hook-exists-test.txt
 '
 
 test_expect_success 'globally disabled hook can be re-enabled locally' '