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..318c637bd8 100644 --- a/Documentation/git-hook.adoc +++ b/Documentation/git-hook.adoc @@ -8,8 +8,8 @@ git-hook - Run git hooks SYNOPSIS -------- [verse] -'git hook' run [--ignore-missing] [--to-stdin=] [-- ] -'git hook' list [-z] +'git hook' run [--allow-unknown-hook-name] [--ignore-missing] [--to-stdin=] [-- ] +'git hook' list [--allow-unknown-hook-name] [-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. @@ -121,6 +121,13 @@ list [-z]:: OPTIONS ------- +--allow-unknown-hook-name:: + By default `git hook run` and `git hook list` will bail out when + `` is not a hook event known to Git (see linkgit:githooks[5] + for the list of known hooks). This is meant to help catch typos + such as `prereceive` when `pre-receive` was intended. Pass this + flag to allow unknown hook names. + --to-stdin:: For "run"; specify a file which will be streamed into the hook's stdin. The hook will receive the entire file from @@ -134,6 +141,12 @@ OPTIONS -z:: Terminate "list" output lines with NUL instead of newlines. +--show-scope:: + For "list"; prefix each configured hook's friendly name with a + tab-separated config scope (e.g. `local`, `global`, `system`), + mirroring the output style of `git config --show-scope`. Traditional + hooks from the hookdir are unaffected. + WRAPPERS -------- @@ -153,7 +166,7 @@ Then, in your 'mywrapper' tool, you can invoke any users' configured hooks by running: ---- -git hook run mywrapper-start-tests \ +git hook run --allow-unknown-hook-name mywrapper-start-tests \ # providing something to stdin --stdin some-tempfile-123 \ # execute hooks in serial diff --git a/Makefile b/Makefile index dbf0022054..5d22394c2e 100644 --- a/Makefile +++ b/Makefile @@ -2675,6 +2675,7 @@ git$X: git.o GIT-LDFLAGS $(BUILTIN_OBJS) $(GITLIBS) help.sp help.s help.o: command-list.h builtin/bugreport.sp builtin/bugreport.s builtin/bugreport.o: hook-list.h +builtin/hook.sp builtin/hook.s builtin/hook.o: hook-list.h builtin/help.sp builtin/help.s builtin/help.o: config-list.h GIT-PREFIX builtin/help.sp builtin/help.s builtin/help.o: EXTRA_CPPFLAGS = \ diff --git a/builtin/hook.c b/builtin/hook.c index 83020dfb4f..c0585587e5 100644 --- a/builtin/hook.c +++ b/builtin/hook.c @@ -4,14 +4,22 @@ #include "environment.h" #include "gettext.h" #include "hook.h" +#include "hook-list.h" #include "parse-options.h" -#include "strvec.h" -#include "abspath.h" #define BUILTIN_HOOK_RUN_USAGE \ - N_("git hook run [--ignore-missing] [--to-stdin=] [-- ]") + N_("git hook run [--allow-unknown-hook-name] [--ignore-missing] [--to-stdin=] [-- ]") #define BUILTIN_HOOK_LIST_USAGE \ - N_("git hook list [-z] ") + N_("git hook list [--allow-unknown-hook-name] [-z] [--show-scope] ") + +static int is_known_hook(const char *name) +{ + const char **p; + for (p = hook_name_list; *p; p++) + if (!strcmp(*p, name)) + return 1; + return 0; +} static const char * const builtin_hook_usage[] = { BUILTIN_HOOK_RUN_USAGE, @@ -35,11 +43,17 @@ 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 allow_unknown = 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_BOOL(0, "allow-unknown-hook-name", &allow_unknown, + N_("allow running a hook with a non-native hook name")), OPT_END(), }; @@ -51,15 +65,22 @@ 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]; + if (!allow_unknown && !is_known_hook(hookname)) { + error(_("unknown hook event '%s';\n" + "use --allow-unknown-hook-name to allow non-native hook names"), + hookname); + return 1; + } + 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 +92,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\t%s%s%c", scope, + h->u.configured.disabled ? "disabled\t" : "", + name, line_terminator); + else + printf("%s%s%c", + h->u.configured.disabled ? "disabled\t" : "", + 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; } @@ -91,8 +123,11 @@ static int run(int argc, const char **argv, const char *prefix, int i; struct run_hooks_opt opt = RUN_HOOKS_OPT_INIT; int ignore_missing = 0; + int allow_unknown = 0; const char *hook_name; struct option run_options[] = { + OPT_BOOL(0, "allow-unknown-hook-name", &allow_unknown, + N_("allow running a hook with a non-native hook name")), OPT_BOOL(0, "ignore-missing", &ignore_missing, N_("silently ignore missing requested ")), OPT_STRING(0, "to-stdin", &opt.path_to_stdin, N_("path"), @@ -124,6 +159,14 @@ static int run(int argc, const char **argv, const char *prefix, repo_config(the_repository, git_default_config, NULL); hook_name = argv[0]; + + if (!allow_unknown && !is_known_hook(hook_name)) { + error(_("unknown hook event '%s';\n" + "use --allow-unknown-hook-name to allow non-native hook names"), + hook_name); + return 1; + } + if (!ignore_missing) opt.error_if_missing = 1; ret = run_hooks_opt(the_repository, hook_name, &opt); diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c index e34edff406..cb3656a034 100644 --- a/builtin/receive-pack.c +++ b/builtin/receive-pack.c @@ -3,46 +3,45 @@ #include "builtin.h" #include "abspath.h" - -#include "config.h" -#include "environment.h" -#include "gettext.h" -#include "hex.h" -#include "lockfile.h" -#include "pack.h" -#include "refs.h" -#include "pkt-line.h" -#include "sideband.h" -#include "run-command.h" -#include "hook.h" -#include "exec-cmd.h" #include "commit.h" -#include "object.h" -#include "remote.h" +#include "commit-reach.h" +#include "config.h" #include "connect.h" -#include "string-list.h" -#include "oid-array.h" #include "connected.h" -#include "strvec.h" -#include "version.h" -#include "gpg-interface.h" -#include "sigchain.h" +#include "environment.h" +#include "exec-cmd.h" #include "fsck.h" -#include "tmp-objdir.h" -#include "oidset.h" -#include "packfile.h" +#include "gettext.h" +#include "gpg-interface.h" +#include "hex.h" +#include "hook.h" +#include "lockfile.h" +#include "object.h" #include "object-file.h" #include "object-name.h" #include "odb.h" +#include "oid-array.h" +#include "oidset.h" +#include "pack.h" +#include "packfile.h" +#include "parse-options.h" +#include "pkt-line.h" #include "protocol.h" -#include "commit-reach.h" +#include "refs.h" +#include "remote.h" +#include "run-command.h" #include "server-info.h" +#include "setup.h" +#include "shallow.h" +#include "sideband.h" +#include "sigchain.h" +#include "string-list.h" +#include "strvec.h" +#include "tmp-objdir.h" #include "trace.h" #include "trace2.h" +#include "version.h" #include "worktree.h" -#include "shallow.h" -#include "setup.h" -#include "parse-options.h" static const char * const receive_pack_usage[] = { N_("git receive-pack "), @@ -904,11 +903,14 @@ 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; strbuf_init(&data->buf, 0); + return data; } @@ -928,7 +930,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 +967,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..cc23276d27 100644 --- a/hook.c +++ b/hook.c @@ -1,16 +1,16 @@ #include "git-compat-util.h" #include "abspath.h" #include "advice.h" +#include "config.h" +#include "environment.h" #include "gettext.h" #include "hook.h" -#include "path.h" #include "parse.h" +#include "path.h" #include "run-command.h" -#include "config.h" +#include "setup.h" #include "strbuf.h" #include "strmap.h" -#include "environment.h" -#include "setup.h" const char *find_hook(struct repository *r, const char *name) { @@ -52,34 +52,26 @@ 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) +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 +83,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 +92,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 +108,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; + bool 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 +132,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 +156,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 +198,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 */ @@ -202,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) { @@ -212,7 +226,12 @@ void hook_cache_clear(struct strmap *cache) strmap_for_each_entry(cache, &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); @@ -235,28 +254,39 @@ 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; + 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 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_or_null(command); + entry->scope = scope; + entry->disabled = is_disabled; + string_list_append(hooks, hname)->util = entry; } strmap_put(cache, e->key, hooks); @@ -289,7 +319,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); } @@ -297,9 +327,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); } @@ -318,17 +348,28 @@ static void list_hooks_add_configured(struct repository *r, /* 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; - if (options && options->feed_pipe_cb_data_alloc) + CALLOC_ARRAY(hook, 1); + + /* + * 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 +392,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 +407,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 +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); @@ -414,7 +465,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 +556,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 +572,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..5c5628dd1f 100644 --- a/hook.h +++ b/hook.h @@ -1,17 +1,21 @@ #ifndef HOOK_H #define HOOK_H -#include "strvec.h" +#include "config.h" #include "run-command.h" #include "string-list.h" #include "strmap.h" +#include "strvec.h" 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; + bool disabled; } configured; } u; @@ -41,13 +47,17 @@ struct hook { * Only useful when using `run_hooks_opt.feed_pipe`, otherwise ignore it. */ void *feed_pipe_cb_data; + + /** + * 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; }; -typedef void (*cb_data_free_fn)(void *data); -typedef void *(*cb_data_alloc_fn)(void *init_ctx); - -struct run_hooks_opt -{ +struct run_hooks_opt { /* Environment vars to be set for each hook */ struct strvec env; @@ -132,14 +142,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,10 +196,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, cb_data_free_fn cb_data_free); +void hook_free(void *p, const char *str); /** * Frees the hook configuration cache stored in `struct repository`. diff --git a/refs.c b/refs.c index 685a0c247b..5d1d28523d 100644 --- a/refs.c +++ b/refs.c @@ -2595,7 +2595,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/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..96749fc06d 100755 --- a/t/t1800-hook.sh +++ b/t/t1800-hook.sh @@ -25,18 +25,47 @@ 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 && grep "unknown option" err ' +test_expect_success 'git hook list: unknown hook name is rejected' ' + test_must_fail git hook list prereceive 2>err && + test_grep "unknown hook event" err +' + +test_expect_success 'git hook run: unknown hook name is rejected' ' + test_must_fail git hook run prereceive 2>err && + test_grep "unknown hook event" err +' + +test_expect_success 'git hook list: known hook name is accepted' ' + test_must_fail git hook list pre-receive 2>err && + test_grep ! "unknown hook event" err +' + +test_expect_success 'git hook run: known hook name is accepted' ' + git hook run --ignore-missing pre-receive 2>err && + test_grep ! "unknown hook event" err +' + +test_expect_success 'git hook run: --allow-unknown-hook-name overrides rejection' ' + git hook run --allow-unknown-hook-name --ignore-missing custom-hook 2>err && + test_grep ! "unknown hook event" err +' + +test_expect_success 'git hook list: --allow-unknown-hook-name overrides rejection' ' + test_must_fail git hook list --allow-unknown-hook-name custom-hook 2>err && + test_grep ! "unknown hook event" err +' + 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_expect_code 1 git hook list --allow-unknown-hook-name test-hook 2>stderr.actual && test_cmp stderr.expect stderr.actual ' @@ -48,7 +77,7 @@ test_expect_success 'git hook list: traditional hook from hookdir' ' cat >expect <<-\EOF && hook from hookdir EOF - git hook list test-hook >actual && + git hook list --allow-unknown-hook-name test-hook >actual && test_cmp expect actual ' @@ -57,7 +86,7 @@ test_expect_success 'git hook list: configured hook' ' test_config hook.myhook.event test-hook --add && echo "myhook" >expect && - git hook list test-hook >actual && + git hook list --allow-unknown-hook-name test-hook >actual && test_cmp expect actual ' @@ -69,7 +98,7 @@ test_expect_success 'git hook list: -z shows NUL-terminated output' ' test_config hook.myhook.event test-hook --add && printf "myhookQhook from hookdirQ" >expect && - git hook list -z test-hook >actual.raw && + git hook list --allow-unknown-hook-name -z test-hook >actual.raw && nul_to_q actual && test_cmp expect actual ' @@ -78,12 +107,12 @@ test_expect_success 'git hook run: nonexistent hook' ' cat >stderr.expect <<-\EOF && error: cannot find a hook named test-hook EOF - test_expect_code 1 git hook run test-hook 2>stderr.actual && + test_expect_code 1 git hook run --allow-unknown-hook-name test-hook 2>stderr.actual && test_cmp stderr.expect stderr.actual ' test_expect_success 'git hook run: nonexistent hook with --ignore-missing' ' - git hook run --ignore-missing does-not-exist 2>stderr.actual && + git hook run --allow-unknown-hook-name --ignore-missing does-not-exist 2>stderr.actual && test_must_be_empty stderr.actual ' @@ -95,7 +124,7 @@ test_expect_success 'git hook run: basic' ' cat >expect <<-\EOF && Test hook EOF - git hook run test-hook 2>actual && + git hook run --allow-unknown-hook-name test-hook 2>actual && test_cmp expect actual ' @@ -109,7 +138,7 @@ test_expect_success 'git hook run: stdout and stderr both write to our stderr' ' Will end up on stderr Will end up on stderr EOF - git hook run test-hook >stdout.actual 2>stderr.actual && + git hook run --allow-unknown-hook-name test-hook >stdout.actual 2>stderr.actual && test_cmp stderr.expect stderr.actual && test_must_be_empty stdout.actual ' @@ -121,12 +150,12 @@ do exit $code EOF - test_expect_code $code git hook run test-hook + test_expect_code $code git hook run --allow-unknown-hook-name test-hook ' done test_expect_success 'git hook run arg u ments without -- is not allowed' ' - test_expect_code 129 git hook run test-hook arg u ments + test_expect_code 129 git hook run --allow-unknown-hook-name test-hook arg u ments ' test_expect_success 'git hook run -- pass arguments' ' @@ -140,7 +169,7 @@ test_expect_success 'git hook run -- pass arguments' ' u ments EOF - git hook run test-hook -- arg "u ments" 2>actual && + git hook run --allow-unknown-hook-name test-hook -- arg "u ments" 2>actual && test_cmp expect actual ' @@ -149,12 +178,12 @@ test_expect_success 'git hook run: out-of-repo runs execute global hooks' ' test_config_global hook.global-hook.command "echo no repo no problems" --add && echo "global-hook" >expect && - nongit git hook list test-hook >actual && + nongit git hook list --allow-unknown-hook-name test-hook >actual && test_cmp expect actual && echo "no repo no problems" >expect && - nongit git hook run test-hook 2>actual && + nongit git hook run --allow-unknown-hook-name test-hook 2>actual && test_cmp expect actual ' @@ -179,11 +208,11 @@ test_expect_success 'git -c core.hooksPath= hook run' ' # Test various ways of specifying the path. See also # t1350-config-hooks-path.sh >actual && - git hook run test-hook -- ignored 2>>actual && - git -c core.hooksPath=my-hooks hook run test-hook -- one 2>>actual && - git -c core.hooksPath=my-hooks/ hook run test-hook -- two 2>>actual && - git -c core.hooksPath="$PWD/my-hooks" hook run test-hook -- three 2>>actual && - git -c core.hooksPath="$PWD/my-hooks/" hook run test-hook -- four 2>>actual && + git hook run --allow-unknown-hook-name test-hook -- ignored 2>>actual && + git -c core.hooksPath=my-hooks hook run --allow-unknown-hook-name test-hook -- one 2>>actual && + git -c core.hooksPath=my-hooks/ hook run --allow-unknown-hook-name test-hook -- two 2>>actual && + git -c core.hooksPath="$PWD/my-hooks" hook run --allow-unknown-hook-name test-hook -- three 2>>actual && + git -c core.hooksPath="$PWD/my-hooks/" hook run --allow-unknown-hook-name test-hook -- four 2>>actual && test_cmp expect actual ' @@ -263,7 +292,7 @@ test_expect_success 'hook can be configured for multiple events' ' # 'ghi' should be included in both 'pre-commit' and 'test-hook' git hook list pre-commit >actual && grep "ghi" actual && - git hook list test-hook >actual && + git hook list --allow-unknown-hook-name test-hook >actual && grep "ghi" actual ' @@ -337,15 +366,15 @@ test_expect_success 'stdin to multiple hooks' ' b3 EOF - git hook run --to-stdin=input test-hook 2>actual && + git hook run --allow-unknown-hook-name --to-stdin=input test-hook 2>actual && test_cmp expected actual ' test_expect_success 'rejects hooks with no commands configured' ' test_config hook.broken.event "test-hook" && - test_must_fail git hook list test-hook 2>actual && + test_must_fail git hook list --allow-unknown-hook-name test-hook 2>actual && test_grep "hook.broken.command" actual && - test_must_fail git hook run test-hook 2>actual && + test_must_fail git hook run --allow-unknown-hook-name test-hook 2>actual && test_grep "hook.broken.command" actual ' @@ -354,11 +383,19 @@ test_expect_success 'disabled hook is not run' ' test_config hook.skipped.command "echo \"Should not run\"" && test_config hook.skipped.enabled false && - git hook run --ignore-missing test-hook 2>actual && + git hook run --allow-unknown-hook-name --ignore-missing test-hook 2>actual && 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 +403,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' ' @@ -377,10 +433,59 @@ test_expect_success 'globally disabled hook can be re-enabled locally' ' test_config hook.global-hook.enabled true && echo "global-hook ran" >expected && - git hook run test-hook 2>actual && + git hook run --allow-unknown-hook-name test-hook 2>actual && 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' ' + setup_hookdir && + test_config_global hook.global-hook.command "echo global" && + test_config_global hook.global-hook.event pre-commit --add && + test_config hook.local-hook.command "echo local" && + test_config hook.local-hook.event pre-commit --add && + + cat >expected <<-\EOF && + global global-hook + local local-hook + hook from hookdir + EOF + git hook list --show-scope pre-commit >actual && + test_cmp expected actual && + + # without --show-scope the scope must not appear + git hook list pre-commit >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 && @@ -388,7 +493,7 @@ test_expect_success 'git hook run a hook with a bad shebang' ' test_expect_code 1 git \ -c core.hooksPath=bad-hooks \ - hook run test-hook >out 2>err && + hook run --allow-unknown-hook-name test-hook >out 2>err && test_must_be_empty out && # TODO: We should emit the same (or at least a more similar) @@ -412,7 +517,7 @@ test_expect_success 'stdin to hooks' ' EOF echo hello >input && - git hook run --to-stdin=input test-hook 2>actual && + git hook run --allow-unknown-hook-name --to-stdin=input test-hook 2>actual && test_cmp expect actual ' diff --git a/transport.c b/transport.c index cb1befba8c..e53936d87b 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;