From fd9e8f102f1342814307a437d936d4e7dc3b005a Mon Sep 17 00:00:00 2001 From: Adrian Ratiu Date: Mon, 9 Mar 2026 02:54:07 +0200 Subject: [PATCH 01/10] hook: move unsorted_string_list_remove() to string-list.[ch] Move the convenience wrapper from hook to string-list since it's a more suitable place. Add a doc comment to the header. Suggested-by: Patrick Steinhardt Signed-off-by: Adrian Ratiu Signed-off-by: Junio C Hamano --- hook.c | 8 -------- string-list.c | 9 +++++++++ string-list.h | 8 ++++++++ 3 files changed, 17 insertions(+), 8 deletions(-) diff --git a/hook.c b/hook.c index 2c8252b2c4..313a6b9937 100644 --- a/hook.c +++ b/hook.c @@ -110,14 +110,6 @@ static void list_hooks_add_default(struct repository *r, const char *hookname, string_list_append(hook_list, hook_path)->util = h; } -static void unsorted_string_list_remove(struct string_list *list, - const char *str) -{ - struct string_list_item *item = unsorted_string_list_lookup(list, str); - if (item) - unsorted_string_list_delete_item(list, item - list->items, 0); -} - /* * Callback struct to collect all hook.* keys in a single config pass. * commands: friendly-name to command map. diff --git a/string-list.c b/string-list.c index fffa2ad4b6..d260b873c8 100644 --- a/string-list.c +++ b/string-list.c @@ -281,6 +281,15 @@ void unsorted_string_list_delete_item(struct string_list *list, int i, int free_ list->nr--; } +void unsorted_string_list_remove(struct string_list *list, const char *str, + int free_util) +{ + struct string_list_item *item = unsorted_string_list_lookup(list, str); + if (item) + unsorted_string_list_delete_item(list, item - list->items, + free_util); +} + /* * append a substring [p..end] to list; return number of things it * appended to the list. diff --git a/string-list.h b/string-list.h index 3ad862a187..b86ee7c099 100644 --- a/string-list.h +++ b/string-list.h @@ -265,6 +265,14 @@ struct string_list_item *unsorted_string_list_lookup(struct string_list *list, */ void unsorted_string_list_delete_item(struct string_list *list, int i, int free_util); +/** + * Remove the first item matching `str` from an unsorted string_list. + * No-op if `str` is not found. If `free_util` is non-zero, the `util` + * pointer of the removed item is freed before deletion. + */ +void unsorted_string_list_remove(struct string_list *list, const char *str, + int free_util); + /** * Split string into substrings on characters in `delim` and append the * substrings to `list`. The input string is not modified. From 3f80ac69cfd83bf787fb7565379336ffadf154b4 Mon Sep 17 00:00:00 2001 From: Adrian Ratiu Date: Mon, 9 Mar 2026 02:54:08 +0200 Subject: [PATCH 02/10] hook: fix minor style issues Fix some minor style nits pointed by Patrick and Junio: * Use CALLOC_ARRAY instead of xcalloc. * Init struct members during declaration. * Simplify if condition boolean logic. * Missing curly braces in if/else stmts. * Unnecessary header includes. * Capitalization in error/warn messages. * Comment spelling: free'd -> freed. These contain no logic changes, the code behaves the same as before. Suggested-by: Junio C Hamano Suggested-by: Patrick Steinhardt Signed-off-by: Adrian Ratiu Signed-off-by: Junio C Hamano --- builtin/hook.c | 6 ++---- builtin/receive-pack.c | 11 +++++++---- hook.c | 25 +++++++++++++------------ refs.c | 3 ++- t/t1800-hook.sh | 2 +- transport.c | 3 ++- 6 files changed, 27 insertions(+), 23 deletions(-) diff --git a/builtin/hook.c b/builtin/hook.c index 83020dfb4f..c622a7399c 100644 --- a/builtin/hook.c +++ b/builtin/hook.c @@ -5,8 +5,6 @@ #include "gettext.h" #include "hook.h" #include "parse-options.h" -#include "strvec.h" -#include "abspath.h" #define BUILTIN_HOOK_RUN_USAGE \ N_("git hook run [--ignore-missing] [--to-stdin=] [-- ]") @@ -51,7 +49,7 @@ static int list(int argc, const char **argv, const char *prefix, * arguments later they probably should be caught by parse_options. */ if (argc != 1) - usage_msg_opt(_("You must specify a hook event name to list."), + usage_msg_opt(_("you must specify a hook event name to list."), builtin_hook_list_usage, list_options); hookname = argv[0]; @@ -59,7 +57,7 @@ static int list(int argc, const char **argv, const char *prefix, head = list_hooks(repo, hookname, NULL); if (!head->nr) { - warning(_("No hooks found for event '%s'"), hookname); + warning(_("no hooks found for event '%s'"), hookname); ret = 1; /* no hooks found */ goto cleanup; } diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c index 415bb57362..6066cbae8e 100644 --- a/builtin/receive-pack.c +++ b/builtin/receive-pack.c @@ -904,7 +904,8 @@ static int feed_receive_hook_cb(int hook_stdin_fd, void *pp_cb UNUSED, void *pp_ static void *receive_hook_feed_state_alloc(void *feed_pipe_ctx) { struct receive_hook_feed_state *init_state = feed_pipe_ctx; - struct receive_hook_feed_state *data = xcalloc(1, sizeof(*data)); + struct receive_hook_feed_state *data; + CALLOC_ARRAY(data, 1); data->report = init_state->report; data->cmd = init_state->cmd; data->skip_broken = init_state->skip_broken; @@ -928,7 +929,11 @@ static int run_receive_hook(struct command *commands, { struct run_hooks_opt opt = RUN_HOOKS_OPT_INIT; struct command *iter = commands; - struct receive_hook_feed_state feed_init_state = { 0 }; + struct receive_hook_feed_state feed_init_state = { + .cmd = commands, + .skip_broken = skip_broken, + .buf = STRBUF_INIT, + }; struct async sideband_async; int sideband_async_started = 0; int saved_stderr = -1; @@ -961,8 +966,6 @@ static int run_receive_hook(struct command *commands, prepare_sideband_async(&sideband_async, &saved_stderr, &sideband_async_started); /* set up stdin callback */ - feed_init_state.cmd = commands; - feed_init_state.skip_broken = skip_broken; opt.feed_pipe_ctx = &feed_init_state; opt.feed_pipe = feed_receive_hook_cb; opt.feed_pipe_cb_data_alloc = receive_hook_feed_state_alloc; diff --git a/hook.c b/hook.c index 313a6b9937..c5a6788dd5 100644 --- a/hook.c +++ b/hook.c @@ -57,9 +57,9 @@ static void hook_clear(struct hook *h, cb_data_free_fn cb_data_free) if (!h) return; - if (h->kind == HOOK_TRADITIONAL) + if (h->kind == HOOK_TRADITIONAL) { free((void *)h->u.traditional.path); - else if (h->kind == HOOK_CONFIGURED) { + } else if (h->kind == HOOK_CONFIGURED) { free((void *)h->u.configured.friendly_name); free((void *)h->u.configured.command); } @@ -91,7 +91,7 @@ static void list_hooks_add_default(struct repository *r, const char *hookname, if (!hook_path) return; - h = xcalloc(1, sizeof(struct hook)); + CALLOC_ARRAY(h, 1); /* * If the hook is to run in a specific dir, a relative path can @@ -154,7 +154,7 @@ static int hook_config_lookup_all(const char *key, const char *value, strmap_get(&data->event_hooks, value); if (!hooks) { - hooks = xcalloc(1, sizeof(*hooks)); + CALLOC_ARRAY(hooks, 1); string_list_init_dup(hooks); strmap_put(&data->event_hooks, value, hooks); } @@ -227,7 +227,8 @@ static void build_hook_config_map(struct repository *r, struct strmap *cache) /* Construct the cache from parsed configs. */ strmap_for_each_entry(&cb_data.event_hooks, &iter, e) { struct string_list *hook_names = e->value; - struct string_list *hooks = xcalloc(1, sizeof(*hooks)); + struct string_list *hooks; + CALLOC_ARRAY(hooks, 1); string_list_init_dup(hooks); @@ -281,7 +282,7 @@ static struct strmap *get_hook_config_cache(struct repository *r) * it just once on the first call. */ if (!r->hook_config_cache) { - r->hook_config_cache = xcalloc(1, sizeof(*cache)); + CALLOC_ARRAY(r->hook_config_cache, 1); strmap_init(r->hook_config_cache); build_hook_config_map(r, r->hook_config_cache); } @@ -289,9 +290,9 @@ static struct strmap *get_hook_config_cache(struct repository *r) } else { /* * Out-of-repo calls (no gitdir) allocate and return a temporary - * map cache which gets free'd immediately by the caller. + * cache which gets freed immediately by the caller. */ - cache = xcalloc(1, sizeof(*cache)); + CALLOC_ARRAY(cache, 1); strmap_init(cache); build_hook_config_map(r, cache); } @@ -311,7 +312,8 @@ static void list_hooks_add_configured(struct repository *r, for (size_t i = 0; configured_hooks && i < configured_hooks->nr; i++) { const char *friendly_name = configured_hooks->items[i].string; const char *command = configured_hooks->items[i].util; - struct hook *hook = xcalloc(1, sizeof(struct hook)); + struct hook *hook; + CALLOC_ARRAY(hook, 1); if (options && options->feed_pipe_cb_data_alloc) hook->feed_pipe_cb_data = @@ -343,7 +345,7 @@ struct string_list *list_hooks(struct repository *r, const char *hookname, if (!hookname) BUG("null hookname was provided to hook_list()!"); - hook_head = xmalloc(sizeof(struct string_list)); + CALLOC_ARRAY(hook_head, 1); string_list_init_dup(hook_head); /* Add hooks from the config, e.g. hook.myhook.event = pre-commit */ @@ -493,8 +495,7 @@ int run_hooks_opt(struct repository *r, const char *hook_name, * Ensure cb_data copy and free functions are either provided together, * or neither one is provided. */ - if ((options->feed_pipe_cb_data_alloc && !options->feed_pipe_cb_data_free) || - (!options->feed_pipe_cb_data_alloc && options->feed_pipe_cb_data_free)) + if (!options->feed_pipe_cb_data_alloc != !options->feed_pipe_cb_data_free) BUG("feed_pipe_cb_data_alloc and feed_pipe_cb_data_free must be set together"); if (options->invoked_hook) diff --git a/refs.c b/refs.c index a3363518e8..bd91c5c882 100644 --- a/refs.c +++ b/refs.c @@ -2599,7 +2599,8 @@ static int transaction_hook_feed_stdin(int hook_stdin_fd, void *pp_cb, void *pp_ static void *transaction_feed_cb_data_alloc(void *feed_pipe_ctx UNUSED) { - struct transaction_feed_cb_data *data = xmalloc(sizeof(*data)); + struct transaction_feed_cb_data *data; + CALLOC_ARRAY(data, 1); strbuf_init(&data->buf, 0); data->index = 0; return data; diff --git a/t/t1800-hook.sh b/t/t1800-hook.sh index b1583e9ef9..952bf97b86 100755 --- a/t/t1800-hook.sh +++ b/t/t1800-hook.sh @@ -34,7 +34,7 @@ test_expect_success 'git hook usage' ' test_expect_success 'git hook list: nonexistent hook' ' cat >stderr.expect <<-\EOF && - warning: No hooks found for event '\''test-hook'\'' + warning: no hooks found for event '\''test-hook'\'' EOF test_expect_code 1 git hook list test-hook 2>stderr.actual && test_cmp stderr.expect stderr.actual diff --git a/transport.c b/transport.c index 107f4fa5dc..56a4015389 100644 --- a/transport.c +++ b/transport.c @@ -1360,7 +1360,8 @@ static int pre_push_hook_feed_stdin(int hook_stdin_fd, void *pp_cb UNUSED, void static void *pre_push_hook_data_alloc(void *feed_pipe_ctx) { - struct feed_pre_push_hook_data *data = xmalloc(sizeof(*data)); + struct feed_pre_push_hook_data *data; + CALLOC_ARRAY(data, 1); strbuf_init(&data->buf, 0); data->refs = (struct ref *)feed_pipe_ctx; return data; From 90f98a0d4770bb8311e87b694fc78818dd63d9d3 Mon Sep 17 00:00:00 2001 From: Adrian Ratiu Date: Mon, 9 Mar 2026 02:54:09 +0200 Subject: [PATCH 03/10] hook: rename cb_data_free/alloc -> hook_data_free/alloc Rename the hook callback function types to use the hook prefix. This is a style fix with no logic changes. Suggested-by: Patrick Steinhardt Signed-off-by: Adrian Ratiu Signed-off-by: Junio C Hamano --- hook.c | 4 ++-- hook.h | 10 +++++----- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/hook.c b/hook.c index c5a6788dd5..bc1c45b16d 100644 --- a/hook.c +++ b/hook.c @@ -52,7 +52,7 @@ const char *find_hook(struct repository *r, const char *name) return path.buf; } -static void hook_clear(struct hook *h, cb_data_free_fn cb_data_free) +static void hook_clear(struct hook *h, hook_data_free_fn cb_data_free) { if (!h) return; @@ -70,7 +70,7 @@ static void hook_clear(struct hook *h, cb_data_free_fn cb_data_free) free(h); } -void hook_list_clear(struct string_list *hooks, cb_data_free_fn cb_data_free) +void hook_list_clear(struct string_list *hooks, hook_data_free_fn cb_data_free) { struct string_list_item *item; diff --git a/hook.h b/hook.h index e949f5d488..e514c1b45b 100644 --- a/hook.h +++ b/hook.h @@ -43,8 +43,8 @@ struct hook { void *feed_pipe_cb_data; }; -typedef void (*cb_data_free_fn)(void *data); -typedef void *(*cb_data_alloc_fn)(void *init_ctx); +typedef void (*hook_data_free_fn)(void *data); +typedef void *(*hook_data_alloc_fn)(void *init_ctx); struct run_hooks_opt { @@ -132,14 +132,14 @@ struct run_hooks_opt * * The `feed_pipe_ctx` pointer can be used to pass initialization data. */ - cb_data_alloc_fn feed_pipe_cb_data_alloc; + hook_data_alloc_fn feed_pipe_cb_data_alloc; /** * Called to free the memory initialized by `feed_pipe_cb_data_alloc`. * * Must always be provided when `feed_pipe_cb_data_alloc` is provided. */ - cb_data_free_fn feed_pipe_cb_data_free; + hook_data_free_fn feed_pipe_cb_data_free; }; #define RUN_HOOKS_OPT_INIT { \ @@ -189,7 +189,7 @@ struct string_list *list_hooks(struct repository *r, const char *hookname, * Frees the memory allocated for the hook list, including the `struct hook` * items and their internal state. */ -void hook_list_clear(struct string_list *hooks, cb_data_free_fn cb_data_free); +void hook_list_clear(struct string_list *hooks, hook_data_free_fn cb_data_free); /** * Frees the hook configuration cache stored in `struct repository`. From c694016e6116cd60905b3d79fd7a708c27ec6d8c Mon Sep 17 00:00:00 2001 From: Adrian Ratiu Date: Mon, 9 Mar 2026 02:54:10 +0200 Subject: [PATCH 04/10] hook: detect & emit two more bugs Trigger a bug when an unknown hook type is encountered while setting up hook execution. Also issue a bug if a configured hook is enabled without a cmd. Mostly useful for defensive coding. Suggested-by: Patrick Steinhardt Signed-off-by: Adrian Ratiu Signed-off-by: Junio C Hamano --- hook.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/hook.c b/hook.c index bc1c45b16d..b8ed4d79e2 100644 --- a/hook.c +++ b/hook.c @@ -408,7 +408,11 @@ static int pick_next_hook(struct child_process *cp, } else if (h->kind == HOOK_CONFIGURED) { /* to enable oneliners, let config-specified hooks run in shell. */ cp->use_shell = true; + if (!h->u.configured.command) + BUG("non-disabled HOOK_CONFIGURED hook has no command"); strvec_push(&cp->args, h->u.configured.command); + } else { + BUG("unknown hook kind"); } if (!cp->args.nr) From 468e466a78f5aa514f401082094a2ca38db100b8 Mon Sep 17 00:00:00 2001 From: Adrian Ratiu Date: Mon, 9 Mar 2026 02:54:11 +0200 Subject: [PATCH 05/10] hook: replace hook_list_clear() -> string_list_clear_func() Replace the custom function with string_list_clear_func() which is a more common pattern for clearing a string_list. To be able to do this, rework hook_clear() into hook_free(), so it can be passed to string_list_clear_func(). A slight complication is the need to keep a copy of the internal cb data free() pointer, however I think it's worth it since the API becomes cleaner, e.g. no more calls with NULL function args like hook_list_clear(hooks, NULL). Suggested-by: Patrick Steinhardt Signed-off-by: Adrian Ratiu Signed-off-by: Junio C Hamano --- builtin/hook.c | 2 +- hook.c | 50 +++++++++++++++++++++++++++++--------------------- hook.h | 20 ++++++++++++++------ 3 files changed, 44 insertions(+), 28 deletions(-) diff --git a/builtin/hook.c b/builtin/hook.c index c622a7399c..8fc647a4de 100644 --- a/builtin/hook.c +++ b/builtin/hook.c @@ -78,7 +78,7 @@ static int list(int argc, const char **argv, const char *prefix, } cleanup: - hook_list_clear(head, NULL); + string_list_clear_func(head, hook_free); free(head); return ret; } diff --git a/hook.c b/hook.c index b8ed4d79e2..f6bb1999ae 100644 --- a/hook.c +++ b/hook.c @@ -52,8 +52,14 @@ const char *find_hook(struct repository *r, const char *name) return path.buf; } -static void hook_clear(struct hook *h, hook_data_free_fn cb_data_free) +/* + * Frees a struct hook stored as the util pointer of a string_list_item. + * Suitable for use as a string_list_clear_func_t callback. + */ +void hook_free(void *p, const char *str UNUSED) { + struct hook *h = p; + if (!h) return; @@ -64,22 +70,12 @@ static void hook_clear(struct hook *h, hook_data_free_fn cb_data_free) free((void *)h->u.configured.command); } - if (cb_data_free) - cb_data_free(h->feed_pipe_cb_data); + if (h->data_free && h->feed_pipe_cb_data) + h->data_free(h->feed_pipe_cb_data); free(h); } -void hook_list_clear(struct string_list *hooks, hook_data_free_fn cb_data_free) -{ - struct string_list_item *item; - - for_each_string_list_item(item, hooks) - hook_clear(item->util, cb_data_free); - - string_list_clear(hooks, 0); -} - /* Helper to detect and add default "traditional" hooks from the hookdir. */ static void list_hooks_add_default(struct repository *r, const char *hookname, struct string_list *hook_list, @@ -100,9 +96,15 @@ static void list_hooks_add_default(struct repository *r, const char *hookname, if (options && options->dir) hook_path = absolute_path(hook_path); - /* Setup per-hook internal state cb data */ - if (options && options->feed_pipe_cb_data_alloc) + /* + * Setup per-hook internal state callback data. + * When provided, the alloc/free callbacks are always provided + * together, so use them to alloc/free the internal hook state. + */ + if (options && options->feed_pipe_cb_data_alloc) { h->feed_pipe_cb_data = options->feed_pipe_cb_data_alloc(options->feed_pipe_ctx); + h->data_free = options->feed_pipe_cb_data_free; + } h->kind = HOOK_TRADITIONAL; h->u.traditional.path = xstrdup(hook_path); @@ -148,7 +150,7 @@ static int hook_config_lookup_all(const char *key, const char *value, struct strmap_entry *e; strmap_for_each_entry(&data->event_hooks, &iter, e) - unsorted_string_list_remove(e->value, hook_name); + unsorted_string_list_remove(e->value, hook_name, 0); } else { struct string_list *hooks = strmap_get(&data->event_hooks, value); @@ -160,7 +162,7 @@ static int hook_config_lookup_all(const char *key, const char *value, } /* Re-insert if necessary to preserve last-seen order. */ - unsorted_string_list_remove(hooks, hook_name); + unsorted_string_list_remove(hooks, hook_name, 0); string_list_append(hooks, hook_name); } } else if (!strcmp(subkey, "command")) { @@ -178,7 +180,7 @@ static int hook_config_lookup_all(const char *key, const char *value, break; case 1: /* enabled: undo a prior disabled entry */ unsorted_string_list_remove(&data->disabled_hooks, - hook_name); + hook_name, 0); break; default: break; /* ignore unrecognised values */ @@ -315,10 +317,16 @@ static void list_hooks_add_configured(struct repository *r, struct hook *hook; CALLOC_ARRAY(hook, 1); - if (options && options->feed_pipe_cb_data_alloc) + /* + * When provided, the alloc/free callbacks are always provided + * together, so use them to alloc/free the internal hook state. + */ + if (options && options->feed_pipe_cb_data_alloc) { hook->feed_pipe_cb_data = options->feed_pipe_cb_data_alloc( options->feed_pipe_ctx); + hook->data_free = options->feed_pipe_cb_data_free; + } hook->kind = HOOK_CONFIGURED; hook->u.configured.friendly_name = xstrdup(friendly_name); @@ -361,7 +369,7 @@ int hook_exists(struct repository *r, const char *name) { struct string_list *hooks = list_hooks(r, name, NULL); int exists = hooks->nr > 0; - hook_list_clear(hooks, NULL); + string_list_clear_func(hooks, hook_free); free(hooks); return exists; } @@ -515,7 +523,7 @@ int run_hooks_opt(struct repository *r, const char *hook_name, run_processes_parallel(&opts); ret = cb_data.rc; cleanup: - hook_list_clear(cb_data.hook_command_list, options->feed_pipe_cb_data_free); + string_list_clear_func(cb_data.hook_command_list, hook_free); free(cb_data.hook_command_list); run_hooks_opt_clear(options); return ret; diff --git a/hook.h b/hook.h index e514c1b45b..168c6495a4 100644 --- a/hook.h +++ b/hook.h @@ -7,6 +7,9 @@ struct repository; +typedef void (*hook_data_free_fn)(void *data); +typedef void *(*hook_data_alloc_fn)(void *init_ctx); + /** * Represents a hook command to be run. * Hooks can be: @@ -41,10 +44,15 @@ struct hook { * Only useful when using `run_hooks_opt.feed_pipe`, otherwise ignore it. */ void *feed_pipe_cb_data; -}; -typedef void (*hook_data_free_fn)(void *data); -typedef void *(*hook_data_alloc_fn)(void *init_ctx); + /** + * Callback to free `feed_pipe_cb_data`. + * + * It is called automatically and points to the `feed_pipe_cb_data_free` + * provided via the `run_hook_opt` parameter. + */ + hook_data_free_fn data_free; +}; struct run_hooks_opt { @@ -186,10 +194,10 @@ struct string_list *list_hooks(struct repository *r, const char *hookname, struct run_hooks_opt *options); /** - * Frees the memory allocated for the hook list, including the `struct hook` - * items and their internal state. + * Frees a struct hook stored as the util pointer of a string_list_item. + * Suitable for use as a string_list_clear_func_t callback. */ -void hook_list_clear(struct string_list *hooks, hook_data_free_fn cb_data_free); +void hook_free(void *p, const char *str UNUSED); /** * Frees the hook configuration cache stored in `struct repository`. From ee360a026801d7cd5b18015cb2a9687100c196c7 Mon Sep 17 00:00:00 2001 From: Adrian Ratiu Date: Mon, 9 Mar 2026 02:54:12 +0200 Subject: [PATCH 06/10] hook: make consistent use of friendly-name in docs Both `name` and `friendly-name` is being used. Standardize on `friendly-name` for consistency since name is rather generic, even when used in the hooks namespace. Suggested-by: Junio C Hamano Signed-off-by: Adrian Ratiu Signed-off-by: Junio C Hamano --- Documentation/config/hook.adoc | 30 +++++++++++++++--------------- Documentation/git-hook.adoc | 6 +++--- hook.c | 2 +- hook.h | 2 +- 4 files changed, 20 insertions(+), 20 deletions(-) diff --git a/Documentation/config/hook.adoc b/Documentation/config/hook.adoc index 64e845a260..9e78f26439 100644 --- a/Documentation/config/hook.adoc +++ b/Documentation/config/hook.adoc @@ -1,23 +1,23 @@ -hook..command:: - The command to execute for `hook.`. `` is a unique - "friendly" name that identifies this hook. (The hook events that - trigger the command are configured with `hook..event`.) The - value can be an executable path or a shell oneliner. If more than - one value is specified for the same ``, only the last value - parsed is used. See linkgit:git-hook[1]. +hook..command:: + The command to execute for `hook.`. `` + is a unique name that identifies this hook. The hook events that + trigger the command are configured with `hook..event`. + The value can be an executable path or a shell oneliner. If more than + one value is specified for the same ``, only the last + value parsed is used. See linkgit:git-hook[1]. -hook..event:: - The hook events that trigger `hook.`. The value is the name - of a hook event, like "pre-commit" or "update". (See +hook..event:: + The hook events that trigger `hook.`. The value is the + name of a hook event, like "pre-commit" or "update". (See linkgit:githooks[5] for a complete list of hook events.) On the - specified event, the associated `hook..command` is executed. - This is a multi-valued key. To run `hook.` on multiple + specified event, the associated `hook..command` is executed. + This is a multi-valued key. To run `hook.` on multiple events, specify the key more than once. An empty value resets the list of events, clearing any previously defined events for - `hook.`. See linkgit:git-hook[1]. + `hook.`. See linkgit:git-hook[1]. -hook..enabled:: - Whether the hook `hook.` is enabled. Defaults to `true`. +hook..enabled:: + Whether the hook `hook.` is enabled. Defaults to `true`. Set to `false` to disable the hook without removing its configuration. This is particularly useful when a hook is defined in a system or global config file and needs to be disabled for a diff --git a/Documentation/git-hook.adoc b/Documentation/git-hook.adoc index 12d2701b52..966388660a 100644 --- a/Documentation/git-hook.adoc +++ b/Documentation/git-hook.adoc @@ -44,7 +44,7 @@ event`), and then `~/bin/spellchecker` will have a chance to check your commit message (during the `commit-msg` hook event). Commands are run in the order Git encounters their associated -`hook..event` configs during the configuration parse (see +`hook..event` configs during the configuration parse (see linkgit:git-config[1]). Although multiple `hook.linter.event` configs can be added, only one `hook.linter.command` event is valid - Git uses "last-one-wins" to determine which command to run. @@ -76,10 +76,10 @@ first start `~/bin/linter --cpp20` and second start `~/bin/leak-detector`. It would evaluate the output of each when deciding whether to proceed with the commit. -For a full list of hook events which you can set your `hook..event` to, +For a full list of hook events which you can set your `hook..event` to, and how hooks are invoked during those events, see linkgit:githooks[5]. -Git will ignore any `hook..event` that specifies an event it doesn't +Git will ignore any `hook..event` that specifies an event it doesn't recognize. This is intended so that tools which wrap Git can use the hook infrastructure to run their own hooks; see "WRAPPERS" for more guidance. diff --git a/hook.c b/hook.c index f6bb1999ae..7f89ae9cc2 100644 --- a/hook.c +++ b/hook.c @@ -116,7 +116,7 @@ static void list_hooks_add_default(struct repository *r, const char *hookname, * 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.name.enabled = false. + * disabled_hooks: set of friendly-names with hook..enabled = false. */ struct hook_all_config_cb { struct strmap commands; diff --git a/hook.h b/hook.h index 168c6495a4..49b40d949b 100644 --- a/hook.h +++ b/hook.h @@ -14,7 +14,7 @@ typedef void *(*hook_data_alloc_fn)(void *init_ctx); * Represents a hook command to be run. * Hooks can be: * 1. "traditional" (found in the hooks directory) - * 2. "configured" (defined in Git's configuration via hook..event). + * 2. "configured" (defined in Git's configuration via hook..event). * The 'kind' field determines which part of the union 'u' is valid. */ struct hook { From 5c887b271287107bac1f841b590ba137a268c74c Mon Sep 17 00:00:00 2001 From: Adrian Ratiu Date: Mon, 9 Mar 2026 02:54:13 +0200 Subject: [PATCH 07/10] t1800: add test to verify hook execution ordering There is a documented expectation that configured hooks are run before the hook from the hookdir. Add a test for it. While at it, I noticed that `git hook list -h` runs twice in the `git hook usage` test, so remove one invocation. Suggested-by: Patrick Steinhardt Signed-off-by: Adrian Ratiu Signed-off-by: Junio C Hamano --- t/t1800-hook.sh | 29 ++++++++++++++++++++++++++++- 1 file changed, 28 insertions(+), 1 deletion(-) diff --git a/t/t1800-hook.sh b/t/t1800-hook.sh index 952bf97b86..7eee84fc39 100755 --- a/t/t1800-hook.sh +++ b/t/t1800-hook.sh @@ -25,7 +25,6 @@ test_expect_success 'git hook usage' ' test_expect_code 129 git hook && test_expect_code 129 git hook run && test_expect_code 129 git hook run -h && - test_expect_code 129 git hook list -h && test_expect_code 129 git hook run --unknown 2>err && test_expect_code 129 git hook list && test_expect_code 129 git hook list -h && @@ -381,6 +380,34 @@ test_expect_success 'globally disabled hook can be re-enabled locally' ' test_cmp expected actual ' +test_expect_success 'configured hooks run before hookdir hook' ' + setup_hookdir && + test_config hook.first.event "pre-commit" && + test_config hook.first.command "echo first" && + test_config hook.second.event "pre-commit" && + test_config hook.second.command "echo second" && + + cat >expected <<-\EOF && + first + second + hook from hookdir + EOF + + git hook list pre-commit >actual && + test_cmp expected actual && + + # "Legacy Hook" is the output of the hookdir pre-commit script + # written by setup_hookdir() above. + cat >expected <<-\EOF && + first + second + "Legacy Hook" + EOF + + git hook run pre-commit 2>actual && + test_cmp expected actual +' + test_expect_success 'git hook run a hook with a bad shebang' ' test_when_finished "rm -rf bad-hooks" && mkdir bad-hooks && From 2a99233083a1f03e25d457214d3897b9a9d44363 Mon Sep 17 00:00:00 2001 From: Adrian Ratiu Date: Mon, 9 Mar 2026 02:54:14 +0200 Subject: [PATCH 08/10] hook: refactor hook_config_cache from strmap to named struct Replace the raw `struct strmap *hook_config_cache` in `struct repository` with a `struct hook_config_cache` which wraps the strmap in a named field. Replace the bare `char *command` util pointer stored in each string_list item with a heap-allocated `struct hook_config_cache_entry` that carries that command string. This is just a refactoring with no behavior changes, to give the cache struct room to grow so it can carry the additional hook metadata we'll be adding in the following commits. Signed-off-by: Adrian Ratiu Signed-off-by: Junio C Hamano --- hook.c | 61 +++++++++++++++++++++++++++++++++------------------- hook.h | 10 ++++++++- repository.h | 3 ++- 3 files changed, 50 insertions(+), 24 deletions(-) diff --git a/hook.c b/hook.c index 7f89ae9cc2..4fe50aa38c 100644 --- a/hook.c +++ b/hook.c @@ -112,6 +112,15 @@ static void list_hooks_add_default(struct repository *r, const char *hookname, string_list_append(hook_list, hook_path)->util = h; } +/* + * Cache entry stored as the .util pointer of string_list items inside the + * hook config cache. For now carries only the command for the hook. Next + * commits will add more data. + */ +struct hook_config_cache_entry { + char *command; +}; + /* * Callback struct to collect all hook.* keys in a single config pass. * commands: friendly-name to command map. @@ -194,26 +203,32 @@ static int hook_config_lookup_all(const char *key, const char *value, /* * The hook config cache maps each hook event name to a string_list where * 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. + * a hook_config_cache_entry. All 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. */ -void hook_cache_clear(struct strmap *cache) +void hook_cache_clear(struct hook_config_cache *cache) { struct hashmap_iter iter; struct strmap_entry *e; - strmap_for_each_entry(cache, &iter, e) { + strmap_for_each_entry(&cache->hooks, &iter, e) { struct string_list *hooks = e->value; - string_list_clear(hooks, 1); /* free util (command) pointers */ + for (size_t i = 0; i < hooks->nr; i++) { + struct hook_config_cache_entry *entry = hooks->items[i].util; + free(entry->command); + free(entry); + } + string_list_clear(hooks, 0); free(hooks); } - strmap_clear(cache, 0); + strmap_clear(&cache->hooks, 0); } /* Populate `cache` with the complete hook configuration */ -static void build_hook_config_map(struct repository *r, struct strmap *cache) +static void build_hook_config_map(struct repository *r, + struct hook_config_cache *cache) { struct hook_all_config_cb cb_data; struct hashmap_iter iter; @@ -236,6 +251,7 @@ static void build_hook_config_map(struct repository *r, struct strmap *cache) for (size_t i = 0; i < hook_names->nr; i++) { const char *hname = hook_names->items[i].string; + struct hook_config_cache_entry *entry; char *command; /* filter out disabled hooks */ @@ -249,12 +265,13 @@ static void build_hook_config_map(struct repository *r, struct strmap *cache) "'hook.%s.event' must be removed;" " aborting."), hname, hname); - /* util stores the command; owned by the cache. */ - string_list_append(hooks, hname)->util = - xstrdup(command); + /* util stores a cache entry; owned by the cache. */ + CALLOC_ARRAY(entry, 1); + entry->command = xstrdup(command); + string_list_append(hooks, hname)->util = entry; } - strmap_put(cache, e->key, hooks); + strmap_put(&cache->hooks, e->key, hooks); } strmap_clear(&cb_data.commands, 1); @@ -267,25 +284,25 @@ static void build_hook_config_map(struct repository *r, struct strmap *cache) } /* - * Return the hook config map for `r`, populating it first if needed. + * Return the hook config cache for `r`, populating it first if needed. * * Out-of-repo calls (r->gitdir == NULL) allocate and return a temporary - * cache map; the caller is responsible for freeing it with + * cache; the caller is responsible for freeing it with * hook_cache_clear() + free(). */ -static struct strmap *get_hook_config_cache(struct repository *r) +static struct hook_config_cache *get_hook_config_cache(struct repository *r) { - struct strmap *cache = NULL; + struct hook_config_cache *cache = NULL; if (r && r->gitdir) { /* - * For in-repo calls, the map is stored in r->hook_config_cache, - * so repeated invocations don't parse the configs, so allocate + * For in-repo calls, the cache is stored in r->hook_config_cache, + * so repeated invocations don't parse the configs; allocate * it just once on the first call. */ if (!r->hook_config_cache) { CALLOC_ARRAY(r->hook_config_cache, 1); - strmap_init(r->hook_config_cache); + strmap_init(&r->hook_config_cache->hooks); build_hook_config_map(r, r->hook_config_cache); } cache = r->hook_config_cache; @@ -295,7 +312,7 @@ static struct strmap *get_hook_config_cache(struct repository *r) * cache which gets freed immediately by the caller. */ CALLOC_ARRAY(cache, 1); - strmap_init(cache); + strmap_init(&cache->hooks); build_hook_config_map(r, cache); } @@ -307,13 +324,13 @@ static void list_hooks_add_configured(struct repository *r, struct string_list *list, struct run_hooks_opt *options) { - struct strmap *cache = get_hook_config_cache(r); - struct string_list *configured_hooks = strmap_get(cache, hookname); + struct hook_config_cache *cache = get_hook_config_cache(r); + struct string_list *configured_hooks = strmap_get(&cache->hooks, hookname); /* Iterate through configured hooks and initialize internal states */ for (size_t i = 0; configured_hooks && i < configured_hooks->nr; i++) { const char *friendly_name = configured_hooks->items[i].string; - const char *command = configured_hooks->items[i].util; + struct hook_config_cache_entry *entry = configured_hooks->items[i].util; struct hook *hook; CALLOC_ARRAY(hook, 1); @@ -330,7 +347,7 @@ 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(command); + hook->u.configured.command = xstrdup(entry->command); string_list_append(list, friendly_name)->util = hook; } diff --git a/hook.h b/hook.h index 49b40d949b..4d0c22f1dc 100644 --- a/hook.h +++ b/hook.h @@ -199,11 +199,19 @@ struct string_list *list_hooks(struct repository *r, const char *hookname, */ void hook_free(void *p, const char *str UNUSED); +/** + * Persistent cache for hook configuration, stored on `struct repository`. + * Populated lazily on first hook use and freed by repo_clear(). + */ +struct hook_config_cache { + struct strmap hooks; /* maps event name -> string_list of hooks */ +}; + /** * Frees the hook configuration cache stored in `struct repository`. * Called by repo_clear(). */ -void hook_cache_clear(struct strmap *cache); +void hook_cache_clear(struct hook_config_cache *cache); /** * Returns the path to the hook file, or NULL if the hook is missing diff --git a/repository.h b/repository.h index 7830eb7d4b..8d6e0b3dd2 100644 --- a/repository.h +++ b/repository.h @@ -12,6 +12,7 @@ struct lock_file; struct pathspec; struct object_database; struct submodule_cache; +struct hook_config_cache; struct promisor_remote_config; struct remote_state; @@ -170,7 +171,7 @@ struct repository { * Lazily-populated cache mapping hook event names to configured hooks. * NULL until first hook use. */ - struct strmap *hook_config_cache; + struct hook_config_cache *hook_config_cache; /* Configurations related to promisor remotes. */ char *repository_format_partial_clone; From dcd7ab7d2ef092ef9350cdfcc57c9bf92d942c20 Mon Sep 17 00:00:00 2001 From: Adrian Ratiu Date: Mon, 9 Mar 2026 02:54:15 +0200 Subject: [PATCH 09/10] hook: show config scope in git hook list Users running "git hook list" can see which hooks are configured but have no way to tell at which config scope (local, global, system...) each hook was defined. Store the scope from ctx->kvi->scope in the single-pass config callback, then carry it through the cache to the hook structs, so we can expose it to users via the "git hook list --show-scope" flag, which mirrors the existing git config --show-scope convention. Without the flag the output is unchanged. Example usage: $ git hook list --show-scope pre-commit linter (global) no-leaks (local) hook from hookdir Traditional hooks from the hookdir are unaffected by --show-scope since the config scope concept does not apply to them. Suggested-by: Junio C Hamano Signed-off-by: Adrian Ratiu Signed-off-by: Junio C Hamano --- Documentation/git-hook.adoc | 9 +++++++-- builtin/hook.c | 14 ++++++++++++-- hook.c | 24 ++++++++++++++++++++---- hook.h | 2 ++ t/t1800-hook.sh | 19 +++++++++++++++++++ 5 files changed, 60 insertions(+), 8 deletions(-) diff --git a/Documentation/git-hook.adoc b/Documentation/git-hook.adoc index 966388660a..4d4e728327 100644 --- a/Documentation/git-hook.adoc +++ b/Documentation/git-hook.adoc @@ -9,7 +9,7 @@ SYNOPSIS -------- [verse] 'git hook' run [--ignore-missing] [--to-stdin=] [-- ] -'git hook' list [-z] +'git hook' list [-z] [--show-scope] DESCRIPTION ----------- @@ -113,7 +113,7 @@ Any positional arguments to the hook should be passed after a mandatory `--` (or `--end-of-options`, see linkgit:gitcli[7]). See linkgit:githooks[5] for arguments hooks might expect (if any). -list [-z]:: +list [-z] [--show-scope]:: Print a list of hooks which will be run on `` event. If no hooks are configured for that event, print a warning and return 1. Use `-z` to terminate output lines with NUL instead of newlines. @@ -134,6 +134,11 @@ OPTIONS -z:: Terminate "list" output lines with NUL instead of newlines. +--show-scope:: + For "list"; print the config scope (e.g. `local`, `global`, `system`) + in parentheses after the friendly name of each configured hook, to show + where it was defined. Traditional hooks from the hookdir are unaffected. + WRAPPERS -------- diff --git a/builtin/hook.c b/builtin/hook.c index 8fc647a4de..c806640361 100644 --- a/builtin/hook.c +++ b/builtin/hook.c @@ -9,7 +9,7 @@ #define BUILTIN_HOOK_RUN_USAGE \ N_("git hook run [--ignore-missing] [--to-stdin=] [-- ]") #define BUILTIN_HOOK_LIST_USAGE \ - N_("git hook list [-z] ") + N_("git hook list [-z] [--show-scope] ") static const char * const builtin_hook_usage[] = { BUILTIN_HOOK_RUN_USAGE, @@ -33,11 +33,14 @@ static int list(int argc, const char **argv, const char *prefix, struct string_list_item *item; const char *hookname = NULL; int line_terminator = '\n'; + int show_scope = 0; int ret = 0; struct option list_options[] = { OPT_SET_INT('z', NULL, &line_terminator, N_("use NUL as line terminator"), '\0'), + OPT_BOOL(0, "show-scope", &show_scope, + N_("show the config scope that defined each hook")), OPT_END(), }; @@ -70,7 +73,14 @@ static int list(int argc, const char **argv, const char *prefix, printf("%s%c", _("hook from hookdir"), line_terminator); break; case HOOK_CONFIGURED: - printf("%s%c", h->u.configured.friendly_name, line_terminator); + if (show_scope) + printf("%s (%s)%c", + h->u.configured.friendly_name, + config_scope_name(h->u.configured.scope), + line_terminator); + else + printf("%s%c", h->u.configured.friendly_name, + line_terminator); break; default: BUG("unknown hook kind"); diff --git a/hook.c b/hook.c index 4fe50aa38c..2c03baeaac 100644 --- a/hook.c +++ b/hook.c @@ -114,11 +114,11 @@ static void list_hooks_add_default(struct repository *r, const char *hookname, /* * Cache entry stored as the .util pointer of string_list items inside the - * hook config cache. For now carries only the command for the hook. Next - * commits will add more data. + * hook config cache. */ struct hook_config_cache_entry { char *command; + enum config_scope scope; }; /* @@ -135,7 +135,7 @@ struct hook_all_config_cb { /* repo_config() callback that collects all hook.* configuration in one pass. */ static int hook_config_lookup_all(const char *key, const char *value, - const struct config_context *ctx UNUSED, + const struct config_context *ctx, void *cb_data) { struct hook_all_config_cb *data = cb_data; @@ -172,7 +172,19 @@ static int hook_config_lookup_all(const char *key, const char *value, /* Re-insert if necessary to preserve last-seen order. */ unsorted_string_list_remove(hooks, hook_name, 0); - string_list_append(hooks, hook_name); + + if (!ctx->kvi) + BUG("hook config callback called without key-value info"); + + /* + * Stash the config scope in the util pointer for + * later retrieval in build_hook_config_map(). This + * intermediate struct is transient and never leaves + * that function, so we pack the enum value into the + * pointer rather than heap-allocating a wrapper. + */ + string_list_append(hooks, hook_name)->util = + (void *)(uintptr_t)ctx->kvi->scope; } } else if (!strcmp(subkey, "command")) { /* Store command overwriting the old value */ @@ -251,6 +263,8 @@ static void build_hook_config_map(struct repository *r, for (size_t i = 0; i < hook_names->nr; i++) { const char *hname = hook_names->items[i].string; + enum config_scope scope = + (enum config_scope)(uintptr_t)hook_names->items[i].util; struct hook_config_cache_entry *entry; char *command; @@ -268,6 +282,7 @@ static void build_hook_config_map(struct repository *r, /* util stores a cache entry; owned by the cache. */ CALLOC_ARRAY(entry, 1); entry->command = xstrdup(command); + entry->scope = scope; string_list_append(hooks, hname)->util = entry; } @@ -348,6 +363,7 @@ 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.scope = entry->scope; string_list_append(list, friendly_name)->util = hook; } diff --git a/hook.h b/hook.h index 4d0c22f1dc..0d711ed21a 100644 --- a/hook.h +++ b/hook.h @@ -1,5 +1,6 @@ #ifndef HOOK_H #define HOOK_H +#include "config.h" #include "strvec.h" #include "run-command.h" #include "string-list.h" @@ -29,6 +30,7 @@ struct hook { struct { const char *friendly_name; const char *command; + enum config_scope scope; } configured; } u; diff --git a/t/t1800-hook.sh b/t/t1800-hook.sh index 7eee84fc39..aed07575e3 100755 --- a/t/t1800-hook.sh +++ b/t/t1800-hook.sh @@ -408,6 +408,25 @@ test_expect_success 'configured hooks run before hookdir hook' ' test_cmp expected actual ' +test_expect_success 'git hook list --show-scope shows config scope' ' + test_config_global hook.global-hook.command "echo global" && + test_config_global hook.global-hook.event test-hook --add && + test_config hook.local-hook.command "echo local" && + test_config hook.local-hook.event test-hook --add && + + cat >expected <<-\EOF && + global-hook (global) + local-hook (local) + EOF + git hook list --show-scope test-hook >actual && + test_cmp expected actual && + + # without --show-scope the scope must not appear + git hook list test-hook >actual && + test_grep ! "(global)" actual && + test_grep ! "(local)" actual +' + test_expect_success 'git hook run a hook with a bad shebang' ' test_when_finished "rm -rf bad-hooks" && mkdir bad-hooks && From 86a30747aeef05c03e3ab9da85157bdc815357b5 Mon Sep 17 00:00:00 2001 From: Adrian Ratiu Date: Mon, 9 Mar 2026 02:54:16 +0200 Subject: [PATCH 10/10] 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 annotated with "(disabled)" in the config order. With --show-scope, it looks like: $ git hook list --show-scope pre-commit linter (global) no-leaks (local, disabled) 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 Signed-off-by: Adrian Ratiu Signed-off-by: Junio C Hamano --- builtin/hook.c | 18 ++++++++++------- hook.c | 54 +++++++++++++++++++++++++++++++++---------------- hook.h | 1 + t/t1800-hook.sh | 33 +++++++++++++++++++++++++++--- 4 files changed, 79 insertions(+), 27 deletions(-) diff --git a/builtin/hook.c b/builtin/hook.c index c806640361..ff446948fa 100644 --- 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 (%s)%c", - h->u.configured.friendly_name, - config_scope_name(h->u.configured.scope), + 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 (%s%s)%c", name, scope, + h->u.configured.disabled ? ", disabled" : "", line_terminator); + else if (h->u.configured.disabled) + printf("%s (disabled)%c", name, line_terminator); else - printf("%s%c", h->u.configured.friendly_name, - line_terminator); + printf("%s%c", name, line_terminator); break; + } default: BUG("unknown hook kind"); } diff --git a/hook.c b/hook.c index 2c03baeaac..4f4f060156 100644 --- a/hook.c +++ b/hook.c @@ -119,6 +119,7 @@ static void list_hooks_add_default(struct repository *r, const char *hookname, struct hook_config_cache_entry { char *command; enum config_scope scope; + int disabled; }; /* @@ -217,8 +218,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 * a hook_config_cache_entry. All 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. Hooks missing a command are filtered + * out at build time; if a disabled hook has no command it is silently + * skipped rather than triggering a fatal error. */ void hook_cache_clear(struct hook_config_cache *cache) { @@ -268,21 +271,26 @@ static void build_hook_config_map(struct repository *r, struct hook_config_cache_entry *entry; char *command; - /* filter out disabled hooks */ - if (unsorted_string_list_lookup(&cb_data.disabled_hooks, - hname)) - continue; + int 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 = command ? xstrdup(command) : NULL; entry->scope = scope; + entry->disabled = is_disabled; string_list_append(hooks, hname)->util = entry; } @@ -362,8 +370,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; } @@ -401,7 +411,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; @@ -416,10 +435,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 0d711ed21a..0432df963f 100644 --- a/hook.h +++ b/hook.h @@ -31,6 +31,7 @@ struct hook { const char *friendly_name; const char *command; enum config_scope scope; + int disabled; } configured; } u; diff --git a/t/t1800-hook.sh b/t/t1800-hook.sh index aed07575e3..cbf7d2bf80 100755 --- 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 "^inactive (disabled)$" 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 "myhook (local, disabled)" 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' '