Re: [PATCH 1/5] run-command: add preliminary support for multiple hooks
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Apr 24, 2019, 02:27 UTC
- Message-ID
- <xmqqo94w2l3k.fsf@gitster-ct.c.googlers.com>
- In-Reply-To
- <20190424004948.728326-2-sandals@crustytoothpaste.net>
"brian m. carlson" <sandals@crustytoothpaste.net> writes:
Show 28 quoted lines
> 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".
Show 11 quoted lines
> 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.
Show 22 quoted lines
> 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.