Re: [PATCH 1/4] hook: run a list of hooks

[Junio C Hamano <gitster@pobox.com>]

Adrian Ratiu <adrian.ratiu@collabora.com> writes:

> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
> index b5379a4895..72fde2207c 100644
> --- a/builtin/receive-pack.c
> +++ b/builtin/receive-pack.c
> @@ -849,7 +849,8 @@ struct receive_hook_feed_state {
>  
>  static int feed_receive_hook_cb(int hook_stdin_fd, void *pp_cb UNUSED, void *pp_task_cb)
>  {
> -	struct receive_hook_feed_state *state = pp_task_cb;
> +	struct string_list_item *h = pp_task_cb;
> +	struct receive_hook_feed_state *state = h->util;
>  	struct command *cmd = state->cmd;

So the new calling convention is to give a string_list_item to the
callback function, whose .util member has the historical "state".  OK.


> @@ -944,6 +963,8 @@ static int run_receive_hook(struct command *commands,
>  	strbuf_init(&feed_state.buf, 0);
>  	opt.feed_pipe_cb_data = &feed_state;
>  	opt.feed_pipe = feed_receive_hook_cb;
> +	opt.copy_feed_pipe_cb_data = copy_receive_hook_feed_state;
> +	opt.free_feed_pipe_cb_data = free_receive_hook_feed_state;
>  
>  	ret = run_hooks_opt(the_repository, hook_name, &opt);
>  
> diff --git a/hook.c b/hook.c
> index cde7198412..fb90f91f3b 100644
> --- a/hook.c
> +++ b/hook.c
> @@ -47,9 +47,49 @@ const char *find_hook(struct repository *r, const char *name)
>  	return path.buf;
>  }
>  
> +/*
> + * Provides a list of hook commands to run for the 'hookname' event.
> + *
> + * This function consolidates hooks from two sources:
> + * 1. The config-based hooks (not yet implemented).
> + * 2. The "traditional" hook found in the repository hooks directory
> + *    (e.g., .git/hooks/pre-commit).
> + *
> + * The list is ordered by execution priority.
> + *
> + * The caller is responsible for freeing the memory of the returned list
> + * using string_list_clear() and free().
> + */
> +static struct string_list *list_hooks(struct repository *r, const char *hookname)
> +{
> +	struct string_list *hook_head;
> +
> +	if (!hookname)
> +		BUG("null hookname was provided to hook_list()!");
> +
> +	hook_head = xmalloc(sizeof(struct string_list));
> +	string_list_init_dup(hook_head);
> +
> +	/*
> +	 * Add the default hook from hookdir. It does not have a friendly name
> +	 * like the hooks specified via configs, so add it with an empty name.
> +	 */
> +	if (r->gitdir && find_hook(r, hookname))
> +		string_list_append(hook_head, "");
> +
> +	return hook_head;
> +}

OK.  I would have expected the caller to supply a string_list
(presumably an empty one would be the most common usage) and the
only responsibility for this function would be to stuff found hooks
to the given string_list.  But this arrangement to give a newly
created string list would work just fine as well.

>  int hook_exists(struct repository *r, const char *name)
>  {
> -	return !!find_hook(r, name);
> +	int exists = 0;
> +	struct string_list *hooks = list_hooks(r, name);
> +
> +	exists = hooks->nr > 0;
> +
> +	string_list_clear(hooks, 1);
> +	free(hooks);
> +	return exists;
>  }

OK.  It is not just "git hook list", but the internal function
list_hooks() that is the topic of this step of the series.  So
anywhere that uses find_hook() in the current codebase is a good
candidate to migrate to use the new function when appropriate, and
this is one of such places.  I presume that find_hook() will stay to
be a "find the program in $GIT_DIR/hooks/ directory" function that
has a single answer (i.e. pathnmame), but the list_hooks() may
return more than one string_list_item, one of them might be the
traditional hook and others may come from somewhere else.  Do they
need to be differentiated in some way (i.e., Is "where did the come
from?" a legitimate thing for a caller of list_hook() to ask)?
There are many unknowns while reading this step because we still
haven't seen the other sources of the hooks.

> @@ -58,10 +98,11 @@ static int pick_next_hook(struct child_process *cp,
>  			  void **pp_task_cb)
>  {
>  	struct hook_cb_data *hook_cb = pp_cb;
> -	const char *hook_path = hook_cb->hook_path;
> +	struct string_list *hook_list = hook_cb->hook_command_list;
> +	struct string_list_item *to_run = hook_cb->next_hook++;
>  
> -	if (!hook_path)
> -		return 0;
> +	if (!to_run || to_run >= hook_list->items + hook_list->nr)
> +		return 0; /* no hook left to run */
>  
>  	cp->no_stdin = 1;
>  	strvec_pushv(&cp->env, hook_cb->options->env.v);
> @@ -85,33 +126,50 @@ static int pick_next_hook(struct child_process *cp,
>  	cp->trace2_hook_name = hook_cb->hook_name;
>  	cp->dir = hook_cb->options->dir;
>  
> -	strvec_push(&cp->args, hook_path);
> +	/* find hook commands */
> +	if (!*to_run->string) {
> +		/* ...from hookdir signified by empty name */

Make a macro or a small helper function that takes a single
string_list_item that is an element in the return value of
list_hooks(), and tells if it is a program in $GIT_DIR/hooks/
directory.  Writing code that depends on this "we did not bother to
invent a way to allow users to give friendly names to traditional
on-disk hooks, so it is registered with an empty string as its name"
convention and having to document it is inefficient, both for
writers of the code and for readers of the code.

Or perhaps the .util member of each of these can be a pointer to
something like:

	struct {
		enum {
                	HOOK_TRADITIONAL, HOOK_CONFIGURED,
		} kind;
		union {
			struct {
				const char *path;
			} traditional;
			struct {
				const char *friendly_name;
				const char *path;
				... maybe others ...
			} configured;
		} u;
	}

so that this codepath does not even have to ...

> +		const char *hook_path = find_hook(hook_cb->repository,
> +						  hook_cb->hook_name);

... run the same find_hook() again here?

> -static int notify_start_failure(struct strbuf *out UNUSED,
> +static int notify_start_failure(struct strbuf *out,
>  				void *pp_cb,
> -				void *pp_task_cp UNUSED)
> +				void *pp_task_cb)
>  {
>  	struct hook_cb_data *hook_cb = pp_cb;
> +	struct string_list_item *hook = pp_task_cb;
>  
>  	hook_cb->rc |= 1;
>  
> +	if (out && hook) {
> +		if (*hook->string)

Ditto.

> +			strbuf_addf(out, _("Couldn't start hook '%s'\n"), hook->string);
> +		else
> +			strbuf_addstr(out, _("Couldn't start hook from hooks directory\n"));
> +	}
> +
>  	return 1;
>  }