All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] commit: use config-based hooks (config-based hooks part II)
@ 2020-10-14 23:25 Emily Shaffer
  2020-10-16 18:34 ` Junio C Hamano
                   ` (2 more replies)
  0 siblings, 3 replies; 53+ messages in thread
From: Emily Shaffer @ 2020-10-14 23:25 UTC (permalink / raw)
  To: git; +Cc: Emily Shaffer

As part of the adoption of config-based hooks, teach run_commit_hook()
to call hook.h instead of run-command.h. This covers 'pre-commit',
'commit-msg', and 'prepare-commit-msg'. Additionally, ask the hook
library - not run-command - whether any hooks will be run, as it's
possible hooks may exist in the config but not the hookdir.

Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
---

This is based on es/config-based-hooks (v5).

Since config-based hooks v4, I split a new branch for the conversions of
existing hook callers. I'm hoping this will make it easier for me to
work on the architecture without needing to juggle these
hopefully-slow-moving patches as frequently, and will reduce the load on
reviewers.

Since v4 also I removed the second patch changing the run_commit_hook()
API. I had figured it might not be well received when I sent it, and it
wasn't, so no worries.

I'm hoping to have updates to this series pretty soon too with new
conversions... so I guess this one may just be noise. Ah well.

 - Emily

 builtin/commit.c                                |  3 ++-
 builtin/merge.c                                 |  3 ++-
 commit.c                                        | 10 +++++++++-
 ...503-pre-commit-and-pre-merge-commit-hooks.sh | 17 +++++++++++++++--
 4 files changed, 28 insertions(+), 5 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 1dfd799ec5..a337ecd4c2 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -36,6 +36,7 @@
 #include "help.h"
 #include "commit-reach.h"
 #include "commit-graph.h"
+#include "hook.h"
 
 static const char * const builtin_commit_usage[] = {
 	N_("git commit [<options>] [--] <pathspec>..."),
@@ -983,7 +984,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 		return 0;
 	}
 
-	if (!no_verify && find_hook("pre-commit")) {
+	if (!no_verify && hook_exists("pre-commit", configured_hookdir_opt())) {
 		/*
 		 * 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
diff --git a/builtin/merge.c b/builtin/merge.c
index 9d5359edc2..36ba138f4e 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -41,6 +41,7 @@
 #include "commit-reach.h"
 #include "wt-status.h"
 #include "commit-graph.h"
+#include "hook.h"
 
 #define DEFAULT_TWOHEAD (1<<0)
 #define DEFAULT_OCTOPUS (1<<1)
@@ -829,7 +830,7 @@ static void prepare_to_commit(struct commit_list *remoteheads)
 	 * and write it out as a tree.  We must do this before we invoke
 	 * the editor and after we invoke run_status above.
 	 */
-	if (find_hook("pre-merge-commit"))
+	if (hook_exists("pre-merge-commit", configured_hookdir_opt()))
 		discard_cache();
 	read_cache_from(index_file);
 	strbuf_addbuf(&msg, &merge_msg);
diff --git a/commit.c b/commit.c
index f53429c0ac..b338f50103 100644
--- a/commit.c
+++ b/commit.c
@@ -21,6 +21,7 @@
 #include "commit-reach.h"
 #include "run-command.h"
 #include "shallow.h"
+#include "hook.h"
 
 static struct commit_extra_header *read_commit_extra_header_lines(const char *buf, size_t len, const char **);
 
@@ -1635,8 +1636,11 @@ int run_commit_hook(int editor_is_used, const char *index_file,
 {
 	struct strvec hook_env = STRVEC_INIT;
 	va_list args;
+	const char *arg;
+	struct strvec hook_args = STRVEC_INIT;
 	int ret;
 
+
 	strvec_pushf(&hook_env, "GIT_INDEX_FILE=%s", index_file);
 
 	/*
@@ -1646,9 +1650,13 @@ int run_commit_hook(int editor_is_used, const char *index_file,
 		strvec_push(&hook_env, "GIT_EDITOR=:");
 
 	va_start(args, name);
-	ret = run_hook_ve(hook_env.v, name, args);
+	while ((arg = va_arg(args, const char *)))
+		strvec_push(&hook_args, arg);
 	va_end(args);
+
+	ret = run_hooks(hook_env.v, name, &hook_args, configured_hookdir_opt());
 	strvec_clear(&hook_env);
+	strvec_clear(&hook_args);
 
 	return ret;
 }
diff --git a/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh b/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh
index b3485450a2..fc93bc3d23 100755
--- a/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh
+++ b/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh
@@ -5,8 +5,8 @@ test_description='pre-commit and pre-merge-commit hooks'
 . ./test-lib.sh
 
 HOOKDIR="$(git rev-parse --git-dir)/hooks"
-PRECOMMIT="$HOOKDIR/pre-commit"
-PREMERGE="$HOOKDIR/pre-merge-commit"
+PRECOMMIT="$(pwd)/$HOOKDIR/pre-commit"
+PREMERGE="$(pwd)/$HOOKDIR/pre-merge-commit"
 
 # Prepare sample scripts that write their $0 to actual_hooks
 test_expect_success 'sample script setup' '
@@ -103,6 +103,19 @@ test_expect_success 'with succeeding hook' '
 	test_cmp expected_hooks actual_hooks
 '
 
+# NEEDSWORK: when 'git hook add' and 'git hook remove' have been added, use that
+# instead
+test_expect_success 'with succeeding hook (config-based)' '
+	test_when_finished "git config --unset hook.pre-commit.command success.sample" &&
+	test_when_finished "rm -f expected_hooks actual_hooks" &&
+	git config hook.pre-commit.command "$HOOKDIR/success.sample" &&
+	echo "$HOOKDIR/success.sample" >expected_hooks &&
+	echo "more" >>file &&
+	git add file &&
+	git commit -m "more" &&
+	test_cmp expected_hooks actual_hooks
+'
+
 test_expect_success 'with succeeding hook (merge)' '
 	test_when_finished "rm -f \"$PREMERGE\" expected_hooks actual_hooks" &&
 	cp "$HOOKDIR/success.sample" "$PREMERGE" &&
-- 
2.28.0.rc0.142.g3c755180ce-goog


^ permalink raw reply related	[flat|nested] 53+ messages in thread

* Re: [PATCH] commit: use config-based hooks (config-based hooks part II)
  2020-10-14 23:25 [PATCH] commit: use config-based hooks (config-based hooks part II) Emily Shaffer
@ 2020-10-16 18:34 ` Junio C Hamano
  2020-12-05  1:49 ` [PATCH 00/17] use config-based hooks (config-based hooks part Emily Shaffer
  2020-12-16  0:31 ` [PATCH] commit: " Josh Steadmon
  2 siblings, 0 replies; 53+ messages in thread
From: Junio C Hamano @ 2020-10-16 18:34 UTC (permalink / raw)
  To: Emily Shaffer; +Cc: git

Emily Shaffer <emilyshaffer@google.com> writes:

> @@ -983,7 +984,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
>  		return 0;
>  	}
>  
> -	if (!no_verify && find_hook("pre-commit")) {
> +	if (!no_verify && hook_exists("pre-commit", configured_hookdir_opt())) {

So, the migration from find_hook(), which is the traditional "only
on the filesystem in $GIT_DIR/hooks", to hook_exists(), which knows
the ones defined in the configuration files, is the same as the
previous round's.  Understood.

> diff --git a/commit.c b/commit.c
> index f53429c0ac..b338f50103 100644
> --- a/commit.c
> +++ b/commit.c
> @@ -21,6 +21,7 @@
>  #include "commit-reach.h"
>  #include "run-command.h"
>  #include "shallow.h"
> +#include "hook.h"
>  
>  static struct commit_extra_header *read_commit_extra_header_lines(const char *buf, size_t len, const char **);
>  
> @@ -1635,8 +1636,11 @@ int run_commit_hook(int editor_is_used, const char *index_file,
>  {
>  	struct strvec hook_env = STRVEC_INIT;
>  	va_list args;
> +	const char *arg;
> +	struct strvec hook_args = STRVEC_INIT;
>  	int ret;
>  
> +
>  	strvec_pushf(&hook_env, "GIT_INDEX_FILE=%s", index_file);

Noise.

> @@ -1646,9 +1650,13 @@ int run_commit_hook(int editor_is_used, const char *index_file,
>  		strvec_push(&hook_env, "GIT_EDITOR=:");
>  
>  	va_start(args, name);
> -	ret = run_hook_ve(hook_env.v, name, args);
> +	while ((arg = va_arg(args, const char *)))
> +		strvec_push(&hook_args, arg);
>  	va_end(args);
> +
> +	ret = run_hooks(hook_env.v, name, &hook_args, configured_hookdir_opt());

And this is the calling convention change.  Generally speaking,
run_hook_ve() and friends (does it still have friends?) are on their
way out, and run_hooks() will be the only thing we need to learn in
the future.

> diff --git a/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh b/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh
> index b3485450a2..fc93bc3d23 100755
> --- a/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh
> +++ b/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh
> @@ -5,8 +5,8 @@ test_description='pre-commit and pre-merge-commit hooks'
>  . ./test-lib.sh
>  
>  HOOKDIR="$(git rev-parse --git-dir)/hooks"
> -PRECOMMIT="$HOOKDIR/pre-commit"
> -PREMERGE="$HOOKDIR/pre-merge-commit"
> +PRECOMMIT="$(pwd)/$HOOKDIR/pre-commit"
> +PREMERGE="$(pwd)/$HOOKDIR/pre-merge-commit"

What makes this change necessary?  Are we going to chdir around or
something?  Isn't the configuration file the ONLY thing that needs
to know their full location?

Should and does hook.*.command configuration honor ~ expansion?  I
think our $TRASH_DIRECTORY is our $HOME during tests, so 

	git config hook.foo.command "~/.git/hooks/success.sample"

might be a worthy thing to test.  Also, do we have a clear
definition on what happens to a relative pathname specified for the
hook commands, or is it left "undefined--do not do that"?  If the
former, that would also be worth testing.  I'd imagine that majority
of hooks defined in ~/.gitconfig WILL point at full path so testing
relative path may not matter too much for that particular use case,
but for ones defined in .git/config, I suspect it would be most
natural to take it as relative to the root of the working tree.

It may be a good change in the longer term, but it felt surprising
to see such a change that would affect 103-5=98 lines of existing
tests was made centrally here, without any explanation in the
proposed log message.

Thanks.

>  # Prepare sample scripts that write their $0 to actual_hooks
>  test_expect_success 'sample script setup' '
> @@ -103,6 +103,19 @@ test_expect_success 'with succeeding hook' '
>  	test_cmp expected_hooks actual_hooks
>  '
>  
> +# NEEDSWORK: when 'git hook add' and 'git hook remove' have been added, use that
> +# instead
> +test_expect_success 'with succeeding hook (config-based)' '
> +	test_when_finished "git config --unset hook.pre-commit.command success.sample" &&
> +	test_when_finished "rm -f expected_hooks actual_hooks" &&
> +	git config hook.pre-commit.command "$HOOKDIR/success.sample" &&
> +	echo "$HOOKDIR/success.sample" >expected_hooks &&
> +	echo "more" >>file &&
> +	git add file &&
> +	git commit -m "more" &&
> +	test_cmp expected_hooks actual_hooks
> +'
> +
>  test_expect_success 'with succeeding hook (merge)' '
>  	test_when_finished "rm -f \"$PREMERGE\" expected_hooks actual_hooks" &&
>  	cp "$HOOKDIR/success.sample" "$PREMERGE" &&

^ permalink raw reply	[flat|nested] 53+ messages in thread

* [PATCH 00/17] use config-based hooks (config-based hooks part
  2020-10-14 23:25 [PATCH] commit: use config-based hooks (config-based hooks part II) Emily Shaffer
  2020-10-16 18:34 ` Junio C Hamano
@ 2020-12-05  1:49 ` Emily Shaffer
  2020-12-05  1:49   ` [PATCH 01/17] commit: use config-based hooks Emily Shaffer
                     ` (17 more replies)
  2020-12-16  0:31 ` [PATCH] commit: " Josh Steadmon
  2 siblings, 18 replies; 53+ messages in thread
From: Emily Shaffer @ 2020-12-05  1:49 UTC (permalink / raw)
  To: git
  Cc: Emily Shaffer, Jeff King, Junio C Hamano, James Ramsay,
	Jonathan Nieder, brian m. carlson,
	Ævar Arnfjörð Bjarmason, Phillip Wood,
	Josh Steadmon, Johannes Schindelin

As of rebasing today, these are all the hooks, converted to hook.h.

I chose between parallel and serial execution based only on reading the docs -
please, please correct me if I'm wrong.

Most of these are pretty straightforward but there are a couple outstanding ones
on the server side, which I think could use some extra attention.

Thanks!
 - Emily

Emily Shaffer (17):
  commit: use config-based hooks
  am: convert applypatch hooks to use config
  merge: use config-based hooks for post-merge hook
  gc: use hook library for pre-auto-gc hook
  rebase: teach pre-rebase to use hook.h
  read-cache: convert post-index-change hook to use config
  receive-pack: convert push-to-checkout hook to hook.h
  git-p4: use 'git hook' to run hooks
  hooks: convert 'post-checkout' hook to hook library
  hook: convert 'post-rewrite' hook to config
  transport: convert pre-push hook to use config
  reference-transaction: look for hooks in config
  receive-pack: convert 'update' hook to hook.h
  proc-receive: acquire hook list from hook.h
  post-update: use hook.h library
  receive-pack: convert receive hooks to hook.h
  run-command: stop thinking about hooks

 Documentation/githooks.txt                    |  45 +++
 builtin/am.c                                  |  30 +-
 builtin/checkout.c                            |  16 +-
 builtin/clone.c                               |   7 +-
 builtin/commit.c                              |  11 +-
 builtin/gc.c                                  |   4 +-
 builtin/merge.c                               |  14 +-
 builtin/rebase.c                              |   7 +-
 builtin/receive-pack.c                        | 326 ++++++++++--------
 builtin/worktree.c                            |  30 +-
 commit.c                                      |  20 +-
 commit.h                                      |   3 +-
 git-p4.py                                     |  70 +---
 hook.c                                        |  39 ++-
 read-cache.c                                  |  12 +-
 refs.c                                        |  33 +-
 reset.c                                       |  15 +-
 run-command.c                                 |  66 ----
 run-command.h                                 |  24 --
 sequencer.c                                   |  83 ++---
 t/t1416-ref-transaction-hooks.sh              |  12 +-
 t/t5411/test-0015-too-many-hooks-error.sh     |  47 +++
 ...3-pre-commit-and-pre-merge-commit-hooks.sh |  17 +-
 transport.c                                   |  55 +--
 24 files changed, 492 insertions(+), 494 deletions(-)
 create mode 100644 t/t5411/test-0015-too-many-hooks-error.sh

-- 
2.28.0.rc0.142.g3c755180ce-goog


^ permalink raw reply	[flat|nested] 53+ messages in thread

* [PATCH 01/17] commit: use config-based hooks
  2020-12-05  1:49 ` [PATCH 00/17] use config-based hooks (config-based hooks part Emily Shaffer
@ 2020-12-05  1:49   ` Emily Shaffer
  2020-12-05  1:49   ` [PATCH 02/17] am: convert applypatch hooks to use config Emily Shaffer
                     ` (16 subsequent siblings)
  17 siblings, 0 replies; 53+ messages in thread
From: Emily Shaffer @ 2020-12-05  1:49 UTC (permalink / raw)
  To: git; +Cc: Emily Shaffer

As part of the adoption of config-based hooks, teach run_commit_hook()
to call hook.h instead of run-command.h. This covers 'pre-commit',
'commit-msg', and 'prepare-commit-msg'. Additionally, ask the hook
library - not run-command - whether any hooks will be run, as it's
possible hooks may exist in the config but not the hookdir.

Because all but 'post-commit' hooks are expected to make some state
change, force all but 'post-commit' hook to run in series. 'post-commit'
"is meant primarily for notification, and cannot affect the outcome of
`git commit`," so it is fine to run in parallel.

Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
---
 Documentation/githooks.txt                    | 10 ++++++++++
 builtin/commit.c                              | 11 +++++-----
 builtin/merge.c                               |  9 +++++----
 commit.c                                      | 20 +++++++++++++------
 commit.h                                      |  3 ++-
 sequencer.c                                   |  7 ++++---
 ...3-pre-commit-and-pre-merge-commit-hooks.sh | 17 ++++++++++++++--
 7 files changed, 56 insertions(+), 21 deletions(-)

diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
index ffccfc7760..8b352be43f 100644
--- a/Documentation/githooks.txt
+++ b/Documentation/githooks.txt
@@ -103,6 +103,8 @@ The default 'pre-commit' hook, when enabled--and with the
 `hooks.allownonascii` config option unset or set to false--prevents
 the use of non-ASCII filenames.
 
+Hooks executed during 'pre-commit' will not be parallelized.
+
 pre-merge-commit
 ~~~~~~~~~~~~~~~~
 
@@ -125,6 +127,8 @@ need to be resolved and the result committed separately (see
 linkgit:git-merge[1]). At that point, this hook will not be executed,
 but the 'pre-commit' hook will, if it is enabled.
 
+Hooks executed during 'pre-merge-commit' will not be parallelized.
+
 prepare-commit-msg
 ~~~~~~~~~~~~~~~~~~
 
@@ -150,6 +154,8 @@ be used as replacement for pre-commit hook.
 The sample `prepare-commit-msg` hook that comes with Git removes the
 help message found in the commented portion of the commit template.
 
+Hooks executed during 'prepare-commit-msg' will not be parallelized.
+
 commit-msg
 ~~~~~~~~~~
 
@@ -166,6 +172,8 @@ file.
 The default 'commit-msg' hook, when enabled, detects duplicate
 `Signed-off-by` trailers, and aborts the commit if one is found.
 
+Hooks executed during 'commit-msg' will not be parallelized.
+
 post-commit
 ~~~~~~~~~~~
 
@@ -175,6 +183,8 @@ invoked after a commit is made.
 This hook is meant primarily for notification, and cannot affect
 the outcome of `git commit`.
 
+Hooks executed during 'post-commit' will run in parallel by default.
+
 pre-rebase
 ~~~~~~~~~~
 
diff --git a/builtin/commit.c b/builtin/commit.c
index 505fe60956..f4dea2b510 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -36,6 +36,7 @@
 #include "help.h"
 #include "commit-reach.h"
 #include "commit-graph.h"
+#include "hook.h"
 
 static const char * const builtin_commit_usage[] = {
 	N_("git commit [<options>] [--] <pathspec>..."),
@@ -699,7 +700,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 	/* This checks and barfs if author is badly specified */
 	determine_author_info(author_ident);
 
-	if (!no_verify && run_commit_hook(use_editor, index_file, "pre-commit", NULL))
+	if (!no_verify && run_commit_hook(use_editor, 0, index_file, "pre-commit", NULL))
 		return 0;
 
 	if (squash_message) {
@@ -983,7 +984,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 		return 0;
 	}
 
-	if (!no_verify && find_hook("pre-commit")) {
+	if (!no_verify && hook_exists("pre-commit", configured_hookdir_opt())) {
 		/*
 		 * 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
@@ -998,7 +999,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 		return 0;
 	}
 
-	if (run_commit_hook(use_editor, index_file, "prepare-commit-msg",
+	if (run_commit_hook(use_editor, 0, index_file, "prepare-commit-msg",
 			    git_path_commit_editmsg(), hook_arg1, hook_arg2, NULL))
 		return 0;
 
@@ -1015,7 +1016,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 	}
 
 	if (!no_verify &&
-	    run_commit_hook(use_editor, index_file, "commit-msg", git_path_commit_editmsg(), NULL)) {
+	    run_commit_hook(use_editor, 0, index_file, "commit-msg", git_path_commit_editmsg(), NULL)) {
 		return 0;
 	}
 
@@ -1701,7 +1702,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 
 	repo_rerere(the_repository, 0);
 	run_auto_maintenance(quiet);
-	run_commit_hook(use_editor, get_index_file(), "post-commit", NULL);
+	run_commit_hook(use_editor, 1, get_index_file(), "post-commit", NULL);
 	if (amend && !no_post_rewrite) {
 		commit_post_rewrite(the_repository, current_head, &oid);
 	}
diff --git a/builtin/merge.c b/builtin/merge.c
index 1cff730715..d654b6923c 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -42,6 +42,7 @@
 #include "commit-reach.h"
 #include "wt-status.h"
 #include "commit-graph.h"
+#include "hook.h"
 
 #define DEFAULT_TWOHEAD (1<<0)
 #define DEFAULT_OCTOPUS (1<<1)
@@ -836,14 +837,14 @@ static void prepare_to_commit(struct commit_list *remoteheads)
 	struct strbuf msg = STRBUF_INIT;
 	const char *index_file = get_index_file();
 
-	if (!no_verify && run_commit_hook(0 < option_edit, index_file, "pre-merge-commit", NULL))
+	if (!no_verify && run_commit_hook(0 < option_edit, 0, index_file, "pre-merge-commit", NULL))
 		abort_commit(remoteheads, NULL);
 	/*
 	 * Re-read the index as pre-merge-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.
 	 */
-	if (find_hook("pre-merge-commit"))
+	if (hook_exists("pre-merge-commit", configured_hookdir_opt()))
 		discard_cache();
 	read_cache_from(index_file);
 	strbuf_addbuf(&msg, &merge_msg);
@@ -864,7 +865,7 @@ static void prepare_to_commit(struct commit_list *remoteheads)
 		append_signoff(&msg, ignore_non_trailer(msg.buf, msg.len), 0);
 	write_merge_heads(remoteheads);
 	write_file_buf(git_path_merge_msg(the_repository), msg.buf, msg.len);
-	if (run_commit_hook(0 < option_edit, get_index_file(), "prepare-commit-msg",
+	if (run_commit_hook(0 < option_edit, 0, get_index_file(), "prepare-commit-msg",
 			    git_path_merge_msg(the_repository), "merge", NULL))
 		abort_commit(remoteheads, NULL);
 	if (0 < option_edit) {
@@ -872,7 +873,7 @@ static void prepare_to_commit(struct commit_list *remoteheads)
 			abort_commit(remoteheads, NULL);
 	}
 
-	if (!no_verify && run_commit_hook(0 < option_edit, get_index_file(),
+	if (!no_verify && run_commit_hook(0 < option_edit, 0, get_index_file(),
 					  "commit-msg",
 					  git_path_merge_msg(the_repository), NULL))
 		abort_commit(remoteheads, NULL);
diff --git a/commit.c b/commit.c
index fe1fa3dc41..1bad721a20 100644
--- a/commit.c
+++ b/commit.c
@@ -21,6 +21,7 @@
 #include "commit-reach.h"
 #include "run-command.h"
 #include "shallow.h"
+#include "hook.h"
 
 static struct commit_extra_header *read_commit_extra_header_lines(const char *buf, size_t len, const char **);
 
@@ -1630,25 +1631,32 @@ size_t ignore_non_trailer(const char *buf, size_t len)
 	return boc ? len - boc : len - cutoff;
 }
 
-int run_commit_hook(int editor_is_used, const char *index_file,
+int run_commit_hook(int editor_is_used, int parallelize, const char *index_file,
 		    const char *name, ...)
 {
-	struct strvec hook_env = STRVEC_INIT;
+	struct run_hooks_opt opt = RUN_HOOKS_OPT_INIT_SYNC;
 	va_list args;
+	const char *arg;
 	int ret;
 
-	strvec_pushf(&hook_env, "GIT_INDEX_FILE=%s", index_file);
+	if (parallelize)
+		opt.jobs = configured_hook_jobs();
+
+	strvec_pushf(&opt.env, "GIT_INDEX_FILE=%s", index_file);
 
 	/*
 	 * Let the hook know that no editor will be launched.
 	 */
 	if (!editor_is_used)
-		strvec_push(&hook_env, "GIT_EDITOR=:");
+		strvec_push(&opt.env, "GIT_EDITOR=:");
 
 	va_start(args, name);
-	ret = run_hook_ve(hook_env.v, name, args);
+	while ((arg = va_arg(args, const char *)))
+		strvec_push(&opt.args, arg);
 	va_end(args);
-	strvec_clear(&hook_env);
+
+	ret = run_hooks(name, &opt);
+	run_hooks_opt_clear(&opt);
 
 	return ret;
 }
diff --git a/commit.h b/commit.h
index 5467786c7b..dd33ead8e0 100644
--- a/commit.h
+++ b/commit.h
@@ -352,6 +352,7 @@ int compare_commits_by_commit_date(const void *a_, const void *b_, void *unused)
 int compare_commits_by_gen_then_commit_date(const void *a_, const void *b_, void *unused);
 
 LAST_ARG_MUST_BE_NULL
-int run_commit_hook(int editor_is_used, const char *index_file, const char *name, ...);
+int run_commit_hook(int editor_is_used, int parallelize, const char *index_file,
+		    const char *name, ...);
 
 #endif /* COMMIT_H */
diff --git a/sequencer.c b/sequencer.c
index 8909a46770..5a98fd2fbc 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -34,6 +34,7 @@
 #include "commit-reach.h"
 #include "rebase-interactive.h"
 #include "reset.h"
+#include "hook.h"
 
 #define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION"
 
@@ -1203,7 +1204,7 @@ static int run_prepare_commit_msg_hook(struct repository *r,
 	} else {
 		arg1 = "message";
 	}
-	if (run_commit_hook(0, r->index_file, "prepare-commit-msg", name,
+	if (run_commit_hook(0, 0, r->index_file, "prepare-commit-msg", name,
 			    arg1, arg2, NULL))
 		ret = error(_("'prepare-commit-msg' hook failed"));
 
@@ -1438,7 +1439,7 @@ static int try_to_commit(struct repository *r,
 		}
 	}
 
-	if (find_hook("prepare-commit-msg")) {
+	if (hook_exists("prepare-commit-msg", configured_hookdir_opt())) {
 		res = run_prepare_commit_msg_hook(r, msg, hook_commit);
 		if (res)
 			goto out;
@@ -1528,7 +1529,7 @@ static int try_to_commit(struct repository *r,
 		goto out;
 	}
 
-	run_commit_hook(0, r->index_file, "post-commit", NULL);
+	run_commit_hook(0, 1, r->index_file, "post-commit", NULL);
 	if (flags & AMEND_MSG)
 		commit_post_rewrite(r, current_head, oid);
 
diff --git a/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh b/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh
index b3485450a2..fc93bc3d23 100755
--- a/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh
+++ b/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh
@@ -5,8 +5,8 @@ test_description='pre-commit and pre-merge-commit hooks'
 . ./test-lib.sh
 
 HOOKDIR="$(git rev-parse --git-dir)/hooks"
-PRECOMMIT="$HOOKDIR/pre-commit"
-PREMERGE="$HOOKDIR/pre-merge-commit"
+PRECOMMIT="$(pwd)/$HOOKDIR/pre-commit"
+PREMERGE="$(pwd)/$HOOKDIR/pre-merge-commit"
 
 # Prepare sample scripts that write their $0 to actual_hooks
 test_expect_success 'sample script setup' '
@@ -103,6 +103,19 @@ test_expect_success 'with succeeding hook' '
 	test_cmp expected_hooks actual_hooks
 '
 
+# NEEDSWORK: when 'git hook add' and 'git hook remove' have been added, use that
+# instead
+test_expect_success 'with succeeding hook (config-based)' '
+	test_when_finished "git config --unset hook.pre-commit.command success.sample" &&
+	test_when_finished "rm -f expected_hooks actual_hooks" &&
+	git config hook.pre-commit.command "$HOOKDIR/success.sample" &&
+	echo "$HOOKDIR/success.sample" >expected_hooks &&
+	echo "more" >>file &&
+	git add file &&
+	git commit -m "more" &&
+	test_cmp expected_hooks actual_hooks
+'
+
 test_expect_success 'with succeeding hook (merge)' '
 	test_when_finished "rm -f \"$PREMERGE\" expected_hooks actual_hooks" &&
 	cp "$HOOKDIR/success.sample" "$PREMERGE" &&
-- 
2.28.0.rc0.142.g3c755180ce-goog


^ permalink raw reply related	[flat|nested] 53+ messages in thread

* [PATCH 02/17] am: convert applypatch hooks to use config
  2020-12-05  1:49 ` [PATCH 00/17] use config-based hooks (config-based hooks part Emily Shaffer
  2020-12-05  1:49   ` [PATCH 01/17] commit: use config-based hooks Emily Shaffer
@ 2020-12-05  1:49   ` Emily Shaffer
  2020-12-05  1:49   ` [PATCH 03/17] merge: use config-based hooks for post-merge hook Emily Shaffer
                     ` (15 subsequent siblings)
  17 siblings, 0 replies; 53+ messages in thread
From: Emily Shaffer @ 2020-12-05  1:49 UTC (permalink / raw)
  To: git; +Cc: Emily Shaffer

Teach pre-applypatch, post-applypatch, and applypatch-msg to use the
hook.h library instead of the run-command.h library. This enables use of
hooks specified in the config, in addition to those in the hookdir.
These three hooks are called only by builtin/am.c.

Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
---
 Documentation/githooks.txt |  6 ++++++
 builtin/am.c               | 12 +++++++++---
 2 files changed, 15 insertions(+), 3 deletions(-)

diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
index 8b352be43f..0842cd812c 100644
--- a/Documentation/githooks.txt
+++ b/Documentation/githooks.txt
@@ -58,6 +58,8 @@ the message file.
 The default 'applypatch-msg' hook, when enabled, runs the
 'commit-msg' hook, if the latter is enabled.
 
+Hooks run during 'applypatch-msg' will not be parallelized.
+
 pre-applypatch
 ~~~~~~~~~~~~~~
 
@@ -73,6 +75,8 @@ make a commit if it does not pass certain test.
 The default 'pre-applypatch' hook, when enabled, runs the
 'pre-commit' hook, if the latter is enabled.
 
+Hooks run during 'pre-applypatch' will be run in parallel by default.
+
 post-applypatch
 ~~~~~~~~~~~~~~~
 
@@ -82,6 +86,8 @@ and is invoked after the patch is applied and a commit is made.
 This hook is meant primarily for notification, and cannot affect
 the outcome of `git am`.
 
+Hooks run during 'post-applypatch' will be run in parallel by default.
+
 pre-commit
 ~~~~~~~~~~
 
diff --git a/builtin/am.c b/builtin/am.c
index f22c73a05b..22d147bc19 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -33,6 +33,7 @@
 #include "string-list.h"
 #include "packfile.h"
 #include "repository.h"
+#include "hook.h"
 
 /**
  * Returns the length of the first line of msg.
@@ -426,9 +427,12 @@ static void am_destroy(const struct am_state *state)
 static int run_applypatch_msg_hook(struct am_state *state)
 {
 	int ret;
+	struct run_hooks_opt opt = RUN_HOOKS_OPT_INIT_SYNC;
 
 	assert(state->msg);
-	ret = run_hook_le(NULL, "applypatch-msg", am_path(state, "final-commit"), NULL);
+	strvec_push(&opt.args, am_path(state, "final-commit"));
+	ret = run_hooks("applypatch-msg", &opt);
+	run_hooks_opt_clear(&opt);
 
 	if (!ret) {
 		FREE_AND_NULL(state->msg);
@@ -1558,8 +1562,9 @@ static void do_commit(const struct am_state *state)
 	struct commit_list *parents = NULL;
 	const char *reflog_msg, *author, *committer = NULL;
 	struct strbuf sb = STRBUF_INIT;
+	struct run_hooks_opt hook_opt = RUN_HOOKS_OPT_INIT_ASYNC;
 
-	if (run_hook_le(NULL, "pre-applypatch", NULL))
+	if (run_hooks("pre-applypatch", &hook_opt))
 		exit(1);
 
 	if (write_cache_as_tree(&tree, 0, NULL))
@@ -1611,8 +1616,9 @@ static void do_commit(const struct am_state *state)
 		fclose(fp);
 	}
 
-	run_hook_le(NULL, "post-applypatch", NULL);
+	run_hooks("post-applypatch", &hook_opt);
 
+	run_hooks_opt_clear(&hook_opt);
 	strbuf_release(&sb);
 }
 
-- 
2.28.0.rc0.142.g3c755180ce-goog


^ permalink raw reply related	[flat|nested] 53+ messages in thread

* [PATCH 03/17] merge: use config-based hooks for post-merge hook
  2020-12-05  1:49 ` [PATCH 00/17] use config-based hooks (config-based hooks part Emily Shaffer
  2020-12-05  1:49   ` [PATCH 01/17] commit: use config-based hooks Emily Shaffer
  2020-12-05  1:49   ` [PATCH 02/17] am: convert applypatch hooks to use config Emily Shaffer
@ 2020-12-05  1:49   ` Emily Shaffer
  2020-12-05  1:49   ` [PATCH 04/17] gc: use hook library for pre-auto-gc hook Emily Shaffer
                     ` (14 subsequent siblings)
  17 siblings, 0 replies; 53+ messages in thread
From: Emily Shaffer @ 2020-12-05  1:49 UTC (permalink / raw)
  To: git; +Cc: Emily Shaffer

Teach post-merge to use the hook.h library instead of the run-command.h
library to run hooks. This means that post-merge hooks can come from the
config as well as from the hookdir. post-merge is invoked only from
builtin/merge.c.

Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
---
 Documentation/githooks.txt | 2 ++
 builtin/merge.c            | 5 ++++-
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
index 0842cd812c..f6ddf1aa22 100644
--- a/Documentation/githooks.txt
+++ b/Documentation/githooks.txt
@@ -236,6 +236,8 @@ save and restore any form of metadata associated with the working tree
 (e.g.: permissions/ownership, ACLS, etc).  See contrib/hooks/setgitperms.perl
 for an example of how to do this.
 
+Hooks executed during 'post-merge' will run in parallel by default.
+
 pre-push
 ~~~~~~~~
 
diff --git a/builtin/merge.c b/builtin/merge.c
index d654b6923c..717fbaa019 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -443,6 +443,7 @@ static void finish(struct commit *head_commit,
 		   const struct object_id *new_head, const char *msg)
 {
 	struct strbuf reflog_message = STRBUF_INIT;
+	struct run_hooks_opt opt = RUN_HOOKS_OPT_INIT_ASYNC;
 	const struct object_id *head = &head_commit->object.oid;
 
 	if (!msg)
@@ -484,7 +485,9 @@ static void finish(struct commit *head_commit,
 	}
 
 	/* Run a post-merge hook */
-	run_hook_le(NULL, "post-merge", squash ? "1" : "0", NULL);
+	strvec_push(&opt.args, squash ? "1" : "0");
+	run_hooks("post-merge", &opt);
+	run_hooks_opt_clear(&opt);
 
 	apply_autostash(git_path_merge_autostash(the_repository));
 	strbuf_release(&reflog_message);
-- 
2.28.0.rc0.142.g3c755180ce-goog


^ permalink raw reply related	[flat|nested] 53+ messages in thread

* [PATCH 04/17] gc: use hook library for pre-auto-gc hook
  2020-12-05  1:49 ` [PATCH 00/17] use config-based hooks (config-based hooks part Emily Shaffer
                     ` (2 preceding siblings ...)
  2020-12-05  1:49   ` [PATCH 03/17] merge: use config-based hooks for post-merge hook Emily Shaffer
@ 2020-12-05  1:49   ` Emily Shaffer
  2020-12-05  1:49   ` [PATCH 05/17] rebase: teach pre-rebase to use hook.h Emily Shaffer
                     ` (13 subsequent siblings)
  17 siblings, 0 replies; 53+ messages in thread
From: Emily Shaffer @ 2020-12-05  1:49 UTC (permalink / raw)
  To: git; +Cc: Emily Shaffer

Using the hook.h library instead of the run-command.h library to run
pre-auto-gc means that those hooks can be set up in config files, as
well as in the hookdir. pre-auto-gc is called only from builtin/gc.c.

Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
---
 Documentation/githooks.txt | 2 ++
 builtin/gc.c               | 4 +++-
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
index f6ddf1aa22..d74308dc20 100644
--- a/Documentation/githooks.txt
+++ b/Documentation/githooks.txt
@@ -553,6 +553,8 @@ This hook is invoked by `git gc --auto` (see linkgit:git-gc[1]). It
 takes no parameter, and exiting with non-zero status from this script
 causes the `git gc --auto` to abort.
 
+Hooks run during 'pre-auto-gc' will be run in parallel by default.
+
 post-rewrite
 ~~~~~~~~~~~~
 
diff --git a/builtin/gc.c b/builtin/gc.c
index 3e8d76fd5a..b01b6eb5fd 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -32,6 +32,7 @@
 #include "remote.h"
 #include "object-store.h"
 #include "exec-cmd.h"
+#include "hook.h"
 
 #define FAILED_RUN "failed to run %s"
 
@@ -340,6 +341,7 @@ static void add_repack_incremental_option(void)
 
 static int need_to_gc(void)
 {
+	struct run_hooks_opt hook_opt = RUN_HOOKS_OPT_INIT_ASYNC;
 	/*
 	 * Setting gc.auto to 0 or negative can disable the
 	 * automatic gc.
@@ -386,7 +388,7 @@ static int need_to_gc(void)
 	else
 		return 0;
 
-	if (run_hook_le(NULL, "pre-auto-gc", NULL))
+	if (run_hooks("pre-auto-gc", &hook_opt))
 		return 0;
 	return 1;
 }
-- 
2.28.0.rc0.142.g3c755180ce-goog


^ permalink raw reply related	[flat|nested] 53+ messages in thread

* [PATCH 05/17] rebase: teach pre-rebase to use hook.h
  2020-12-05  1:49 ` [PATCH 00/17] use config-based hooks (config-based hooks part Emily Shaffer
                     ` (3 preceding siblings ...)
  2020-12-05  1:49   ` [PATCH 04/17] gc: use hook library for pre-auto-gc hook Emily Shaffer
@ 2020-12-05  1:49   ` Emily Shaffer
  2020-12-05  1:49   ` [PATCH 06/17] read-cache: convert post-index-change hook to use config Emily Shaffer
                     ` (12 subsequent siblings)
  17 siblings, 0 replies; 53+ messages in thread
From: Emily Shaffer @ 2020-12-05  1:49 UTC (permalink / raw)
  To: git; +Cc: Emily Shaffer

By using hook.h instead of run-command.h to run hooks, pre-rebase hooks
can now be specified in the config as well as in the hookdir. pre-rebase
is not called anywhere besides builtin/rebase.c.

Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
---
 Documentation/githooks.txt | 2 ++
 builtin/rebase.c           | 7 +++++--
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
index d74308dc20..5dc0690607 100644
--- a/Documentation/githooks.txt
+++ b/Documentation/githooks.txt
@@ -200,6 +200,8 @@ two parameters.  The first parameter is the upstream from which
 the series was forked.  The second parameter is the branch being
 rebased, and is not set when rebasing the current branch.
 
+Hooks executed during 'pre-rebase' will run in parallel by default.
+
 post-checkout
 ~~~~~~~~~~~~~
 
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 19c7b377aa..f61ca3e5af 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -28,6 +28,7 @@
 #include "sequencer.h"
 #include "rebase-interactive.h"
 #include "reset.h"
+#include "hook.h"
 
 #define DEFAULT_REFLOG_ACTION "rebase"
 
@@ -1312,6 +1313,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 	char *squash_onto_name = NULL;
 	int reschedule_failed_exec = -1;
 	int allow_preemptive_ff = 1;
+	struct run_hooks_opt hook_opt = RUN_HOOKS_OPT_INIT_ASYNC;
 	struct option builtin_rebase_options[] = {
 		OPT_STRING(0, "onto", &options.onto_name,
 			   N_("revision"),
@@ -2024,9 +2026,9 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 	}
 
 	/* If a hook exists, give it a chance to interrupt*/
+	strvec_pushl(&hook_opt.args, options.upstream_arg, argc ? argv[0] : NULL, NULL);
 	if (!ok_to_skip_pre_rebase &&
-	    run_hook_le(NULL, "pre-rebase", options.upstream_arg,
-			argc ? argv[0] : NULL, NULL))
+	    run_hooks("pre-rebase", &hook_opt))
 		die(_("The pre-rebase hook refused to rebase."));
 
 	if (options.flags & REBASE_DIFFSTAT) {
@@ -2106,6 +2108,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 	ret = !!run_specific_rebase(&options, action);
 
 cleanup:
+	run_hooks_opt_clear(&hook_opt);
 	strbuf_release(&buf);
 	strbuf_release(&revisions);
 	free(options.head_name);
-- 
2.28.0.rc0.142.g3c755180ce-goog


^ permalink raw reply related	[flat|nested] 53+ messages in thread

* [PATCH 06/17] read-cache: convert post-index-change hook to use config
  2020-12-05  1:49 ` [PATCH 00/17] use config-based hooks (config-based hooks part Emily Shaffer
                     ` (4 preceding siblings ...)
  2020-12-05  1:49   ` [PATCH 05/17] rebase: teach pre-rebase to use hook.h Emily Shaffer
@ 2020-12-05  1:49   ` Emily Shaffer
  2020-12-05  1:49   ` [PATCH 07/17] receive-pack: convert push-to-checkout hook to hook.h Emily Shaffer
                     ` (11 subsequent siblings)
  17 siblings, 0 replies; 53+ messages in thread
From: Emily Shaffer @ 2020-12-05  1:49 UTC (permalink / raw)
  To: git; +Cc: Emily Shaffer

By using hook.h instead of run-command.h to run, post-index-change hooks
can now be specified in the config in addition to the hookdir.
post-index-change is not run anywhere besides in read-cache.c.

Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
---
 Documentation/githooks.txt |  2 ++
 read-cache.c               | 12 +++++++++---
 2 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
index 5dc0690607..8249ecec5f 100644
--- a/Documentation/githooks.txt
+++ b/Documentation/githooks.txt
@@ -711,6 +711,8 @@ and "0" meaning they were not.
 Only one parameter should be set to "1" when the hook runs.  The hook
 running passing "1", "1" should not be possible.
 
+Hooks run during 'post-index-change' will be run in parallel by default.
+
 GIT
 ---
 Part of the linkgit:git[1] suite
diff --git a/read-cache.c b/read-cache.c
index ecf6f68994..dcfc080aaa 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -25,6 +25,7 @@
 #include "fsmonitor.h"
 #include "thread-utils.h"
 #include "progress.h"
+#include "hook.h"
 
 /* Mask for the name length in ce_flags in the on-disk index */
 
@@ -3052,6 +3053,7 @@ static int do_write_locked_index(struct index_state *istate, struct lock_file *l
 				 unsigned flags)
 {
 	int ret;
+	struct run_hooks_opt hook_opt = RUN_HOOKS_OPT_INIT_ASYNC;
 
 	/*
 	 * TODO trace2: replace "the_repository" with the actual repo instance
@@ -3070,9 +3072,13 @@ static int do_write_locked_index(struct index_state *istate, struct lock_file *l
 	else
 		ret = close_lock_file_gently(lock);
 
-	run_hook_le(NULL, "post-index-change",
-			istate->updated_workdir ? "1" : "0",
-			istate->updated_skipworktree ? "1" : "0", NULL);
+	strvec_pushl(&hook_opt.args,
+		     istate->updated_workdir ? "1" : "0",
+		     istate->updated_skipworktree ? "1" : "0",
+		     NULL);
+	run_hooks("post-index-change", &hook_opt);
+	run_hooks_opt_clear(&hook_opt);
+
 	istate->updated_workdir = 0;
 	istate->updated_skipworktree = 0;
 
-- 
2.28.0.rc0.142.g3c755180ce-goog


^ permalink raw reply related	[flat|nested] 53+ messages in thread

* [PATCH 07/17] receive-pack: convert push-to-checkout hook to hook.h
  2020-12-05  1:49 ` [PATCH 00/17] use config-based hooks (config-based hooks part Emily Shaffer
                     ` (5 preceding siblings ...)
  2020-12-05  1:49   ` [PATCH 06/17] read-cache: convert post-index-change hook to use config Emily Shaffer
@ 2020-12-05  1:49   ` Emily Shaffer
  2020-12-05  1:49   ` [PATCH 08/17] git-p4: use 'git hook' to run hooks Emily Shaffer
                     ` (10 subsequent siblings)
  17 siblings, 0 replies; 53+ messages in thread
From: Emily Shaffer @ 2020-12-05  1:49 UTC (permalink / raw)
  To: git; +Cc: Emily Shaffer

By using hook.h instead of run-command.h to invoke push-to-checkout,
hooks can now be specified in the config as well as in the hookdir.
push-to-checkout is not called anywhere but in builtin/receive-pack.c.

Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
---
 Documentation/githooks.txt |  1 +
 builtin/receive-pack.c     | 15 +++++++++++----
 2 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
index 8249ecec5f..8de512ee5d 100644
--- a/Documentation/githooks.txt
+++ b/Documentation/githooks.txt
@@ -547,6 +547,7 @@ that switches branches while
 keeping the local changes in the working tree that do not interfere
 with the difference between the branches.
 
+Hooks executed during 'push-to-checkout' will not be parallelized.
 
 pre-auto-gc
 ~~~~~~~~~~~
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index f1f0f7bef6..d64a3076be 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -29,6 +29,7 @@
 #include "commit-reach.h"
 #include "worktree.h"
 #include "shallow.h"
+#include "hook.h"
 
 static const char * const receive_pack_usage[] = {
 	N_("git receive-pack <git-dir>"),
@@ -1427,12 +1428,18 @@ static const char *push_to_checkout(unsigned char *hash,
 				    struct strvec *env,
 				    const char *work_tree)
 {
+	struct run_hooks_opt opt = RUN_HOOKS_OPT_INIT_SYNC;
+
 	strvec_pushf(env, "GIT_WORK_TREE=%s", absolute_path(work_tree));
-	if (run_hook_le(env->v, push_to_checkout_hook,
-			hash_to_hex(hash), NULL))
+	strvec_pushv(&opt.env, env->v);
+	strvec_push(&opt.args, hash_to_hex(hash));
+	if (run_hooks(push_to_checkout_hook, &opt)) {
+		run_hooks_opt_clear(&opt);
 		return "push-to-checkout hook declined";
-	else
+	} else {
+		run_hooks_opt_clear(&opt);
 		return NULL;
+	}
 }
 
 static const char *update_worktree(unsigned char *sha1, const struct worktree *worktree)
@@ -1456,7 +1463,7 @@ static const char *update_worktree(unsigned char *sha1, const struct worktree *w
 
 	strvec_pushf(&env, "GIT_DIR=%s", absolute_path(git_dir));
 
-	if (!find_hook(push_to_checkout_hook))
+	if (!hook_exists(push_to_checkout_hook, configured_hookdir_opt()))
 		retval = push_to_deploy(sha1, &env, work_tree);
 	else
 		retval = push_to_checkout(sha1, &env, work_tree);
-- 
2.28.0.rc0.142.g3c755180ce-goog


^ permalink raw reply related	[flat|nested] 53+ messages in thread

* [PATCH 08/17] git-p4: use 'git hook' to run hooks
  2020-12-05  1:49 ` [PATCH 00/17] use config-based hooks (config-based hooks part Emily Shaffer
                     ` (6 preceding siblings ...)
  2020-12-05  1:49   ` [PATCH 07/17] receive-pack: convert push-to-checkout hook to hook.h Emily Shaffer
@ 2020-12-05  1:49   ` Emily Shaffer
  2020-12-16  0:27     ` Josh Steadmon
  2020-12-05  1:49   ` [PATCH 09/17] hooks: convert 'post-checkout' hook to hook library Emily Shaffer
                     ` (9 subsequent siblings)
  17 siblings, 1 reply; 53+ messages in thread
From: Emily Shaffer @ 2020-12-05  1:49 UTC (permalink / raw)
  To: git; +Cc: Emily Shaffer

Instead of duplicating the behavior of run-command.h:run_hook_le() in
Python, we can directly call 'git hook run'. As a bonus, this means
git-p4 learns how to find hook specifications from the Git config as
well as from the hookdir.

Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
---

Notes:
    Maybe there is a better way to do this - I had a hard time getting this to run
    locally, and Python is not my forte, so if anybody has a better approach I'd
    love to just take that patch instead :)

 git-p4.py | 70 +++++++------------------------------------------------
 1 file changed, 9 insertions(+), 61 deletions(-)

diff --git a/git-p4.py b/git-p4.py
index 6ae5bbfe99..73161a56d9 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -208,70 +208,18 @@ def decode_path(path):
 
 def run_git_hook(cmd, param=[]):
     """Execute a hook if the hook exists."""
-    if verbose:
-        sys.stderr.write("Looking for hook: %s\n" % cmd)
-        sys.stderr.flush()
-
-    hooks_path = gitConfig("core.hooksPath")
-    if len(hooks_path) <= 0:
-        hooks_path = os.path.join(os.environ["GIT_DIR"], "hooks")
-
-    if not isinstance(param, list):
-        param=[param]
-
-    # resolve hook file name, OS depdenent
-    hook_file = os.path.join(hooks_path, cmd)
-    if platform.system() == 'Windows':
-        if not os.path.isfile(hook_file):
-            # look for the file with an extension
-            files = glob.glob(hook_file + ".*")
-            if not files:
-                return True
-            files.sort()
-            hook_file = files.pop()
-            while hook_file.upper().endswith(".SAMPLE"):
-                # The file is a sample hook. We don't want it
-                if len(files) > 0:
-                    hook_file = files.pop()
-                else:
-                    return True
-
-    if not os.path.isfile(hook_file) or not os.access(hook_file, os.X_OK):
+    print('ESS: entering run_git_hook')
+    if not cmd:
         return True
 
-    return run_hook_command(hook_file, param) == 0
-
-def run_hook_command(cmd, param):
-    """Executes a git hook command
-       cmd = the command line file to be executed. This can be
-       a file that is run by OS association.
-
-       param = a list of parameters to pass to the cmd command
-
-       On windows, the extension is checked to see if it should
-       be run with the Git for Windows Bash shell.  If there
-       is no file extension, the file is deemed a bash shell
-       and will be handed off to sh.exe. Otherwise, Windows
-       will be called with the shell to handle the file assocation.
-
-       For non Windows operating systems, the file is called
-       as an executable.
-    """
-    cli = [cmd] + param
-    use_shell = False
-    if platform.system() == 'Windows':
-        (root,ext) = os.path.splitext(cmd)
-        if ext == "":
-            exe_path = os.environ.get("EXEPATH")
-            if exe_path is None:
-                exe_path = ""
-            else:
-                exe_path = os.path.join(exe_path, "bin")
-            cli = [os.path.join(exe_path, "SH.EXE")] + cli
-        else:
-            use_shell = True
-    return subprocess.call(cli, shell=use_shell)
+    """args are specified with -a <arg> -a <arg> -a <arg>"""
+    args = (['git', 'hook', 'run'] +
+	    ["-a" + arg for arg in param] +
+	    [cmd])
+    print ('ESS: args:')
+    print (args)
 
+    return subprocess.call(args) == 0
 
 def write_pipe(c, stdin):
     if verbose:
-- 
2.28.0.rc0.142.g3c755180ce-goog


^ permalink raw reply related	[flat|nested] 53+ messages in thread

* [PATCH 09/17] hooks: convert 'post-checkout' hook to hook library
  2020-12-05  1:49 ` [PATCH 00/17] use config-based hooks (config-based hooks part Emily Shaffer
                     ` (7 preceding siblings ...)
  2020-12-05  1:49   ` [PATCH 08/17] git-p4: use 'git hook' to run hooks Emily Shaffer
@ 2020-12-05  1:49   ` Emily Shaffer
  2020-12-05  1:49   ` [PATCH 10/17] hook: convert 'post-rewrite' hook to config Emily Shaffer
                     ` (8 subsequent siblings)
  17 siblings, 0 replies; 53+ messages in thread
From: Emily Shaffer @ 2020-12-05  1:49 UTC (permalink / raw)
  To: git; +Cc: Emily Shaffer

By using the 'hook.h' library, 'post-checkout' hooks can now be
specified in the config as well as in the hook directory.

Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
---
 Documentation/githooks.txt |  2 ++
 builtin/checkout.c         | 16 +++++++++++-----
 builtin/clone.c            |  7 +++++--
 builtin/worktree.c         | 30 ++++++++++++++----------------
 reset.c                    | 15 +++++++++++----
 5 files changed, 43 insertions(+), 27 deletions(-)

diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
index 8de512ee5d..14035ef725 100644
--- a/Documentation/githooks.txt
+++ b/Documentation/githooks.txt
@@ -224,6 +224,8 @@ This hook can be used to perform repository validity checks, auto-display
 differences from the previous HEAD if different, or set working dir metadata
 properties.
 
+Hooks executed during 'post-checkout' will not be parallelized.
+
 post-merge
 ~~~~~~~~~~
 
diff --git a/builtin/checkout.c b/builtin/checkout.c
index 9b82119129..20966452b8 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -9,6 +9,7 @@
 #include "config.h"
 #include "diff.h"
 #include "dir.h"
+#include "hook.h"
 #include "ll-merge.h"
 #include "lockfile.h"
 #include "merge-recursive.h"
@@ -104,13 +105,18 @@ struct branch_info {
 static int post_checkout_hook(struct commit *old_commit, struct commit *new_commit,
 			      int changed)
 {
-	return run_hook_le(NULL, "post-checkout",
-			   oid_to_hex(old_commit ? &old_commit->object.oid : &null_oid),
-			   oid_to_hex(new_commit ? &new_commit->object.oid : &null_oid),
-			   changed ? "1" : "0", NULL);
+	struct run_hooks_opt opt = RUN_HOOKS_OPT_INIT_SYNC;
+	int rc;
 	/* "new_commit" can be NULL when checking out from the index before
 	   a commit exists. */
-
+	strvec_pushl(&opt.args,
+		     oid_to_hex(old_commit ? &old_commit->object.oid : &null_oid),
+		     oid_to_hex(new_commit ? &new_commit->object.oid : &null_oid),
+		     changed ? "1" : "0",
+		     NULL);
+	rc = run_hooks("post-checkout", &opt);
+	run_hooks_opt_clear(&opt);
+	return rc;
 }
 
 static int update_some(const struct object_id *oid, struct strbuf *base,
diff --git a/builtin/clone.c b/builtin/clone.c
index a0841923cf..307336e576 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -32,6 +32,7 @@
 #include "connected.h"
 #include "packfile.h"
 #include "list-objects-filter-options.h"
+#include "hook.h"
 
 /*
  * Overall FIXMEs:
@@ -771,6 +772,7 @@ static int checkout(int submodule_progress)
 	struct tree *tree;
 	struct tree_desc t;
 	int err = 0;
+	struct run_hooks_opt hook_opt = RUN_HOOKS_OPT_INIT_SYNC;
 
 	if (option_no_checkout)
 		return 0;
@@ -816,8 +818,9 @@ static int checkout(int submodule_progress)
 	if (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))
 		die(_("unable to write new index file"));
 
-	err |= run_hook_le(NULL, "post-checkout", oid_to_hex(&null_oid),
-			   oid_to_hex(&oid), "1", NULL);
+	strvec_pushl(&hook_opt.args, oid_to_hex(&null_oid), oid_to_hex(&oid), "1", NULL);
+	err |= run_hooks("post-checkout", &hook_opt);
+	run_hooks_opt_clear(&hook_opt);
 
 	if (!err && (option_recurse_submodules.nr > 0)) {
 		struct strvec args = STRVEC_INIT;
diff --git a/builtin/worktree.c b/builtin/worktree.c
index 197fd24a55..9a87c4c120 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -12,6 +12,7 @@
 #include "submodule.h"
 #include "utf8.h"
 #include "worktree.h"
+#include "hook.h"
 
 static const char * const worktree_usage[] = {
 	N_("git worktree add [<options>] <path> [<commit-ish>]"),
@@ -455,22 +456,19 @@ static int add_worktree(const char *path, const char *refname,
 	 * is_junk is cleared, but do return appropriate code when hook fails.
 	 */
 	if (!ret && opts->checkout) {
-		const char *hook = find_hook("post-checkout");
-		if (hook) {
-			const char *env[] = { "GIT_DIR", "GIT_WORK_TREE", NULL };
-			cp.git_cmd = 0;
-			cp.no_stdin = 1;
-			cp.stdout_to_stderr = 1;
-			cp.dir = path;
-			cp.env = env;
-			cp.argv = NULL;
-			cp.trace2_hook_name = "post-checkout";
-			strvec_pushl(&cp.args, absolute_path(hook),
-				     oid_to_hex(&null_oid),
-				     oid_to_hex(&commit->object.oid),
-				     "1", NULL);
-			ret = run_command(&cp);
-		}
+		struct run_hooks_opt opt = RUN_HOOKS_OPT_INIT_SYNC;
+
+		strvec_pushl(&opt.env, "GIT_DIR", "GIT_WORK_TREE", NULL);
+		strvec_pushl(&opt.args,
+			     oid_to_hex(&null_oid),
+			     oid_to_hex(&commit->object.oid),
+			     "1",
+			     NULL);
+		opt.dir = path;
+
+		ret = run_hooks("post-checkout", &opt);
+
+		run_hooks_opt_clear(&opt);
 	}
 
 	strvec_clear(&child_env);
diff --git a/reset.c b/reset.c
index 2f4fbd07c5..e6bfaf67e1 100644
--- a/reset.c
+++ b/reset.c
@@ -7,6 +7,7 @@
 #include "tree-walk.h"
 #include "tree.h"
 #include "unpack-trees.h"
+#include "hook.h"
 
 int reset_head(struct repository *r, struct object_id *oid, const char *action,
 	       const char *switch_to_branch, unsigned flags,
@@ -126,10 +127,16 @@ int reset_head(struct repository *r, struct object_id *oid, const char *action,
 			ret = create_symref("HEAD", switch_to_branch,
 					    reflog_head);
 	}
-	if (run_hook)
-		run_hook_le(NULL, "post-checkout",
-			    oid_to_hex(orig ? orig : &null_oid),
-			    oid_to_hex(oid), "1", NULL);
+	if (run_hook) {
+		struct run_hooks_opt opt = RUN_HOOKS_OPT_INIT_SYNC;
+		strvec_pushl(&opt.args,
+			     oid_to_hex(orig ? orig : &null_oid),
+			     oid_to_hex(oid),
+			     "1",
+			     NULL);
+		run_hooks("post-checkout", &opt);
+		run_hooks_opt_clear(&opt);
+	}
 
 leave_reset_head:
 	strbuf_release(&msg);
-- 
2.28.0.rc0.142.g3c755180ce-goog


^ permalink raw reply related	[flat|nested] 53+ messages in thread

* [PATCH 10/17] hook: convert 'post-rewrite' hook to config
  2020-12-05  1:49 ` [PATCH 00/17] use config-based hooks (config-based hooks part Emily Shaffer
                     ` (8 preceding siblings ...)
  2020-12-05  1:49   ` [PATCH 09/17] hooks: convert 'post-checkout' hook to hook library Emily Shaffer
@ 2020-12-05  1:49   ` Emily Shaffer
  2020-12-08 23:02     ` Josh Steadmon
  2020-12-05  1:49   ` [PATCH 11/17] transport: convert pre-push hook to use config Emily Shaffer
                     ` (7 subsequent siblings)
  17 siblings, 1 reply; 53+ messages in thread
From: Emily Shaffer @ 2020-12-05  1:49 UTC (permalink / raw)
  To: git; +Cc: Emily Shaffer

By using 'hook.h' for 'post-rewrite', we simplify hook invocations by
not needing to put together our own 'struct child_process' and we also
learn to run hooks specified in the config as well as the hook dir.

Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
---
 Documentation/githooks.txt |  2 +
 builtin/am.c               | 18 +++------
 sequencer.c                | 76 +++++++++++++++-----------------------
 3 files changed, 36 insertions(+), 60 deletions(-)

diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
index 14035ef725..db290984f6 100644
--- a/Documentation/githooks.txt
+++ b/Documentation/githooks.txt
@@ -585,6 +585,8 @@ The hook always runs after the automatic note copying (see
 "notes.rewrite.<command>" in linkgit:git-config[1]) has happened, and
 thus has access to these notes.
 
+Hooks run during 'post-rewrite' will be run in parallel by default.
+
 The following command-specific comments apply:
 
 rebase::
diff --git a/builtin/am.c b/builtin/am.c
index 22d147bc19..c7cad2cb32 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -449,23 +449,15 @@ static int run_applypatch_msg_hook(struct am_state *state)
  */
 static int run_post_rewrite_hook(const struct am_state *state)
 {
-	struct child_process cp = CHILD_PROCESS_INIT;
-	const char *hook = find_hook("post-rewrite");
+	struct run_hooks_opt opt = RUN_HOOKS_OPT_INIT_ASYNC;
 	int ret;
 
-	if (!hook)
-		return 0;
-
-	strvec_push(&cp.args, hook);
-	strvec_push(&cp.args, "rebase");
+	strvec_push(&opt.args, "rebase");
+	opt.path_to_stdin = am_path(state, "rewritten");
 
-	cp.in = xopen(am_path(state, "rewritten"), O_RDONLY);
-	cp.stdout_to_stderr = 1;
-	cp.trace2_hook_name = "post-rewrite";
+	ret = run_hooks("post-rewrite", &opt);
 
-	ret = run_command(&cp);
-
-	close(cp.in);
+	run_hooks_opt_clear(&opt);
 	return ret;
 }
 
diff --git a/sequencer.c b/sequencer.c
index 5a98fd2fbc..4befd862ff 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -35,6 +35,7 @@
 #include "rebase-interactive.h"
 #include "reset.h"
 #include "hook.h"
+#include "string-list.h"
 
 #define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION"
 
@@ -1143,33 +1144,23 @@ int update_head_with_reflog(const struct commit *old_head,
 static int run_rewrite_hook(const struct object_id *oldoid,
 			    const struct object_id *newoid)
 {
-	struct child_process proc = CHILD_PROCESS_INIT;
-	const char *argv[3];
+	struct run_hooks_opt opt = RUN_HOOKS_OPT_INIT_ASYNC;
+	struct strbuf tmp = STRBUF_INIT;
 	int code;
-	struct strbuf sb = STRBUF_INIT;
 
-	argv[0] = find_hook("post-rewrite");
-	if (!argv[0])
-		return 0;
+	strvec_push(&opt.args, "amend");
 
-	argv[1] = "amend";
-	argv[2] = NULL;
-
-	proc.argv = argv;
-	proc.in = -1;
-	proc.stdout_to_stderr = 1;
-	proc.trace2_hook_name = "post-rewrite";
-
-	code = start_command(&proc);
-	if (code)
-		return code;
-	strbuf_addf(&sb, "%s %s\n", oid_to_hex(oldoid), oid_to_hex(newoid));
-	sigchain_push(SIGPIPE, SIG_IGN);
-	write_in_full(proc.in, sb.buf, sb.len);
-	close(proc.in);
-	strbuf_release(&sb);
-	sigchain_pop(SIGPIPE);
-	return finish_command(&proc);
+	strbuf_addf(&tmp,
+		    "%s %s",
+		    oid_to_hex(oldoid),
+		    oid_to_hex(newoid));
+	string_list_append(&opt.str_stdin, tmp.buf);
+
+	code = run_hooks("post-rewrite", &opt);
+
+	run_hooks_opt_clear(&opt);
+	strbuf_release(&tmp);
+	return code;
 }
 
 void commit_post_rewrite(struct repository *r,
@@ -4317,30 +4308,21 @@ static int pick_commits(struct repository *r,
 		flush_rewritten_pending();
 		if (!stat(rebase_path_rewritten_list(), &st) &&
 				st.st_size > 0) {
-			struct child_process child = CHILD_PROCESS_INIT;
-			const char *post_rewrite_hook =
-				find_hook("post-rewrite");
-
-			child.in = open(rebase_path_rewritten_list(), O_RDONLY);
-			child.git_cmd = 1;
-			strvec_push(&child.args, "notes");
-			strvec_push(&child.args, "copy");
-			strvec_push(&child.args, "--for-rewrite=rebase");
+			struct child_process notes_cp = CHILD_PROCESS_INIT;
+			struct run_hooks_opt hook_opt = RUN_HOOKS_OPT_INIT_ASYNC;
+
+			notes_cp.in = open(rebase_path_rewritten_list(), O_RDONLY);
+			notes_cp.git_cmd = 1;
+			strvec_push(&notes_cp.args, "notes");
+			strvec_push(&notes_cp.args, "copy");
+			strvec_push(&notes_cp.args, "--for-rewrite=rebase");
 			/* we don't care if this copying failed */
-			run_command(&child);
-
-			if (post_rewrite_hook) {
-				struct child_process hook = CHILD_PROCESS_INIT;
-
-				hook.in = open(rebase_path_rewritten_list(),
-					O_RDONLY);
-				hook.stdout_to_stderr = 1;
-				hook.trace2_hook_name = "post-rewrite";
-				strvec_push(&hook.args, post_rewrite_hook);
-				strvec_push(&hook.args, "rebase");
-				/* we don't care if this hook failed */
-				run_command(&hook);
-			}
+			run_command(&notes_cp);
+
+			hook_opt.path_to_stdin = rebase_path_rewritten_list();
+			strvec_push(&hook_opt.args, "rebase");
+			run_hooks("post-rewrite", &hook_opt);
+			run_hooks_opt_clear(&hook_opt);
 		}
 		apply_autostash(rebase_path_autostash());
 
-- 
2.28.0.rc0.142.g3c755180ce-goog


^ permalink raw reply related	[flat|nested] 53+ messages in thread

* [PATCH 11/17] transport: convert pre-push hook to use config
  2020-12-05  1:49 ` [PATCH 00/17] use config-based hooks (config-based hooks part Emily Shaffer
                     ` (9 preceding siblings ...)
  2020-12-05  1:49   ` [PATCH 10/17] hook: convert 'post-rewrite' hook to config Emily Shaffer
@ 2020-12-05  1:49   ` Emily Shaffer
  2020-12-05  1:49   ` [PATCH 12/17] reference-transaction: look for hooks in config Emily Shaffer
                     ` (6 subsequent siblings)
  17 siblings, 0 replies; 53+ messages in thread
From: Emily Shaffer @ 2020-12-05  1:49 UTC (permalink / raw)
  To: git; +Cc: Emily Shaffer

By using the hook.h:run_hooks API, pre-push hooks can be specified in
the config as well as in the hookdir.

Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
---
 Documentation/githooks.txt |  2 ++
 transport.c                | 55 +++++++++-----------------------------
 2 files changed, 14 insertions(+), 43 deletions(-)

diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
index db290984f6..8f5524055b 100644
--- a/Documentation/githooks.txt
+++ b/Documentation/githooks.txt
@@ -271,6 +271,8 @@ If this hook exits with a non-zero status, `git push` will abort without
 pushing anything.  Information about why the push is rejected may be sent
 to the user by writing to standard error.
 
+Hooks executed during 'pre-push' will run in parallel by default.
+
 [[pre-receive]]
 pre-receive
 ~~~~~~~~~~~
diff --git a/transport.c b/transport.c
index 47da955e4f..6c67bc0fea 100644
--- a/transport.c
+++ b/transport.c
@@ -22,6 +22,7 @@
 #include "protocol.h"
 #include "object-store.h"
 #include "color.h"
+#include "hook.h"
 
 static int transport_use_color = -1;
 static char transport_colors[][COLOR_MAXLEN] = {
@@ -1162,31 +1163,13 @@ static void die_with_unpushed_submodules(struct string_list *needs_pushing)
 static int run_pre_push_hook(struct transport *transport,
 			     struct ref *remote_refs)
 {
-	int ret = 0, x;
+	int ret = 0;
+	struct run_hooks_opt opt = RUN_HOOKS_OPT_INIT_ASYNC;
+	struct strbuf tmp = STRBUF_INIT;
 	struct ref *r;
-	struct child_process proc = CHILD_PROCESS_INIT;
-	struct strbuf buf;
-	const char *argv[4];
-
-	if (!(argv[0] = find_hook("pre-push")))
-		return 0;
-
-	argv[1] = transport->remote->name;
-	argv[2] = transport->url;
-	argv[3] = NULL;
-
-	proc.argv = argv;
-	proc.in = -1;
-	proc.trace2_hook_name = "pre-push";
-
-	if (start_command(&proc)) {
-		finish_command(&proc);
-		return -1;
-	}
 
-	sigchain_push(SIGPIPE, SIG_IGN);
-
-	strbuf_init(&buf, 256);
+	strvec_push(&opt.args, transport->remote->name);
+	strvec_push(&opt.args, transport->url);
 
 	for (r = remote_refs; r; r = r->next) {
 		if (!r->peer_ref) continue;
@@ -1195,30 +1178,16 @@ static int run_pre_push_hook(struct transport *transport,
 		if (r->status == REF_STATUS_REJECT_REMOTE_UPDATED) continue;
 		if (r->status == REF_STATUS_UPTODATE) continue;
 
-		strbuf_reset(&buf);
-		strbuf_addf( &buf, "%s %s %s %s\n",
+		strbuf_reset(&tmp);
+		strbuf_addf(&tmp, "%s %s %s %s",
 			 r->peer_ref->name, oid_to_hex(&r->new_oid),
 			 r->name, oid_to_hex(&r->old_oid));
-
-		if (write_in_full(proc.in, buf.buf, buf.len) < 0) {
-			/* We do not mind if a hook does not read all refs. */
-			if (errno != EPIPE)
-				ret = -1;
-			break;
-		}
+		string_list_append(&opt.str_stdin, tmp.buf);
 	}
 
-	strbuf_release(&buf);
-
-	x = close(proc.in);
-	if (!ret)
-		ret = x;
-
-	sigchain_pop(SIGPIPE);
-
-	x = finish_command(&proc);
-	if (!ret)
-		ret = x;
+	ret = run_hooks("pre-push", &opt);
+	run_hooks_opt_clear(&opt);
+	strbuf_release(&tmp);
 
 	return ret;
 }
-- 
2.28.0.rc0.142.g3c755180ce-goog


^ permalink raw reply related	[flat|nested] 53+ messages in thread

* [PATCH 12/17] reference-transaction: look for hooks in config
  2020-12-05  1:49 ` [PATCH 00/17] use config-based hooks (config-based hooks part Emily Shaffer
                     ` (10 preceding siblings ...)
  2020-12-05  1:49   ` [PATCH 11/17] transport: convert pre-push hook to use config Emily Shaffer
@ 2020-12-05  1:49   ` Emily Shaffer
  2020-12-05  1:49   ` [PATCH 13/17] receive-pack: convert 'update' hook to hook.h Emily Shaffer
                     ` (5 subsequent siblings)
  17 siblings, 0 replies; 53+ messages in thread
From: Emily Shaffer @ 2020-12-05  1:49 UTC (permalink / raw)
  To: git; +Cc: Emily Shaffer

By using the hook.h library, reference-transaction hooks can be
specified in the config instead.

The expected output of the test is not fully updated to reflect the
absolute path of the hook called because the 'update' hook has not yet
been converted to use hook.h.

Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
---
 Documentation/githooks.txt       |  2 ++
 refs.c                           | 33 +++++++-------------------------
 t/t1416-ref-transaction-hooks.sh |  8 ++++----
 3 files changed, 13 insertions(+), 30 deletions(-)

diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
index 8f5524055b..3a35500132 100644
--- a/Documentation/githooks.txt
+++ b/Documentation/githooks.txt
@@ -521,6 +521,8 @@ The exit status of the hook is ignored for any state except for the
 cause the transaction to be aborted. The hook will not be called with
 "aborted" state in that case.
 
+Hooks run during 'reference-transaction' will be run in parallel by default.
+
 push-to-checkout
 ~~~~~~~~~~~~~~~~
 
diff --git a/refs.c b/refs.c
index 392f0bbf68..b67bca367e 100644
--- a/refs.c
+++ b/refs.c
@@ -18,6 +18,7 @@
 #include "strvec.h"
 #include "repository.h"
 #include "sigchain.h"
+#include "hook.h"
 
 /*
  * List of all available backends
@@ -1957,47 +1958,27 @@ int ref_update_reject_duplicates(struct string_list *refnames,
 static int run_transaction_hook(struct ref_transaction *transaction,
 				const char *state)
 {
-	struct child_process proc = CHILD_PROCESS_INIT;
 	struct strbuf buf = STRBUF_INIT;
-	const char *hook;
+	struct run_hooks_opt opt = RUN_HOOKS_OPT_INIT_ASYNC;
 	int ret = 0, i;
 
-	hook = find_hook("reference-transaction");
-	if (!hook)
-		return ret;
-
-	strvec_pushl(&proc.args, hook, state, NULL);
-	proc.in = -1;
-	proc.stdout_to_stderr = 1;
-	proc.trace2_hook_name = "reference-transaction";
-
-	ret = start_command(&proc);
-	if (ret)
-		return ret;
-
-	sigchain_push(SIGPIPE, SIG_IGN);
+	strvec_push(&opt.args, state);
 
 	for (i = 0; i < transaction->nr; i++) {
 		struct ref_update *update = transaction->updates[i];
 
 		strbuf_reset(&buf);
-		strbuf_addf(&buf, "%s %s %s\n",
+		strbuf_addf(&buf, "%s %s %s",
 			    oid_to_hex(&update->old_oid),
 			    oid_to_hex(&update->new_oid),
 			    update->refname);
-
-		if (write_in_full(proc.in, buf.buf, buf.len) < 0) {
-			if (errno != EPIPE)
-				ret = -1;
-			break;
-		}
+		string_list_append(&opt.str_stdin, buf.buf);
 	}
 
-	close(proc.in);
-	sigchain_pop(SIGPIPE);
+	ret = run_hooks("reference-transaction", &opt);
+	run_hooks_opt_clear(&opt);
 	strbuf_release(&buf);
 
-	ret |= finish_command(&proc);
 	return ret;
 }
 
diff --git a/t/t1416-ref-transaction-hooks.sh b/t/t1416-ref-transaction-hooks.sh
index f6e741c6c0..17f11f5cb0 100755
--- a/t/t1416-ref-transaction-hooks.sh
+++ b/t/t1416-ref-transaction-hooks.sh
@@ -122,11 +122,11 @@ test_expect_success 'interleaving hook calls succeed' '
 
 	cat >expect <<-EOF &&
 		hooks/update refs/tags/PRE $ZERO_OID $PRE_OID
-		hooks/reference-transaction prepared
-		hooks/reference-transaction committed
+		$(pwd)/target-repo.git/hooks/reference-transaction prepared
+		$(pwd)/target-repo.git/hooks/reference-transaction committed
 		hooks/update refs/tags/POST $ZERO_OID $POST_OID
-		hooks/reference-transaction prepared
-		hooks/reference-transaction committed
+		$(pwd)/target-repo.git/hooks/reference-transaction prepared
+		$(pwd)/target-repo.git/hooks/reference-transaction committed
 	EOF
 
 	git push ./target-repo.git PRE POST &&
-- 
2.28.0.rc0.142.g3c755180ce-goog


^ permalink raw reply related	[flat|nested] 53+ messages in thread

* [PATCH 13/17] receive-pack: convert 'update' hook to hook.h
  2020-12-05  1:49 ` [PATCH 00/17] use config-based hooks (config-based hooks part Emily Shaffer
                     ` (11 preceding siblings ...)
  2020-12-05  1:49   ` [PATCH 12/17] reference-transaction: look for hooks in config Emily Shaffer
@ 2020-12-05  1:49   ` Emily Shaffer
  2020-12-05  1:49   ` [PATCH 14/17] proc-receive: acquire hook list from hook.h Emily Shaffer
                     ` (4 subsequent siblings)
  17 siblings, 0 replies; 53+ messages in thread
From: Emily Shaffer @ 2020-12-05  1:49 UTC (permalink / raw)
  To: git; +Cc: Emily Shaffer

By using hook.h to invoke the 'update' hook, now hooks can be specified
in the config in addition to the hookdir.

Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
---
 Documentation/githooks.txt       |  2 +
 builtin/receive-pack.c           | 65 +++++++++++++++++++++-----------
 t/t1416-ref-transaction-hooks.sh |  4 +-
 3 files changed, 48 insertions(+), 23 deletions(-)

diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
index 3a35500132..2b3a74f249 100644
--- a/Documentation/githooks.txt
+++ b/Documentation/githooks.txt
@@ -359,6 +359,8 @@ The default 'update' hook, when enabled--and with
 `hooks.allowunannotated` config option unset or set to false--prevents
 unannotated tags to be pushed.
 
+Hooks executed during 'update' are run in parallel by default.
+
 [[proc-receive]]
 proc-receive
 ~~~~~~~~~~~~
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index d64a3076be..cd79ad6bdc 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -930,33 +930,56 @@ static int run_receive_hook(struct command *commands,
 	return status;
 }
 
-static int run_update_hook(struct command *cmd)
+static void hook_output_to_sideband(struct strbuf *output, void *cb_data)
 {
-	const char *argv[5];
-	struct child_process proc = CHILD_PROCESS_INIT;
-	int code;
+	int keepalive_active = 0;
 
-	argv[0] = find_hook("update");
-	if (!argv[0])
-		return 0;
+	if (keepalive_in_sec <= 0)
+		use_keepalive = KEEPALIVE_NEVER;
+	if (use_keepalive == KEEPALIVE_ALWAYS)
+		keepalive_active = 1;
 
-	argv[1] = cmd->ref_name;
-	argv[2] = oid_to_hex(&cmd->old_oid);
-	argv[3] = oid_to_hex(&cmd->new_oid);
-	argv[4] = NULL;
+	/* send a keepalive if there is no data to write */
+	if (keepalive_active && !output->len) {
+		static const char buf[] = "0005\1";
+		write_or_die(1, buf, sizeof(buf) - 1);
+		return;
+	}
 
-	proc.no_stdin = 1;
-	proc.stdout_to_stderr = 1;
-	proc.err = use_sideband ? -1 : 0;
-	proc.argv = argv;
-	proc.trace2_hook_name = "update";
+	if (use_keepalive == KEEPALIVE_AFTER_NUL && !keepalive_active) {
+		const char *first_null = memchr(output->buf, '\0', output->len);
+		if (first_null) {
+			/* The null bit is excluded. */
+			size_t before_null = first_null - output->buf;
+			size_t after_null = output->len - (before_null + 1);
+			keepalive_active = 1;
+			send_sideband(1, 2, output->buf, before_null, use_sideband);
+			send_sideband(1, 2, first_null + 1, after_null, use_sideband);
+
+			return;
+		}
+	}
+
+	send_sideband(1, 2, output->buf, output->len, use_sideband);
+}
+
+static int run_update_hook(struct command *cmd)
+{
+	struct run_hooks_opt opt = RUN_HOOKS_OPT_INIT_ASYNC;
+	int code;
+
+	strvec_pushl(&opt.args,
+		     cmd->ref_name,
+		     oid_to_hex(&cmd->old_oid),
+		     oid_to_hex(&cmd->new_oid),
+		     NULL);
 
-	code = start_command(&proc);
-	if (code)
-		return code;
 	if (use_sideband)
-		copy_to_sideband(proc.err, -1, NULL);
-	return finish_command(&proc);
+		opt.consume_sideband = hook_output_to_sideband;
+
+	code = run_hooks("update", &opt);
+	run_hooks_opt_clear(&opt);
+	return code;
 }
 
 static struct command *find_command_by_refname(struct command *list,
diff --git a/t/t1416-ref-transaction-hooks.sh b/t/t1416-ref-transaction-hooks.sh
index 17f11f5cb0..28359f6099 100755
--- a/t/t1416-ref-transaction-hooks.sh
+++ b/t/t1416-ref-transaction-hooks.sh
@@ -121,10 +121,10 @@ test_expect_success 'interleaving hook calls succeed' '
 	EOF
 
 	cat >expect <<-EOF &&
-		hooks/update refs/tags/PRE $ZERO_OID $PRE_OID
+		$(pwd)/target-repo.git/hooks/update refs/tags/PRE $ZERO_OID $PRE_OID
 		$(pwd)/target-repo.git/hooks/reference-transaction prepared
 		$(pwd)/target-repo.git/hooks/reference-transaction committed
-		hooks/update refs/tags/POST $ZERO_OID $POST_OID
+		$(pwd)/target-repo.git/hooks/update refs/tags/POST $ZERO_OID $POST_OID
 		$(pwd)/target-repo.git/hooks/reference-transaction prepared
 		$(pwd)/target-repo.git/hooks/reference-transaction committed
 	EOF
-- 
2.28.0.rc0.142.g3c755180ce-goog


^ permalink raw reply related	[flat|nested] 53+ messages in thread

* [PATCH 14/17] proc-receive: acquire hook list from hook.h
  2020-12-05  1:49 ` [PATCH 00/17] use config-based hooks (config-based hooks part Emily Shaffer
                     ` (12 preceding siblings ...)
  2020-12-05  1:49   ` [PATCH 13/17] receive-pack: convert 'update' hook to hook.h Emily Shaffer
@ 2020-12-05  1:49   ` Emily Shaffer
  2020-12-05  1:49   ` [PATCH 15/17] post-update: use hook.h library Emily Shaffer
                     ` (3 subsequent siblings)
  17 siblings, 0 replies; 53+ messages in thread
From: Emily Shaffer @ 2020-12-05  1:49 UTC (permalink / raw)
  To: git; +Cc: Emily Shaffer

The proc-receive hook differs from most other hooks Git invokes because
the hook and the parent Git process engage in bidirectional
communication via stdin/stdout. This bidirectional communication is
unsuitable for multiple hooks, whether they are in series or in
parallel, and is incompatible with run-command.h:run_processes_parallel:

- The proc-receive hook is intended to modify the state of the Git repo.
  From 'git help githooks':
    This [proc-receive] hook is responsible for updating the relevant
    references and reporting the results back to 'receive-pack'.
  This prevents parallelization and implies, at least, specific ordering
  of hook execution.
- The proc-receive hook can reject a push by aborting early with an
  error code. If a former hook ran through the entire push contents
  successfully but a later hook rejects some of the push, the repo may
  be left in a partially-updated (and corrupt) state.
- The callback model of the run_processes_parallel() API is unsuited to
  the current implementation of proc-receive, which loops through
  "send-receive-consider" with the child process. proc-receive today
  relies on stateful communication with the child process, which would be
  unwieldy to implement with callbacks and saved state.
- Additionally, run_processes_parallel() is designed to collate the
  output of many child processes into a single output (stderr or callback),
  and would require significant work to tell the caller which process sent
  the output, and indeed to collect any output before the child process
  has exited.

So, rather than using hook.h:run_hooks() to invoke the proc-receive
hook, receive-pack.c can learn to ask hook.h:hook_list() for the
location of a hook to run. This allows users to configure their
proc-receive in a global config for all repos if they want, or a local
config if they just don't want to use the hookdir. Because running more
than one proc-receive hook doesn't make sense from a repo state
perspective, we can explicitly ban configuring more than one
proc-receive hook at a time.

If a user wants to globally configure one proc-receive hook for most of
their repos, but override that hook in a single repo, they should use
'skip' to manually remove the global hook in their special repo:

~/.gitconfig:
[hook.proc-receive]
  command = /usr/bin/usual-proc-receive

~/special-repo/.git/config:
[hookcmd./usr/bin/usual-proc-receive]
  skip = true
[hook.proc-receive]
  command = /usr/bin/special-proc-receive

Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
---
 Documentation/githooks.txt                |  4 ++
 builtin/receive-pack.c                    | 33 +++++++++++++++-
 t/t5411/test-0015-too-many-hooks-error.sh | 47 +++++++++++++++++++++++
 3 files changed, 82 insertions(+), 2 deletions(-)
 create mode 100644 t/t5411/test-0015-too-many-hooks-error.sh

diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
index 2b3a74f249..2c59c537f9 100644
--- a/Documentation/githooks.txt
+++ b/Documentation/githooks.txt
@@ -423,6 +423,10 @@ the input.  The exit status of the 'proc-receive' hook only determines
 the success or failure of the group of commands sent to it, unless
 atomic push is in use.
 
+It is forbidden to specify more than one hook for 'proc-receive'. If a
+globally-configured 'proc-receive' must be overridden, use
+'hookcmd.<global-hook>.skip = true' to ignore it.
+
 [[post-receive]]
 post-receive
 ~~~~~~~~~~~~
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index cd79ad6bdc..b929a3505c 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -1137,11 +1137,40 @@ static int run_proc_receive_hook(struct command *commands,
 	int version = 0;
 	int code;
 
-	argv[0] = find_hook("proc-receive");
-	if (!argv[0]) {
+	struct strbuf hookname = STRBUF_INIT;
+	struct hook *proc_receive = NULL;
+	struct list_head *pos, *hooks;
+
+	strbuf_addstr(&hookname, "proc-receive");
+	hooks = hook_list(&hookname);
+
+	list_for_each(pos, hooks) {
+		if (proc_receive) {
+			rp_error("only one 'proc-receive' hook can be specified");
+			return -1;
+		}
+		proc_receive = list_entry(pos, struct hook, list);
+		/* check if the hookdir hook should be ignored */
+		if (proc_receive->from_hookdir) {
+			switch (configured_hookdir_opt()) {
+			case hookdir_interactive:
+			case hookdir_no:
+				proc_receive = NULL;
+				break;
+			default:
+				break;
+			}
+		}
+
+	}
+
+	if (!proc_receive) {
 		rp_error("cannot find hook 'proc-receive'");
 		return -1;
 	}
+
+
+	argv[0] = proc_receive->command.buf;
 	argv[1] = NULL;
 
 	proc.argv = argv;
diff --git a/t/t5411/test-0015-too-many-hooks-error.sh b/t/t5411/test-0015-too-many-hooks-error.sh
new file mode 100644
index 0000000000..de07b27b88
--- /dev/null
+++ b/t/t5411/test-0015-too-many-hooks-error.sh
@@ -0,0 +1,47 @@
+test_expect_success "setup too  many proc-receive hooks (ok, $PROTOCOL)" '
+	write_script "proc-receive" <<-EOF &&
+	printf >&2 "# proc-receive hook\n"
+	test-tool proc-receive -v \
+		-r "ok refs/for/master/topic"
+	EOF
+
+	git -C "$upstream" config --add "hook.proc-receive.command" proc-receive &&
+	cp proc-receive "$upstream/hooks/proc-receive"
+'
+
+# Refs of upstream : master(A)
+# Refs of workbench: master(A)  tags/v123
+# git push         :                       next(A)  refs/for/master/topic(A)
+test_expect_success "proc-receive: reject more than one configured hook" '
+	test_must_fail git -C workbench push origin \
+		HEAD:next \
+		HEAD:refs/for/master/topic \
+		>out 2>&1 &&
+	make_user_friendly_and_stable_output <out >actual &&
+	cat >expect <<-EOF &&
+	remote: # pre-receive hook
+	remote: pre-receive< <ZERO-OID> <COMMIT-A> refs/heads/next
+	remote: pre-receive< <ZERO-OID> <COMMIT-A> refs/for/master/topic
+	remote: error: only one "proc-receive" hook can be specified
+	remote: # post-receive hook
+	remote: post-receive< <ZERO-OID> <COMMIT-A> refs/heads/next
+	To <URL/of/upstream.git>
+	 * [new branch] HEAD -> next
+	 ! [remote rejected] HEAD -> refs/for/master/topic (fail to run proc-receive hook)
+	EOF
+	test_cmp expect actual &&
+	git -C "$upstream" show-ref >out &&
+	make_user_friendly_and_stable_output <out >actual &&
+	cat >expect <<-EOF &&
+	<COMMIT-A> refs/heads/master
+	<COMMIT-A> refs/heads/next
+	EOF
+	test_cmp expect actual
+'
+
+# Refs of upstream : master(A)             next(A)
+# Refs of workbench: master(A)  tags/v123
+test_expect_success "cleanup ($PROTOCOL)" '
+	git -C "$upstream" config --unset "hook.proc-receive.command" "proc-receive" &&
+	git -C "$upstream" update-ref -d refs/heads/next
+'
-- 
2.28.0.rc0.142.g3c755180ce-goog


^ permalink raw reply related	[flat|nested] 53+ messages in thread

* [PATCH 15/17] post-update: use hook.h library
  2020-12-05  1:49 ` [PATCH 00/17] use config-based hooks (config-based hooks part Emily Shaffer
                     ` (13 preceding siblings ...)
  2020-12-05  1:49   ` [PATCH 14/17] proc-receive: acquire hook list from hook.h Emily Shaffer
@ 2020-12-05  1:49   ` Emily Shaffer
  2020-12-05  1:49   ` [PATCH 16/17] receive-pack: convert receive hooks to hook.h Emily Shaffer
                     ` (2 subsequent siblings)
  17 siblings, 0 replies; 53+ messages in thread
From: Emily Shaffer @ 2020-12-05  1:49 UTC (permalink / raw)
  To: git; +Cc: Emily Shaffer

By using run_hooks() instead of run_hook_le(), 'post-update' hooks can
be specified in the config as well as the hookdir.

Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
---
 Documentation/githooks.txt |  2 ++
 builtin/receive-pack.c     | 26 +++++++-------------------
 2 files changed, 9 insertions(+), 19 deletions(-)

diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
index 2c59c537f9..1412cd5846 100644
--- a/Documentation/githooks.txt
+++ b/Documentation/githooks.txt
@@ -498,6 +498,8 @@ Both standard output and standard error output are forwarded to
 `git send-pack` on the other end, so you can simply `echo` messages
 for the user.
 
+Hooks run during 'post-update' will be run in parallel by default.
+
 reference-transaction
 ~~~~~~~~~~~~~~~~~~~~~
 
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index b929a3505c..66af00fd8c 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -1678,33 +1678,21 @@ static const char *update(struct command *cmd, struct shallow_info *si)
 static void run_update_post_hook(struct command *commands)
 {
 	struct command *cmd;
-	struct child_process proc = CHILD_PROCESS_INIT;
-	const char *hook;
-
-	hook = find_hook("post-update");
-	if (!hook)
-		return;
+	struct run_hooks_opt opt = RUN_HOOKS_OPT_INIT_ASYNC;
 
 	for (cmd = commands; cmd; cmd = cmd->next) {
 		if (cmd->error_string || cmd->did_not_exist)
 			continue;
-		if (!proc.args.nr)
-			strvec_push(&proc.args, hook);
-		strvec_push(&proc.args, cmd->ref_name);
+		strvec_push(&opt.args, cmd->ref_name);
 	}
-	if (!proc.args.nr)
+	if (!opt.args.nr)
 		return;
 
-	proc.no_stdin = 1;
-	proc.stdout_to_stderr = 1;
-	proc.err = use_sideband ? -1 : 0;
-	proc.trace2_hook_name = "post-update";
+	if (use_sideband)
+		opt.consume_sideband = hook_output_to_sideband;
 
-	if (!start_command(&proc)) {
-		if (use_sideband)
-			copy_to_sideband(proc.err, -1, NULL);
-		finish_command(&proc);
-	}
+	run_hooks("post-update", &opt);
+	run_hooks_opt_clear(&opt);
 }
 
 static void check_aliased_update_internal(struct command *cmd,
-- 
2.28.0.rc0.142.g3c755180ce-goog


^ permalink raw reply related	[flat|nested] 53+ messages in thread

* [PATCH 16/17] receive-pack: convert receive hooks to hook.h
  2020-12-05  1:49 ` [PATCH 00/17] use config-based hooks (config-based hooks part Emily Shaffer
                     ` (14 preceding siblings ...)
  2020-12-05  1:49   ` [PATCH 15/17] post-update: use hook.h library Emily Shaffer
@ 2020-12-05  1:49   ` Emily Shaffer
  2020-12-05  1:49   ` [PATCH 17/17] run-command: stop thinking about hooks Emily Shaffer
  2020-12-22  0:04   ` [PATCH v3 00/17] use config-based hooks (config-based hooks part II) Emily Shaffer
  17 siblings, 0 replies; 53+ messages in thread
From: Emily Shaffer @ 2020-12-05  1:49 UTC (permalink / raw)
  To: git; +Cc: Emily Shaffer

By using the hook.h library to run receive hooks, they can be specified
in the config as well as in the hookdir.

Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
---
 Documentation/githooks.txt |   4 +
 builtin/receive-pack.c     | 197 +++++++++++++++++--------------------
 2 files changed, 94 insertions(+), 107 deletions(-)

diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
index 1412cd5846..c450f7a27e 100644
--- a/Documentation/githooks.txt
+++ b/Documentation/githooks.txt
@@ -314,6 +314,8 @@ will be set to zero, `GIT_PUSH_OPTION_COUNT=0`.
 See the section on "Quarantine Environment" in
 linkgit:git-receive-pack[1] for some caveats.
 
+Hooks executed during 'pre-receive' will not be parallelized.
+
 [[update]]
 update
 ~~~~~~
@@ -466,6 +468,8 @@ environment variables will not be set. If the client selects
 to use push options, but doesn't transmit any, the count variable
 will be set to zero, `GIT_PUSH_OPTION_COUNT=0`.
 
+Hooks executed during 'post-receive' are run in parallel by default.
+
 [[post-update]]
 post-update
 ~~~~~~~~~~~
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 66af00fd8c..c30c8e3a55 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -740,7 +740,7 @@ static int check_cert_push_options(const struct string_list *push_options)
 	return retval;
 }
 
-static void prepare_push_cert_sha1(struct child_process *proc)
+static void prepare_push_cert_sha1(struct run_hooks_opt *opt)
 {
 	static int already_done;
 
@@ -764,110 +764,42 @@ static void prepare_push_cert_sha1(struct child_process *proc)
 		nonce_status = check_nonce(push_cert.buf, bogs);
 	}
 	if (!is_null_oid(&push_cert_oid)) {
-		strvec_pushf(&proc->env_array, "GIT_PUSH_CERT=%s",
+		strvec_pushf(&opt->env, "GIT_PUSH_CERT=%s",
 			     oid_to_hex(&push_cert_oid));
-		strvec_pushf(&proc->env_array, "GIT_PUSH_CERT_SIGNER=%s",
+		strvec_pushf(&opt->env, "GIT_PUSH_CERT_SIGNER=%s",
 			     sigcheck.signer ? sigcheck.signer : "");
-		strvec_pushf(&proc->env_array, "GIT_PUSH_CERT_KEY=%s",
+		strvec_pushf(&opt->env, "GIT_PUSH_CERT_KEY=%s",
 			     sigcheck.key ? sigcheck.key : "");
-		strvec_pushf(&proc->env_array, "GIT_PUSH_CERT_STATUS=%c",
+		strvec_pushf(&opt->env, "GIT_PUSH_CERT_STATUS=%c",
 			     sigcheck.result);
 		if (push_cert_nonce) {
-			strvec_pushf(&proc->env_array,
+			strvec_pushf(&opt->env,
 				     "GIT_PUSH_CERT_NONCE=%s",
 				     push_cert_nonce);
-			strvec_pushf(&proc->env_array,
+			strvec_pushf(&opt->env,
 				     "GIT_PUSH_CERT_NONCE_STATUS=%s",
 				     nonce_status);
 			if (nonce_status == NONCE_SLOP)
-				strvec_pushf(&proc->env_array,
+				strvec_pushf(&opt->env,
 					     "GIT_PUSH_CERT_NONCE_SLOP=%ld",
 					     nonce_stamp_slop);
 		}
 	}
 }
 
+struct receive_hook_feed_context {
+	struct command *cmd;
+	int skip_broken;
+};
+
 struct receive_hook_feed_state {
 	struct command *cmd;
 	struct ref_push_report *report;
 	int skip_broken;
 	struct strbuf buf;
-	const struct string_list *push_options;
 };
 
-typedef int (*feed_fn)(void *, const char **, size_t *);
-static int run_and_feed_hook(const char *hook_name, feed_fn feed,
-			     struct receive_hook_feed_state *feed_state)
-{
-	struct child_process proc = CHILD_PROCESS_INIT;
-	struct async muxer;
-	const char *argv[2];
-	int code;
-
-	argv[0] = find_hook(hook_name);
-	if (!argv[0])
-		return 0;
-
-	argv[1] = NULL;
-
-	proc.argv = argv;
-	proc.in = -1;
-	proc.stdout_to_stderr = 1;
-	proc.trace2_hook_name = hook_name;
-
-	if (feed_state->push_options) {
-		int i;
-		for (i = 0; i < feed_state->push_options->nr; i++)
-			strvec_pushf(&proc.env_array,
-				     "GIT_PUSH_OPTION_%d=%s", i,
-				     feed_state->push_options->items[i].string);
-		strvec_pushf(&proc.env_array, "GIT_PUSH_OPTION_COUNT=%d",
-			     feed_state->push_options->nr);
-	} else
-		strvec_pushf(&proc.env_array, "GIT_PUSH_OPTION_COUNT");
-
-	if (tmp_objdir)
-		strvec_pushv(&proc.env_array, tmp_objdir_env(tmp_objdir));
-
-	if (use_sideband) {
-		memset(&muxer, 0, sizeof(muxer));
-		muxer.proc = copy_to_sideband;
-		muxer.in = -1;
-		code = start_async(&muxer);
-		if (code)
-			return code;
-		proc.err = muxer.in;
-	}
-
-	prepare_push_cert_sha1(&proc);
-
-	code = start_command(&proc);
-	if (code) {
-		if (use_sideband)
-			finish_async(&muxer);
-		return code;
-	}
-
-	sigchain_push(SIGPIPE, SIG_IGN);
-
-	while (1) {
-		const char *buf;
-		size_t n;
-		if (feed(feed_state, &buf, &n))
-			break;
-		if (write_in_full(proc.in, buf, n) < 0)
-			break;
-	}
-	close(proc.in);
-	if (use_sideband)
-		finish_async(&muxer);
-
-	sigchain_pop(SIGPIPE);
-
-	return finish_command(&proc);
-}
-
-static int feed_receive_hook(void *state_, const char **bufp, size_t *sizep)
+static int feed_receive_hook(void *state_)
 {
 	struct receive_hook_feed_state *state = state_;
 	struct command *cmd = state->cmd;
@@ -876,9 +808,7 @@ static int feed_receive_hook(void *state_, const char **bufp, size_t *sizep)
 	       state->skip_broken && (cmd->error_string || cmd->did_not_exist))
 		cmd = cmd->next;
 	if (!cmd)
-		return -1; /* EOF */
-	if (!bufp)
-		return 0; /* OK, can feed something. */
+		return 1; /* EOF - close the pipe*/
 	strbuf_reset(&state->buf);
 	if (!state->report)
 		state->report = cmd->report;
@@ -902,32 +832,36 @@ static int feed_receive_hook(void *state_, const char **bufp, size_t *sizep)
 			    cmd->ref_name);
 		state->cmd = cmd->next;
 	}
-	if (bufp) {
-		*bufp = state->buf.buf;
-		*sizep = state->buf.len;
-	}
 	return 0;
 }
 
-static int run_receive_hook(struct command *commands,
-			    const char *hook_name,
-			    int skip_broken,
-			    const struct string_list *push_options)
+static int feed_receive_hook_cb(struct strbuf *pipe, void *pp_cb, void *pp_task_cb)
 {
-	struct receive_hook_feed_state state;
-	int status;
-
-	strbuf_init(&state.buf, 0);
-	state.cmd = commands;
-	state.skip_broken = skip_broken;
-	state.report = NULL;
-	if (feed_receive_hook(&state, NULL, NULL))
-		return 0;
-	state.cmd = commands;
-	state.push_options = push_options;
-	status = run_and_feed_hook(hook_name, feed_receive_hook, &state);
-	strbuf_release(&state.buf);
-	return status;
+	struct hook *hook = pp_task_cb;
+	struct receive_hook_feed_state *feed_state = hook->feed_pipe_cb_data;
+	int rc;
+
+	/* first-time setup */
+	if (!feed_state) {
+		struct hook_cb_data *hook_cb = pp_cb;
+		struct run_hooks_opt *opt = hook_cb->options;
+		struct receive_hook_feed_context *ctx = opt->feed_pipe_ctx;
+		if (!ctx)
+			BUG("run_hooks_opt.feed_pipe_ctx required for receive hook");
+
+		feed_state = xmalloc(sizeof(struct receive_hook_feed_state));
+		strbuf_init(&feed_state->buf, 0);
+		feed_state->cmd = ctx->cmd;
+		feed_state->skip_broken = ctx->skip_broken;
+		feed_state->report = NULL;
+
+		hook->feed_pipe_cb_data = feed_state;
+	}
+
+	rc = feed_receive_hook(feed_state);
+	if (!rc)
+		strbuf_addbuf(pipe, &feed_state->buf);
+	return rc;
 }
 
 static void hook_output_to_sideband(struct strbuf *output, void *cb_data)
@@ -963,6 +897,55 @@ static void hook_output_to_sideband(struct strbuf *output, void *cb_data)
 	send_sideband(1, 2, output->buf, output->len, use_sideband);
 }
 
+static int run_receive_hook(struct command *commands,
+			    const char *hook_name,
+			    int skip_broken,
+			    const struct string_list *push_options)
+{
+	struct run_hooks_opt opt = RUN_HOOKS_OPT_INIT_ASYNC;
+	struct receive_hook_feed_context ctx;
+	int rc;
+	struct command *iter = commands;
+
+	/* if there are no valid commands, don't invoke the hook at all. */
+	while (iter && skip_broken && (iter->error_string || iter->did_not_exist))
+		iter = iter->next;
+	if (!iter)
+		return 0;
+
+	/* pre-receive hooks should run in series as the hook updates refs */
+	if (!strcmp(hook_name, "pre-receive"))
+		opt.jobs = 1;
+
+	if (push_options) {
+		int i;
+		for (i = 0; i < push_options->nr; i++)
+			strvec_pushf(&opt.env, "GIT_PUSH_OPTION_%d=%s", i,
+				     push_options->items[i].string);
+		strvec_pushf(&opt.env, "GIT_PUSH_OPTION_COUNT=%d", push_options->nr);
+	} else
+		strvec_push(&opt.env, "GIT_PUSH_OPTION_COUNT");
+
+	if (tmp_objdir)
+		strvec_pushv(&opt.env, tmp_objdir_env(tmp_objdir));
+
+	prepare_push_cert_sha1(&opt);
+
+	/* set up sideband printer */
+	if (use_sideband)
+		opt.consume_sideband = hook_output_to_sideband;
+
+	/* set up stdin callback */
+	ctx.cmd = commands;
+	ctx.skip_broken = skip_broken;
+	opt.feed_pipe = feed_receive_hook_cb;
+	opt.feed_pipe_ctx = &ctx;
+
+	rc = run_hooks(hook_name, &opt);
+	run_hooks_opt_clear(&opt);
+	return rc;
+}
+
 static int run_update_hook(struct command *cmd)
 {
 	struct run_hooks_opt opt = RUN_HOOKS_OPT_INIT_ASYNC;
-- 
2.28.0.rc0.142.g3c755180ce-goog


^ permalink raw reply related	[flat|nested] 53+ messages in thread

* [PATCH 17/17] run-command: stop thinking about hooks
  2020-12-05  1:49 ` [PATCH 00/17] use config-based hooks (config-based hooks part Emily Shaffer
                     ` (15 preceding siblings ...)
  2020-12-05  1:49   ` [PATCH 16/17] receive-pack: convert receive hooks to hook.h Emily Shaffer
@ 2020-12-05  1:49   ` Emily Shaffer
  2020-12-22  0:04   ` [PATCH v3 00/17] use config-based hooks (config-based hooks part II) Emily Shaffer
  17 siblings, 0 replies; 53+ messages in thread
From: Emily Shaffer @ 2020-12-05  1:49 UTC (permalink / raw)
  To: git; +Cc: Emily Shaffer

hook.h has replaced all run-command.h hook-related functionality.
run-command.h:run_hooks_le/ve and find_hook are no longer used anywhere
in the codebase. So, let's delete the dead code - or, in the one case
where it's still needed, move it to an internal function in hook.c.

Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
---
 hook.c        | 39 ++++++++++++++++++++++++++++--
 run-command.c | 66 ---------------------------------------------------
 run-command.h | 24 -------------------
 3 files changed, 37 insertions(+), 92 deletions(-)

diff --git a/hook.c b/hook.c
index 78d7721b74..85fd59865d 100644
--- a/hook.c
+++ b/hook.c
@@ -197,6 +197,41 @@ static int should_include_hookdir(const char *path, enum hookdir_opt cfg)
 	}
 }
 
+static const char *find_legacy_hook(const char *name)
+{
+	static struct strbuf path = STRBUF_INIT;
+
+	strbuf_reset(&path);
+	strbuf_git_path(&path, "hooks/%s", name);
+	if (access(path.buf, X_OK) < 0) {
+		int err = errno;
+
+#ifdef STRIP_EXTENSION
+		strbuf_addstr(&path, STRIP_EXTENSION);
+		if (access(path.buf, X_OK) >= 0)
+			return path.buf;
+		if (errno == EACCES)
+			err = errno;
+#endif
+
+		if (err == EACCES && advice_ignored_hook) {
+			static struct string_list advise_given = STRING_LIST_INIT_DUP;
+
+			if (!string_list_lookup(&advise_given, name)) {
+				string_list_insert(&advise_given, name);
+				advise(_("The '%s' hook was ignored because "
+					 "it's not set as executable.\n"
+					 "You can disable this warning with "
+					 "`git config advice.ignoredHook false`."),
+				       path.buf);
+			}
+		}
+		return NULL;
+	}
+	return path.buf;
+}
+
+
 struct list_head* hook_list(const struct strbuf* hookname)
 {
 	struct strbuf hook_key = STRBUF_INIT;
@@ -214,7 +249,7 @@ struct list_head* hook_list(const struct strbuf* hookname)
 	git_config(hook_config_lookup, (void*)&cb_data);
 
 	if (have_git_dir())
-		legacy_hook_path = find_hook(hookname->buf);
+		legacy_hook_path = find_legacy_hook(hookname->buf);
 
 	/* Unconditionally add legacy hook, but annotate it. */
 	if (legacy_hook_path) {
@@ -245,7 +280,7 @@ int hook_exists(const char *hookname, enum hookdir_opt should_run_hookdir)
 	int could_run_hookdir = (should_run_hookdir == hookdir_interactive ||
 				should_run_hookdir == hookdir_warn ||
 				should_run_hookdir == hookdir_yes)
-				&& !!find_hook(hookname);
+				&& !!find_legacy_hook(hookname);
 
 	strbuf_addf(&hook_key, "hook.%s.command", hookname);
 
diff --git a/run-command.c b/run-command.c
index 0dce6bec83..16656135dd 100644
--- a/run-command.c
+++ b/run-command.c
@@ -1310,72 +1310,6 @@ int async_with_fork(void)
 #endif
 }
 
-const char *find_hook(const char *name)
-{
-	static struct strbuf path = STRBUF_INIT;
-
-	strbuf_reset(&path);
-	strbuf_git_path(&path, "hooks/%s", name);
-	if (access(path.buf, X_OK) < 0) {
-		int err = errno;
-
-#ifdef STRIP_EXTENSION
-		strbuf_addstr(&path, STRIP_EXTENSION);
-		if (access(path.buf, X_OK) >= 0)
-			return path.buf;
-		if (errno == EACCES)
-			err = errno;
-#endif
-
-		if (err == EACCES && advice_ignored_hook) {
-			static struct string_list advise_given = STRING_LIST_INIT_DUP;
-
-			if (!string_list_lookup(&advise_given, name)) {
-				string_list_insert(&advise_given, name);
-				advise(_("The '%s' hook was ignored because "
-					 "it's not set as executable.\n"
-					 "You can disable this warning with "
-					 "`git config advice.ignoredHook false`."),
-				       path.buf);
-			}
-		}
-		return NULL;
-	}
-	return path.buf;
-}
-
-int run_hook_ve(const char *const *env, const char *name, va_list args)
-{
-	struct child_process hook = CHILD_PROCESS_INIT;
-	const char *p;
-
-	p = find_hook(name);
-	if (!p)
-		return 0;
-
-	strvec_push(&hook.args, p);
-	while ((p = va_arg(args, const char *)))
-		strvec_push(&hook.args, p);
-	hook.env = env;
-	hook.no_stdin = 1;
-	hook.stdout_to_stderr = 1;
-	hook.trace2_hook_name = name;
-
-	return run_command(&hook);
-}
-
-int run_hook_le(const char *const *env, const char *name, ...)
-{
-	va_list args;
-	int ret;
-
-	va_start(args, name);
-	ret = run_hook_ve(env, name, args);
-	va_end(args);
-
-	return ret;
-}
-
 struct io_pump {
 	/* initialized by caller */
 	int fd;
diff --git a/run-command.h b/run-command.h
index 2ad8271f56..e67bd22c5a 100644
--- a/run-command.h
+++ b/run-command.h
@@ -194,30 +194,6 @@ int finish_command_in_signal(struct child_process *);
  */
 int run_command(struct child_process *);
 
-/*
- * Returns the path to the hook file, or NULL if the hook is missing
- * or disabled. Note that this points to static storage that will be
- * overwritten by further calls to find_hook and run_hook_*.
- */
-const char *find_hook(const char *name);
-
-/**
- * Run a hook.
- * The first argument is a pathname to an index file, or NULL
- * if the hook uses the default index file or no index is needed.
- * The second argument is the name of the hook.
- * The further arguments correspond to the hook arguments.
- * The last argument has to be NULL to terminate the arguments list.
- * If the hook does not exist or is not executable, the return
- * value will be zero.
- * If it is executable, the hook will be executed and the exit
- * status of the hook is returned.
- * On execution, .stdout_to_stderr and .no_stdin will be set.
- */
-LAST_ARG_MUST_BE_NULL
-int run_hook_le(const char *const *env, const char *name, ...);
-int run_hook_ve(const char *const *env, const char *name, va_list args);
-
 /*
  * Trigger an auto-gc
  */
-- 
2.28.0.rc0.142.g3c755180ce-goog


^ permalink raw reply related	[flat|nested] 53+ messages in thread

* Re: [PATCH 10/17] hook: convert 'post-rewrite' hook to config
  2020-12-05  1:49   ` [PATCH 10/17] hook: convert 'post-rewrite' hook to config Emily Shaffer
@ 2020-12-08 23:02     ` Josh Steadmon
  2020-12-15 23:42       ` Emily Shaffer
  0 siblings, 1 reply; 53+ messages in thread
From: Josh Steadmon @ 2020-12-08 23:02 UTC (permalink / raw)
  To: Emily Shaffer; +Cc: git

On 2020.12.04 17:49, Emily Shaffer wrote:
> diff --git a/sequencer.c b/sequencer.c
> index 5a98fd2fbc..4befd862ff 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -35,6 +35,7 @@
>  #include "rebase-interactive.h"
>  #include "reset.h"
>  #include "hook.h"
> +#include "string-list.h"
>  
>  #define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION"
>  
> @@ -1143,33 +1144,23 @@ int update_head_with_reflog(const struct commit *old_head,
>  static int run_rewrite_hook(const struct object_id *oldoid,
>  			    const struct object_id *newoid)
>  {
> -	struct child_process proc = CHILD_PROCESS_INIT;
> -	const char *argv[3];
> +	struct run_hooks_opt opt = RUN_HOOKS_OPT_INIT_ASYNC;
> +	struct strbuf tmp = STRBUF_INIT;
>  	int code;
> -	struct strbuf sb = STRBUF_INIT;
>  
> -	argv[0] = find_hook("post-rewrite");
> -	if (!argv[0])
> -		return 0;
> +	strvec_push(&opt.args, "amend");
>  
> -	argv[1] = "amend";
> -	argv[2] = NULL;
> -
> -	proc.argv = argv;
> -	proc.in = -1;
> -	proc.stdout_to_stderr = 1;
> -	proc.trace2_hook_name = "post-rewrite";
> -
> -	code = start_command(&proc);
> -	if (code)
> -		return code;
> -	strbuf_addf(&sb, "%s %s\n", oid_to_hex(oldoid), oid_to_hex(newoid));
> -	sigchain_push(SIGPIPE, SIG_IGN);

Here and in a few other later patches, we're removing some signal
handling that doesn't seem to be replicated in the run_hooks()
implementation. Can you add a note to the commit message about why this
is OK?

> -	write_in_full(proc.in, sb.buf, sb.len);
> -	close(proc.in);
> -	strbuf_release(&sb);
> -	sigchain_pop(SIGPIPE);
> -	return finish_command(&proc);
> +	strbuf_addf(&tmp,
> +		    "%s %s",
> +		    oid_to_hex(oldoid),
> +		    oid_to_hex(newoid));
> +	string_list_append(&opt.str_stdin, tmp.buf);
> +
> +	code = run_hooks("post-rewrite", &opt);
> +
> +	run_hooks_opt_clear(&opt);
> +	strbuf_release(&tmp);
> +	return code;
>  }
>  
>  void commit_post_rewrite(struct repository *r,
> @@ -4317,30 +4308,21 @@ static int pick_commits(struct repository *r,
>  		flush_rewritten_pending();
>  		if (!stat(rebase_path_rewritten_list(), &st) &&
>  				st.st_size > 0) {
> -			struct child_process child = CHILD_PROCESS_INIT;
> -			const char *post_rewrite_hook =
> -				find_hook("post-rewrite");
> -
> -			child.in = open(rebase_path_rewritten_list(), O_RDONLY);
> -			child.git_cmd = 1;
> -			strvec_push(&child.args, "notes");
> -			strvec_push(&child.args, "copy");
> -			strvec_push(&child.args, "--for-rewrite=rebase");
> +			struct child_process notes_cp = CHILD_PROCESS_INIT;
> +			struct run_hooks_opt hook_opt = RUN_HOOKS_OPT_INIT_ASYNC;
> +
> +			notes_cp.in = open(rebase_path_rewritten_list(), O_RDONLY);
> +			notes_cp.git_cmd = 1;
> +			strvec_push(&notes_cp.args, "notes");
> +			strvec_push(&notes_cp.args, "copy");
> +			strvec_push(&notes_cp.args, "--for-rewrite=rebase");
>  			/* we don't care if this copying failed */
> -			run_command(&child);
> -
> -			if (post_rewrite_hook) {
> -				struct child_process hook = CHILD_PROCESS_INIT;
> -
> -				hook.in = open(rebase_path_rewritten_list(),
> -					O_RDONLY);
> -				hook.stdout_to_stderr = 1;
> -				hook.trace2_hook_name = "post-rewrite";
> -				strvec_push(&hook.args, post_rewrite_hook);
> -				strvec_push(&hook.args, "rebase");
> -				/* we don't care if this hook failed */
> -				run_command(&hook);
> -			}
> +			run_command(&notes_cp);
> +
> +			hook_opt.path_to_stdin = rebase_path_rewritten_list();
> +			strvec_push(&hook_opt.args, "rebase");
> +			run_hooks("post-rewrite", &hook_opt);
> +			run_hooks_opt_clear(&hook_opt);
>  		}
>  		apply_autostash(rebase_path_autostash());
>  
> -- 
> 2.28.0.rc0.142.g3c755180ce-goog
> 

^ permalink raw reply	[flat|nested] 53+ messages in thread

* Re: [PATCH 10/17] hook: convert 'post-rewrite' hook to config
  2020-12-08 23:02     ` Josh Steadmon
@ 2020-12-15 23:42       ` Emily Shaffer
  0 siblings, 0 replies; 53+ messages in thread
From: Emily Shaffer @ 2020-12-15 23:42 UTC (permalink / raw)
  To: Josh Steadmon, git

On Tue, Dec 08, 2020 at 03:02:13PM -0800, Josh Steadmon wrote:
> 
> On 2020.12.04 17:49, Emily Shaffer wrote:
> > diff --git a/sequencer.c b/sequencer.c
> > index 5a98fd2fbc..4befd862ff 100644
> > --- a/sequencer.c
> > +++ b/sequencer.c
> > @@ -35,6 +35,7 @@
> >  #include "rebase-interactive.h"
> >  #include "reset.h"
> >  #include "hook.h"
> > +#include "string-list.h"
> >  
> >  #define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION"
> >  
> > @@ -1143,33 +1144,23 @@ int update_head_with_reflog(const struct commit *old_head,
> >  static int run_rewrite_hook(const struct object_id *oldoid,
> >  			    const struct object_id *newoid)
> >  {
> > -	struct child_process proc = CHILD_PROCESS_INIT;
> > -	const char *argv[3];
> > +	struct run_hooks_opt opt = RUN_HOOKS_OPT_INIT_ASYNC;
> > +	struct strbuf tmp = STRBUF_INIT;
> >  	int code;
> > -	struct strbuf sb = STRBUF_INIT;
> >  
> > -	argv[0] = find_hook("post-rewrite");
> > -	if (!argv[0])
> > -		return 0;
> > +	strvec_push(&opt.args, "amend");
> >  
> > -	argv[1] = "amend";
> > -	argv[2] = NULL;
> > -
> > -	proc.argv = argv;
> > -	proc.in = -1;
> > -	proc.stdout_to_stderr = 1;
> > -	proc.trace2_hook_name = "post-rewrite";
> > -
> > -	code = start_command(&proc);
> > -	if (code)
> > -		return code;
> > -	strbuf_addf(&sb, "%s %s\n", oid_to_hex(oldoid), oid_to_hex(newoid));
> > -	sigchain_push(SIGPIPE, SIG_IGN);
> 
> Here and in a few other later patches, we're removing some signal
> handling that doesn't seem to be replicated in the run_hooks()
> implementation. Can you add a note to the commit message about why this
> is OK?

Yeah, I have added it to the commit message for next rollup. I think
offline you said you found it - but this signal handling was added when
I taught run_processes_parallel() to use a callback to provide stdin. It
lives in run-command.h now.

 - Emily

^ permalink raw reply	[flat|nested] 53+ messages in thread

* Re: [PATCH 08/17] git-p4: use 'git hook' to run hooks
  2020-12-05  1:49   ` [PATCH 08/17] git-p4: use 'git hook' to run hooks Emily Shaffer
@ 2020-12-16  0:27     ` Josh Steadmon
  2020-12-16 20:19       ` Emily Shaffer
  0 siblings, 1 reply; 53+ messages in thread
From: Josh Steadmon @ 2020-12-16  0:27 UTC (permalink / raw)
  To: Emily Shaffer; +Cc: git

On 2020.12.04 17:49, Emily Shaffer wrote:
> diff --git a/git-p4.py b/git-p4.py
> index 6ae5bbfe99..73161a56d9 100755
> --- a/git-p4.py
> +++ b/git-p4.py
> @@ -208,70 +208,18 @@ def decode_path(path):
>  
>  def run_git_hook(cmd, param=[]):
>      """Execute a hook if the hook exists."""
> -    if verbose:
> -        sys.stderr.write("Looking for hook: %s\n" % cmd)
> -        sys.stderr.flush()
> -
> -    hooks_path = gitConfig("core.hooksPath")
> -    if len(hooks_path) <= 0:
> -        hooks_path = os.path.join(os.environ["GIT_DIR"], "hooks")
> -
> -    if not isinstance(param, list):
> -        param=[param]
> -
> -    # resolve hook file name, OS depdenent
> -    hook_file = os.path.join(hooks_path, cmd)
> -    if platform.system() == 'Windows':
> -        if not os.path.isfile(hook_file):
> -            # look for the file with an extension
> -            files = glob.glob(hook_file + ".*")
> -            if not files:
> -                return True
> -            files.sort()
> -            hook_file = files.pop()
> -            while hook_file.upper().endswith(".SAMPLE"):
> -                # The file is a sample hook. We don't want it
> -                if len(files) > 0:
> -                    hook_file = files.pop()
> -                else:
> -                    return True
> -
> -    if not os.path.isfile(hook_file) or not os.access(hook_file, os.X_OK):
> +    print('ESS: entering run_git_hook')

Looks like a stray debug message here?


> +    if not cmd:
>          return True
>  
> -    return run_hook_command(hook_file, param) == 0
> -
> -def run_hook_command(cmd, param):
> -    """Executes a git hook command
> -       cmd = the command line file to be executed. This can be
> -       a file that is run by OS association.
> -
> -       param = a list of parameters to pass to the cmd command
> -
> -       On windows, the extension is checked to see if it should
> -       be run with the Git for Windows Bash shell.  If there
> -       is no file extension, the file is deemed a bash shell
> -       and will be handed off to sh.exe. Otherwise, Windows
> -       will be called with the shell to handle the file assocation.
> -
> -       For non Windows operating systems, the file is called
> -       as an executable.
> -    """
> -    cli = [cmd] + param
> -    use_shell = False
> -    if platform.system() == 'Windows':
> -        (root,ext) = os.path.splitext(cmd)
> -        if ext == "":
> -            exe_path = os.environ.get("EXEPATH")
> -            if exe_path is None:
> -                exe_path = ""
> -            else:
> -                exe_path = os.path.join(exe_path, "bin")
> -            cli = [os.path.join(exe_path, "SH.EXE")] + cli
> -        else:
> -            use_shell = True
> -    return subprocess.call(cli, shell=use_shell)
> +    """args are specified with -a <arg> -a <arg> -a <arg>"""
> +    args = (['git', 'hook', 'run'] +
> +	    ["-a" + arg for arg in param] +
> +	    [cmd])
> +    print ('ESS: args:')

Here as well.


> +    print (args)
>  
> +    return subprocess.call(args) == 0
>  
>  def write_pipe(c, stdin):
>      if verbose:
> -- 
> 2.28.0.rc0.142.g3c755180ce-goog
> 

^ permalink raw reply	[flat|nested] 53+ messages in thread

* Re: [PATCH] commit: use config-based hooks (config-based hooks part II)
  2020-10-14 23:25 [PATCH] commit: use config-based hooks (config-based hooks part II) Emily Shaffer
  2020-10-16 18:34 ` Junio C Hamano
  2020-12-05  1:49 ` [PATCH 00/17] use config-based hooks (config-based hooks part Emily Shaffer
@ 2020-12-16  0:31 ` Josh Steadmon
  2 siblings, 0 replies; 53+ messages in thread
From: Josh Steadmon @ 2020-12-16  0:31 UTC (permalink / raw)
  To: Emily Shaffer; +Cc: git

On 2020.10.14 16:25, Emily Shaffer wrote:
> As part of the adoption of config-based hooks, teach run_commit_hook()
> to call hook.h instead of run-command.h. This covers 'pre-commit',
> 'commit-msg', and 'prepare-commit-msg'. Additionally, ask the hook
> library - not run-command - whether any hooks will be run, as it's
> possible hooks may exist in the config but not the hookdir.
> 
> Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
> ---

Apart from the minor issues I noted on patches 8 & 10, this looks good
to me.

Reviewed-by: Josh Steadmon <steadmon@google.com>

^ permalink raw reply	[flat|nested] 53+ messages in thread

* Re: [PATCH 08/17] git-p4: use 'git hook' to run hooks
  2020-12-16  0:27     ` Josh Steadmon
@ 2020-12-16 20:19       ` Emily Shaffer
  0 siblings, 0 replies; 53+ messages in thread
From: Emily Shaffer @ 2020-12-16 20:19 UTC (permalink / raw)
  To: Josh Steadmon, git

On Tue, Dec 15, 2020 at 04:27:12PM -0800, Josh Steadmon wrote:
> 
> On 2020.12.04 17:49, Emily Shaffer wrote:
> > +    print('ESS: entering run_git_hook')
> 
> Looks like a stray debug message here?
> 
...
> > +    print ('ESS: args:')
> 
> Here as well.


:X Have fixed locally and will include with a new reroll this week.
Oops. :)

 - Emily

^ permalink raw reply	[flat|nested] 53+ messages in thread

* [PATCH v3 00/17] use config-based hooks (config-based hooks part II)
  2020-12-05  1:49 ` [PATCH 00/17] use config-based hooks (config-based hooks part Emily Shaffer
                     ` (16 preceding siblings ...)
  2020-12-05  1:49   ` [PATCH 17/17] run-command: stop thinking about hooks Emily Shaffer
@ 2020-12-22  0:04   ` Emily Shaffer
  2020-12-22  0:04     ` [PATCH v3 01/17] commit: use config-based hooks Emily Shaffer
                       ` (19 more replies)
  17 siblings, 20 replies; 53+ messages in thread
From: Emily Shaffer @ 2020-12-22  0:04 UTC (permalink / raw)
  To: git
  Cc: Emily Shaffer, Jeff King, Junio C Hamano, James Ramsay,
	Jonathan Nieder, brian m. carlson,
	Ævar Arnfjörð Bjarmason, Phillip Wood,
	Josh Steadmon, Johannes Schindelin

Since v2:
 - Renamed 'master' to 'main' in new t5411 (proc-receive) test.
 - Removed some accidentally included debug strings.
 - Fixed a nasty bug in the reference-transaction hook conversion where calling
   'oid_to_hex()' would invalidate references to the output of earlier
   'oid_to_hex()' runs farther up the callstack. Instead, the hook callsite now
   uses 'oid_to_hex_r()'.

Another thing I wanted to do in this series but ended up not having time
for before the holidays was to figure out a way to consolidate
Documentation/githooks.txt and Documentation/git-hook.txt. My personal
preference would be to remove githooks.txt's contents, move the "Hooks"
header from there into git-hook.txt, and have 'git help githooks'/'git
help hooks' redirect to git-hook.txt; I don't have a patch to share here
because I ran out of time before vacation :) What do others envision the
documentation looking like?

Thanks!
 - Emily

CI run: https://github.com/nasamuffin/git/actions/runs/436905873

Emily Shaffer (17):
  commit: use config-based hooks
  am: convert applypatch hooks to use config
  merge: use config-based hooks for post-merge hook
  gc: use hook library for pre-auto-gc hook
  rebase: teach pre-rebase to use hook.h
  read-cache: convert post-index-change hook to use config
  receive-pack: convert push-to-checkout hook to hook.h
  git-p4: use 'git hook' to run hooks
  hooks: convert 'post-checkout' hook to hook library
  hook: convert 'post-rewrite' hook to config
  transport: convert pre-push hook to use config
  reference-transaction: look for hooks in config
  receive-pack: convert 'update' hook to hook.h
  proc-receive: acquire hook list from hook.h
  post-update: use hook.h library
  receive-pack: convert receive hooks to hook.h
  run-command: stop thinking about hooks

 Documentation/githooks.txt                    |  45 +++
 builtin/am.c                                  |  30 +-
 builtin/checkout.c                            |  16 +-
 builtin/clone.c                               |   7 +-
 builtin/commit.c                              |  11 +-
 builtin/gc.c                                  |   4 +-
 builtin/merge.c                               |  14 +-
 builtin/rebase.c                              |   7 +-
 builtin/receive-pack.c                        | 326 ++++++++++--------
 builtin/worktree.c                            |  30 +-
 commit.c                                      |  20 +-
 commit.h                                      |   3 +-
 git-p4.py                                     |  67 +---
 hook.c                                        |  39 ++-
 read-cache.c                                  |  12 +-
 refs.c                                        |  38 +-
 reset.c                                       |  15 +-
 run-command.c                                 |  66 ----
 run-command.h                                 |  24 --
 sequencer.c                                   |  83 ++---
 t/t1416-ref-transaction-hooks.sh              |  12 +-
 t/t5411/test-0015-too-many-hooks-error.sh     |  47 +++
 ...3-pre-commit-and-pre-merge-commit-hooks.sh |  17 +-
 transport.c                                   |  55 +--
 24 files changed, 493 insertions(+), 495 deletions(-)
 create mode 100644 t/t5411/test-0015-too-many-hooks-error.sh

-- 
2.28.0.rc0.142.g3c755180ce-goog


^ permalink raw reply	[flat|nested] 53+ messages in thread

* [PATCH v3 01/17] commit: use config-based hooks
  2020-12-22  0:04   ` [PATCH v3 00/17] use config-based hooks (config-based hooks part II) Emily Shaffer
@ 2020-12-22  0:04     ` Emily Shaffer
  2021-02-01 22:08       ` Junio C Hamano
  2021-02-01 23:02       ` Junio C Hamano
  2020-12-22  0:04     ` [PATCH v3 02/17] am: convert applypatch hooks to use config Emily Shaffer
                       ` (18 subsequent siblings)
  19 siblings, 2 replies; 53+ messages in thread
From: Emily Shaffer @ 2020-12-22  0:04 UTC (permalink / raw)
  To: git; +Cc: Emily Shaffer

As part of the adoption of config-based hooks, teach run_commit_hook()
to call hook.h instead of run-command.h. This covers 'pre-commit',
'commit-msg', and 'prepare-commit-msg'. Additionally, ask the hook
library - not run-command - whether any hooks will be run, as it's
possible hooks may exist in the config but not the hookdir.

Because all but 'post-commit' hooks are expected to make some state
change, force all but 'post-commit' hook to run in series. 'post-commit'
"is meant primarily for notification, and cannot affect the outcome of
`git commit`," so it is fine to run in parallel.

Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
---
 Documentation/githooks.txt                    | 10 ++++++++++
 builtin/commit.c                              | 11 +++++-----
 builtin/merge.c                               |  9 +++++----
 commit.c                                      | 20 +++++++++++++------
 commit.h                                      |  3 ++-
 sequencer.c                                   |  7 ++++---
 ...3-pre-commit-and-pre-merge-commit-hooks.sh | 17 ++++++++++++++--
 7 files changed, 56 insertions(+), 21 deletions(-)

diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
index ffccfc7760..8b352be43f 100644
--- a/Documentation/githooks.txt
+++ b/Documentation/githooks.txt
@@ -103,6 +103,8 @@ The default 'pre-commit' hook, when enabled--and with the
 `hooks.allownonascii` config option unset or set to false--prevents
 the use of non-ASCII filenames.
 
+Hooks executed during 'pre-commit' will not be parallelized.
+
 pre-merge-commit
 ~~~~~~~~~~~~~~~~
 
@@ -125,6 +127,8 @@ need to be resolved and the result committed separately (see
 linkgit:git-merge[1]). At that point, this hook will not be executed,
 but the 'pre-commit' hook will, if it is enabled.
 
+Hooks executed during 'pre-merge-commit' will not be parallelized.
+
 prepare-commit-msg
 ~~~~~~~~~~~~~~~~~~
 
@@ -150,6 +154,8 @@ be used as replacement for pre-commit hook.
 The sample `prepare-commit-msg` hook that comes with Git removes the
 help message found in the commented portion of the commit template.
 
+Hooks executed during 'prepare-commit-msg' will not be parallelized.
+
 commit-msg
 ~~~~~~~~~~
 
@@ -166,6 +172,8 @@ file.
 The default 'commit-msg' hook, when enabled, detects duplicate
 `Signed-off-by` trailers, and aborts the commit if one is found.
 
+Hooks executed during 'commit-msg' will not be parallelized.
+
 post-commit
 ~~~~~~~~~~~
 
@@ -175,6 +183,8 @@ invoked after a commit is made.
 This hook is meant primarily for notification, and cannot affect
 the outcome of `git commit`.
 
+Hooks executed during 'post-commit' will run in parallel by default.
+
 pre-rebase
 ~~~~~~~~~~
 
diff --git a/builtin/commit.c b/builtin/commit.c
index 505fe60956..f4dea2b510 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -36,6 +36,7 @@
 #include "help.h"
 #include "commit-reach.h"
 #include "commit-graph.h"
+#include "hook.h"
 
 static const char * const builtin_commit_usage[] = {
 	N_("git commit [<options>] [--] <pathspec>..."),
@@ -699,7 +700,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 	/* This checks and barfs if author is badly specified */
 	determine_author_info(author_ident);
 
-	if (!no_verify && run_commit_hook(use_editor, index_file, "pre-commit", NULL))
+	if (!no_verify && run_commit_hook(use_editor, 0, index_file, "pre-commit", NULL))
 		return 0;
 
 	if (squash_message) {
@@ -983,7 +984,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 		return 0;
 	}
 
-	if (!no_verify && find_hook("pre-commit")) {
+	if (!no_verify && hook_exists("pre-commit", configured_hookdir_opt())) {
 		/*
 		 * 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
@@ -998,7 +999,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 		return 0;
 	}
 
-	if (run_commit_hook(use_editor, index_file, "prepare-commit-msg",
+	if (run_commit_hook(use_editor, 0, index_file, "prepare-commit-msg",
 			    git_path_commit_editmsg(), hook_arg1, hook_arg2, NULL))
 		return 0;
 
@@ -1015,7 +1016,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 	}
 
 	if (!no_verify &&
-	    run_commit_hook(use_editor, index_file, "commit-msg", git_path_commit_editmsg(), NULL)) {
+	    run_commit_hook(use_editor, 0, index_file, "commit-msg", git_path_commit_editmsg(), NULL)) {
 		return 0;
 	}
 
@@ -1701,7 +1702,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 
 	repo_rerere(the_repository, 0);
 	run_auto_maintenance(quiet);
-	run_commit_hook(use_editor, get_index_file(), "post-commit", NULL);
+	run_commit_hook(use_editor, 1, get_index_file(), "post-commit", NULL);
 	if (amend && !no_post_rewrite) {
 		commit_post_rewrite(the_repository, current_head, &oid);
 	}
diff --git a/builtin/merge.c b/builtin/merge.c
index 1cff730715..d654b6923c 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -42,6 +42,7 @@
 #include "commit-reach.h"
 #include "wt-status.h"
 #include "commit-graph.h"
+#include "hook.h"
 
 #define DEFAULT_TWOHEAD (1<<0)
 #define DEFAULT_OCTOPUS (1<<1)
@@ -836,14 +837,14 @@ static void prepare_to_commit(struct commit_list *remoteheads)
 	struct strbuf msg = STRBUF_INIT;
 	const char *index_file = get_index_file();
 
-	if (!no_verify && run_commit_hook(0 < option_edit, index_file, "pre-merge-commit", NULL))
+	if (!no_verify && run_commit_hook(0 < option_edit, 0, index_file, "pre-merge-commit", NULL))
 		abort_commit(remoteheads, NULL);
 	/*
 	 * Re-read the index as pre-merge-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.
 	 */
-	if (find_hook("pre-merge-commit"))
+	if (hook_exists("pre-merge-commit", configured_hookdir_opt()))
 		discard_cache();
 	read_cache_from(index_file);
 	strbuf_addbuf(&msg, &merge_msg);
@@ -864,7 +865,7 @@ static void prepare_to_commit(struct commit_list *remoteheads)
 		append_signoff(&msg, ignore_non_trailer(msg.buf, msg.len), 0);
 	write_merge_heads(remoteheads);
 	write_file_buf(git_path_merge_msg(the_repository), msg.buf, msg.len);
-	if (run_commit_hook(0 < option_edit, get_index_file(), "prepare-commit-msg",
+	if (run_commit_hook(0 < option_edit, 0, get_index_file(), "prepare-commit-msg",
 			    git_path_merge_msg(the_repository), "merge", NULL))
 		abort_commit(remoteheads, NULL);
 	if (0 < option_edit) {
@@ -872,7 +873,7 @@ static void prepare_to_commit(struct commit_list *remoteheads)
 			abort_commit(remoteheads, NULL);
 	}
 
-	if (!no_verify && run_commit_hook(0 < option_edit, get_index_file(),
+	if (!no_verify && run_commit_hook(0 < option_edit, 0, get_index_file(),
 					  "commit-msg",
 					  git_path_merge_msg(the_repository), NULL))
 		abort_commit(remoteheads, NULL);
diff --git a/commit.c b/commit.c
index fe1fa3dc41..1bad721a20 100644
--- a/commit.c
+++ b/commit.c
@@ -21,6 +21,7 @@
 #include "commit-reach.h"
 #include "run-command.h"
 #include "shallow.h"
+#include "hook.h"
 
 static struct commit_extra_header *read_commit_extra_header_lines(const char *buf, size_t len, const char **);
 
@@ -1630,25 +1631,32 @@ size_t ignore_non_trailer(const char *buf, size_t len)
 	return boc ? len - boc : len - cutoff;
 }
 
-int run_commit_hook(int editor_is_used, const char *index_file,
+int run_commit_hook(int editor_is_used, int parallelize, const char *index_file,
 		    const char *name, ...)
 {
-	struct strvec hook_env = STRVEC_INIT;
+	struct run_hooks_opt opt = RUN_HOOKS_OPT_INIT_SYNC;
 	va_list args;
+	const char *arg;
 	int ret;
 
-	strvec_pushf(&hook_env, "GIT_INDEX_FILE=%s", index_file);
+	if (parallelize)
+		opt.jobs = configured_hook_jobs();
+
+	strvec_pushf(&opt.env, "GIT_INDEX_FILE=%s", index_file);
 
 	/*
 	 * Let the hook know that no editor will be launched.
 	 */
 	if (!editor_is_used)
-		strvec_push(&hook_env, "GIT_EDITOR=:");
+		strvec_push(&opt.env, "GIT_EDITOR=:");
 
 	va_start(args, name);
-	ret = run_hook_ve(hook_env.v, name, args);
+	while ((arg = va_arg(args, const char *)))
+		strvec_push(&opt.args, arg);
 	va_end(args);
-	strvec_clear(&hook_env);
+
+	ret = run_hooks(name, &opt);
+	run_hooks_opt_clear(&opt);
 
 	return ret;
 }
diff --git a/commit.h b/commit.h
index 5467786c7b..dd33ead8e0 100644
--- a/commit.h
+++ b/commit.h
@@ -352,6 +352,7 @@ int compare_commits_by_commit_date(const void *a_, const void *b_, void *unused)
 int compare_commits_by_gen_then_commit_date(const void *a_, const void *b_, void *unused);
 
 LAST_ARG_MUST_BE_NULL
-int run_commit_hook(int editor_is_used, const char *index_file, const char *name, ...);
+int run_commit_hook(int editor_is_used, int parallelize, const char *index_file,
+		    const char *name, ...);
 
 #endif /* COMMIT_H */
diff --git a/sequencer.c b/sequencer.c
index 8909a46770..5a98fd2fbc 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -34,6 +34,7 @@
 #include "commit-reach.h"
 #include "rebase-interactive.h"
 #include "reset.h"
+#include "hook.h"
 
 #define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION"
 
@@ -1203,7 +1204,7 @@ static int run_prepare_commit_msg_hook(struct repository *r,
 	} else {
 		arg1 = "message";
 	}
-	if (run_commit_hook(0, r->index_file, "prepare-commit-msg", name,
+	if (run_commit_hook(0, 0, r->index_file, "prepare-commit-msg", name,
 			    arg1, arg2, NULL))
 		ret = error(_("'prepare-commit-msg' hook failed"));
 
@@ -1438,7 +1439,7 @@ static int try_to_commit(struct repository *r,
 		}
 	}
 
-	if (find_hook("prepare-commit-msg")) {
+	if (hook_exists("prepare-commit-msg", configured_hookdir_opt())) {
 		res = run_prepare_commit_msg_hook(r, msg, hook_commit);
 		if (res)
 			goto out;
@@ -1528,7 +1529,7 @@ static int try_to_commit(struct repository *r,
 		goto out;
 	}
 
-	run_commit_hook(0, r->index_file, "post-commit", NULL);
+	run_commit_hook(0, 1, r->index_file, "post-commit", NULL);
 	if (flags & AMEND_MSG)
 		commit_post_rewrite(r, current_head, oid);
 
diff --git a/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh b/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh
index b3485450a2..fc93bc3d23 100755
--- a/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh
+++ b/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh
@@ -5,8 +5,8 @@ test_description='pre-commit and pre-merge-commit hooks'
 . ./test-lib.sh
 
 HOOKDIR="$(git rev-parse --git-dir)/hooks"
-PRECOMMIT="$HOOKDIR/pre-commit"
-PREMERGE="$HOOKDIR/pre-merge-commit"
+PRECOMMIT="$(pwd)/$HOOKDIR/pre-commit"
+PREMERGE="$(pwd)/$HOOKDIR/pre-merge-commit"
 
 # Prepare sample scripts that write their $0 to actual_hooks
 test_expect_success 'sample script setup' '
@@ -103,6 +103,19 @@ test_expect_success 'with succeeding hook' '
 	test_cmp expected_hooks actual_hooks
 '
 
+# NEEDSWORK: when 'git hook add' and 'git hook remove' have been added, use that
+# instead
+test_expect_success 'with succeeding hook (config-based)' '
+	test_when_finished "git config --unset hook.pre-commit.command success.sample" &&
+	test_when_finished "rm -f expected_hooks actual_hooks" &&
+	git config hook.pre-commit.command "$HOOKDIR/success.sample" &&
+	echo "$HOOKDIR/success.sample" >expected_hooks &&
+	echo "more" >>file &&
+	git add file &&
+	git commit -m "more" &&
+	test_cmp expected_hooks actual_hooks
+'
+
 test_expect_success 'with succeeding hook (merge)' '
 	test_when_finished "rm -f \"$PREMERGE\" expected_hooks actual_hooks" &&
 	cp "$HOOKDIR/success.sample" "$PREMERGE" &&
-- 
2.28.0.rc0.142.g3c755180ce-goog


^ permalink raw reply related	[flat|nested] 53+ messages in thread

* [PATCH v3 02/17] am: convert applypatch hooks to use config
  2020-12-22  0:04   ` [PATCH v3 00/17] use config-based hooks (config-based hooks part II) Emily Shaffer
  2020-12-22  0:04     ` [PATCH v3 01/17] commit: use config-based hooks Emily Shaffer
@ 2020-12-22  0:04     ` Emily Shaffer
  2021-02-01 22:05       ` Junio C Hamano
  2020-12-22  0:04     ` [PATCH v3 03/17] merge: use config-based hooks for post-merge hook Emily Shaffer
                       ` (17 subsequent siblings)
  19 siblings, 1 reply; 53+ messages in thread
From: Emily Shaffer @ 2020-12-22  0:04 UTC (permalink / raw)
  To: git; +Cc: Emily Shaffer

Teach pre-applypatch, post-applypatch, and applypatch-msg to use the
hook.h library instead of the run-command.h library. This enables use of
hooks specified in the config, in addition to those in the hookdir.
These three hooks are called only by builtin/am.c.

Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
---
 Documentation/githooks.txt |  6 ++++++
 builtin/am.c               | 12 +++++++++---
 2 files changed, 15 insertions(+), 3 deletions(-)

diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
index 8b352be43f..0842cd812c 100644
--- a/Documentation/githooks.txt
+++ b/Documentation/githooks.txt
@@ -58,6 +58,8 @@ the message file.
 The default 'applypatch-msg' hook, when enabled, runs the
 'commit-msg' hook, if the latter is enabled.
 
+Hooks run during 'applypatch-msg' will not be parallelized.
+
 pre-applypatch
 ~~~~~~~~~~~~~~
 
@@ -73,6 +75,8 @@ make a commit if it does not pass certain test.
 The default 'pre-applypatch' hook, when enabled, runs the
 'pre-commit' hook, if the latter is enabled.
 
+Hooks run during 'pre-applypatch' will be run in parallel by default.
+
 post-applypatch
 ~~~~~~~~~~~~~~~
 
@@ -82,6 +86,8 @@ and is invoked after the patch is applied and a commit is made.
 This hook is meant primarily for notification, and cannot affect
 the outcome of `git am`.
 
+Hooks run during 'post-applypatch' will be run in parallel by default.
+
 pre-commit
 ~~~~~~~~~~
 
diff --git a/builtin/am.c b/builtin/am.c
index f22c73a05b..22d147bc19 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -33,6 +33,7 @@
 #include "string-list.h"
 #include "packfile.h"
 #include "repository.h"
+#include "hook.h"
 
 /**
  * Returns the length of the first line of msg.
@@ -426,9 +427,12 @@ static void am_destroy(const struct am_state *state)
 static int run_applypatch_msg_hook(struct am_state *state)
 {
 	int ret;
+	struct run_hooks_opt opt = RUN_HOOKS_OPT_INIT_SYNC;
 
 	assert(state->msg);
-	ret = run_hook_le(NULL, "applypatch-msg", am_path(state, "final-commit"), NULL);
+	strvec_push(&opt.args, am_path(state, "final-commit"));
+	ret = run_hooks("applypatch-msg", &opt);
+	run_hooks_opt_clear(&opt);
 
 	if (!ret) {
 		FREE_AND_NULL(state->msg);
@@ -1558,8 +1562,9 @@ static void do_commit(const struct am_state *state)
 	struct commit_list *parents = NULL;
 	const char *reflog_msg, *author, *committer = NULL;
 	struct strbuf sb = STRBUF_INIT;
+	struct run_hooks_opt hook_opt = RUN_HOOKS_OPT_INIT_ASYNC;
 
-	if (run_hook_le(NULL, "pre-applypatch", NULL))
+	if (run_hooks("pre-applypatch", &hook_opt))
 		exit(1);
 
 	if (write_cache_as_tree(&tree, 0, NULL))
@@ -1611,8 +1616,9 @@ static void do_commit(const struct am_state *state)
 		fclose(fp);
 	}
 
-	run_hook_le(NULL, "post-applypatch", NULL);
+	run_hooks("post-applypatch", &hook_opt);
 
+	run_hooks_opt_clear(&hook_opt);
 	strbuf_release(&sb);
 }
 
-- 
2.28.0.rc0.142.g3c755180ce-goog


^ permalink raw reply related	[flat|nested] 53+ messages in thread

* [PATCH v3 03/17] merge: use config-based hooks for post-merge hook
  2020-12-22  0:04   ` [PATCH v3 00/17] use config-based hooks (config-based hooks part II) Emily Shaffer
  2020-12-22  0:04     ` [PATCH v3 01/17] commit: use config-based hooks Emily Shaffer
  2020-12-22  0:04     ` [PATCH v3 02/17] am: convert applypatch hooks to use config Emily Shaffer
@ 2020-12-22  0:04     ` Emily Shaffer
  2020-12-22  0:04     ` [PATCH v3 04/17] gc: use hook library for pre-auto-gc hook Emily Shaffer
                       ` (16 subsequent siblings)
  19 siblings, 0 replies; 53+ messages in thread
From: Emily Shaffer @ 2020-12-22  0:04 UTC (permalink / raw)
  To: git; +Cc: Emily Shaffer

Teach post-merge to use the hook.h library instead of the run-command.h
library to run hooks. This means that post-merge hooks can come from the
config as well as from the hookdir. post-merge is invoked only from
builtin/merge.c.

Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
---
 Documentation/githooks.txt | 2 ++
 builtin/merge.c            | 5 ++++-
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
index 0842cd812c..f6ddf1aa22 100644
--- a/Documentation/githooks.txt
+++ b/Documentation/githooks.txt
@@ -236,6 +236,8 @@ save and restore any form of metadata associated with the working tree
 (e.g.: permissions/ownership, ACLS, etc).  See contrib/hooks/setgitperms.perl
 for an example of how to do this.
 
+Hooks executed during 'post-merge' will run in parallel by default.
+
 pre-push
 ~~~~~~~~
 
diff --git a/builtin/merge.c b/builtin/merge.c
index d654b6923c..717fbaa019 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -443,6 +443,7 @@ static void finish(struct commit *head_commit,
 		   const struct object_id *new_head, const char *msg)
 {
 	struct strbuf reflog_message = STRBUF_INIT;
+	struct run_hooks_opt opt = RUN_HOOKS_OPT_INIT_ASYNC;
 	const struct object_id *head = &head_commit->object.oid;
 
 	if (!msg)
@@ -484,7 +485,9 @@ static void finish(struct commit *head_commit,
 	}
 
 	/* Run a post-merge hook */
-	run_hook_le(NULL, "post-merge", squash ? "1" : "0", NULL);
+	strvec_push(&opt.args, squash ? "1" : "0");
+	run_hooks("post-merge", &opt);
+	run_hooks_opt_clear(&opt);
 
 	apply_autostash(git_path_merge_autostash(the_repository));
 	strbuf_release(&reflog_message);
-- 
2.28.0.rc0.142.g3c755180ce-goog


^ permalink raw reply related	[flat|nested] 53+ messages in thread

* [PATCH v3 04/17] gc: use hook library for pre-auto-gc hook
  2020-12-22  0:04   ` [PATCH v3 00/17] use config-based hooks (config-based hooks part II) Emily Shaffer
                       ` (2 preceding siblings ...)
  2020-12-22  0:04     ` [PATCH v3 03/17] merge: use config-based hooks for post-merge hook Emily Shaffer
@ 2020-12-22  0:04     ` Emily Shaffer
  2020-12-22  0:04     ` [PATCH v3 05/17] rebase: teach pre-rebase to use hook.h Emily Shaffer
                       ` (15 subsequent siblings)
  19 siblings, 0 replies; 53+ messages in thread
From: Emily Shaffer @ 2020-12-22  0:04 UTC (permalink / raw)
  To: git; +Cc: Emily Shaffer

Using the hook.h library instead of the run-command.h library to run
pre-auto-gc means that those hooks can be set up in config files, as
well as in the hookdir. pre-auto-gc is called only from builtin/gc.c.

Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
---
 Documentation/githooks.txt | 2 ++
 builtin/gc.c               | 4 +++-
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
index f6ddf1aa22..d74308dc20 100644
--- a/Documentation/githooks.txt
+++ b/Documentation/githooks.txt
@@ -553,6 +553,8 @@ This hook is invoked by `git gc --auto` (see linkgit:git-gc[1]). It
 takes no parameter, and exiting with non-zero status from this script
 causes the `git gc --auto` to abort.
 
+Hooks run during 'pre-auto-gc' will be run in parallel by default.
+
 post-rewrite
 ~~~~~~~~~~~~
 
diff --git a/builtin/gc.c b/builtin/gc.c
index b57fda4924..edcc873c70 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -32,6 +32,7 @@
 #include "remote.h"
 #include "object-store.h"
 #include "exec-cmd.h"
+#include "hook.h"
 
 #define FAILED_RUN "failed to run %s"
 
@@ -340,6 +341,7 @@ static void add_repack_incremental_option(void)
 
 static int need_to_gc(void)
 {
+	struct run_hooks_opt hook_opt = RUN_HOOKS_OPT_INIT_ASYNC;
 	/*
 	 * Setting gc.auto to 0 or negative can disable the
 	 * automatic gc.
@@ -386,7 +388,7 @@ static int need_to_gc(void)
 	else
 		return 0;
 
-	if (run_hook_le(NULL, "pre-auto-gc", NULL))
+	if (run_hooks("pre-auto-gc", &hook_opt))
 		return 0;
 	return 1;
 }
-- 
2.28.0.rc0.142.g3c755180ce-goog


^ permalink raw reply related	[flat|nested] 53+ messages in thread

* [PATCH v3 05/17] rebase: teach pre-rebase to use hook.h
  2020-12-22  0:04   ` [PATCH v3 00/17] use config-based hooks (config-based hooks part II) Emily Shaffer
                       ` (3 preceding siblings ...)
  2020-12-22  0:04     ` [PATCH v3 04/17] gc: use hook library for pre-auto-gc hook Emily Shaffer
@ 2020-12-22  0:04     ` Emily Shaffer
  2020-12-22  0:04     ` [PATCH v3 06/17] read-cache: convert post-index-change hook to use config Emily Shaffer
                       ` (14 subsequent siblings)
  19 siblings, 0 replies; 53+ messages in thread
From: Emily Shaffer @ 2020-12-22  0:04 UTC (permalink / raw)
  To: git; +Cc: Emily Shaffer

By using hook.h instead of run-command.h to run hooks, pre-rebase hooks
can now be specified in the config as well as in the hookdir. pre-rebase
is not called anywhere besides builtin/rebase.c.

Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
---
 Documentation/githooks.txt | 2 ++
 builtin/rebase.c           | 7 +++++--
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
index d74308dc20..5dc0690607 100644
--- a/Documentation/githooks.txt
+++ b/Documentation/githooks.txt
@@ -200,6 +200,8 @@ two parameters.  The first parameter is the upstream from which
 the series was forked.  The second parameter is the branch being
 rebased, and is not set when rebasing the current branch.
 
+Hooks executed during 'pre-rebase' will run in parallel by default.
+
 post-checkout
 ~~~~~~~~~~~~~
 
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 19c7b377aa..f61ca3e5af 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -28,6 +28,7 @@
 #include "sequencer.h"
 #include "rebase-interactive.h"
 #include "reset.h"
+#include "hook.h"
 
 #define DEFAULT_REFLOG_ACTION "rebase"
 
@@ -1312,6 +1313,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 	char *squash_onto_name = NULL;
 	int reschedule_failed_exec = -1;
 	int allow_preemptive_ff = 1;
+	struct run_hooks_opt hook_opt = RUN_HOOKS_OPT_INIT_ASYNC;
 	struct option builtin_rebase_options[] = {
 		OPT_STRING(0, "onto", &options.onto_name,
 			   N_("revision"),
@@ -2024,9 +2026,9 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 	}
 
 	/* If a hook exists, give it a chance to interrupt*/
+	strvec_pushl(&hook_opt.args, options.upstream_arg, argc ? argv[0] : NULL, NULL);
 	if (!ok_to_skip_pre_rebase &&
-	    run_hook_le(NULL, "pre-rebase", options.upstream_arg,
-			argc ? argv[0] : NULL, NULL))
+	    run_hooks("pre-rebase", &hook_opt))
 		die(_("The pre-rebase hook refused to rebase."));
 
 	if (options.flags & REBASE_DIFFSTAT) {
@@ -2106,6 +2108,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 	ret = !!run_specific_rebase(&options, action);
 
 cleanup:
+	run_hooks_opt_clear(&hook_opt);
 	strbuf_release(&buf);
 	strbuf_release(&revisions);
 	free(options.head_name);
-- 
2.28.0.rc0.142.g3c755180ce-goog


^ permalink raw reply related	[flat|nested] 53+ messages in thread

* [PATCH v3 06/17] read-cache: convert post-index-change hook to use config
  2020-12-22  0:04   ` [PATCH v3 00/17] use config-based hooks (config-based hooks part II) Emily Shaffer
                       ` (4 preceding siblings ...)
  2020-12-22  0:04     ` [PATCH v3 05/17] rebase: teach pre-rebase to use hook.h Emily Shaffer
@ 2020-12-22  0:04     ` Emily Shaffer
  2020-12-22  0:04     ` [PATCH v3 07/17] receive-pack: convert push-to-checkout hook to hook.h Emily Shaffer
                       ` (13 subsequent siblings)
  19 siblings, 0 replies; 53+ messages in thread
From: Emily Shaffer @ 2020-12-22  0:04 UTC (permalink / raw)
  To: git; +Cc: Emily Shaffer

By using hook.h instead of run-command.h to run, post-index-change hooks
can now be specified in the config in addition to the hookdir.
post-index-change is not run anywhere besides in read-cache.c.

Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
---
 Documentation/githooks.txt |  2 ++
 read-cache.c               | 12 +++++++++---
 2 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
index 5dc0690607..8249ecec5f 100644
--- a/Documentation/githooks.txt
+++ b/Documentation/githooks.txt
@@ -711,6 +711,8 @@ and "0" meaning they were not.
 Only one parameter should be set to "1" when the hook runs.  The hook
 running passing "1", "1" should not be possible.
 
+Hooks run during 'post-index-change' will be run in parallel by default.
+
 GIT
 ---
 Part of the linkgit:git[1] suite
diff --git a/read-cache.c b/read-cache.c
index ecf6f68994..dcfc080aaa 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -25,6 +25,7 @@
 #include "fsmonitor.h"
 #include "thread-utils.h"
 #include "progress.h"
+#include "hook.h"
 
 /* Mask for the name length in ce_flags in the on-disk index */
 
@@ -3052,6 +3053,7 @@ static int do_write_locked_index(struct index_state *istate, struct lock_file *l
 				 unsigned flags)
 {
 	int ret;
+	struct run_hooks_opt hook_opt = RUN_HOOKS_OPT_INIT_ASYNC;
 
 	/*
 	 * TODO trace2: replace "the_repository" with the actual repo instance
@@ -3070,9 +3072,13 @@ static int do_write_locked_index(struct index_state *istate, struct lock_file *l
 	else
 		ret = close_lock_file_gently(lock);
 
-	run_hook_le(NULL, "post-index-change",
-			istate->updated_workdir ? "1" : "0",
-			istate->updated_skipworktree ? "1" : "0", NULL);
+	strvec_pushl(&hook_opt.args,
+		     istate->updated_workdir ? "1" : "0",
+		     istate->updated_skipworktree ? "1" : "0",
+		     NULL);
+	run_hooks("post-index-change", &hook_opt);
+	run_hooks_opt_clear(&hook_opt);
+
 	istate->updated_workdir = 0;
 	istate->updated_skipworktree = 0;
 
-- 
2.28.0.rc0.142.g3c755180ce-goog


^ permalink raw reply related	[flat|nested] 53+ messages in thread

* [PATCH v3 07/17] receive-pack: convert push-to-checkout hook to hook.h
  2020-12-22  0:04   ` [PATCH v3 00/17] use config-based hooks (config-based hooks part II) Emily Shaffer
                       ` (5 preceding siblings ...)
  2020-12-22  0:04     ` [PATCH v3 06/17] read-cache: convert post-index-change hook to use config Emily Shaffer
@ 2020-12-22  0:04     ` Emily Shaffer
  2020-12-22  0:04     ` [PATCH v3 08/17] git-p4: use 'git hook' to run hooks Emily Shaffer
                       ` (12 subsequent siblings)
  19 siblings, 0 replies; 53+ messages in thread
From: Emily Shaffer @ 2020-12-22  0:04 UTC (permalink / raw)
  To: git; +Cc: Emily Shaffer

By using hook.h instead of run-command.h to invoke push-to-checkout,
hooks can now be specified in the config as well as in the hookdir.
push-to-checkout is not called anywhere but in builtin/receive-pack.c.

Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
---
 Documentation/githooks.txt |  1 +
 builtin/receive-pack.c     | 15 +++++++++++----
 2 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
index 8249ecec5f..8de512ee5d 100644
--- a/Documentation/githooks.txt
+++ b/Documentation/githooks.txt
@@ -547,6 +547,7 @@ that switches branches while
 keeping the local changes in the working tree that do not interfere
 with the difference between the branches.
 
+Hooks executed during 'push-to-checkout' will not be parallelized.
 
 pre-auto-gc
 ~~~~~~~~~~~
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index d49d050e6e..1c0bad0448 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -29,6 +29,7 @@
 #include "commit-reach.h"
 #include "worktree.h"
 #include "shallow.h"
+#include "hook.h"
 
 static const char * const receive_pack_usage[] = {
 	N_("git receive-pack <git-dir>"),
@@ -1435,12 +1436,18 @@ static const char *push_to_checkout(unsigned char *hash,
 				    struct strvec *env,
 				    const char *work_tree)
 {
+	struct run_hooks_opt opt = RUN_HOOKS_OPT_INIT_SYNC;
+
 	strvec_pushf(env, "GIT_WORK_TREE=%s", absolute_path(work_tree));
-	if (run_hook_le(env->v, push_to_checkout_hook,
-			hash_to_hex(hash), NULL))
+	strvec_pushv(&opt.env, env->v);
+	strvec_push(&opt.args, hash_to_hex(hash));
+	if (run_hooks(push_to_checkout_hook, &opt)) {
+		run_hooks_opt_clear(&opt);
 		return "push-to-checkout hook declined";
-	else
+	} else {
+		run_hooks_opt_clear(&opt);
 		return NULL;
+	}
 }
 
 static const char *update_worktree(unsigned char *sha1, const struct worktree *worktree)
@@ -1464,7 +1471,7 @@ static const char *update_worktree(unsigned char *sha1, const struct worktree *w
 
 	strvec_pushf(&env, "GIT_DIR=%s", absolute_path(git_dir));
 
-	if (!find_hook(push_to_checkout_hook))
+	if (!hook_exists(push_to_checkout_hook, configured_hookdir_opt()))
 		retval = push_to_deploy(sha1, &env, work_tree);
 	else
 		retval = push_to_checkout(sha1, &env, work_tree);
-- 
2.28.0.rc0.142.g3c755180ce-goog


^ permalink raw reply related	[flat|nested] 53+ messages in thread

* [PATCH v3 08/17] git-p4: use 'git hook' to run hooks
  2020-12-22  0:04   ` [PATCH v3 00/17] use config-based hooks (config-based hooks part II) Emily Shaffer
                       ` (6 preceding siblings ...)
  2020-12-22  0:04     ` [PATCH v3 07/17] receive-pack: convert push-to-checkout hook to hook.h Emily Shaffer
@ 2020-12-22  0:04     ` Emily Shaffer
  2020-12-22  0:04     ` [PATCH v3 09/17] hooks: convert 'post-checkout' hook to hook library Emily Shaffer
                       ` (11 subsequent siblings)
  19 siblings, 0 replies; 53+ messages in thread
From: Emily Shaffer @ 2020-12-22  0:04 UTC (permalink / raw)
  To: git; +Cc: Emily Shaffer

Instead of duplicating the behavior of run-command.h:run_hook_le() in
Python, we can directly call 'git hook run'. As a bonus, this means
git-p4 learns how to find hook specifications from the Git config as
well as from the hookdir.

Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
---

Notes:
    Maybe there is a better way to do this - I had a hard time getting this to run
    locally, and Python is not my forte, so if anybody has a better approach I'd
    love to just take that patch instead :)
    
    Since v6, removed the developer debug print statements.... :X
    
    Maybe there is a better way to do this - I had a hard time getting this to run
    locally, and Python is not my forte, so if anybody has a better approach I'd
    love to just take that patch instead :)

 git-p4.py | 67 +++++--------------------------------------------------
 1 file changed, 6 insertions(+), 61 deletions(-)

diff --git a/git-p4.py b/git-p4.py
index 6ae5bbfe99..6e23e2ad1a 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -208,70 +208,15 @@ def decode_path(path):
 
 def run_git_hook(cmd, param=[]):
     """Execute a hook if the hook exists."""
-    if verbose:
-        sys.stderr.write("Looking for hook: %s\n" % cmd)
-        sys.stderr.flush()
-
-    hooks_path = gitConfig("core.hooksPath")
-    if len(hooks_path) <= 0:
-        hooks_path = os.path.join(os.environ["GIT_DIR"], "hooks")
-
-    if not isinstance(param, list):
-        param=[param]
-
-    # resolve hook file name, OS depdenent
-    hook_file = os.path.join(hooks_path, cmd)
-    if platform.system() == 'Windows':
-        if not os.path.isfile(hook_file):
-            # look for the file with an extension
-            files = glob.glob(hook_file + ".*")
-            if not files:
-                return True
-            files.sort()
-            hook_file = files.pop()
-            while hook_file.upper().endswith(".SAMPLE"):
-                # The file is a sample hook. We don't want it
-                if len(files) > 0:
-                    hook_file = files.pop()
-                else:
-                    return True
-
-    if not os.path.isfile(hook_file) or not os.access(hook_file, os.X_OK):
+    if not cmd:
         return True
 
-    return run_hook_command(hook_file, param) == 0
-
-def run_hook_command(cmd, param):
-    """Executes a git hook command
-       cmd = the command line file to be executed. This can be
-       a file that is run by OS association.
-
-       param = a list of parameters to pass to the cmd command
-
-       On windows, the extension is checked to see if it should
-       be run with the Git for Windows Bash shell.  If there
-       is no file extension, the file is deemed a bash shell
-       and will be handed off to sh.exe. Otherwise, Windows
-       will be called with the shell to handle the file assocation.
-
-       For non Windows operating systems, the file is called
-       as an executable.
-    """
-    cli = [cmd] + param
-    use_shell = False
-    if platform.system() == 'Windows':
-        (root,ext) = os.path.splitext(cmd)
-        if ext == "":
-            exe_path = os.environ.get("EXEPATH")
-            if exe_path is None:
-                exe_path = ""
-            else:
-                exe_path = os.path.join(exe_path, "bin")
-            cli = [os.path.join(exe_path, "SH.EXE")] + cli
-        else:
-            use_shell = True
-    return subprocess.call(cli, shell=use_shell)
+    """args are specified with -a <arg> -a <arg> -a <arg>"""
+    args = (['git', 'hook', 'run'] +
+	    ["-a" + arg for arg in param] +
+	    [cmd])
 
+    return subprocess.call(args) == 0
 
 def write_pipe(c, stdin):
     if verbose:
-- 
2.28.0.rc0.142.g3c755180ce-goog


^ permalink raw reply related	[flat|nested] 53+ messages in thread

* [PATCH v3 09/17] hooks: convert 'post-checkout' hook to hook library
  2020-12-22  0:04   ` [PATCH v3 00/17] use config-based hooks (config-based hooks part II) Emily Shaffer
                       ` (7 preceding siblings ...)
  2020-12-22  0:04     ` [PATCH v3 08/17] git-p4: use 'git hook' to run hooks Emily Shaffer
@ 2020-12-22  0:04     ` Emily Shaffer
  2020-12-22  0:04     ` [PATCH v3 10/17] hook: convert 'post-rewrite' hook to config Emily Shaffer
                       ` (10 subsequent siblings)
  19 siblings, 0 replies; 53+ messages in thread
From: Emily Shaffer @ 2020-12-22  0:04 UTC (permalink / raw)
  To: git; +Cc: Emily Shaffer

By using the 'hook.h' library, 'post-checkout' hooks can now be
specified in the config as well as in the hook directory.

Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
---
 Documentation/githooks.txt |  2 ++
 builtin/checkout.c         | 16 +++++++++++-----
 builtin/clone.c            |  7 +++++--
 builtin/worktree.c         | 30 ++++++++++++++----------------
 reset.c                    | 15 +++++++++++----
 5 files changed, 43 insertions(+), 27 deletions(-)

diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
index 8de512ee5d..14035ef725 100644
--- a/Documentation/githooks.txt
+++ b/Documentation/githooks.txt
@@ -224,6 +224,8 @@ This hook can be used to perform repository validity checks, auto-display
 differences from the previous HEAD if different, or set working dir metadata
 properties.
 
+Hooks executed during 'post-checkout' will not be parallelized.
+
 post-merge
 ~~~~~~~~~~
 
diff --git a/builtin/checkout.c b/builtin/checkout.c
index 9b82119129..20966452b8 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -9,6 +9,7 @@
 #include "config.h"
 #include "diff.h"
 #include "dir.h"
+#include "hook.h"
 #include "ll-merge.h"
 #include "lockfile.h"
 #include "merge-recursive.h"
@@ -104,13 +105,18 @@ struct branch_info {
 static int post_checkout_hook(struct commit *old_commit, struct commit *new_commit,
 			      int changed)
 {
-	return run_hook_le(NULL, "post-checkout",
-			   oid_to_hex(old_commit ? &old_commit->object.oid : &null_oid),
-			   oid_to_hex(new_commit ? &new_commit->object.oid : &null_oid),
-			   changed ? "1" : "0", NULL);
+	struct run_hooks_opt opt = RUN_HOOKS_OPT_INIT_SYNC;
+	int rc;
 	/* "new_commit" can be NULL when checking out from the index before
 	   a commit exists. */
-
+	strvec_pushl(&opt.args,
+		     oid_to_hex(old_commit ? &old_commit->object.oid : &null_oid),
+		     oid_to_hex(new_commit ? &new_commit->object.oid : &null_oid),
+		     changed ? "1" : "0",
+		     NULL);
+	rc = run_hooks("post-checkout", &opt);
+	run_hooks_opt_clear(&opt);
+	return rc;
 }
 
 static int update_some(const struct object_id *oid, struct strbuf *base,
diff --git a/builtin/clone.c b/builtin/clone.c
index a0841923cf..307336e576 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -32,6 +32,7 @@
 #include "connected.h"
 #include "packfile.h"
 #include "list-objects-filter-options.h"
+#include "hook.h"
 
 /*
  * Overall FIXMEs:
@@ -771,6 +772,7 @@ static int checkout(int submodule_progress)
 	struct tree *tree;
 	struct tree_desc t;
 	int err = 0;
+	struct run_hooks_opt hook_opt = RUN_HOOKS_OPT_INIT_SYNC;
 
 	if (option_no_checkout)
 		return 0;
@@ -816,8 +818,9 @@ static int checkout(int submodule_progress)
 	if (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))
 		die(_("unable to write new index file"));
 
-	err |= run_hook_le(NULL, "post-checkout", oid_to_hex(&null_oid),
-			   oid_to_hex(&oid), "1", NULL);
+	strvec_pushl(&hook_opt.args, oid_to_hex(&null_oid), oid_to_hex(&oid), "1", NULL);
+	err |= run_hooks("post-checkout", &hook_opt);
+	run_hooks_opt_clear(&hook_opt);
 
 	if (!err && (option_recurse_submodules.nr > 0)) {
 		struct strvec args = STRVEC_INIT;
diff --git a/builtin/worktree.c b/builtin/worktree.c
index 197fd24a55..9a87c4c120 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -12,6 +12,7 @@
 #include "submodule.h"
 #include "utf8.h"
 #include "worktree.h"
+#include "hook.h"
 
 static const char * const worktree_usage[] = {
 	N_("git worktree add [<options>] <path> [<commit-ish>]"),
@@ -455,22 +456,19 @@ static int add_worktree(const char *path, const char *refname,
 	 * is_junk is cleared, but do return appropriate code when hook fails.
 	 */
 	if (!ret && opts->checkout) {
-		const char *hook = find_hook("post-checkout");
-		if (hook) {
-			const char *env[] = { "GIT_DIR", "GIT_WORK_TREE", NULL };
-			cp.git_cmd = 0;
-			cp.no_stdin = 1;
-			cp.stdout_to_stderr = 1;
-			cp.dir = path;
-			cp.env = env;
-			cp.argv = NULL;
-			cp.trace2_hook_name = "post-checkout";
-			strvec_pushl(&cp.args, absolute_path(hook),
-				     oid_to_hex(&null_oid),
-				     oid_to_hex(&commit->object.oid),
-				     "1", NULL);
-			ret = run_command(&cp);
-		}
+		struct run_hooks_opt opt = RUN_HOOKS_OPT_INIT_SYNC;
+
+		strvec_pushl(&opt.env, "GIT_DIR", "GIT_WORK_TREE", NULL);
+		strvec_pushl(&opt.args,
+			     oid_to_hex(&null_oid),
+			     oid_to_hex(&commit->object.oid),
+			     "1",
+			     NULL);
+		opt.dir = path;
+
+		ret = run_hooks("post-checkout", &opt);
+
+		run_hooks_opt_clear(&opt);
 	}
 
 	strvec_clear(&child_env);
diff --git a/reset.c b/reset.c
index 2f4fbd07c5..e6bfaf67e1 100644
--- a/reset.c
+++ b/reset.c
@@ -7,6 +7,7 @@
 #include "tree-walk.h"
 #include "tree.h"
 #include "unpack-trees.h"
+#include "hook.h"
 
 int reset_head(struct repository *r, struct object_id *oid, const char *action,
 	       const char *switch_to_branch, unsigned flags,
@@ -126,10 +127,16 @@ int reset_head(struct repository *r, struct object_id *oid, const char *action,
 			ret = create_symref("HEAD", switch_to_branch,
 					    reflog_head);
 	}
-	if (run_hook)
-		run_hook_le(NULL, "post-checkout",
-			    oid_to_hex(orig ? orig : &null_oid),
-			    oid_to_hex(oid), "1", NULL);
+	if (run_hook) {
+		struct run_hooks_opt opt = RUN_HOOKS_OPT_INIT_SYNC;
+		strvec_pushl(&opt.args,
+			     oid_to_hex(orig ? orig : &null_oid),
+			     oid_to_hex(oid),
+			     "1",
+			     NULL);
+		run_hooks("post-checkout", &opt);
+		run_hooks_opt_clear(&opt);
+	}
 
 leave_reset_head:
 	strbuf_release(&msg);
-- 
2.28.0.rc0.142.g3c755180ce-goog


^ permalink raw reply related	[flat|nested] 53+ messages in thread

* [PATCH v3 10/17] hook: convert 'post-rewrite' hook to config
  2020-12-22  0:04   ` [PATCH v3 00/17] use config-based hooks (config-based hooks part II) Emily Shaffer
                       ` (8 preceding siblings ...)
  2020-12-22  0:04     ` [PATCH v3 09/17] hooks: convert 'post-checkout' hook to hook library Emily Shaffer
@ 2020-12-22  0:04     ` Emily Shaffer
  2020-12-22  0:04     ` [PATCH v3 11/17] transport: convert pre-push hook to use config Emily Shaffer
                       ` (9 subsequent siblings)
  19 siblings, 0 replies; 53+ messages in thread
From: Emily Shaffer @ 2020-12-22  0:04 UTC (permalink / raw)
  To: git; +Cc: Emily Shaffer

By using 'hook.h' for 'post-rewrite', we simplify hook invocations by
not needing to put together our own 'struct child_process' and we also
learn to run hooks specified in the config as well as the hook dir.

The signal handling that's being removed by this commit now takes place
in run-command.h:run_processes_parallel(), so it is OK to remove them
here.

Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
---
 Documentation/githooks.txt |  2 +
 builtin/am.c               | 18 +++------
 sequencer.c                | 76 +++++++++++++++-----------------------
 3 files changed, 36 insertions(+), 60 deletions(-)

diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
index 14035ef725..db290984f6 100644
--- a/Documentation/githooks.txt
+++ b/Documentation/githooks.txt
@@ -585,6 +585,8 @@ The hook always runs after the automatic note copying (see
 "notes.rewrite.<command>" in linkgit:git-config[1]) has happened, and
 thus has access to these notes.
 
+Hooks run during 'post-rewrite' will be run in parallel by default.
+
 The following command-specific comments apply:
 
 rebase::
diff --git a/builtin/am.c b/builtin/am.c
index 22d147bc19..c7cad2cb32 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -449,23 +449,15 @@ static int run_applypatch_msg_hook(struct am_state *state)
  */
 static int run_post_rewrite_hook(const struct am_state *state)
 {
-	struct child_process cp = CHILD_PROCESS_INIT;
-	const char *hook = find_hook("post-rewrite");
+	struct run_hooks_opt opt = RUN_HOOKS_OPT_INIT_ASYNC;
 	int ret;
 
-	if (!hook)
-		return 0;
-
-	strvec_push(&cp.args, hook);
-	strvec_push(&cp.args, "rebase");
+	strvec_push(&opt.args, "rebase");
+	opt.path_to_stdin = am_path(state, "rewritten");
 
-	cp.in = xopen(am_path(state, "rewritten"), O_RDONLY);
-	cp.stdout_to_stderr = 1;
-	cp.trace2_hook_name = "post-rewrite";
+	ret = run_hooks("post-rewrite", &opt);
 
-	ret = run_command(&cp);
-
-	close(cp.in);
+	run_hooks_opt_clear(&opt);
 	return ret;
 }
 
diff --git a/sequencer.c b/sequencer.c
index 5a98fd2fbc..4befd862ff 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -35,6 +35,7 @@
 #include "rebase-interactive.h"
 #include "reset.h"
 #include "hook.h"
+#include "string-list.h"
 
 #define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION"
 
@@ -1143,33 +1144,23 @@ int update_head_with_reflog(const struct commit *old_head,
 static int run_rewrite_hook(const struct object_id *oldoid,
 			    const struct object_id *newoid)
 {
-	struct child_process proc = CHILD_PROCESS_INIT;
-	const char *argv[3];
+	struct run_hooks_opt opt = RUN_HOOKS_OPT_INIT_ASYNC;
+	struct strbuf tmp = STRBUF_INIT;
 	int code;
-	struct strbuf sb = STRBUF_INIT;
 
-	argv[0] = find_hook("post-rewrite");
-	if (!argv[0])
-		return 0;
+	strvec_push(&opt.args, "amend");
 
-	argv[1] = "amend";
-	argv[2] = NULL;
-
-	proc.argv = argv;
-	proc.in = -1;
-	proc.stdout_to_stderr = 1;
-	proc.trace2_hook_name = "post-rewrite";
-
-	code = start_command(&proc);
-	if (code)
-		return code;
-	strbuf_addf(&sb, "%s %s\n", oid_to_hex(oldoid), oid_to_hex(newoid));
-	sigchain_push(SIGPIPE, SIG_IGN);
-	write_in_full(proc.in, sb.buf, sb.len);
-	close(proc.in);
-	strbuf_release(&sb);
-	sigchain_pop(SIGPIPE);
-	return finish_command(&proc);
+	strbuf_addf(&tmp,
+		    "%s %s",
+		    oid_to_hex(oldoid),
+		    oid_to_hex(newoid));
+	string_list_append(&opt.str_stdin, tmp.buf);
+
+	code = run_hooks("post-rewrite", &opt);
+
+	run_hooks_opt_clear(&opt);
+	strbuf_release(&tmp);
+	return code;
 }
 
 void commit_post_rewrite(struct repository *r,
@@ -4317,30 +4308,21 @@ static int pick_commits(struct repository *r,
 		flush_rewritten_pending();
 		if (!stat(rebase_path_rewritten_list(), &st) &&
 				st.st_size > 0) {
-			struct child_process child = CHILD_PROCESS_INIT;
-			const char *post_rewrite_hook =
-				find_hook("post-rewrite");
-
-			child.in = open(rebase_path_rewritten_list(), O_RDONLY);
-			child.git_cmd = 1;
-			strvec_push(&child.args, "notes");
-			strvec_push(&child.args, "copy");
-			strvec_push(&child.args, "--for-rewrite=rebase");
+			struct child_process notes_cp = CHILD_PROCESS_INIT;
+			struct run_hooks_opt hook_opt = RUN_HOOKS_OPT_INIT_ASYNC;
+
+			notes_cp.in = open(rebase_path_rewritten_list(), O_RDONLY);
+			notes_cp.git_cmd = 1;
+			strvec_push(&notes_cp.args, "notes");
+			strvec_push(&notes_cp.args, "copy");
+			strvec_push(&notes_cp.args, "--for-rewrite=rebase");
 			/* we don't care if this copying failed */
-			run_command(&child);
-
-			if (post_rewrite_hook) {
-				struct child_process hook = CHILD_PROCESS_INIT;
-
-				hook.in = open(rebase_path_rewritten_list(),
-					O_RDONLY);
-				hook.stdout_to_stderr = 1;
-				hook.trace2_hook_name = "post-rewrite";
-				strvec_push(&hook.args, post_rewrite_hook);
-				strvec_push(&hook.args, "rebase");
-				/* we don't care if this hook failed */
-				run_command(&hook);
-			}
+			run_command(&notes_cp);
+
+			hook_opt.path_to_stdin = rebase_path_rewritten_list();
+			strvec_push(&hook_opt.args, "rebase");
+			run_hooks("post-rewrite", &hook_opt);
+			run_hooks_opt_clear(&hook_opt);
 		}
 		apply_autostash(rebase_path_autostash());
 
-- 
2.28.0.rc0.142.g3c755180ce-goog


^ permalink raw reply related	[flat|nested] 53+ messages in thread

* [PATCH v3 11/17] transport: convert pre-push hook to use config
  2020-12-22  0:04   ` [PATCH v3 00/17] use config-based hooks (config-based hooks part II) Emily Shaffer
                       ` (9 preceding siblings ...)
  2020-12-22  0:04     ` [PATCH v3 10/17] hook: convert 'post-rewrite' hook to config Emily Shaffer
@ 2020-12-22  0:04     ` Emily Shaffer
  2020-12-22  0:04     ` [PATCH v3 12/17] reference-transaction: look for hooks in config Emily Shaffer
                       ` (8 subsequent siblings)
  19 siblings, 0 replies; 53+ messages in thread
From: Emily Shaffer @ 2020-12-22  0:04 UTC (permalink / raw)
  To: git; +Cc: Emily Shaffer

By using the hook.h:run_hooks API, pre-push hooks can be specified in
the config as well as in the hookdir.

Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
---
 Documentation/githooks.txt |  2 ++
 transport.c                | 55 +++++++++-----------------------------
 2 files changed, 14 insertions(+), 43 deletions(-)

diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
index db290984f6..8f5524055b 100644
--- a/Documentation/githooks.txt
+++ b/Documentation/githooks.txt
@@ -271,6 +271,8 @@ If this hook exits with a non-zero status, `git push` will abort without
 pushing anything.  Information about why the push is rejected may be sent
 to the user by writing to standard error.
 
+Hooks executed during 'pre-push' will run in parallel by default.
+
 [[pre-receive]]
 pre-receive
 ~~~~~~~~~~~
diff --git a/transport.c b/transport.c
index 679a35e7c1..58fee65815 100644
--- a/transport.c
+++ b/transport.c
@@ -22,6 +22,7 @@
 #include "protocol.h"
 #include "object-store.h"
 #include "color.h"
+#include "hook.h"
 
 static int transport_use_color = -1;
 static char transport_colors[][COLOR_MAXLEN] = {
@@ -1172,31 +1173,13 @@ static void die_with_unpushed_submodules(struct string_list *needs_pushing)
 static int run_pre_push_hook(struct transport *transport,
 			     struct ref *remote_refs)
 {
-	int ret = 0, x;
+	int ret = 0;
+	struct run_hooks_opt opt = RUN_HOOKS_OPT_INIT_ASYNC;
+	struct strbuf tmp = STRBUF_INIT;
 	struct ref *r;
-	struct child_process proc = CHILD_PROCESS_INIT;
-	struct strbuf buf;
-	const char *argv[4];
-
-	if (!(argv[0] = find_hook("pre-push")))
-		return 0;
-
-	argv[1] = transport->remote->name;
-	argv[2] = transport->url;
-	argv[3] = NULL;
-
-	proc.argv = argv;
-	proc.in = -1;
-	proc.trace2_hook_name = "pre-push";
-
-	if (start_command(&proc)) {
-		finish_command(&proc);
-		return -1;
-	}
 
-	sigchain_push(SIGPIPE, SIG_IGN);
-
-	strbuf_init(&buf, 256);
+	strvec_push(&opt.args, transport->remote->name);
+	strvec_push(&opt.args, transport->url);
 
 	for (r = remote_refs; r; r = r->next) {
 		if (!r->peer_ref) continue;
@@ -1205,30 +1188,16 @@ static int run_pre_push_hook(struct transport *transport,
 		if (r->status == REF_STATUS_REJECT_REMOTE_UPDATED) continue;
 		if (r->status == REF_STATUS_UPTODATE) continue;
 
-		strbuf_reset(&buf);
-		strbuf_addf( &buf, "%s %s %s %s\n",
+		strbuf_reset(&tmp);
+		strbuf_addf(&tmp, "%s %s %s %s",
 			 r->peer_ref->name, oid_to_hex(&r->new_oid),
 			 r->name, oid_to_hex(&r->old_oid));
-
-		if (write_in_full(proc.in, buf.buf, buf.len) < 0) {
-			/* We do not mind if a hook does not read all refs. */
-			if (errno != EPIPE)
-				ret = -1;
-			break;
-		}
+		string_list_append(&opt.str_stdin, tmp.buf);
 	}
 
-	strbuf_release(&buf);
-
-	x = close(proc.in);
-	if (!ret)
-		ret = x;
-
-	sigchain_pop(SIGPIPE);
-
-	x = finish_command(&proc);
-	if (!ret)
-		ret = x;
+	ret = run_hooks("pre-push", &opt);
+	run_hooks_opt_clear(&opt);
+	strbuf_release(&tmp);
 
 	return ret;
 }
-- 
2.28.0.rc0.142.g3c755180ce-goog


^ permalink raw reply related	[flat|nested] 53+ messages in thread

* [PATCH v3 12/17] reference-transaction: look for hooks in config
  2020-12-22  0:04   ` [PATCH v3 00/17] use config-based hooks (config-based hooks part II) Emily Shaffer
                       ` (10 preceding siblings ...)
  2020-12-22  0:04     ` [PATCH v3 11/17] transport: convert pre-push hook to use config Emily Shaffer
@ 2020-12-22  0:04     ` Emily Shaffer
  2020-12-22  0:04     ` [PATCH v3 13/17] receive-pack: convert 'update' hook to hook.h Emily Shaffer
                       ` (7 subsequent siblings)
  19 siblings, 0 replies; 53+ messages in thread
From: Emily Shaffer @ 2020-12-22  0:04 UTC (permalink / raw)
  To: git; +Cc: Emily Shaffer

By using the hook.h library, reference-transaction hooks can be
specified in the config instead.

The expected output of the test is not fully updated to reflect the
absolute path of the hook called because the 'update' hook has not yet
been converted to use hook.h.

Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
---
 Documentation/githooks.txt       |  2 ++
 refs.c                           | 38 +++++++++-----------------------
 t/t1416-ref-transaction-hooks.sh |  8 +++----
 3 files changed, 17 insertions(+), 31 deletions(-)

diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
index 8f5524055b..3a35500132 100644
--- a/Documentation/githooks.txt
+++ b/Documentation/githooks.txt
@@ -521,6 +521,8 @@ The exit status of the hook is ignored for any state except for the
 cause the transaction to be aborted. The hook will not be called with
 "aborted" state in that case.
 
+Hooks run during 'reference-transaction' will be run in parallel by default.
+
 push-to-checkout
 ~~~~~~~~~~~~~~~~
 
diff --git a/refs.c b/refs.c
index 392f0bbf68..b025dc4140 100644
--- a/refs.c
+++ b/refs.c
@@ -18,6 +18,7 @@
 #include "strvec.h"
 #include "repository.h"
 #include "sigchain.h"
+#include "hook.h"
 
 /*
  * List of all available backends
@@ -1957,47 +1958,30 @@ int ref_update_reject_duplicates(struct string_list *refnames,
 static int run_transaction_hook(struct ref_transaction *transaction,
 				const char *state)
 {
-	struct child_process proc = CHILD_PROCESS_INIT;
 	struct strbuf buf = STRBUF_INIT;
-	const char *hook;
+	struct run_hooks_opt opt = RUN_HOOKS_OPT_INIT_ASYNC;
 	int ret = 0, i;
+	char o[GIT_MAX_HEXSZ + 1], n[GIT_MAX_HEXSZ + 1];
 
-	hook = find_hook("reference-transaction");
-	if (!hook)
+	if (!hook_exists("reference-transaction", configured_hookdir_opt()))
 		return ret;
 
-	strvec_pushl(&proc.args, hook, state, NULL);
-	proc.in = -1;
-	proc.stdout_to_stderr = 1;
-	proc.trace2_hook_name = "reference-transaction";
-
-	ret = start_command(&proc);
-	if (ret)
-		return ret;
-
-	sigchain_push(SIGPIPE, SIG_IGN);
+	strvec_push(&opt.args, state);
 
 	for (i = 0; i < transaction->nr; i++) {
 		struct ref_update *update = transaction->updates[i];
+		oid_to_hex_r(o, &update->old_oid);
+		oid_to_hex_r(n, &update->new_oid);
 
 		strbuf_reset(&buf);
-		strbuf_addf(&buf, "%s %s %s\n",
-			    oid_to_hex(&update->old_oid),
-			    oid_to_hex(&update->new_oid),
-			    update->refname);
-
-		if (write_in_full(proc.in, buf.buf, buf.len) < 0) {
-			if (errno != EPIPE)
-				ret = -1;
-			break;
-		}
+		strbuf_addf(&buf, "%s %s %s", o, n, update->refname);
+		string_list_append(&opt.str_stdin, buf.buf);
 	}
 
-	close(proc.in);
-	sigchain_pop(SIGPIPE);
+	ret = run_hooks("reference-transaction", &opt);
+	run_hooks_opt_clear(&opt);
 	strbuf_release(&buf);
 
-	ret |= finish_command(&proc);
 	return ret;
 }
 
diff --git a/t/t1416-ref-transaction-hooks.sh b/t/t1416-ref-transaction-hooks.sh
index f6e741c6c0..17f11f5cb0 100755
--- a/t/t1416-ref-transaction-hooks.sh
+++ b/t/t1416-ref-transaction-hooks.sh
@@ -122,11 +122,11 @@ test_expect_success 'interleaving hook calls succeed' '
 
 	cat >expect <<-EOF &&
 		hooks/update refs/tags/PRE $ZERO_OID $PRE_OID
-		hooks/reference-transaction prepared
-		hooks/reference-transaction committed
+		$(pwd)/target-repo.git/hooks/reference-transaction prepared
+		$(pwd)/target-repo.git/hooks/reference-transaction committed
 		hooks/update refs/tags/POST $ZERO_OID $POST_OID
-		hooks/reference-transaction prepared
-		hooks/reference-transaction committed
+		$(pwd)/target-repo.git/hooks/reference-transaction prepared
+		$(pwd)/target-repo.git/hooks/reference-transaction committed
 	EOF
 
 	git push ./target-repo.git PRE POST &&
-- 
2.28.0.rc0.142.g3c755180ce-goog


^ permalink raw reply related	[flat|nested] 53+ messages in thread

* [PATCH v3 13/17] receive-pack: convert 'update' hook to hook.h
  2020-12-22  0:04   ` [PATCH v3 00/17] use config-based hooks (config-based hooks part II) Emily Shaffer
                       ` (11 preceding siblings ...)
  2020-12-22  0:04     ` [PATCH v3 12/17] reference-transaction: look for hooks in config Emily Shaffer
@ 2020-12-22  0:04     ` Emily Shaffer
  2020-12-22  0:04     ` [PATCH v3 14/17] proc-receive: acquire hook list from hook.h Emily Shaffer
                       ` (6 subsequent siblings)
  19 siblings, 0 replies; 53+ messages in thread
From: Emily Shaffer @ 2020-12-22  0:04 UTC (permalink / raw)
  To: git; +Cc: Emily Shaffer

By using hook.h to invoke the 'update' hook, now hooks can be specified
in the config in addition to the hookdir.

Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
---
 Documentation/githooks.txt       |  2 +
 builtin/receive-pack.c           | 65 +++++++++++++++++++++-----------
 t/t1416-ref-transaction-hooks.sh |  4 +-
 3 files changed, 48 insertions(+), 23 deletions(-)

diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
index 3a35500132..2b3a74f249 100644
--- a/Documentation/githooks.txt
+++ b/Documentation/githooks.txt
@@ -359,6 +359,8 @@ The default 'update' hook, when enabled--and with
 `hooks.allowunannotated` config option unset or set to false--prevents
 unannotated tags to be pushed.
 
+Hooks executed during 'update' are run in parallel by default.
+
 [[proc-receive]]
 proc-receive
 ~~~~~~~~~~~~
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 1c0bad0448..57f3bb979c 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -938,33 +938,56 @@ static int run_receive_hook(struct command *commands,
 	return status;
 }
 
-static int run_update_hook(struct command *cmd)
+static void hook_output_to_sideband(struct strbuf *output, void *cb_data)
 {
-	const char *argv[5];
-	struct child_process proc = CHILD_PROCESS_INIT;
-	int code;
+	int keepalive_active = 0;
 
-	argv[0] = find_hook("update");
-	if (!argv[0])
-		return 0;
+	if (keepalive_in_sec <= 0)
+		use_keepalive = KEEPALIVE_NEVER;
+	if (use_keepalive == KEEPALIVE_ALWAYS)
+		keepalive_active = 1;
 
-	argv[1] = cmd->ref_name;
-	argv[2] = oid_to_hex(&cmd->old_oid);
-	argv[3] = oid_to_hex(&cmd->new_oid);
-	argv[4] = NULL;
+	/* send a keepalive if there is no data to write */
+	if (keepalive_active && !output->len) {
+		static const char buf[] = "0005\1";
+		write_or_die(1, buf, sizeof(buf) - 1);
+		return;
+	}
 
-	proc.no_stdin = 1;
-	proc.stdout_to_stderr = 1;
-	proc.err = use_sideband ? -1 : 0;
-	proc.argv = argv;
-	proc.trace2_hook_name = "update";
+	if (use_keepalive == KEEPALIVE_AFTER_NUL && !keepalive_active) {
+		const char *first_null = memchr(output->buf, '\0', output->len);
+		if (first_null) {
+			/* The null bit is excluded. */
+			size_t before_null = first_null - output->buf;
+			size_t after_null = output->len - (before_null + 1);
+			keepalive_active = 1;
+			send_sideband(1, 2, output->buf, before_null, use_sideband);
+			send_sideband(1, 2, first_null + 1, after_null, use_sideband);
+
+			return;
+		}
+	}
+
+	send_sideband(1, 2, output->buf, output->len, use_sideband);
+}
+
+static int run_update_hook(struct command *cmd)
+{
+	struct run_hooks_opt opt = RUN_HOOKS_OPT_INIT_ASYNC;
+	int code;
+
+	strvec_pushl(&opt.args,
+		     cmd->ref_name,
+		     oid_to_hex(&cmd->old_oid),
+		     oid_to_hex(&cmd->new_oid),
+		     NULL);
 
-	code = start_command(&proc);
-	if (code)
-		return code;
 	if (use_sideband)
-		copy_to_sideband(proc.err, -1, NULL);
-	return finish_command(&proc);
+		opt.consume_sideband = hook_output_to_sideband;
+
+	code = run_hooks("update", &opt);
+	run_hooks_opt_clear(&opt);
+	return code;
 }
 
 static struct command *find_command_by_refname(struct command *list,
diff --git a/t/t1416-ref-transaction-hooks.sh b/t/t1416-ref-transaction-hooks.sh
index 17f11f5cb0..28359f6099 100755
--- a/t/t1416-ref-transaction-hooks.sh
+++ b/t/t1416-ref-transaction-hooks.sh
@@ -121,10 +121,10 @@ test_expect_success 'interleaving hook calls succeed' '
 	EOF
 
 	cat >expect <<-EOF &&
-		hooks/update refs/tags/PRE $ZERO_OID $PRE_OID
+		$(pwd)/target-repo.git/hooks/update refs/tags/PRE $ZERO_OID $PRE_OID
 		$(pwd)/target-repo.git/hooks/reference-transaction prepared
 		$(pwd)/target-repo.git/hooks/reference-transaction committed
-		hooks/update refs/tags/POST $ZERO_OID $POST_OID
+		$(pwd)/target-repo.git/hooks/update refs/tags/POST $ZERO_OID $POST_OID
 		$(pwd)/target-repo.git/hooks/reference-transaction prepared
 		$(pwd)/target-repo.git/hooks/reference-transaction committed
 	EOF
-- 
2.28.0.rc0.142.g3c755180ce-goog


^ permalink raw reply related	[flat|nested] 53+ messages in thread

* [PATCH v3 14/17] proc-receive: acquire hook list from hook.h
  2020-12-22  0:04   ` [PATCH v3 00/17] use config-based hooks (config-based hooks part II) Emily Shaffer
                       ` (12 preceding siblings ...)
  2020-12-22  0:04     ` [PATCH v3 13/17] receive-pack: convert 'update' hook to hook.h Emily Shaffer
@ 2020-12-22  0:04     ` Emily Shaffer
  2020-12-22  0:04     ` [PATCH v3 15/17] post-update: use hook.h library Emily Shaffer
                       ` (5 subsequent siblings)
  19 siblings, 0 replies; 53+ messages in thread
From: Emily Shaffer @ 2020-12-22  0:04 UTC (permalink / raw)
  To: git; +Cc: Emily Shaffer

The proc-receive hook differs from most other hooks Git invokes because
the hook and the parent Git process engage in bidirectional
communication via stdin/stdout. This bidirectional communication is
unsuitable for multiple hooks, whether they are in series or in
parallel, and is incompatible with run-command.h:run_processes_parallel:

- The proc-receive hook is intended to modify the state of the Git repo.
  From 'git help githooks':
    This [proc-receive] hook is responsible for updating the relevant
    references and reporting the results back to 'receive-pack'.
  This prevents parallelization and implies, at least, specific ordering
  of hook execution.
- The proc-receive hook can reject a push by aborting early with an
  error code. If a former hook ran through the entire push contents
  successfully but a later hook rejects some of the push, the repo may
  be left in a partially-updated (and corrupt) state.
- The callback model of the run_processes_parallel() API is unsuited to
  the current implementation of proc-receive, which loops through
  "send-receive-consider" with the child process. proc-receive today
  relies on stateful communication with the child process, which would be
  unwieldy to implement with callbacks and saved state.
- Additionally, run_processes_parallel() is designed to collate the
  output of many child processes into a single output (stderr or callback),
  and would require significant work to tell the caller which process sent
  the output, and indeed to collect any output before the child process
  has exited.

So, rather than using hook.h:run_hooks() to invoke the proc-receive
hook, receive-pack.c can learn to ask hook.h:hook_list() for the
location of a hook to run. This allows users to configure their
proc-receive in a global config for all repos if they want, or a local
config if they just don't want to use the hookdir. Because running more
than one proc-receive hook doesn't make sense from a repo state
perspective, we can explicitly ban configuring more than one
proc-receive hook at a time.

If a user wants to globally configure one proc-receive hook for most of
their repos, but override that hook in a single repo, they should use
'skip' to manually remove the global hook in their special repo:

~/.gitconfig:
[hook.proc-receive]
  command = /usr/bin/usual-proc-receive

~/special-repo/.git/config:
[hookcmd./usr/bin/usual-proc-receive]
  skip = true
[hook.proc-receive]
  command = /usr/bin/special-proc-receive

Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
---
 Documentation/githooks.txt                |  4 ++
 builtin/receive-pack.c                    | 33 +++++++++++++++-
 t/t5411/test-0015-too-many-hooks-error.sh | 47 +++++++++++++++++++++++
 3 files changed, 82 insertions(+), 2 deletions(-)
 create mode 100644 t/t5411/test-0015-too-many-hooks-error.sh

diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
index 2b3a74f249..2c59c537f9 100644
--- a/Documentation/githooks.txt
+++ b/Documentation/githooks.txt
@@ -423,6 +423,10 @@ the input.  The exit status of the 'proc-receive' hook only determines
 the success or failure of the group of commands sent to it, unless
 atomic push is in use.
 
+It is forbidden to specify more than one hook for 'proc-receive'. If a
+globally-configured 'proc-receive' must be overridden, use
+'hookcmd.<global-hook>.skip = true' to ignore it.
+
 [[post-receive]]
 post-receive
 ~~~~~~~~~~~~
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 57f3bb979c..4a6ad27404 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -1145,11 +1145,40 @@ static int run_proc_receive_hook(struct command *commands,
 	int version = 0;
 	int code;
 
-	argv[0] = find_hook("proc-receive");
-	if (!argv[0]) {
+	struct strbuf hookname = STRBUF_INIT;
+	struct hook *proc_receive = NULL;
+	struct list_head *pos, *hooks;
+
+	strbuf_addstr(&hookname, "proc-receive");
+	hooks = hook_list(&hookname);
+
+	list_for_each(pos, hooks) {
+		if (proc_receive) {
+			rp_error("only one 'proc-receive' hook can be specified");
+			return -1;
+		}
+		proc_receive = list_entry(pos, struct hook, list);
+		/* check if the hookdir hook should be ignored */
+		if (proc_receive->from_hookdir) {
+			switch (configured_hookdir_opt()) {
+			case HOOKDIR_INTERACTIVE:
+			case HOOKDIR_NO:
+				proc_receive = NULL;
+				break;
+			default:
+				break;
+			}
+		}
+
+	}
+
+	if (!proc_receive) {
 		rp_error("cannot find hook 'proc-receive'");
 		return -1;
 	}
+
+
+	argv[0] = proc_receive->command.buf;
 	argv[1] = NULL;
 
 	proc.argv = argv;
diff --git a/t/t5411/test-0015-too-many-hooks-error.sh b/t/t5411/test-0015-too-many-hooks-error.sh
new file mode 100644
index 0000000000..2d64534510
--- /dev/null
+++ b/t/t5411/test-0015-too-many-hooks-error.sh
@@ -0,0 +1,47 @@
+test_expect_success "setup too  many proc-receive hooks (ok, $PROTOCOL)" '
+	write_script "proc-receive" <<-EOF &&
+	printf >&2 "# proc-receive hook\n"
+	test-tool proc-receive -v \
+		-r "ok refs/for/main/topic"
+	EOF
+
+	git -C "$upstream" config --add "hook.proc-receive.command" proc-receive &&
+	cp proc-receive "$upstream/hooks/proc-receive"
+'
+
+# Refs of upstream : main(A)
+# Refs of workbench: main(A)  tags/v123
+# git push         :                       next(A)  refs/for/main/topic(A)
+test_expect_success "proc-receive: reject more than one configured hook" '
+	test_must_fail git -C workbench push origin \
+		HEAD:next \
+		HEAD:refs/for/main/topic \
+		>out 2>&1 &&
+	make_user_friendly_and_stable_output <out >actual &&
+	cat >expect <<-EOF &&
+	remote: # pre-receive hook
+	remote: pre-receive< <ZERO-OID> <COMMIT-A> refs/heads/next
+	remote: pre-receive< <ZERO-OID> <COMMIT-A> refs/for/main/topic
+	remote: error: only one "proc-receive" hook can be specified
+	remote: # post-receive hook
+	remote: post-receive< <ZERO-OID> <COMMIT-A> refs/heads/next
+	To <URL/of/upstream.git>
+	 * [new branch] HEAD -> next
+	 ! [remote rejected] HEAD -> refs/for/main/topic (fail to run proc-receive hook)
+	EOF
+	test_cmp expect actual &&
+	git -C "$upstream" show-ref >out &&
+	make_user_friendly_and_stable_output <out >actual &&
+	cat >expect <<-EOF &&
+	<COMMIT-A> refs/heads/main
+	<COMMIT-A> refs/heads/next
+	EOF
+	test_cmp expect actual
+'
+
+# Refs of upstream : main(A)             next(A)
+# Refs of workbench: main(A)  tags/v123
+test_expect_success "cleanup ($PROTOCOL)" '
+	git -C "$upstream" config --unset "hook.proc-receive.command" "proc-receive" &&
+	git -C "$upstream" update-ref -d refs/heads/next
+'
-- 
2.28.0.rc0.142.g3c755180ce-goog


^ permalink raw reply related	[flat|nested] 53+ messages in thread

* [PATCH v3 15/17] post-update: use hook.h library
  2020-12-22  0:04   ` [PATCH v3 00/17] use config-based hooks (config-based hooks part II) Emily Shaffer
                       ` (13 preceding siblings ...)
  2020-12-22  0:04     ` [PATCH v3 14/17] proc-receive: acquire hook list from hook.h Emily Shaffer
@ 2020-12-22  0:04     ` Emily Shaffer
  2020-12-22  0:04     ` [PATCH v3 16/17] receive-pack: convert receive hooks to hook.h Emily Shaffer
                       ` (4 subsequent siblings)
  19 siblings, 0 replies; 53+ messages in thread
From: Emily Shaffer @ 2020-12-22  0:04 UTC (permalink / raw)
  To: git; +Cc: Emily Shaffer

By using run_hooks() instead of run_hook_le(), 'post-update' hooks can
be specified in the config as well as the hookdir.

Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
---
 Documentation/githooks.txt |  2 ++
 builtin/receive-pack.c     | 26 +++++++-------------------
 2 files changed, 9 insertions(+), 19 deletions(-)

diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
index 2c59c537f9..1412cd5846 100644
--- a/Documentation/githooks.txt
+++ b/Documentation/githooks.txt
@@ -498,6 +498,8 @@ Both standard output and standard error output are forwarded to
 `git send-pack` on the other end, so you can simply `echo` messages
 for the user.
 
+Hooks run during 'post-update' will be run in parallel by default.
+
 reference-transaction
 ~~~~~~~~~~~~~~~~~~~~~
 
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 4a6ad27404..5c23d5ed86 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -1686,33 +1686,21 @@ static const char *update(struct command *cmd, struct shallow_info *si)
 static void run_update_post_hook(struct command *commands)
 {
 	struct command *cmd;
-	struct child_process proc = CHILD_PROCESS_INIT;
-	const char *hook;
-
-	hook = find_hook("post-update");
-	if (!hook)
-		return;
+	struct run_hooks_opt opt = RUN_HOOKS_OPT_INIT_ASYNC;
 
 	for (cmd = commands; cmd; cmd = cmd->next) {
 		if (cmd->error_string || cmd->did_not_exist)
 			continue;
-		if (!proc.args.nr)
-			strvec_push(&proc.args, hook);
-		strvec_push(&proc.args, cmd->ref_name);
+		strvec_push(&opt.args, cmd->ref_name);
 	}
-	if (!proc.args.nr)
+	if (!opt.args.nr)
 		return;
 
-	proc.no_stdin = 1;
-	proc.stdout_to_stderr = 1;
-	proc.err = use_sideband ? -1 : 0;
-	proc.trace2_hook_name = "post-update";
+	if (use_sideband)
+		opt.consume_sideband = hook_output_to_sideband;
 
-	if (!start_command(&proc)) {
-		if (use_sideband)
-			copy_to_sideband(proc.err, -1, NULL);
-		finish_command(&proc);
-	}
+	run_hooks("post-update", &opt);
+	run_hooks_opt_clear(&opt);
 }
 
 static void check_aliased_update_internal(struct command *cmd,
-- 
2.28.0.rc0.142.g3c755180ce-goog


^ permalink raw reply related	[flat|nested] 53+ messages in thread

* [PATCH v3 16/17] receive-pack: convert receive hooks to hook.h
  2020-12-22  0:04   ` [PATCH v3 00/17] use config-based hooks (config-based hooks part II) Emily Shaffer
                       ` (14 preceding siblings ...)
  2020-12-22  0:04     ` [PATCH v3 15/17] post-update: use hook.h library Emily Shaffer
@ 2020-12-22  0:04     ` Emily Shaffer
  2020-12-22  0:04     ` [PATCH v3 17/17] run-command: stop thinking about hooks Emily Shaffer
                       ` (3 subsequent siblings)
  19 siblings, 0 replies; 53+ messages in thread
From: Emily Shaffer @ 2020-12-22  0:04 UTC (permalink / raw)
  To: git; +Cc: Emily Shaffer

By using the hook.h library to run receive hooks, they can be specified
in the config as well as in the hookdir.

Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
---
 Documentation/githooks.txt |   4 +
 builtin/receive-pack.c     | 197 +++++++++++++++++--------------------
 2 files changed, 94 insertions(+), 107 deletions(-)

diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
index 1412cd5846..c450f7a27e 100644
--- a/Documentation/githooks.txt
+++ b/Documentation/githooks.txt
@@ -314,6 +314,8 @@ will be set to zero, `GIT_PUSH_OPTION_COUNT=0`.
 See the section on "Quarantine Environment" in
 linkgit:git-receive-pack[1] for some caveats.
 
+Hooks executed during 'pre-receive' will not be parallelized.
+
 [[update]]
 update
 ~~~~~~
@@ -466,6 +468,8 @@ environment variables will not be set. If the client selects
 to use push options, but doesn't transmit any, the count variable
 will be set to zero, `GIT_PUSH_OPTION_COUNT=0`.
 
+Hooks executed during 'post-receive' are run in parallel by default.
+
 [[post-update]]
 post-update
 ~~~~~~~~~~~
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 5c23d5ed86..348d5b0a9b 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -748,7 +748,7 @@ static int check_cert_push_options(const struct string_list *push_options)
 	return retval;
 }
 
-static void prepare_push_cert_sha1(struct child_process *proc)
+static void prepare_push_cert_sha1(struct run_hooks_opt *opt)
 {
 	static int already_done;
 
@@ -772,110 +772,42 @@ static void prepare_push_cert_sha1(struct child_process *proc)
 		nonce_status = check_nonce(push_cert.buf, bogs);
 	}
 	if (!is_null_oid(&push_cert_oid)) {
-		strvec_pushf(&proc->env_array, "GIT_PUSH_CERT=%s",
+		strvec_pushf(&opt->env, "GIT_PUSH_CERT=%s",
 			     oid_to_hex(&push_cert_oid));
-		strvec_pushf(&proc->env_array, "GIT_PUSH_CERT_SIGNER=%s",
+		strvec_pushf(&opt->env, "GIT_PUSH_CERT_SIGNER=%s",
 			     sigcheck.signer ? sigcheck.signer : "");
-		strvec_pushf(&proc->env_array, "GIT_PUSH_CERT_KEY=%s",
+		strvec_pushf(&opt->env, "GIT_PUSH_CERT_KEY=%s",
 			     sigcheck.key ? sigcheck.key : "");
-		strvec_pushf(&proc->env_array, "GIT_PUSH_CERT_STATUS=%c",
+		strvec_pushf(&opt->env, "GIT_PUSH_CERT_STATUS=%c",
 			     sigcheck.result);
 		if (push_cert_nonce) {
-			strvec_pushf(&proc->env_array,
+			strvec_pushf(&opt->env,
 				     "GIT_PUSH_CERT_NONCE=%s",
 				     push_cert_nonce);
-			strvec_pushf(&proc->env_array,
+			strvec_pushf(&opt->env,
 				     "GIT_PUSH_CERT_NONCE_STATUS=%s",
 				     nonce_status);
 			if (nonce_status == NONCE_SLOP)
-				strvec_pushf(&proc->env_array,
+				strvec_pushf(&opt->env,
 					     "GIT_PUSH_CERT_NONCE_SLOP=%ld",
 					     nonce_stamp_slop);
 		}
 	}
 }
 
+struct receive_hook_feed_context {
+	struct command *cmd;
+	int skip_broken;
+};
+
 struct receive_hook_feed_state {
 	struct command *cmd;
 	struct ref_push_report *report;
 	int skip_broken;
 	struct strbuf buf;
-	const struct string_list *push_options;
 };
 
-typedef int (*feed_fn)(void *, const char **, size_t *);
-static int run_and_feed_hook(const char *hook_name, feed_fn feed,
-			     struct receive_hook_feed_state *feed_state)
-{
-	struct child_process proc = CHILD_PROCESS_INIT;
-	struct async muxer;
-	const char *argv[2];
-	int code;
-
-	argv[0] = find_hook(hook_name);
-	if (!argv[0])
-		return 0;
-
-	argv[1] = NULL;
-
-	proc.argv = argv;
-	proc.in = -1;
-	proc.stdout_to_stderr = 1;
-	proc.trace2_hook_name = hook_name;
-
-	if (feed_state->push_options) {
-		int i;
-		for (i = 0; i < feed_state->push_options->nr; i++)
-			strvec_pushf(&proc.env_array,
-				     "GIT_PUSH_OPTION_%d=%s", i,
-				     feed_state->push_options->items[i].string);
-		strvec_pushf(&proc.env_array, "GIT_PUSH_OPTION_COUNT=%d",
-			     feed_state->push_options->nr);
-	} else
-		strvec_pushf(&proc.env_array, "GIT_PUSH_OPTION_COUNT");
-
-	if (tmp_objdir)
-		strvec_pushv(&proc.env_array, tmp_objdir_env(tmp_objdir));
-
-	if (use_sideband) {
-		memset(&muxer, 0, sizeof(muxer));
-		muxer.proc = copy_to_sideband;
-		muxer.in = -1;
-		code = start_async(&muxer);
-		if (code)
-			return code;
-		proc.err = muxer.in;
-	}
-
-	prepare_push_cert_sha1(&proc);
-
-	code = start_command(&proc);
-	if (code) {
-		if (use_sideband)
-			finish_async(&muxer);
-		return code;
-	}
-
-	sigchain_push(SIGPIPE, SIG_IGN);
-
-	while (1) {
-		const char *buf;
-		size_t n;
-		if (feed(feed_state, &buf, &n))
-			break;
-		if (write_in_full(proc.in, buf, n) < 0)
-			break;
-	}
-	close(proc.in);
-	if (use_sideband)
-		finish_async(&muxer);
-
-	sigchain_pop(SIGPIPE);
-
-	return finish_command(&proc);
-}
-
-static int feed_receive_hook(void *state_, const char **bufp, size_t *sizep)
+static int feed_receive_hook(void *state_)
 {
 	struct receive_hook_feed_state *state = state_;
 	struct command *cmd = state->cmd;
@@ -884,9 +816,7 @@ static int feed_receive_hook(void *state_, const char **bufp, size_t *sizep)
 	       state->skip_broken && (cmd->error_string || cmd->did_not_exist))
 		cmd = cmd->next;
 	if (!cmd)
-		return -1; /* EOF */
-	if (!bufp)
-		return 0; /* OK, can feed something. */
+		return 1; /* EOF - close the pipe*/
 	strbuf_reset(&state->buf);
 	if (!state->report)
 		state->report = cmd->report;
@@ -910,32 +840,36 @@ static int feed_receive_hook(void *state_, const char **bufp, size_t *sizep)
 			    cmd->ref_name);
 		state->cmd = cmd->next;
 	}
-	if (bufp) {
-		*bufp = state->buf.buf;
-		*sizep = state->buf.len;
-	}
 	return 0;
 }
 
-static int run_receive_hook(struct command *commands,
-			    const char *hook_name,
-			    int skip_broken,
-			    const struct string_list *push_options)
+static int feed_receive_hook_cb(struct strbuf *pipe, void *pp_cb, void *pp_task_cb)
 {
-	struct receive_hook_feed_state state;
-	int status;
-
-	strbuf_init(&state.buf, 0);
-	state.cmd = commands;
-	state.skip_broken = skip_broken;
-	state.report = NULL;
-	if (feed_receive_hook(&state, NULL, NULL))
-		return 0;
-	state.cmd = commands;
-	state.push_options = push_options;
-	status = run_and_feed_hook(hook_name, feed_receive_hook, &state);
-	strbuf_release(&state.buf);
-	return status;
+	struct hook *hook = pp_task_cb;
+	struct receive_hook_feed_state *feed_state = hook->feed_pipe_cb_data;
+	int rc;
+
+	/* first-time setup */
+	if (!feed_state) {
+		struct hook_cb_data *hook_cb = pp_cb;
+		struct run_hooks_opt *opt = hook_cb->options;
+		struct receive_hook_feed_context *ctx = opt->feed_pipe_ctx;
+		if (!ctx)
+			BUG("run_hooks_opt.feed_pipe_ctx required for receive hook");
+
+		feed_state = xmalloc(sizeof(struct receive_hook_feed_state));
+		strbuf_init(&feed_state->buf, 0);
+		feed_state->cmd = ctx->cmd;
+		feed_state->skip_broken = ctx->skip_broken;
+		feed_state->report = NULL;
+
+		hook->feed_pipe_cb_data = feed_state;
+	}
+
+	rc = feed_receive_hook(feed_state);
+	if (!rc)
+		strbuf_addbuf(pipe, &feed_state->buf);
+	return rc;
 }
 
 static void hook_output_to_sideband(struct strbuf *output, void *cb_data)
@@ -971,6 +905,55 @@ static void hook_output_to_sideband(struct strbuf *output, void *cb_data)
 	send_sideband(1, 2, output->buf, output->len, use_sideband);
 }
 
+static int run_receive_hook(struct command *commands,
+			    const char *hook_name,
+			    int skip_broken,
+			    const struct string_list *push_options)
+{
+	struct run_hooks_opt opt = RUN_HOOKS_OPT_INIT_ASYNC;
+	struct receive_hook_feed_context ctx;
+	int rc;
+	struct command *iter = commands;
+
+	/* if there are no valid commands, don't invoke the hook at all. */
+	while (iter && skip_broken && (iter->error_string || iter->did_not_exist))
+		iter = iter->next;
+	if (!iter)
+		return 0;
+
+	/* pre-receive hooks should run in series as the hook updates refs */
+	if (!strcmp(hook_name, "pre-receive"))
+		opt.jobs = 1;
+
+	if (push_options) {
+		int i;
+		for (i = 0; i < push_options->nr; i++)
+			strvec_pushf(&opt.env, "GIT_PUSH_OPTION_%d=%s", i,
+				     push_options->items[i].string);
+		strvec_pushf(&opt.env, "GIT_PUSH_OPTION_COUNT=%d", push_options->nr);
+	} else
+		strvec_push(&opt.env, "GIT_PUSH_OPTION_COUNT");
+
+	if (tmp_objdir)
+		strvec_pushv(&opt.env, tmp_objdir_env(tmp_objdir));
+
+	prepare_push_cert_sha1(&opt);
+
+	/* set up sideband printer */
+	if (use_sideband)
+		opt.consume_sideband = hook_output_to_sideband;
+
+	/* set up stdin callback */
+	ctx.cmd = commands;
+	ctx.skip_broken = skip_broken;
+	opt.feed_pipe = feed_receive_hook_cb;
+	opt.feed_pipe_ctx = &ctx;
+
+	rc = run_hooks(hook_name, &opt);
+	run_hooks_opt_clear(&opt);
+	return rc;
+}
+
 static int run_update_hook(struct command *cmd)
 {
 	struct run_hooks_opt opt = RUN_HOOKS_OPT_INIT_ASYNC;
-- 
2.28.0.rc0.142.g3c755180ce-goog


^ permalink raw reply related	[flat|nested] 53+ messages in thread

* [PATCH v3 17/17] run-command: stop thinking about hooks
  2020-12-22  0:04   ` [PATCH v3 00/17] use config-based hooks (config-based hooks part II) Emily Shaffer
                       ` (15 preceding siblings ...)
  2020-12-22  0:04     ` [PATCH v3 16/17] receive-pack: convert receive hooks to hook.h Emily Shaffer
@ 2020-12-22  0:04     ` Emily Shaffer
  2020-12-28 19:59     ` [PATCH v3 00/17] use config-based hooks (config-based hooks part II) Emily Shaffer
                       ` (2 subsequent siblings)
  19 siblings, 0 replies; 53+ messages in thread
From: Emily Shaffer @ 2020-12-22  0:04 UTC (permalink / raw)
  To: git; +Cc: Emily Shaffer

hook.h has replaced all run-command.h hook-related functionality.
run-command.h:run_hooks_le/ve and find_hook are no longer used anywhere
in the codebase. So, let's delete the dead code - or, in the one case
where it's still needed, move it to an internal function in hook.c.

Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
---
 hook.c        | 39 ++++++++++++++++++++++++++++--
 run-command.c | 66 ---------------------------------------------------
 run-command.h | 24 -------------------
 3 files changed, 37 insertions(+), 92 deletions(-)

diff --git a/hook.c b/hook.c
index dc241f7ec5..0ac984983e 100644
--- a/hook.c
+++ b/hook.c
@@ -196,6 +196,41 @@ static int should_include_hookdir(const char *path, enum hookdir_opt cfg)
 	}
 }
 
+static const char *find_legacy_hook(const char *name)
+{
+	static struct strbuf path = STRBUF_INIT;
+
+	strbuf_reset(&path);
+	strbuf_git_path(&path, "hooks/%s", name);
+	if (access(path.buf, X_OK) < 0) {
+		int err = errno;
+
+#ifdef STRIP_EXTENSION
+		strbuf_addstr(&path, STRIP_EXTENSION);
+		if (access(path.buf, X_OK) >= 0)
+			return path.buf;
+		if (errno == EACCES)
+			err = errno;
+#endif
+
+		if (err == EACCES && advice_ignored_hook) {
+			static struct string_list advise_given = STRING_LIST_INIT_DUP;
+
+			if (!string_list_lookup(&advise_given, name)) {
+				string_list_insert(&advise_given, name);
+				advise(_("The '%s' hook was ignored because "
+					 "it's not set as executable.\n"
+					 "You can disable this warning with "
+					 "`git config advice.ignoredHook false`."),
+				       path.buf);
+			}
+		}
+		return NULL;
+	}
+	return path.buf;
+}
+
+
 struct list_head* hook_list(const struct strbuf* hookname)
 {
 	struct strbuf hook_key = STRBUF_INIT;
@@ -213,7 +248,7 @@ struct list_head* hook_list(const struct strbuf* hookname)
 	git_config(hook_config_lookup, (void*)&cb_data);
 
 	if (have_git_dir())
-		legacy_hook_path = find_hook(hookname->buf);
+		legacy_hook_path = find_legacy_hook(hookname->buf);
 
 	/* Unconditionally add legacy hook, but annotate it. */
 	if (legacy_hook_path) {
@@ -244,7 +279,7 @@ int hook_exists(const char *hookname, enum hookdir_opt should_run_hookdir)
 	int could_run_hookdir = (should_run_hookdir == HOOKDIR_INTERACTIVE ||
 				should_run_hookdir == HOOKDIR_WARN ||
 				should_run_hookdir == HOOKDIR_YES)
-				&& !!find_hook(hookname);
+				&& !!find_legacy_hook(hookname);
 
 	strbuf_addf(&hook_key, "hook.%s.command", hookname);
 
diff --git a/run-command.c b/run-command.c
index 0dce6bec83..16656135dd 100644
--- a/run-command.c
+++ b/run-command.c
@@ -1310,72 +1310,6 @@ int async_with_fork(void)
 #endif
 }
 
-const char *find_hook(const char *name)
-{
-	static struct strbuf path = STRBUF_INIT;
-
-	strbuf_reset(&path);
-	strbuf_git_path(&path, "hooks/%s", name);
-	if (access(path.buf, X_OK) < 0) {
-		int err = errno;
-
-#ifdef STRIP_EXTENSION
-		strbuf_addstr(&path, STRIP_EXTENSION);
-		if (access(path.buf, X_OK) >= 0)
-			return path.buf;
-		if (errno == EACCES)
-			err = errno;
-#endif
-
-		if (err == EACCES && advice_ignored_hook) {
-			static struct string_list advise_given = STRING_LIST_INIT_DUP;
-
-			if (!string_list_lookup(&advise_given, name)) {
-				string_list_insert(&advise_given, name);
-				advise(_("The '%s' hook was ignored because "
-					 "it's not set as executable.\n"
-					 "You can disable this warning with "
-					 "`git config advice.ignoredHook false`."),
-				       path.buf);
-			}
-		}
-		return NULL;
-	}
-	return path.buf;
-}
-
-int run_hook_ve(const char *const *env, const char *name, va_list args)
-{
-	struct child_process hook = CHILD_PROCESS_INIT;
-	const char *p;
-
-	p = find_hook(name);
-	if (!p)
-		return 0;
-
-	strvec_push(&hook.args, p);
-	while ((p = va_arg(args, const char *)))
-		strvec_push(&hook.args, p);
-	hook.env = env;
-	hook.no_stdin = 1;
-	hook.stdout_to_stderr = 1;
-	hook.trace2_hook_name = name;
-
-	return run_command(&hook);
-}
-
-int run_hook_le(const char *const *env, const char *name, ...)
-{
-	va_list args;
-	int ret;
-
-	va_start(args, name);
-	ret = run_hook_ve(env, name, args);
-	va_end(args);
-
-	return ret;
-}
-
 struct io_pump {
 	/* initialized by caller */
 	int fd;
diff --git a/run-command.h b/run-command.h
index 2ad8271f56..e67bd22c5a 100644
--- a/run-command.h
+++ b/run-command.h
@@ -194,30 +194,6 @@ int finish_command_in_signal(struct child_process *);
  */
 int run_command(struct child_process *);
 
-/*
- * Returns the path to the hook file, or NULL if the hook is missing
- * or disabled. Note that this points to static storage that will be
- * overwritten by further calls to find_hook and run_hook_*.
- */
-const char *find_hook(const char *name);
-
-/**
- * Run a hook.
- * The first argument is a pathname to an index file, or NULL
- * if the hook uses the default index file or no index is needed.
- * The second argument is the name of the hook.
- * The further arguments correspond to the hook arguments.
- * The last argument has to be NULL to terminate the arguments list.
- * If the hook does not exist or is not executable, the return
- * value will be zero.
- * If it is executable, the hook will be executed and the exit
- * status of the hook is returned.
- * On execution, .stdout_to_stderr and .no_stdin will be set.
- */
-LAST_ARG_MUST_BE_NULL
-int run_hook_le(const char *const *env, const char *name, ...);
-int run_hook_ve(const char *const *env, const char *name, va_list args);
-
 /*
  * Trigger an auto-gc
  */
-- 
2.28.0.rc0.142.g3c755180ce-goog


^ permalink raw reply related	[flat|nested] 53+ messages in thread

* Re: [PATCH v3 00/17] use config-based hooks (config-based hooks part II)
  2020-12-22  0:04   ` [PATCH v3 00/17] use config-based hooks (config-based hooks part II) Emily Shaffer
                       ` (16 preceding siblings ...)
  2020-12-22  0:04     ` [PATCH v3 17/17] run-command: stop thinking about hooks Emily Shaffer
@ 2020-12-28 19:59     ` Emily Shaffer
  2020-12-28 22:40     ` [PATCH v3 18/17] doc: make git-hook.txt point of truth Emily Shaffer
  2021-02-18 22:32     ` [PATCH v3 00/17] use config-based hooks (config-based hooks part II) Josh Steadmon
  19 siblings, 0 replies; 53+ messages in thread
From: Emily Shaffer @ 2020-12-28 19:59 UTC (permalink / raw)
  To: git
  Cc: Jeff King, Junio C Hamano, James Ramsay, Jonathan Nieder,
	brian m. carlson, Ævar Arnfjörð Bjarmason,
	Phillip Wood, Josh Steadmon, Johannes Schindelin

On Mon, Dec 21, 2020 at 04:04:18PM -0800, Emily Shaffer wrote:
> 
> Another thing I wanted to do in this series but ended up not having time
> for before the holidays was to figure out a way to consolidate
> Documentation/githooks.txt and Documentation/git-hook.txt. My personal
> preference would be to remove githooks.txt's contents, move the "Hooks"
> header from there into git-hook.txt, and have 'git help githooks'/'git
> help hooks' redirect to git-hook.txt; I don't have a patch to share here
> because I ran out of time before vacation :) What do others envision the
> documentation looking like?

I could use some tips on this, actually. I don't see anywhere that we
make one document redirect to another, or else I don't know what to look
for. If I replace the contents of `githooks.txt` with
`include::git-hook.txt[]`, `generate-cmdlist.sh` chokes because there is
no name to grep for in `githooks.txt`. Is there a documented example of
a manpage redirecting to a different manpage?

 - Emily

^ permalink raw reply	[flat|nested] 53+ messages in thread

* [PATCH v3 18/17] doc: make git-hook.txt point of truth
  2020-12-22  0:04   ` [PATCH v3 00/17] use config-based hooks (config-based hooks part II) Emily Shaffer
                       ` (17 preceding siblings ...)
  2020-12-28 19:59     ` [PATCH v3 00/17] use config-based hooks (config-based hooks part II) Emily Shaffer
@ 2020-12-28 22:40     ` Emily Shaffer
  2020-12-28 23:15       ` Emily Shaffer
  2021-02-18 22:32     ` [PATCH v3 00/17] use config-based hooks (config-based hooks part II) Josh Steadmon
  19 siblings, 1 reply; 53+ messages in thread
From: Emily Shaffer @ 2020-12-28 22:40 UTC (permalink / raw)
  To: git
  Cc: Emily Shaffer, Jeff King, Junio C Hamano, James Ramsay,
	Jonathan Nieder, brian m. carlson,
	Ævar Arnfjörð Bjarmason, Phillip Wood,
	Josh Steadmon, Johannes Schindelin

By showing the list of all hooks in 'git help hook' for users to refer
to, 'git help hook' becomes a one-stop shop for hook authorship. Since
some may still have muscle memory for 'git help githooks', though,
reference the 'git hook' commands and otherwise don't remove content.

Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
---

Sorry for the wonky subject. It seemed unnecessary to send the entirety
of the topic again when there were no changes. (I was able to push this
patch to my fork without -f, so indeed the rest is unchanged.)

I'd really prefer if 'git help githooks' opens the 'git-hook.txt' manpage,
but I couldn't figure out how to do that. This seemed like the next best thing
to me - users can still find information at the old manpage, but get a little
nudge towards the new manpage in case they didn't notice it.

Extremely open to other notes about the direction of these docs; I'm not
confident in anything except that it'd be annoying to have 'git help
githooks' remain unchanged.

 - Emily

 Documentation/git-hook.txt |   5 +
 Documentation/githooks.txt | 701 +------------------------------------
 2 files changed, 13 insertions(+), 693 deletions(-)

diff --git a/Documentation/git-hook.txt b/Documentation/git-hook.txt
index 01cee4ad81..cb8e383ec9 100644
--- a/Documentation/git-hook.txt
+++ b/Documentation/git-hook.txt
@@ -112,6 +112,11 @@ CONFIGURATION
 -------------
 include::config/hook.txt[]
 
+HOOKS
+-----
+
+include::native-hooks.txt[]
+
 GIT
 ---
 Part of the linkgit:git[1] suite
diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
index c450f7a27e..4d06369924 100644
--- a/Documentation/githooks.txt
+++ b/Documentation/githooks.txt
@@ -8,15 +8,20 @@ githooks - Hooks used by Git
 SYNOPSIS
 --------
 $GIT_DIR/hooks/* (or \`git config core.hooksPath`/*)
+[verse]
+'git hook' list <hook-name>
+'git hook' run <hook-name>
 
 
 DESCRIPTION
 -----------
 
-Hooks are programs you can place in a hooks directory to trigger
-actions at certain points in git's execution. Hooks that don't have
+Hooks are programs you can place in a hooks directory or specify in the config
+to trigger actions at certain points in Git's execution. Hooks that don't have
 the executable bit set are ignored.
 
+For information about specifying hooks in the config, check linkgit:git-hook[1].
+
 By default the hooks directory is `$GIT_DIR/hooks`, but that can be
 changed via the `core.hooksPath` configuration variable (see
 linkgit:git-config[1]).
@@ -42,697 +47,7 @@ The currently supported hooks are described below.
 HOOKS
 -----
 
-applypatch-msg
-~~~~~~~~~~~~~~
-
-This hook is invoked by linkgit:git-am[1].  It takes a single
-parameter, the name of the file that holds the proposed commit
-log message.  Exiting with a non-zero status causes `git am` to abort
-before applying the patch.
-
-The hook is allowed to edit the message file in place, and can
-be used to normalize the message into some project standard
-format. It can also be used to refuse the commit after inspecting
-the message file.
-
-The default 'applypatch-msg' hook, when enabled, runs the
-'commit-msg' hook, if the latter is enabled.
-
-Hooks run during 'applypatch-msg' will not be parallelized.
-
-pre-applypatch
-~~~~~~~~~~~~~~
-
-This hook is invoked by linkgit:git-am[1].  It takes no parameter, and is
-invoked after the patch is applied, but before a commit is made.
-
-If it exits with non-zero status, then the working tree will not be
-committed after applying the patch.
-
-It can be used to inspect the current working tree and refuse to
-make a commit if it does not pass certain test.
-
-The default 'pre-applypatch' hook, when enabled, runs the
-'pre-commit' hook, if the latter is enabled.
-
-Hooks run during 'pre-applypatch' will be run in parallel by default.
-
-post-applypatch
-~~~~~~~~~~~~~~~
-
-This hook is invoked by linkgit:git-am[1].  It takes no parameter,
-and is invoked after the patch is applied and a commit is made.
-
-This hook is meant primarily for notification, and cannot affect
-the outcome of `git am`.
-
-Hooks run during 'post-applypatch' will be run in parallel by default.
-
-pre-commit
-~~~~~~~~~~
-
-This hook is invoked by linkgit:git-commit[1], and can be bypassed
-with the `--no-verify` option.  It takes no parameters, and is
-invoked before obtaining the proposed commit log message and
-making a commit.  Exiting with a non-zero status from this script
-causes the `git commit` command to abort before creating a commit.
-
-The default 'pre-commit' hook, when enabled, catches introduction
-of lines with trailing whitespaces and aborts the commit when
-such a line is found.
-
-All the `git commit` hooks are invoked with the environment
-variable `GIT_EDITOR=:` if the command will not bring up an editor
-to modify the commit message.
-
-The default 'pre-commit' hook, when enabled--and with the
-`hooks.allownonascii` config option unset or set to false--prevents
-the use of non-ASCII filenames.
-
-Hooks executed during 'pre-commit' will not be parallelized.
-
-pre-merge-commit
-~~~~~~~~~~~~~~~~
-
-This hook is invoked by linkgit:git-merge[1], and can be bypassed
-with the `--no-verify` option.  It takes no parameters, and is
-invoked after the merge has been carried out successfully and before
-obtaining the proposed commit log message to
-make a commit.  Exiting with a non-zero status from this script
-causes the `git merge` command to abort before creating a commit.
-
-The default 'pre-merge-commit' hook, when enabled, runs the
-'pre-commit' hook, if the latter is enabled.
-
-This hook is invoked with the environment variable
-`GIT_EDITOR=:` if the command will not bring up an editor
-to modify the commit message.
-
-If the merge cannot be carried out automatically, the conflicts
-need to be resolved and the result committed separately (see
-linkgit:git-merge[1]). At that point, this hook will not be executed,
-but the 'pre-commit' hook will, if it is enabled.
-
-Hooks executed during 'pre-merge-commit' will not be parallelized.
-
-prepare-commit-msg
-~~~~~~~~~~~~~~~~~~
-
-This hook is invoked by linkgit:git-commit[1] right after preparing the
-default log message, and before the editor is started.
-
-It takes one to three parameters.  The first is the name of the file
-that contains the commit log message.  The second is the source of the commit
-message, and can be: `message` (if a `-m` or `-F` option was
-given); `template` (if a `-t` option was given or the
-configuration option `commit.template` is set); `merge` (if the
-commit is a merge or a `.git/MERGE_MSG` file exists); `squash`
-(if a `.git/SQUASH_MSG` file exists); or `commit`, followed by
-a commit SHA-1 (if a `-c`, `-C` or `--amend` option was given).
-
-If the exit status is non-zero, `git commit` will abort.
-
-The purpose of the hook is to edit the message file in place, and
-it is not suppressed by the `--no-verify` option.  A non-zero exit
-means a failure of the hook and aborts the commit.  It should not
-be used as replacement for pre-commit hook.
-
-The sample `prepare-commit-msg` hook that comes with Git removes the
-help message found in the commented portion of the commit template.
-
-Hooks executed during 'prepare-commit-msg' will not be parallelized.
-
-commit-msg
-~~~~~~~~~~
-
-This hook is invoked by linkgit:git-commit[1] and linkgit:git-merge[1], and can be
-bypassed with the `--no-verify` option.  It takes a single parameter,
-the name of the file that holds the proposed commit log message.
-Exiting with a non-zero status causes the command to abort.
-
-The hook is allowed to edit the message file in place, and can be used
-to normalize the message into some project standard format. It
-can also be used to refuse the commit after inspecting the message
-file.
-
-The default 'commit-msg' hook, when enabled, detects duplicate
-`Signed-off-by` trailers, and aborts the commit if one is found.
-
-Hooks executed during 'commit-msg' will not be parallelized.
-
-post-commit
-~~~~~~~~~~~
-
-This hook is invoked by linkgit:git-commit[1]. It takes no parameters, and is
-invoked after a commit is made.
-
-This hook is meant primarily for notification, and cannot affect
-the outcome of `git commit`.
-
-Hooks executed during 'post-commit' will run in parallel by default.
-
-pre-rebase
-~~~~~~~~~~
-
-This hook is called by linkgit:git-rebase[1] and can be used to prevent a
-branch from getting rebased.  The hook may be called with one or
-two parameters.  The first parameter is the upstream from which
-the series was forked.  The second parameter is the branch being
-rebased, and is not set when rebasing the current branch.
-
-Hooks executed during 'pre-rebase' will run in parallel by default.
-
-post-checkout
-~~~~~~~~~~~~~
-
-This hook is invoked when a linkgit:git-checkout[1] or
-linkgit:git-switch[1] is run after having updated the
-worktree.  The hook is given three parameters: the ref of the previous HEAD,
-the ref of the new HEAD (which may or may not have changed), and a flag
-indicating whether the checkout was a branch checkout (changing branches,
-flag=1) or a file checkout (retrieving a file from the index, flag=0).
-This hook cannot affect the outcome of `git switch` or `git checkout`,
-other than that the hook's exit status becomes the exit status of
-these two commands.
-
-It is also run after linkgit:git-clone[1], unless the `--no-checkout` (`-n`) option is
-used. The first parameter given to the hook is the null-ref, the second the
-ref of the new HEAD and the flag is always 1. Likewise for `git worktree add`
-unless `--no-checkout` is used.
-
-This hook can be used to perform repository validity checks, auto-display
-differences from the previous HEAD if different, or set working dir metadata
-properties.
-
-Hooks executed during 'post-checkout' will not be parallelized.
-
-post-merge
-~~~~~~~~~~
-
-This hook is invoked by linkgit:git-merge[1], which happens when a `git pull`
-is done on a local repository.  The hook takes a single parameter, a status
-flag specifying whether or not the merge being done was a squash merge.
-This hook cannot affect the outcome of `git merge` and is not executed,
-if the merge failed due to conflicts.
-
-This hook can be used in conjunction with a corresponding pre-commit hook to
-save and restore any form of metadata associated with the working tree
-(e.g.: permissions/ownership, ACLS, etc).  See contrib/hooks/setgitperms.perl
-for an example of how to do this.
-
-Hooks executed during 'post-merge' will run in parallel by default.
-
-pre-push
-~~~~~~~~
-
-This hook is called by linkgit:git-push[1] and can be used to prevent
-a push from taking place.  The hook is called with two parameters
-which provide the name and location of the destination remote, if a
-named remote is not being used both values will be the same.
-
-Information about what is to be pushed is provided on the hook's standard
-input with lines of the form:
-
-  <local ref> SP <local sha1> SP <remote ref> SP <remote sha1> LF
-
-For instance, if the command +git push origin master:foreign+ were run the
-hook would receive a line like the following:
-
-  refs/heads/master 67890 refs/heads/foreign 12345
-
-although the full, 40-character SHA-1s would be supplied.  If the foreign ref
-does not yet exist the `<remote SHA-1>` will be 40 `0`.  If a ref is to be
-deleted, the `<local ref>` will be supplied as `(delete)` and the `<local
-SHA-1>` will be 40 `0`.  If the local commit was specified by something other
-than a name which could be expanded (such as `HEAD~`, or a SHA-1) it will be
-supplied as it was originally given.
-
-If this hook exits with a non-zero status, `git push` will abort without
-pushing anything.  Information about why the push is rejected may be sent
-to the user by writing to standard error.
-
-Hooks executed during 'pre-push' will run in parallel by default.
-
-[[pre-receive]]
-pre-receive
-~~~~~~~~~~~
-
-This hook is invoked by linkgit:git-receive-pack[1] when it reacts to
-`git push` and updates reference(s) in its repository.
-Just before starting to update refs on the remote repository, the
-pre-receive hook is invoked.  Its exit status determines the success
-or failure of the update.
-
-This hook executes once for the receive operation. It takes no
-arguments, but for each ref to be updated it receives on standard
-input a line of the format:
-
-  <old-value> SP <new-value> SP <ref-name> LF
-
-where `<old-value>` is the old object name stored in the ref,
-`<new-value>` is the new object name to be stored in the ref and
-`<ref-name>` is the full name of the ref.
-When creating a new ref, `<old-value>` is 40 `0`.
-
-If the hook exits with non-zero status, none of the refs will be
-updated. If the hook exits with zero, updating of individual refs can
-still be prevented by the <<update,'update'>> hook.
-
-Both standard output and standard error output are forwarded to
-`git send-pack` on the other end, so you can simply `echo` messages
-for the user.
-
-The number of push options given on the command line of
-`git push --push-option=...` can be read from the environment
-variable `GIT_PUSH_OPTION_COUNT`, and the options themselves are
-found in `GIT_PUSH_OPTION_0`, `GIT_PUSH_OPTION_1`,...
-If it is negotiated to not use the push options phase, the
-environment variables will not be set. If the client selects
-to use push options, but doesn't transmit any, the count variable
-will be set to zero, `GIT_PUSH_OPTION_COUNT=0`.
-
-See the section on "Quarantine Environment" in
-linkgit:git-receive-pack[1] for some caveats.
-
-Hooks executed during 'pre-receive' will not be parallelized.
-
-[[update]]
-update
-~~~~~~
-
-This hook is invoked by linkgit:git-receive-pack[1] when it reacts to
-`git push` and updates reference(s) in its repository.
-Just before updating the ref on the remote repository, the update hook
-is invoked.  Its exit status determines the success or failure of
-the ref update.
-
-The hook executes once for each ref to be updated, and takes
-three parameters:
-
- - the name of the ref being updated,
- - the old object name stored in the ref,
- - and the new object name to be stored in the ref.
-
-A zero exit from the update hook allows the ref to be updated.
-Exiting with a non-zero status prevents `git receive-pack`
-from updating that ref.
-
-This hook can be used to prevent 'forced' update on certain refs by
-making sure that the object name is a commit object that is a
-descendant of the commit object named by the old object name.
-That is, to enforce a "fast-forward only" policy.
-
-It could also be used to log the old..new status.  However, it
-does not know the entire set of branches, so it would end up
-firing one e-mail per ref when used naively, though.  The
-<<post-receive,'post-receive'>> hook is more suited to that.
-
-In an environment that restricts the users' access only to git
-commands over the wire, this hook can be used to implement access
-control without relying on filesystem ownership and group
-membership. See linkgit:git-shell[1] for how you might use the login
-shell to restrict the user's access to only git commands.
-
-Both standard output and standard error output are forwarded to
-`git send-pack` on the other end, so you can simply `echo` messages
-for the user.
-
-The default 'update' hook, when enabled--and with
-`hooks.allowunannotated` config option unset or set to false--prevents
-unannotated tags to be pushed.
-
-Hooks executed during 'update' are run in parallel by default.
-
-[[proc-receive]]
-proc-receive
-~~~~~~~~~~~~
-
-This hook is invoked by linkgit:git-receive-pack[1].  If the server has
-set the multi-valued config variable `receive.procReceiveRefs`, and the
-commands sent to 'receive-pack' have matching reference names, these
-commands will be executed by this hook, instead of by the internal
-`execute_commands()` function.  This hook is responsible for updating
-the relevant references and reporting the results back to 'receive-pack'.
-
-This hook executes once for the receive operation.  It takes no
-arguments, but uses a pkt-line format protocol to communicate with
-'receive-pack' to read commands, push-options and send results.  In the
-following example for the protocol, the letter 'S' stands for
-'receive-pack' and the letter 'H' stands for this hook.
-
-    # Version and features negotiation.
-    S: PKT-LINE(version=1\0push-options atomic...)
-    S: flush-pkt
-    H: PKT-LINE(version=1\0push-options...)
-    H: flush-pkt
-
-    # Send commands from server to the hook.
-    S: PKT-LINE(<old-oid> <new-oid> <ref>)
-    S: ... ...
-    S: flush-pkt
-    # Send push-options only if the 'push-options' feature is enabled.
-    S: PKT-LINE(push-option)
-    S: ... ...
-    S: flush-pkt
-
-    # Receive result from the hook.
-    # OK, run this command successfully.
-    H: PKT-LINE(ok <ref>)
-    # NO, I reject it.
-    H: PKT-LINE(ng <ref> <reason>)
-    # Fall through, let 'receive-pack' to execute it.
-    H: PKT-LINE(ok <ref>)
-    H: PKT-LINE(option fall-through)
-    # OK, but has an alternate reference.  The alternate reference name
-    # and other status can be given in option directives.
-    H: PKT-LINE(ok <ref>)
-    H: PKT-LINE(option refname <refname>)
-    H: PKT-LINE(option old-oid <old-oid>)
-    H: PKT-LINE(option new-oid <new-oid>)
-    H: PKT-LINE(option forced-update)
-    H: ... ...
-    H: flush-pkt
-
-Each command for the 'proc-receive' hook may point to a pseudo-reference
-and always has a zero-old as its old-oid, while the 'proc-receive' hook
-may update an alternate reference and the alternate reference may exist
-already with a non-zero old-oid.  For this case, this hook will use
-"option" directives to report extended attributes for the reference given
-by the leading "ok" directive.
-
-The report of the commands of this hook should have the same order as
-the input.  The exit status of the 'proc-receive' hook only determines
-the success or failure of the group of commands sent to it, unless
-atomic push is in use.
-
-It is forbidden to specify more than one hook for 'proc-receive'. If a
-globally-configured 'proc-receive' must be overridden, use
-'hookcmd.<global-hook>.skip = true' to ignore it.
-
-[[post-receive]]
-post-receive
-~~~~~~~~~~~~
-
-This hook is invoked by linkgit:git-receive-pack[1] when it reacts to
-`git push` and updates reference(s) in its repository.
-It executes on the remote repository once after all the refs have
-been updated.
-
-This hook executes once for the receive operation.  It takes no
-arguments, but gets the same information as the
-<<pre-receive,'pre-receive'>>
-hook does on its standard input.
-
-This hook does not affect the outcome of `git receive-pack`, as it
-is called after the real work is done.
-
-This supersedes the <<post-update,'post-update'>> hook in that it gets
-both old and new values of all the refs in addition to their
-names.
-
-Both standard output and standard error output are forwarded to
-`git send-pack` on the other end, so you can simply `echo` messages
-for the user.
-
-The default 'post-receive' hook is empty, but there is
-a sample script `post-receive-email` provided in the `contrib/hooks`
-directory in Git distribution, which implements sending commit
-emails.
-
-The number of push options given on the command line of
-`git push --push-option=...` can be read from the environment
-variable `GIT_PUSH_OPTION_COUNT`, and the options themselves are
-found in `GIT_PUSH_OPTION_0`, `GIT_PUSH_OPTION_1`,...
-If it is negotiated to not use the push options phase, the
-environment variables will not be set. If the client selects
-to use push options, but doesn't transmit any, the count variable
-will be set to zero, `GIT_PUSH_OPTION_COUNT=0`.
-
-Hooks executed during 'post-receive' are run in parallel by default.
-
-[[post-update]]
-post-update
-~~~~~~~~~~~
-
-This hook is invoked by linkgit:git-receive-pack[1] when it reacts to
-`git push` and updates reference(s) in its repository.
-It executes on the remote repository once after all the refs have
-been updated.
-
-It takes a variable number of parameters, each of which is the
-name of ref that was actually updated.
-
-This hook is meant primarily for notification, and cannot affect
-the outcome of `git receive-pack`.
-
-The 'post-update' hook can tell what are the heads that were pushed,
-but it does not know what their original and updated values are,
-so it is a poor place to do log old..new. The
-<<post-receive,'post-receive'>> hook does get both original and
-updated values of the refs. You might consider it instead if you need
-them.
-
-When enabled, the default 'post-update' hook runs
-`git update-server-info` to keep the information used by dumb
-transports (e.g., HTTP) up to date.  If you are publishing
-a Git repository that is accessible via HTTP, you should
-probably enable this hook.
-
-Both standard output and standard error output are forwarded to
-`git send-pack` on the other end, so you can simply `echo` messages
-for the user.
-
-Hooks run during 'post-update' will be run in parallel by default.
-
-reference-transaction
-~~~~~~~~~~~~~~~~~~~~~
-
-This hook is invoked by any Git command that performs reference
-updates. It executes whenever a reference transaction is prepared,
-committed or aborted and may thus get called multiple times.
-
-The hook takes exactly one argument, which is the current state the
-given reference transaction is in:
-
-    - "prepared": All reference updates have been queued to the
-      transaction and references were locked on disk.
-
-    - "committed": The reference transaction was committed and all
-      references now have their respective new value.
-
-    - "aborted": The reference transaction was aborted, no changes
-      were performed and the locks have been released.
-
-For each reference update that was added to the transaction, the hook
-receives on standard input a line of the format:
-
-  <old-value> SP <new-value> SP <ref-name> LF
-
-The exit status of the hook is ignored for any state except for the
-"prepared" state. In the "prepared" state, a non-zero exit status will
-cause the transaction to be aborted. The hook will not be called with
-"aborted" state in that case.
-
-Hooks run during 'reference-transaction' will be run in parallel by default.
-
-push-to-checkout
-~~~~~~~~~~~~~~~~
-
-This hook is invoked by linkgit:git-receive-pack[1] when it reacts to
-`git push` and updates reference(s) in its repository, and when
-the push tries to update the branch that is currently checked out
-and the `receive.denyCurrentBranch` configuration variable is set to
-`updateInstead`.  Such a push by default is refused if the working
-tree and the index of the remote repository has any difference from
-the currently checked out commit; when both the working tree and the
-index match the current commit, they are updated to match the newly
-pushed tip of the branch.  This hook is to be used to override the
-default behaviour.
-
-The hook receives the commit with which the tip of the current
-branch is going to be updated.  It can exit with a non-zero status
-to refuse the push (when it does so, it must not modify the index or
-the working tree).  Or it can make any necessary changes to the
-working tree and to the index to bring them to the desired state
-when the tip of the current branch is updated to the new commit, and
-exit with a zero status.
-
-For example, the hook can simply run `git read-tree -u -m HEAD "$1"`
-in order to emulate `git fetch` that is run in the reverse direction
-with `git push`, as the two-tree form of `git read-tree -u -m` is
-essentially the same as `git switch` or `git checkout`
-that switches branches while
-keeping the local changes in the working tree that do not interfere
-with the difference between the branches.
-
-Hooks executed during 'push-to-checkout' will not be parallelized.
-
-pre-auto-gc
-~~~~~~~~~~~
-
-This hook is invoked by `git gc --auto` (see linkgit:git-gc[1]). It
-takes no parameter, and exiting with non-zero status from this script
-causes the `git gc --auto` to abort.
-
-Hooks run during 'pre-auto-gc' will be run in parallel by default.
-
-post-rewrite
-~~~~~~~~~~~~
-
-This hook is invoked by commands that rewrite commits
-(linkgit:git-commit[1] when called with `--amend` and
-linkgit:git-rebase[1]; however, full-history (re)writing tools like
-linkgit:git-fast-import[1] or
-https://github.com/newren/git-filter-repo[git-filter-repo] typically
-do not call it!).  Its first argument denotes the command it was
-invoked by: currently one of `amend` or `rebase`.  Further
-command-dependent arguments may be passed in the future.
-
-The hook receives a list of the rewritten commits on stdin, in the
-format
-
-  <old-sha1> SP <new-sha1> [ SP <extra-info> ] LF
-
-The 'extra-info' is again command-dependent.  If it is empty, the
-preceding SP is also omitted.  Currently, no commands pass any
-'extra-info'.
-
-The hook always runs after the automatic note copying (see
-"notes.rewrite.<command>" in linkgit:git-config[1]) has happened, and
-thus has access to these notes.
-
-Hooks run during 'post-rewrite' will be run in parallel by default.
-
-The following command-specific comments apply:
-
-rebase::
-	For the 'squash' and 'fixup' operation, all commits that were
-	squashed are listed as being rewritten to the squashed commit.
-	This means that there will be several lines sharing the same
-	'new-sha1'.
-+
-The commits are guaranteed to be listed in the order that they were
-processed by rebase.
-
-sendemail-validate
-~~~~~~~~~~~~~~~~~~
-
-This hook is invoked by linkgit:git-send-email[1].  It takes a single parameter,
-the name of the file that holds the e-mail to be sent.  Exiting with a
-non-zero status causes `git send-email` to abort before sending any
-e-mails.
-
-fsmonitor-watchman
-~~~~~~~~~~~~~~~~~~
-
-This hook is invoked when the configuration option `core.fsmonitor` is
-set to `.git/hooks/fsmonitor-watchman` or `.git/hooks/fsmonitor-watchmanv2`
-depending on the version of the hook to use.
-
-Version 1 takes two arguments, a version (1) and the time in elapsed
-nanoseconds since midnight, January 1, 1970.
-
-Version 2 takes two arguments, a version (2) and a token that is used
-for identifying changes since the token. For watchman this would be
-a clock id. This version must output to stdout the new token followed
-by a NUL before the list of files.
-
-The hook should output to stdout the list of all files in the working
-directory that may have changed since the requested time.  The logic
-should be inclusive so that it does not miss any potential changes.
-The paths should be relative to the root of the working directory
-and be separated by a single NUL.
-
-It is OK to include files which have not actually changed.  All changes
-including newly-created and deleted files should be included. When
-files are renamed, both the old and the new name should be included.
-
-Git will limit what files it checks for changes as well as which
-directories are checked for untracked files based on the path names
-given.
-
-An optimized way to tell git "all files have changed" is to return
-the filename `/`.
-
-The exit status determines whether git will use the data from the
-hook to limit its search.  On error, it will fall back to verifying
-all files and folders.
-
-p4-changelist
-~~~~~~~~~~~~~
-
-This hook is invoked by `git-p4 submit`.
-
-The `p4-changelist` hook is executed after the changelist
-message has been edited by the user. It can be bypassed with the
-`--no-verify` option. It takes a single parameter, the name
-of the file that holds the proposed changelist text. Exiting
-with a non-zero status causes the command to abort.
-
-The hook is allowed to edit the changelist file and can be used
-to normalize the text into some project standard format. It can
-also be used to refuse the Submit after inspect the message file.
-
-Run `git-p4 submit --help` for details.
-
-p4-prepare-changelist
-~~~~~~~~~~~~~~~~~~~~~
-
-This hook is invoked by `git-p4 submit`.
-
-The `p4-prepare-changelist` hook is executed right after preparing
-the default changelist message and before the editor is started.
-It takes one parameter, the name of the file that contains the
-changelist text. Exiting with a non-zero status from the script
-will abort the process.
-
-The purpose of the hook is to edit the message file in place,
-and it is not supressed by the `--no-verify` option. This hook
-is called even if `--prepare-p4-only` is set.
-
-Run `git-p4 submit --help` for details.
-
-p4-post-changelist
-~~~~~~~~~~~~~~~~~~
-
-This hook is invoked by `git-p4 submit`.
-
-The `p4-post-changelist` hook is invoked after the submit has
-successfully occurred in P4. It takes no parameters and is meant
-primarily for notification and cannot affect the outcome of the
-git p4 submit action.
-
-Run `git-p4 submit --help` for details.
-
-p4-pre-submit
-~~~~~~~~~~~~~
-
-This hook is invoked by `git-p4 submit`. It takes no parameters and nothing
-from standard input. Exiting with non-zero status from this script prevent
-`git-p4 submit` from launching. It can be bypassed with the `--no-verify`
-command line option. Run `git-p4 submit --help` for details.
-
-
-
-post-index-change
-~~~~~~~~~~~~~~~~~
-
-This hook is invoked when the index is written in read-cache.c
-do_write_locked_index.
-
-The first parameter passed to the hook is the indicator for the
-working directory being updated.  "1" meaning working directory
-was updated or "0" when the working directory was not updated.
-
-The second parameter passed to the hook is the indicator for whether
-or not the index was updated and the skip-worktree bit could have
-changed.  "1" meaning skip-worktree bits could have been updated
-and "0" meaning they were not.
-
-Only one parameter should be set to "1" when the hook runs.  The hook
-running passing "1", "1" should not be possible.
-
-Hooks run during 'post-index-change' will be run in parallel by default.
+include::native-hooks.txt[]
 
 GIT
 ---
-- 
2.29.2.490.gc7ae633391


^ permalink raw reply related	[flat|nested] 53+ messages in thread

* Re: [PATCH v3 18/17] doc: make git-hook.txt point of truth
  2020-12-28 22:40     ` [PATCH v3 18/17] doc: make git-hook.txt point of truth Emily Shaffer
@ 2020-12-28 23:15       ` Emily Shaffer
  0 siblings, 0 replies; 53+ messages in thread
From: Emily Shaffer @ 2020-12-28 23:15 UTC (permalink / raw)
  To: git
  Cc: Jeff King, Junio C Hamano, James Ramsay, Jonathan Nieder,
	brian m. carlson, Ævar Arnfjörð Bjarmason,
	Phillip Wood, Josh Steadmon, Johannes Schindelin

On Mon, Dec 28, 2020 at 02:40:15PM -0800, Emily Shaffer wrote:
> 
> By showing the list of all hooks in 'git help hook' for users to refer
> to, 'git help hook' becomes a one-stop shop for hook authorship. Since
> some may still have muscle memory for 'git help githooks', though,
> reference the 'git hook' commands and otherwise don't remove content.
> 
> Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
> ---
> 
> Sorry for the wonky subject. It seemed unnecessary to send the entirety
> of the topic again when there were no changes. (I was able to push this
> patch to my fork without -f, so indeed the rest is unchanged.)
> 
> I'd really prefer if 'git help githooks' opens the 'git-hook.txt' manpage,
> but I couldn't figure out how to do that. This seemed like the next best thing
> to me - users can still find information at the old manpage, but get a little
> nudge towards the new manpage in case they didn't notice it.
> 
> Extremely open to other notes about the direction of these docs; I'm not
> confident in anything except that it'd be annoying to have 'git help
> githooks' remain unchanged.

Even now, this is missing the native-hooks.txt from the commit. Ugh.
This commit breaks the build; please do not add it to the topic. I will
include a fixed version with the next reroll. Sorry.

That said, the intent is clear enough for me to still feel comfortable
inviting review.

 - Emily

^ permalink raw reply	[flat|nested] 53+ messages in thread

* Re: [PATCH v3 02/17] am: convert applypatch hooks to use config
  2020-12-22  0:04     ` [PATCH v3 02/17] am: convert applypatch hooks to use config Emily Shaffer
@ 2021-02-01 22:05       ` Junio C Hamano
  0 siblings, 0 replies; 53+ messages in thread
From: Junio C Hamano @ 2021-02-01 22:05 UTC (permalink / raw)
  To: Emily Shaffer; +Cc: git

Emily Shaffer <emilyshaffer@google.com> writes:

> Teach pre-applypatch, post-applypatch, and applypatch-msg to use the
> hook.h library instead of the run-command.h library. This enables use of
> hooks specified in the config, in addition to those in the hookdir.
> These three hooks are called only by builtin/am.c.

I was interested in the applypatch-msg the most; it gets fed the
"final-commit" file as its input and expected to modify it in place,
so if you allow two or more to work in series, you do not have to
anything extra (like swapping the output from the previous step to
the input for the next step), which makes this a relatively easy
conversion.

^ permalink raw reply	[flat|nested] 53+ messages in thread

* Re: [PATCH v3 01/17] commit: use config-based hooks
  2020-12-22  0:04     ` [PATCH v3 01/17] commit: use config-based hooks Emily Shaffer
@ 2021-02-01 22:08       ` Junio C Hamano
  2021-03-10 19:51         ` Emily Shaffer
  2021-02-01 23:02       ` Junio C Hamano
  1 sibling, 1 reply; 53+ messages in thread
From: Junio C Hamano @ 2021-02-01 22:08 UTC (permalink / raw)
  To: Emily Shaffer; +Cc: git

Emily Shaffer <emilyshaffer@google.com> writes:

> Because all but 'post-commit' hooks are expected to make some state
> change, force all but 'post-commit' hook to run in series.

OK.  And a sequence of hooks that run in series are expected to use
the output from the previous one as its input?

> 'post-commit'
> "is meant primarily for notification, and cannot affect the outcome of
> `git commit`," so it is fine to run in parallel.

Makes sense.


^ permalink raw reply	[flat|nested] 53+ messages in thread

* Re: [PATCH v3 01/17] commit: use config-based hooks
  2020-12-22  0:04     ` [PATCH v3 01/17] commit: use config-based hooks Emily Shaffer
  2021-02-01 22:08       ` Junio C Hamano
@ 2021-02-01 23:02       ` Junio C Hamano
  2021-03-10 19:39         ` Emily Shaffer
  1 sibling, 1 reply; 53+ messages in thread
From: Junio C Hamano @ 2021-02-01 23:02 UTC (permalink / raw)
  To: Emily Shaffer; +Cc: git

Emily Shaffer <emilyshaffer@google.com> writes:

> @@ -175,6 +183,8 @@ invoked after a commit is made.
>  This hook is meant primarily for notification, and cannot affect
>  the outcome of `git commit`.
>  
> +Hooks executed during 'post-commit' will run in parallel by default.

The "by default" makes it sound as if there is a way to force
'post-commit' alone serially, but other than hook.jobs that can be
used to uniformly make everything run serially, I didn't find a way
to do so.

The comment applies to all the "by default" in this latter half of
the series.

Thanks.

^ permalink raw reply	[flat|nested] 53+ messages in thread

* Re: [PATCH v3 00/17] use config-based hooks (config-based hooks part II)
  2020-12-22  0:04   ` [PATCH v3 00/17] use config-based hooks (config-based hooks part II) Emily Shaffer
                       ` (18 preceding siblings ...)
  2020-12-28 22:40     ` [PATCH v3 18/17] doc: make git-hook.txt point of truth Emily Shaffer
@ 2021-02-18 22:32     ` Josh Steadmon
  19 siblings, 0 replies; 53+ messages in thread
From: Josh Steadmon @ 2021-02-18 22:32 UTC (permalink / raw)
  To: Emily Shaffer
  Cc: git, Jeff King, Junio C Hamano, James Ramsay, Jonathan Nieder,
	brian m. carlson, Ævar Arnfjörð Bjarmason,
	Phillip Wood, Johannes Schindelin

On 2020.12.21 16:04, Emily Shaffer wrote:
> Since v2:
>  - Renamed 'master' to 'main' in new t5411 (proc-receive) test.
>  - Removed some accidentally included debug strings.
>  - Fixed a nasty bug in the reference-transaction hook conversion where calling
>    'oid_to_hex()' would invalidate references to the output of earlier
>    'oid_to_hex()' runs farther up the callstack. Instead, the hook callsite now
>    uses 'oid_to_hex_r()'.
> 
> Another thing I wanted to do in this series but ended up not having time
> for before the holidays was to figure out a way to consolidate
> Documentation/githooks.txt and Documentation/git-hook.txt. My personal
> preference would be to remove githooks.txt's contents, move the "Hooks"
> header from there into git-hook.txt, and have 'git help githooks'/'git
> help hooks' redirect to git-hook.txt; I don't have a patch to share here
> because I ran out of time before vacation :) What do others envision the
> documentation looking like?
> 
> Thanks!
>  - Emily
> 
> CI run: https://github.com/nasamuffin/git/actions/runs/436905873
> 
> Emily Shaffer (17):
>   commit: use config-based hooks
>   am: convert applypatch hooks to use config
>   merge: use config-based hooks for post-merge hook
>   gc: use hook library for pre-auto-gc hook
>   rebase: teach pre-rebase to use hook.h
>   read-cache: convert post-index-change hook to use config
>   receive-pack: convert push-to-checkout hook to hook.h
>   git-p4: use 'git hook' to run hooks
>   hooks: convert 'post-checkout' hook to hook library
>   hook: convert 'post-rewrite' hook to config
>   transport: convert pre-push hook to use config
>   reference-transaction: look for hooks in config
>   receive-pack: convert 'update' hook to hook.h
>   proc-receive: acquire hook list from hook.h
>   post-update: use hook.h library
>   receive-pack: convert receive hooks to hook.h
>   run-command: stop thinking about hooks
> 
>  Documentation/githooks.txt                    |  45 +++
>  builtin/am.c                                  |  30 +-
>  builtin/checkout.c                            |  16 +-
>  builtin/clone.c                               |   7 +-
>  builtin/commit.c                              |  11 +-
>  builtin/gc.c                                  |   4 +-
>  builtin/merge.c                               |  14 +-
>  builtin/rebase.c                              |   7 +-
>  builtin/receive-pack.c                        | 326 ++++++++++--------
>  builtin/worktree.c                            |  30 +-
>  commit.c                                      |  20 +-
>  commit.h                                      |   3 +-
>  git-p4.py                                     |  67 +---
>  hook.c                                        |  39 ++-
>  read-cache.c                                  |  12 +-
>  refs.c                                        |  38 +-
>  reset.c                                       |  15 +-
>  run-command.c                                 |  66 ----
>  run-command.h                                 |  24 --
>  sequencer.c                                   |  83 ++---
>  t/t1416-ref-transaction-hooks.sh              |  12 +-
>  t/t5411/test-0015-too-many-hooks-error.sh     |  47 +++
>  ...3-pre-commit-and-pre-merge-commit-hooks.sh |  17 +-
>  transport.c                                   |  55 +--
>  24 files changed, 493 insertions(+), 495 deletions(-)
>  create mode 100644 t/t5411/test-0015-too-many-hooks-error.sh
> 
> -- 
> 2.28.0.rc0.142.g3c755180ce-goog
> 

My only complaints on the previous version of this series have now been
resolved, so LGTM.

Reviewed-by: Josh Steadmon <steadmon@google.com>

^ permalink raw reply	[flat|nested] 53+ messages in thread

* Re: [PATCH v3 01/17] commit: use config-based hooks
  2021-02-01 23:02       ` Junio C Hamano
@ 2021-03-10 19:39         ` Emily Shaffer
  0 siblings, 0 replies; 53+ messages in thread
From: Emily Shaffer @ 2021-03-10 19:39 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git

On Mon, Feb 01, 2021 at 03:02:03PM -0800, Junio C Hamano wrote:
> 
> Emily Shaffer <emilyshaffer@google.com> writes:
> 
> > @@ -175,6 +183,8 @@ invoked after a commit is made.
> >  This hook is meant primarily for notification, and cannot affect
> >  the outcome of `git commit`.
> >  
> > +Hooks executed during 'post-commit' will run in parallel by default.
> 
> The "by default" makes it sound as if there is a way to force
> 'post-commit' alone serially, but other than hook.jobs that can be
> used to uniformly make everything run serially, I didn't find a way
> to do so.

Yeah, hook.jobs is the way to make it run serially, and as you say, it
can't be applied everywhere. I'll try to change the language.

> 
> The comment applies to all the "by default" in this latter half of
> the series.
> 
> Thanks.

^ permalink raw reply	[flat|nested] 53+ messages in thread

* Re: [PATCH v3 01/17] commit: use config-based hooks
  2021-02-01 22:08       ` Junio C Hamano
@ 2021-03-10 19:51         ` Emily Shaffer
  2021-03-10 22:36           ` Junio C Hamano
  0 siblings, 1 reply; 53+ messages in thread
From: Emily Shaffer @ 2021-03-10 19:51 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git

On Mon, Feb 01, 2021 at 02:08:50PM -0800, Junio C Hamano wrote:
> 
> Emily Shaffer <emilyshaffer@google.com> writes:
> 
> > Because all but 'post-commit' hooks are expected to make some state
> > change, force all but 'post-commit' hook to run in series.
> 
> OK.  And a sequence of hooks that run in series are expected to use
> the output from the previous one as its input?

Kind of, but phrasing it that way is a little odd (to me). The commit
hooks take as input the path to a tempfile which holds the proposed
commit message, and they modify the contents of that file. They don't
make output via stdout or receive the commit message via stdin. Most
hooks work this way, I think with the sole exception of 'proc-receive',
which is implemented weirdly (I said more about it when you asked about
it in your part I review, here:
https://lore.kernel.org/git/YEkgWDthF7sObtAt@google.com).

I'm not sure how to phrase it better. If I were writing a hook I imagine
the combination of taking a filename as an argument and the note about
not running these hooks in parallel would be pretty self-evident. Hmmm.
Maybe instead of the copy/pasted warning about "will not be
parallelized" I could write:

  Hooks executed during prepare-commit-msg will not be parallelized,
  because the file provided in the first argument is expected to be edited
  by each hook.

> 
> > 'post-commit'
> > "is meant primarily for notification, and cannot affect the outcome of
> > `git commit`," so it is fine to run in parallel.
> 
> Makes sense.
> 

^ permalink raw reply	[flat|nested] 53+ messages in thread

* Re: [PATCH v3 01/17] commit: use config-based hooks
  2021-03-10 19:51         ` Emily Shaffer
@ 2021-03-10 22:36           ` Junio C Hamano
  0 siblings, 0 replies; 53+ messages in thread
From: Junio C Hamano @ 2021-03-10 22:36 UTC (permalink / raw)
  To: Emily Shaffer; +Cc: git

Emily Shaffer <emilyshaffer@google.com> writes:

> parallelized" I could write:
>
>   Hooks executed during prepare-commit-msg will not be parallelized,
>   because the file provided in the first argument is expected to be edited
>   by each hook.

Yeah.  It's been quite a while since I wrote the message you are
responding to, and it took me a while to get the context back in my
head, but the above looks reasonable to me.  We could tweak it a bit
to say "the file ... serves as an input to be modified in place by
each hook" to make it even clearer, but I think yours is clear
enough.

Thanks.

^ permalink raw reply	[flat|nested] 53+ messages in thread

end of thread, other threads:[~2021-03-10 22:37 UTC | newest]

Thread overview: 53+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-14 23:25 [PATCH] commit: use config-based hooks (config-based hooks part II) Emily Shaffer
2020-10-16 18:34 ` Junio C Hamano
2020-12-05  1:49 ` [PATCH 00/17] use config-based hooks (config-based hooks part Emily Shaffer
2020-12-05  1:49   ` [PATCH 01/17] commit: use config-based hooks Emily Shaffer
2020-12-05  1:49   ` [PATCH 02/17] am: convert applypatch hooks to use config Emily Shaffer
2020-12-05  1:49   ` [PATCH 03/17] merge: use config-based hooks for post-merge hook Emily Shaffer
2020-12-05  1:49   ` [PATCH 04/17] gc: use hook library for pre-auto-gc hook Emily Shaffer
2020-12-05  1:49   ` [PATCH 05/17] rebase: teach pre-rebase to use hook.h Emily Shaffer
2020-12-05  1:49   ` [PATCH 06/17] read-cache: convert post-index-change hook to use config Emily Shaffer
2020-12-05  1:49   ` [PATCH 07/17] receive-pack: convert push-to-checkout hook to hook.h Emily Shaffer
2020-12-05  1:49   ` [PATCH 08/17] git-p4: use 'git hook' to run hooks Emily Shaffer
2020-12-16  0:27     ` Josh Steadmon
2020-12-16 20:19       ` Emily Shaffer
2020-12-05  1:49   ` [PATCH 09/17] hooks: convert 'post-checkout' hook to hook library Emily Shaffer
2020-12-05  1:49   ` [PATCH 10/17] hook: convert 'post-rewrite' hook to config Emily Shaffer
2020-12-08 23:02     ` Josh Steadmon
2020-12-15 23:42       ` Emily Shaffer
2020-12-05  1:49   ` [PATCH 11/17] transport: convert pre-push hook to use config Emily Shaffer
2020-12-05  1:49   ` [PATCH 12/17] reference-transaction: look for hooks in config Emily Shaffer
2020-12-05  1:49   ` [PATCH 13/17] receive-pack: convert 'update' hook to hook.h Emily Shaffer
2020-12-05  1:49   ` [PATCH 14/17] proc-receive: acquire hook list from hook.h Emily Shaffer
2020-12-05  1:49   ` [PATCH 15/17] post-update: use hook.h library Emily Shaffer
2020-12-05  1:49   ` [PATCH 16/17] receive-pack: convert receive hooks to hook.h Emily Shaffer
2020-12-05  1:49   ` [PATCH 17/17] run-command: stop thinking about hooks Emily Shaffer
2020-12-22  0:04   ` [PATCH v3 00/17] use config-based hooks (config-based hooks part II) Emily Shaffer
2020-12-22  0:04     ` [PATCH v3 01/17] commit: use config-based hooks Emily Shaffer
2021-02-01 22:08       ` Junio C Hamano
2021-03-10 19:51         ` Emily Shaffer
2021-03-10 22:36           ` Junio C Hamano
2021-02-01 23:02       ` Junio C Hamano
2021-03-10 19:39         ` Emily Shaffer
2020-12-22  0:04     ` [PATCH v3 02/17] am: convert applypatch hooks to use config Emily Shaffer
2021-02-01 22:05       ` Junio C Hamano
2020-12-22  0:04     ` [PATCH v3 03/17] merge: use config-based hooks for post-merge hook Emily Shaffer
2020-12-22  0:04     ` [PATCH v3 04/17] gc: use hook library for pre-auto-gc hook Emily Shaffer
2020-12-22  0:04     ` [PATCH v3 05/17] rebase: teach pre-rebase to use hook.h Emily Shaffer
2020-12-22  0:04     ` [PATCH v3 06/17] read-cache: convert post-index-change hook to use config Emily Shaffer
2020-12-22  0:04     ` [PATCH v3 07/17] receive-pack: convert push-to-checkout hook to hook.h Emily Shaffer
2020-12-22  0:04     ` [PATCH v3 08/17] git-p4: use 'git hook' to run hooks Emily Shaffer
2020-12-22  0:04     ` [PATCH v3 09/17] hooks: convert 'post-checkout' hook to hook library Emily Shaffer
2020-12-22  0:04     ` [PATCH v3 10/17] hook: convert 'post-rewrite' hook to config Emily Shaffer
2020-12-22  0:04     ` [PATCH v3 11/17] transport: convert pre-push hook to use config Emily Shaffer
2020-12-22  0:04     ` [PATCH v3 12/17] reference-transaction: look for hooks in config Emily Shaffer
2020-12-22  0:04     ` [PATCH v3 13/17] receive-pack: convert 'update' hook to hook.h Emily Shaffer
2020-12-22  0:04     ` [PATCH v3 14/17] proc-receive: acquire hook list from hook.h Emily Shaffer
2020-12-22  0:04     ` [PATCH v3 15/17] post-update: use hook.h library Emily Shaffer
2020-12-22  0:04     ` [PATCH v3 16/17] receive-pack: convert receive hooks to hook.h Emily Shaffer
2020-12-22  0:04     ` [PATCH v3 17/17] run-command: stop thinking about hooks Emily Shaffer
2020-12-28 19:59     ` [PATCH v3 00/17] use config-based hooks (config-based hooks part II) Emily Shaffer
2020-12-28 22:40     ` [PATCH v3 18/17] doc: make git-hook.txt point of truth Emily Shaffer
2020-12-28 23:15       ` Emily Shaffer
2021-02-18 22:32     ` [PATCH v3 00/17] use config-based hooks (config-based hooks part II) Josh Steadmon
2020-12-16  0:31 ` [PATCH] commit: " Josh Steadmon

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.