From b06770e5d8948c7cad76d7507423376eacf1e005 Mon Sep 17 00:00:00 2001 From: Adrian Ratiu Date: Wed, 25 Mar 2026 21:54:54 +0200 Subject: hook: fix minor style issues Fix some minor style nits pointed out by Patrick, Junio and Eric: * 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 and full-stop in error/warn messages. * Curly brace on separate line when defining struct. * Comment spelling: free'd -> freed. * Sort the included headers. * Blank line fixes to improve readability. These contain no logic changes, the code behaves the same as before. Suggested-by: Eric Sunshine Suggested-by: Junio C Hamano Suggested-by: Patrick Steinhardt Signed-off-by: Adrian Ratiu Signed-off-by: Junio C Hamano --- hook.h | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) (limited to 'hook.h') diff --git a/hook.h b/hook.h index e949f5d488..1c447cbb6b 100644 --- a/hook.h +++ b/hook.h @@ -1,9 +1,9 @@ #ifndef HOOK_H #define HOOK_H -#include "strvec.h" #include "run-command.h" #include "string-list.h" #include "strmap.h" +#include "strvec.h" struct repository; @@ -46,8 +46,7 @@ struct hook { 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; -- cgit v1.3 From 8f7db6f8b585a3eef4ba595efd2d098f9abf3606 Mon Sep 17 00:00:00 2001 From: Adrian Ratiu Date: Wed, 25 Mar 2026 21:54:55 +0200 Subject: 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(-) (limited to 'hook.h') diff --git a/hook.c b/hook.c index 935237fc1d..4a0db5cfeb 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 1c447cbb6b..965794a5b8 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 { /* Environment vars to be set for each hook */ @@ -131,14 +131,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 { \ @@ -188,7 +188,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`. -- cgit v1.3 From a8b1ba86d494ea8825292c91c243e5d84fd7ee2c Mon Sep 17 00:00:00 2001 From: Adrian Ratiu Date: Wed, 25 Mar 2026 21:54:57 +0200 Subject: 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). In other words, the callers don't need to keep track of hook internal state to determine when cleanup is necessary or not (pass NULL) because each `struct hook` now owns its data_free callback. Suggested-by: Patrick Steinhardt Signed-off-by: Adrian Ratiu Signed-off-by: Junio C Hamano --- builtin/hook.c | 2 +- hook.c | 40 ++++++++++++++++++++++------------------ hook.h | 20 ++++++++++++++------ 3 files changed, 37 insertions(+), 25 deletions(-) (limited to 'hook.h') diff --git a/builtin/hook.c b/builtin/hook.c index e641614b84..54b737990b 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 b0226ed716..021110f216 100644 --- a/hook.c +++ b/hook.c @@ -52,8 +52,10 @@ 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) +void hook_free(void *p, const char *str UNUSED) { + struct hook *h = p; + if (!h) return; @@ -64,22 +66,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 +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); @@ -316,10 +314,16 @@ static void list_hooks_add_configured(struct repository *r, 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); @@ -362,7 +366,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; } @@ -516,7 +520,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 965794a5b8..a56ac20ccf 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 { /* Environment vars to be set for each hook */ @@ -185,10 +193,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); /** * Frees the hook configuration cache stored in `struct repository`. -- cgit v1.3 From 2e5dbaff169dfb28fa8e8c4f992d8252a4ef1312 Mon Sep 17 00:00:00 2001 From: Adrian Ratiu Date: Wed, 25 Mar 2026 21:54:58 +0200 Subject: 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(-) (limited to 'hook.h') 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 021110f216..dc0c3de667 100644 --- a/hook.c +++ b/hook.c @@ -112,7 +112,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 a56ac20ccf..d2cf59e649 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 { -- cgit v1.3 From b66efad2b1f53755a80699dc39f94e2b15d6af67 Mon Sep 17 00:00:00 2001 From: Adrian Ratiu Date: Wed, 25 Mar 2026 21:55:01 +0200 Subject: 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. The scope is printed as a tab-separated prefix (like "git config --show-scope"), making it unambiguously machine-parseable even when the friendly name contains spaces. Example usage: $ git hook list --show-scope pre-commit global linter local no-leaks 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 | 10 ++++++++-- builtin/hook.c | 14 ++++++++++++-- hook.c | 24 ++++++++++++++++++++---- hook.h | 2 ++ t/t1800-hook.sh | 21 +++++++++++++++++++++ 5 files changed, 63 insertions(+), 8 deletions(-) (limited to 'hook.h') diff --git a/Documentation/git-hook.adoc b/Documentation/git-hook.adoc index 966388660a..e7d399ae57 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,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 -------- diff --git a/builtin/hook.c b/builtin/hook.c index 54b737990b..4cc65a0dc5 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\t%s%c", + config_scope_name(h->u.configured.scope), + h->u.configured.friendly_name, + 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 54f99f4989..74f5a1df35 100644 --- a/hook.c +++ b/hook.c @@ -110,11 +110,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; }; /* @@ -131,7 +131,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; @@ -168,7 +168,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 */ @@ -246,6 +258,8 @@ 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; + enum config_scope scope = + (enum config_scope)(uintptr_t)hook_names->items[i].util; struct hook_config_cache_entry *entry; char *command; @@ -263,6 +277,7 @@ static void build_hook_config_map(struct repository *r, struct strmap *cache) /* 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; } @@ -344,6 +359,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 d2cf59e649..a0432e8307 100644 --- a/hook.h +++ b/hook.h @@ -1,5 +1,6 @@ #ifndef HOOK_H #define HOOK_H +#include "config.h" #include "run-command.h" #include "string-list.h" #include "strmap.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..6fc6603da8 100755 --- a/t/t1800-hook.sh +++ b/t/t1800-hook.sh @@ -408,6 +408,27 @@ test_expect_success 'configured hooks run before hookdir hook' ' 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 && -- cgit v1.3 From e17bd99281ae01a758d717bdfaa759bbeefb6149 Mon Sep 17 00:00:00 2001 From: Adrian Ratiu Date: Wed, 25 Mar 2026 21:55:02 +0200 Subject: hook: show disabled hooks in "git hook list" Disabled hooks were filtered out of the cache entirely, making them invisible to "git hook list". Keep them in the cache with a new "disabled" flag which is propagated to the respective struct hook. "git hook list" now shows disabled hooks as tab-separated columns, with the status as a prefix before the name (like scope with --show-scope). With --show-scope it looks like: $ git hook list --show-scope pre-commit global linter local disabled no-leaks hook from hookdir A disabled hook without a command issues a warning instead of the fatal "hook.X.command must be configured" error. We could also throw an error, however it seemd a bit excessive to me in this case. Suggested-by: Patrick Steinhardt Signed-off-by: Adrian Ratiu Signed-off-by: Junio C Hamano --- builtin/hook.c | 20 ++++++++++++-------- hook.c | 54 +++++++++++++++++++++++++++++++++++++----------------- hook.h | 1 + t/t1800-hook.sh | 33 ++++++++++++++++++++++++++++++--- 4 files changed, 80 insertions(+), 28 deletions(-) (limited to 'hook.h') diff --git a/builtin/hook.c b/builtin/hook.c index 4cc65a0dc5..f671e7f91a 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\t%s%c", - config_scope_name(h->u.configured.scope), - h->u.configured.friendly_name, - line_terminator); + case HOOK_CONFIGURED: { + const char *name = h->u.configured.friendly_name; + const char *scope = show_scope ? + config_scope_name(h->u.configured.scope) : NULL; + if (scope) + printf("%s\t%s%s%c", scope, + h->u.configured.disabled ? "disabled\t" : "", + name, line_terminator); else - printf("%s%c", h->u.configured.friendly_name, - line_terminator); + printf("%s%s%c", + h->u.configured.disabled ? "disabled\t" : "", + name, line_terminator); break; + } default: BUG("unknown hook kind"); } diff --git a/hook.c b/hook.c index 74f5a1df35..cc23276d27 100644 --- a/hook.c +++ b/hook.c @@ -115,6 +115,7 @@ static void list_hooks_add_default(struct repository *r, const char *hookname, struct hook_config_cache_entry { char *command; enum config_scope scope; + bool disabled; }; /* @@ -213,8 +214,10 @@ static int hook_config_lookup_all(const char *key, const char *value, * every item's string is the hook's friendly-name and its util pointer is * the corresponding command string. Both strings are owned by the map. * - * Disabled hooks and hooks missing a command are already filtered out at - * parse time, so callers can iterate the list directly. + * Disabled hooks are kept in the cache with entry->disabled set, so that + * "git hook list" can display them. A non-disabled hook missing a command + * is fatal; a disabled hook missing a command emits a warning and is kept + * in the cache with entry->command = NULL. */ void hook_cache_clear(struct strmap *cache) { @@ -263,21 +266,26 @@ static void build_hook_config_map(struct repository *r, struct strmap *cache) struct hook_config_cache_entry *entry; char *command; - /* filter out disabled hooks */ - if (unsorted_string_list_lookup(&cb_data.disabled_hooks, - hname)) - continue; + bool is_disabled = + !!unsorted_string_list_lookup( + &cb_data.disabled_hooks, hname); command = strmap_get(&cb_data.commands, hname); - if (!command) - die(_("'hook.%s.command' must be configured or " - "'hook.%s.event' must be removed;" - " aborting."), hname, hname); + if (!command) { + if (is_disabled) + warning(_("disabled hook '%s' has no " + "command configured"), hname); + else + die(_("'hook.%s.command' must be configured or " + "'hook.%s.event' must be removed;" + " aborting."), hname, hname); + } /* util stores a cache entry; owned by the cache. */ CALLOC_ARRAY(entry, 1); - entry->command = xstrdup(command); + entry->command = xstrdup_or_null(command); entry->scope = scope; + entry->disabled = is_disabled; string_list_append(hooks, hname)->util = entry; } @@ -358,8 +366,10 @@ static void list_hooks_add_configured(struct repository *r, hook->kind = HOOK_CONFIGURED; hook->u.configured.friendly_name = xstrdup(friendly_name); - hook->u.configured.command = xstrdup(entry->command); + hook->u.configured.command = + entry->command ? xstrdup(entry->command) : NULL; hook->u.configured.scope = entry->scope; + hook->u.configured.disabled = entry->disabled; string_list_append(list, friendly_name)->util = hook; } @@ -397,7 +407,16 @@ struct string_list *list_hooks(struct repository *r, const char *hookname, int hook_exists(struct repository *r, const char *name) { struct string_list *hooks = list_hooks(r, name, NULL); - int exists = hooks->nr > 0; + int exists = 0; + + for (size_t i = 0; i < hooks->nr; i++) { + struct hook *h = hooks->items[i].util; + if (h->kind == HOOK_TRADITIONAL || + !h->u.configured.disabled) { + exists = 1; + break; + } + } string_list_clear_func(hooks, hook_free); free(hooks); return exists; @@ -412,10 +431,11 @@ static int pick_next_hook(struct child_process *cp, struct string_list *hook_list = hook_cb->hook_command_list; struct hook *h; - if (hook_cb->hook_to_run_index >= hook_list->nr) - return 0; - - h = hook_list->items[hook_cb->hook_to_run_index++].util; + do { + if (hook_cb->hook_to_run_index >= hook_list->nr) + return 0; + h = hook_list->items[hook_cb->hook_to_run_index++].util; + } while (h->kind == HOOK_CONFIGURED && h->u.configured.disabled); cp->no_stdin = 1; strvec_pushv(&cp->env, hook_cb->options->env.v); diff --git a/hook.h b/hook.h index a0432e8307..5c5628dd1f 100644 --- a/hook.h +++ b/hook.h @@ -31,6 +31,7 @@ struct hook { const char *friendly_name; const char *command; enum config_scope scope; + bool disabled; } configured; } u; diff --git a/t/t1800-hook.sh b/t/t1800-hook.sh index 6fc6603da8..8c5237449d 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 "^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' ' -- cgit v1.3