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

>> 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?