Re: [PATCH 1/5] run-command: add preliminary support for multiple hooks

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

"brian m. carlson" <sandals@crustytoothpaste.net> writes:

> diff --git a/builtin/commit.c b/builtin/commit.c
> index f17537474a..e7cf6b16ba 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -666,6 +666,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
>  	struct strbuf sb = STRBUF_INIT;
>  	const char *hook_arg1 = NULL;
>  	const char *hook_arg2 = NULL;
> +	struct string_list *hooks;
>  	int clean_message_contents = (cleanup_mode != COMMIT_MSG_CLEANUP_NONE);
>  	int old_display_comment_prefix;
>  
> @@ -943,13 +944,15 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
>  		return 0;
>  	}
>  
> -	if (!no_verify && find_hook("pre-commit")) {
> +	hooks = find_hooks("pre-commit");
> +	if (!no_verify && hooks) {
>  		/*
>  		 * Re-read the index as pre-commit hook could have updated it,
>  		 * and write it out as a tree.  We must do this before we invoke
>  		 * the editor and after we invoke run_status above.
>  		 */
>  		discard_cache();
> +		free_hooks(hooks);
>  	}
>  	read_cache_from(index_file);

OK, so find_hook() that used to return a single hook now can return
a list of hook scripts.  Running the single one becomes a simple
special case of "run each of them in turn, and stop at the first
failure".

> diff --git a/run-command.c b/run-command.c
> index 3449db319b..669af5ebc7 100644
> --- a/run-command.c
> +++ b/run-command.c
> @@ -1308,58 +1308,137 @@ int async_with_fork(void)
>  #endif
>  }
>  
> +static int has_hook(struct strbuf *path, int strip)
> +{
> +	if (access(path->buf, X_OK) < 0) {

Does ".git/post-commit" that is not an executable exist?

It was perfectly fine for find_hook() to say "there is no hook for
post-commit" in the old world in such a case, because the
unexecutable file it found is not going to be run anyway.

But it is not clear if has_hook(), that affects "there is no single
hook file for post-commit, so let's look at post-commit.d" decision
made by find_hooks(), should behave that way.  It somehow feels more
intuitive if a post-commit file that is not executable, by merely
existing, stops post-commit.d directory from being scanned, at least
to me.

>  int run_hook_ve(const char *const *env, const char *name, va_list args)
>  {
> -	struct child_process hook = CHILD_PROCESS_INIT;
> +	struct string_list *hooks;
> +	struct string_list arglist = STRING_LIST_INIT_NODUP;
>  	const char *p;
> +	struct string_list_item *q;
> +	int ret = 0;
>   ...
> +		hook.env = env;
> +		hook.no_stdin = 1;
> +		hook.stdout_to_stderr = 1;
> +		hook.trace2_hook_name = name;
> +
> +		ret = run_command(&hook);
> +		if (ret)
> +			break;
> +	}
> +	string_list_clear(&arglist, 0);
> +	free_hooks(hooks);
> +	return ret;
>  }

These "run with command line arguments as its sole input, with the
exit status as its sole output" style hooks are easily handled and
the above looks like reasonable enhancement to the existing
abstraction (e.g. run 'prepare-commit-msg' hook with these
arguments).

I however wonder how the hooks in the other style should/can be
handled, that are fed data from their standard input stream, and
returns more than one bit via their standard output stream.  In
any case, they are not in the scope of this step.