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..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 ----------- @@ -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. @@ -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 83020dfb4f..ff446948fa 100644 --- a/builtin/hook.c +++ b/builtin/hook.c @@ -5,13 +5,11 @@ #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=] [-- ]") #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, @@ -35,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(), }; @@ -51,7 +52,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 +60,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; } @@ -71,16 +72,27 @@ 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: - printf("%s%c", 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 (%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", name, line_terminator); break; + } default: BUG("unknown hook kind"); } } cleanup: - hook_list_clear(head, NULL); + string_list_clear_func(head, hook_free); free(head); return ret; } diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c index d6225df890..4b63ccdfa3 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 2c8252b2c4..4f4f060156 100644 --- a/hook.c +++ b/hook.c @@ -52,34 +52,30 @@ 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) +/* + * 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; - 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); } - 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, cb_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, @@ -91,7 +87,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 @@ -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); @@ -110,19 +112,21 @@ 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); -} +/* + * Cache entry stored as the .util pointer of string_list items inside the + * hook config cache. + */ +struct hook_config_cache_entry { + char *command; + enum config_scope scope; + int disabled; +}; /* * 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; @@ -132,7 +136,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; @@ -156,20 +160,32 @@ 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); if (!hooks) { - hooks = xcalloc(1, sizeof(*hooks)); + CALLOC_ARRAY(hooks, 1); string_list_init_dup(hooks); strmap_put(&data->event_hooks, value, hooks); } /* Re-insert if necessary to preserve last-seen order. */ - unsorted_string_list_remove(hooks, hook_name); - string_list_append(hooks, hook_name); + unsorted_string_list_remove(hooks, hook_name, 0); + + 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 */ @@ -186,7 +202,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 */ @@ -200,26 +216,34 @@ 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. + * 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 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; @@ -235,31 +259,42 @@ 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); 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; - /* 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 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 = command ? xstrdup(command) : NULL; + entry->scope = scope; + entry->disabled = is_disabled; + 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); @@ -272,35 +307,35 @@ 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) { - r->hook_config_cache = xcalloc(1, sizeof(*cache)); - strmap_init(r->hook_config_cache); + CALLOC_ARRAY(r->hook_config_cache, 1); + strmap_init(&r->hook_config_cache->hooks); build_hook_config_map(r, r->hook_config_cache); } cache = r->hook_config_cache; } 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)); - strmap_init(cache); + CALLOC_ARRAY(cache, 1); + strmap_init(&cache->hooks); build_hook_config_map(r, cache); } @@ -312,23 +347,33 @@ 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 *hook = xcalloc(1, sizeof(struct hook)); + struct hook_config_cache_entry *entry = configured_hooks->items[i].util; + 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); - hook->u.configured.command = xstrdup(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; } @@ -351,7 +396,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 */ @@ -366,8 +411,17 @@ 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; - hook_list_clear(hooks, NULL); + 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; } @@ -381,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); @@ -414,7 +469,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) @@ -501,8 +560,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) @@ -518,7 +576,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 e949f5d488..0432df963f 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" @@ -7,11 +8,14 @@ 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: * 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 { @@ -26,6 +30,8 @@ struct hook { struct { const char *friendly_name; const char *command; + enum config_scope scope; + int disabled; } configured; } u; @@ -41,10 +47,15 @@ struct hook { * Only useful when using `run_hooks_opt.feed_pipe`, otherwise ignore it. */ void *feed_pipe_cb_data; -}; -typedef void (*cb_data_free_fn)(void *data); -typedef void *(*cb_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 { @@ -132,14 +143,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 { \ @@ -186,16 +197,24 @@ 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, cb_data_free_fn cb_data_free); +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/refs.c b/refs.c index 6fb8f9d10c..33c7961802 100644 --- a/refs.c +++ b/refs.c @@ -2591,7 +2591,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/repository.h b/repository.h index 078059a6e0..3fd73d2c54 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; 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. diff --git a/t/t1800-hook.sh b/t/t1800-hook.sh index b1583e9ef9..cbf7d2bf80 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 && @@ -34,7 +33,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 @@ -358,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" && @@ -366,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' ' @@ -381,6 +407,53 @@ 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 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 && diff --git a/transport.c b/transport.c index 259e9d7b70..32c461130d 100644 --- a/transport.c +++ b/transport.c @@ -1363,7 +1363,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;