[PATCH v2 00/10] config-hook cleanups and two small 'git hook list' features

[Adrian Ratiu <adrian.ratiu@collabora.com>]

Hello everyone,

v2 addresses all the feedback received in v1, many thanks to everyone
who contributed.

This series is just minor cleanups / refactorings + two minor feature additions
to `git hook list`, which resulted from the previous series review discussions:

1. The ability to show the config scope (--show-scope).
2. The ability to show which hooks are disabled.

This is now based on the master branch.

I have pushed the branch to Github [1] and provided a clean CI run [2] with
the exception of a known breakage for some MacOS builders (REG_ENHANCED).

Thanks again,
Adrian

1: https://github.com/10ne1/git/tree/dev/aratiu/config-cleanups-v2
2: https://github.com/10ne1/git/actions/runs/23340298770

Changes in v2:
* Cleanly rebased on master, no conflicts (Adrian)
* Fix first patch build break by updating call-sites in same commit (Szeder)
* Drop UNUSED from function declaration in the header file (Eric)
* Drop the new struct hook_config_cache because it's redundant (Junio)
* git hook list now prints in tab separated output format similar
  to git config --show-scope to improve machine parseability (Junio, Patrick)
* Fix small style issues, comments, type, commit messages. (Eric, Patrick)

Range-diff v1 -> v2:
 1:  6f4ac0ddf8 !  1:  aeefa72f33 hook: move unsorted_string_list_remove() to string-list.[ch]
    @@ Commit message
         Move the convenience wrapper from hook to string-list since
         it's a more suitable place. Add a doc comment to the header.
     
    +    Also add a free_util arg to make the function more generic
    +    and make the API similar to other functions in string-list.h.
    +    Update the existing call-sites.
    +
         Suggested-by: Patrick Steinhardt <ps@pks.im>
         Signed-off-by: Adrian Ratiu <adrian.ratiu@collabora.com>
     
    @@ hook.c: static void list_hooks_add_default(struct repository *r, const char *hoo
      /*
       * Callback struct to collect all hook.* keys in a single config pass.
       * commands: friendly-name to command map.
    +@@ hook.c: 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);
    +@@ hook.c: static int hook_config_lookup_all(const char *key, const char *value,
    + 			}
    + 
    + 			/* Re-insert if necessary to preserve last-seen order. */
    +-			unsorted_string_list_remove(hooks, hook_name);
    ++			unsorted_string_list_remove(hooks, hook_name, 0);
    + 			string_list_append(hooks, hook_name);
    + 		}
    + 	} else if (!strcmp(subkey, "command")) {
    +@@ hook.c: 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 */
     
      ## string-list.c ##
     @@ string-list.c: void unsorted_string_list_delete_item(struct string_list *list, int i, int free_
 2:  df876b8bc5 !  2:  90e821bdfa hook: fix minor style issues
    @@ Metadata
      ## Commit message ##
         hook: fix minor style issues
     
    -    Fix some minor style nits pointed by Patrick and Junio:
    +    Fix some minor style nits pointed 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 in error/warn messages.
    +      * Capitalization and full-stop in error/warn messages.
    +      * Curly brace on separate line when defining struct.
           * Comment spelling: free'd -> freed.
     
         These contain no logic changes, the code behaves the same as before.
     
    +    Suggested-by: Eric Sunshine <sunshine@sunshineco.com>
         Suggested-by: Junio C Hamano <gitster@pobox.com>
         Suggested-by: Patrick Steinhardt <ps@pks.im>
         Signed-off-by: Adrian Ratiu <adrian.ratiu@collabora.com>
    @@ builtin/hook.c: static int list(int argc, const char **argv, const char *prefix,
      	 */
      	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."),
    ++		usage_msg_opt(_("you must specify a hook event name to list"),
      			      builtin_hook_list_usage, list_options);
      
      	hookname = argv[0];
    @@ hook.c: int run_hooks_opt(struct repository *r, const char *hook_name,
      
      	if (options->invoked_hook)
     
    + ## hook.h ##
    +@@ hook.h: 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;
    + 
    +
      ## refs.c ##
     @@ refs.c: static int transaction_hook_feed_stdin(int hook_stdin_fd, void *pp_cb, void *pp_
      
 3:  7cf6e32087 !  3:  dee1dd49a4 hook: rename cb_data_free/alloc -> hook_data_free/alloc
    @@ hook.h: struct hook {
     +typedef void (*hook_data_free_fn)(void *data);
     +typedef void *(*hook_data_alloc_fn)(void *init_ctx);
      
    - struct run_hooks_opt
    - {
    -@@ hook.h: struct run_hooks_opt
    + struct run_hooks_opt {
    + 	/* Environment vars to be set for each hook */
    +@@ hook.h: struct run_hooks_opt {
      	 *
      	 * The `feed_pipe_ctx` pointer can be used to pass initialization data.
      	 */
 4:  06a9f3bfcb =  4:  86f61204b3 hook: detect & emit two more bugs
 5:  47190d7a22 !  5:  30542de351 hook: replace hook_list_clear() -> string_list_clear_func()
    @@ hook.c: static void list_hooks_add_default(struct repository *r, const char *hoo
      
      	h->kind = HOOK_TRADITIONAL;
      	h->u.traditional.path = xstrdup(hook_path);
    -@@ hook.c: 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);
    -@@ hook.c: static int hook_config_lookup_all(const char *key, const char *value,
    - 			}
    - 
    - 			/* Re-insert if necessary to preserve last-seen order. */
    --			unsorted_string_list_remove(hooks, hook_name);
    -+			unsorted_string_list_remove(hooks, hook_name, 0);
    - 			string_list_append(hooks, hook_name);
    - 		}
    - 	} else if (!strcmp(subkey, "command")) {
    -@@ hook.c: 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 */
     @@ hook.c: static void list_hooks_add_configured(struct repository *r,
      		struct hook *hook;
      		CALLOC_ARRAY(hook, 1);
    @@ hook.h: struct hook {
     +	hook_data_free_fn data_free;
     +};
      
    - struct run_hooks_opt
    - {
    + struct run_hooks_opt {
    + 	/* Environment vars to be set for each hook */
     @@ hook.h: struct string_list *list_hooks(struct repository *r, const char *hookname,
      			       struct run_hooks_opt *options);
      
    @@ hook.h: struct string_list *list_hooks(struct repository *r, const char *hooknam
     + * Suitable for use as a string_list_clear_func_t callback.
       */
     -void hook_list_clear(struct string_list *hooks, hook_data_free_fn cb_data_free);
    -+void hook_free(void *p, const char *str UNUSED);
    ++void hook_free(void *p, const char *str);
      
      /**
       * Frees the hook configuration cache stored in `struct repository`.
 6:  469b8a7192 =  6:  4e1374b84e hook: make consistent use of friendly-name in docs
 7:  dff30a5c06 =  7:  075f8202aa t1800: add test to verify hook execution ordering
 8:  e53a000982 <  -:  ---------- hook: refactor hook_config_cache from strmap to named struct
 -:  ---------- >  8:  8f948bbbe7 hook: introduce hook_config_cache_entry for per-hook data
 9:  7d7753b48a !  9:  dbf81604ed hook: show config scope in git hook list
    @@ Commit message
     
         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
    -    linter (global)
    -    no-leaks (local)
    +    global  linter
    +    local   no-leaks
         hook from hookdir
     
         Traditional hooks from the hookdir are unaffected by --show-scope since
    @@ Documentation/git-hook.adoc: OPTIONS
      	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.
    ++	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
      --------
    @@ builtin/hook.c: static int list(int argc, const char **argv, const char *prefix,
      		case HOOK_CONFIGURED:
     -			printf("%s%c", h->u.configured.friendly_name, line_terminator);
     +			if (show_scope)
    -+				printf("%s (%s)%c",
    -+				       h->u.configured.friendly_name,
    ++				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,
    @@ hook.c: static int hook_config_lookup_all(const char *key, const char *value,
      		}
      	} else if (!strcmp(subkey, "command")) {
      		/* Store command overwriting the old value */
    -@@ hook.c: static void build_hook_config_map(struct repository *r,
    +@@ hook.c: 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;
    @@ hook.c: static void build_hook_config_map(struct repository *r,
      			struct hook_config_cache_entry *entry;
      			char *command;
      
    -@@ hook.c: static void build_hook_config_map(struct repository *r,
    +@@ hook.c: 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);
    @@ t/t1800-hook.sh: test_expect_success 'configured hooks run before hookdir hook'
     +	test_config hook.local-hook.event test-hook --add &&
     +
     +	cat >expected <<-\EOF &&
    -+	global-hook (global)
    -+	local-hook (local)
    ++	global	global-hook
    ++	local	local-hook
     +	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_grep ! "^global	" actual &&
    ++	test_grep ! "^local	" actual
     +'
     +
      test_expect_success 'git hook run a hook with a bad shebang' '
10:  dabb17aad6 ! 10:  2a67244b20 hook: show disabled hooks in "git hook list"
    @@ Commit message
         invisible to "git hook list". Keep them in the cache with a new
         "disabled" flag which is propagated to the respective struct hook.
     
    -    "git hook list" now shows disabled hooks annotated with "(disabled)"
    -    in the config order. With --show-scope, it looks like:
    +    "git hook list" 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
    -    linter (global)
    -    no-leaks (local, disabled)
    +    global  linter
    +    local   disabled        no-leaks
         hook from hookdir
     
         A disabled hook without a command issues a warning instead of the
    @@ builtin/hook.c: static int list(int argc, const char **argv, const char *prefix,
      			break;
     -		case HOOK_CONFIGURED:
     -			if (show_scope)
    --				printf("%s (%s)%c",
    --				       h->u.configured.friendly_name,
    +-				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 (%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);
    ++				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%c", name, line_terminator);
    ++				printf("%s%s%c",
    ++				       h->u.configured.disabled ? "disabled\t" : "",
    ++				       name, line_terminator);
      			break;
     +		}
      		default:
    @@ hook.c: static void list_hooks_add_default(struct repository *r, const char *hoo
      struct hook_config_cache_entry {
      	char *command;
      	enum config_scope scope;
    -+	int disabled;
    ++	unsigned int disabled:1;
      };
      
      /*
     @@ hook.c: static int hook_config_lookup_all(const char *key, const char *value,
       * every item's string is the hook's friendly-name and its util pointer is
    -  * a hook_config_cache_entry. All strings are owned by the map.
    +  * 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. 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.
    ++ * "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 hook_config_cache *cache)
    + void hook_cache_clear(struct strmap *cache)
      {
    -@@ hook.c: static void build_hook_config_map(struct repository *r,
    +@@ hook.c: static void build_hook_config_map(struct repository *r, struct strmap *cache)
      			struct hook_config_cache_entry *entry;
      			char *command;
      
    @@ hook.c: static void build_hook_config_map(struct repository *r,
      			/* util stores a cache entry; owned by the cache. */
      			CALLOC_ARRAY(entry, 1);
     -			entry->command = xstrdup(command);
    -+			entry->command = command ? xstrdup(command) : NULL;
    ++			entry->command = xstrdup_or_null(command);
      			entry->scope = scope;
     +			entry->disabled = is_disabled;
      			string_list_append(hooks, hname)->util = entry;
    @@ hook.h: struct hook {
      			const char *friendly_name;
      			const char *command;
      			enum config_scope scope;
    -+			int disabled;
    ++			unsigned int disabled:1;
      		} configured;
      	} u;
      
    @@ t/t1800-hook.sh: test_expect_success 'disabled hook does not appear in git hook
     -	test_grep "active" actual &&
     -	test_grep ! "inactive" actual
     +	test_grep "^active$" actual &&
    -+	test_grep "^inactive (disabled)$" actual
    ++	test_grep "^disabled	inactive$" actual
     +'
     +
     +test_expect_success 'disabled hook shows scope with --show-scope' '
    @@ t/t1800-hook.sh: test_expect_success 'disabled hook does not appear in git hook
     +	test_config hook.myhook.enabled false &&
     +
     +	git hook list --show-scope pre-commit >actual &&
    -+	test_grep "myhook (local, disabled)" actual
    ++	test_grep "^local	disabled	myhook$" actual
     +'
     +
     +test_expect_success 'disabled configured hook is not reported as existing by hook_exists' '

Adrian Ratiu (10):
  hook: move unsorted_string_list_remove() to string-list.[ch]
  hook: fix minor style issues
  hook: rename cb_data_free/alloc -> hook_data_free/alloc
  hook: detect & emit two more bugs
  hook: replace hook_list_clear() -> string_list_clear_func()
  hook: make consistent use of friendly-name in docs
  t1800: add test to verify hook execution ordering
  hook: introduce hook_config_cache_entry for per-hook data
  hook: show config scope in git hook list
  hook: show disabled hooks in "git hook list"

 Documentation/config/hook.adoc |  30 +++---
 Documentation/git-hook.adoc    |  16 ++-
 builtin/hook.c                 |  28 +++--
 builtin/receive-pack.c         |  11 +-
 hook.c                         | 187 +++++++++++++++++++++------------
 hook.h                         |  32 ++++--
 refs.c                         |   3 +-
 string-list.c                  |   9 ++
 string-list.h                  |   8 ++
 t/t1800-hook.sh                |  83 ++++++++++++++-
 transport.c                    |   3 +-
 11 files changed, 295 insertions(+), 115 deletions(-)

-- 
2.52.0.732.gb351b5166d.dirty