Emily Shaffer <emilyshaffer@xxxxxxxxxx> writes:
> To prepare for multihook support, teach hook.[hc] to take a list of
> hooks at run_hooks and run_found_hooks. Right now the list is always one
> entry, but in the future we will allow users to supply more than one
> executable for a single hook event.
>
> Signed-off-by: Emily Shaffer <emilyshaffer@xxxxxxxxxx>
> ---
> builtin/hook.c | 14 ++++---
> hook.c | 103 +++++++++++++++++++++++++++++++++++--------------
> hook.h | 16 +++++++-
> 3 files changed, 96 insertions(+), 37 deletions(-)
>
> diff --git a/builtin/hook.c b/builtin/hook.c
> index 5eb7cf73a4..4d39c9e75e 100644
> --- a/builtin/hook.c
> +++ b/builtin/hook.c
> @@ -25,7 +25,8 @@ static int run(int argc, const char **argv, const char *prefix)
> struct run_hooks_opt opt = RUN_HOOKS_OPT_INIT;
> int ignore_missing = 0;
> const char *hook_name;
> - const char *hook_path;
> + struct list_head *hooks;
> +
Natural. We used to use the path to the hook because we were
expecting only one. We now use the name to find a list of hooks.
All the caller sees is just list_head without any direct visibility
into it, which feels like a great abstraction. Presumably everything
will go through the API functions taking this opaque "list of hooks"
thing (or "the first one in the list" if the API function does not
iterate over it, perhaps?).
> struct option run_options[] = {
> OPT_BOOL(0, "ignore-missing", &ignore_missing,
> N_("exit quietly with a zero exit code if the requested hook cannot be found")),
> @@ -58,15 +59,16 @@ static int run(int argc, const char **argv, const char *prefix)
> git_config(git_default_config, NULL);
>
> hook_name = argv[0];
> - if (ignore_missing)
> - return run_hooks_oneshot(hook_name, &opt);
> - hook_path = find_hook(hook_name);
> - if (!hook_path) {
> + hooks = hook_list(hook_name);
OK.
> + if (list_empty(hooks)) {
> + /* ... act like run_hooks_oneshot() under --ignore-missing */
> + if (ignore_missing)
> + return 0;
OK.
> error("cannot find a hook named %s", hook_name);
> return 1;
> }
> diff --git a/hook.c b/hook.c
> index ee20b2e365..80e150548c 100644
> --- a/hook.c
> +++ b/hook.c
> @@ -4,6 +4,28 @@
> #include "hook-list.h"
> #include "config.h"
>
> +static void free_hook(struct hook *ptr)
> +{
> + if (ptr) {
> + free(ptr->feed_pipe_cb_data);
> + }
Lose the extra {}, as we do not do more than the above free() even
at the end of the series?
> + free(ptr);
> +}
> +
> +static void remove_hook(struct list_head *to_remove)
> +{
> + struct hook *hook_to_remove = list_entry(to_remove, struct hook, list);
> + list_del(to_remove);
> + free_hook(hook_to_remove);
> +}
> +
> +void clear_hook_list(struct list_head *head)
> +{
> + struct list_head *pos, *tmp;
> + list_for_each_safe(pos, tmp, head)
> + remove_hook(pos);
> +}
OK.
> +struct list_head* hook_list(const char* hookname)
Shift both of the asterisks; our asterisks do not stick to the type
but to the identifier.
> +{
> + struct list_head *hook_head = xmalloc(sizeof(struct list_head));
> +
> + INIT_LIST_HEAD(hook_head);
> +
> + if (!hookname)
> + return NULL;
Checking for invalid hookname first would avoid leaking hook_head,
no? The caller of hook_list() we saw earlier immediately calls
list_empty() which will segfault. The caller need to be tightened,
but I wonder if it would be a programmer's error to pass a NULL
hookname to this function. If so, this can simply be a BUG() and
the earlier allocation and initialization of hook_head can stay
where they are. Otherwise, the caller should see if this returns a
NULL.
> + if (have_git_dir()) {
> + const char *hook_path = find_hook(hookname);
> +
> + /* Add the hook from the hookdir */
> + if (hook_path) {
> + struct hook *to_add = xmalloc(sizeof(*to_add));
> + to_add->hook_path = hook_path;
> + to_add->feed_pipe_cb_data = NULL;
> + list_add_tail(&to_add->list, hook_head);
> + }
> + }
Calling this function to grab a list of hooks when we are not in any
repository is not an error but just we get "there is nothing to
run". Does the design give us a more useful behaviour, compared to
alternatives like "you have to be in a repository or calling this
function is an error"?
Not an objection wrapped in a rhetorical question, but a genuine
question. "It would help this and that usecase" would be an ideal
answer, "We could do either way, but we happened to have written the
code this way first, and at the end of the series we did not see any
practical downsides" would also be a great answer.
> + return hook_head;
> +}
> +
> +/*
> + * Provides a linked list of 'struct hook' detailing commands which should run
> + * in response to the 'hookname' event, in execution order.
> + */
> +struct list_head* hook_list(const char *hookname);
struct list_head *hook_list(const char *hookname);
