threads / patch / 56692

patch, 13 partshook.[ch]: new library to run hooks + simple hook conversion

Subject: [PATCH 00/13] hook.[ch]: new library to run hooks + simple hook conversion

## tl;dr

18 messages between Oct 12, 2021 and Oct 13, 2021. Diffs are folded; open one to read it.

replies: 17people: 3as markdown or json

Ævar Arnfjörð Bjarmason· Oct 12, 2021, 13:30 UTC · lore

This series is part 2 of an incremental restart of the greater "config-based hooks" topic by Emily Shaffer that I've been helping with. See [1] for part 1, and [2] (search for "mark2") for a summary of parts to come.

This goes on top of the "ab/config-based-hooks-1" topic that's already in "next" and marked for graduation to "master".

In this topic we build upon the skeleton hook.[ch] library and build infrastructure added in part 1 and add a hook running library and new "git hook" command.

The new "git hook" command is (for now) only for the benefit of in-tree non-C programs that need to run hooks, and this series converts git-send-email and git-p4 to use it.

At the end of the series we remove "run_hook_{le,ve}()" from run-command.c, as we've migrated all its callers.

What we don't do is convert any of the more complex in-tree hooks that require input on stdin such as "pre-receive" and the like, that's in later parts once this lands.

This series is approximately patch 6-20/36 of the previous 36 patch ab/config-based-hooks-base topic. A range-diff to that v5[3] is included below.

The changes since that v5 are rather trivial, they are:
 * Formatting changes to reduce the diff to parts that come after this
   (which Emily & I were juggling at the time), and re-flowing some
   overly long lines.
 * The new test is now marked for SANITIZE=leak testing, with the new
   test facility I've added recently (and which just landed in
   "master"). They all pass, the new API doesn't leak.
 * I rewrote some of the git-send-email.perl code to avoid
   de-duplication and hardcoding (just using intermediate variables).
Things to focus on reviewing:
 * This should all be pretty solid and well tested, but the git-p4.py
   part in particular I've never tested for real (not having access to
   p4), and think Emily hasn't either[4].
   The relevant patch looks trivially correct to me[5], and I've tried it
   out in the Python REPL. But if any of the CC'd people active in
   git-p4.py development could give it some end-to-end testing that
   would be much appreciated.
 * Both Python and Perl code now calls the in-between "git hook run"
   command rather than calling hooks directly. Will this behave
   differently due to any special behavior running via a git built-in
   adds?
   I vaguely recall a third-party "git-foo" program breaking in the
   past when invoked as "git foo" but not "git-foo", due to git
   squatting on SIGINT, but none of that should be relevant here
   (we're not starting a pager etc.).
1. https://lore.kernel.org/git/cover-v2-0.8-00000000000-20210926T185800Z-avarab@gmail.com/
2. https://lore.kernel.org/git/875yut8nns.fsf@evledraar.gmail.com/
3. https://lore.kernel.org/git/cover-v5-00.36-00000000000-20210902T125110Z-avarab@gmail.com/
4. https://lore.kernel.org/git/20210311021037.3001235-26-emilyshaffer@google.com/
5. https://lore.kernel.org/git/patch-09.13-69cc447a1e1-20211012T131934Z-avarab@gmail.com
Emily Shaffer (12):
  hook: add 'run' subcommand
  gc: use hook library for pre-auto-gc hook
  rebase: convert pre-rebase to use hook.h
  am: convert applypatch to use hook.h
  hooks: convert 'post-checkout' hook to hook library
  merge: convert post-merge to use hook.h
  send-email: use 'git hook run' for 'sendemail-validate'
  git-p4: use 'git hook' to run hooks
  commit: convert {pre-commit,prepare-commit-msg} hook to hook.h
  read-cache: convert post-index-change to use hook.h
  receive-pack: convert push-to-checkout hook to hook.h
  run-command: remove old run_hook_{le,ve}() hook API
Ævar Arnfjörð Bjarmason (1):
  git hook run: add an --ignore-missing flag
 .gitignore                 |   1 +
 Documentation/git-hook.txt |  46 +++++++++++++
 Documentation/githooks.txt |   4 ++
 Makefile                   |   1 +
 builtin.h                  |   1 +
 builtin/am.c               |   8 ++-
 builtin/checkout.c         |  14 ++--
 builtin/clone.c            |   7 +-
 builtin/gc.c               |   3 +-
 builtin/hook.c             |  96 ++++++++++++++++++++++++++
 builtin/merge.c            |   4 +-
 builtin/rebase.c           |   8 ++-
 builtin/receive-pack.c     |   7 +-
 builtin/worktree.c         |  28 ++++----
 command-list.txt           |   1 +
 commit.c                   |  15 +++--
 git-p4.py                  |  72 ++------------------
 git-send-email.perl        |  22 +++---
 git.c                      |   1 +
 hook.c                     | 130 +++++++++++++++++++++++++++++++++++
 hook.h                     |  61 +++++++++++++++++
 read-cache.c               |  11 ++-
 reset.c                    |  14 ++--
 run-command.c              |  32 ---------
 run-command.h              |  17 -----
 t/t1800-hook.sh            | 134 +++++++++++++++++++++++++++++++++++++
 t/t9001-send-email.sh      |   4 +-
 27 files changed, 572 insertions(+), 170 deletions(-)
 create mode 100644 Documentation/git-hook.txt
 create mode 100644 builtin/hook.c
 create mode 100755 t/t1800-hook.sh
Range-diff:
 1:  72dd1010f5b !  1:  a39c0748d3f hook: add 'run' subcommand
    @@ hook.h
     +	struct run_hooks_opt *options;
     +};
      
    - /**
    + /*
       * Returns the path to the hook file, or NULL if the hook is missing
     @@ hook.h: const char *find_hook(const char *name);
       */
    @@ t/t1800-hook.sh (new)
     +
     +test_description='git-hook command'
     +
    ++TEST_PASSES_SANITIZE_LEAK=true
     +. ./test-lib.sh
     +
     +test_expect_success 'git hook usage' '
 2:  821cc9bf11e !  2:  dbac4204f7b gc: use hook library for pre-auto-gc hook
    @@ hook.c: int run_hooks(const char *hook_name, const char *hook_path,
     +int run_hooks_oneshot(const char *hook_name, struct run_hooks_opt *options)
     +{
     +	const char *hook_path;
    -+	int ret;
     +	struct run_hooks_opt hook_opt_scratch = RUN_HOOKS_OPT_INIT;
    ++	int ret = 0;
     +
     +	if (!options)
     +		options = &hook_opt_scratch;
     +
     +	hook_path = find_hook(hook_name);
    -+	if (!hook_path) {
    -+		ret = 0;
    ++	if (!hook_path)
     +		goto cleanup;
    -+	}
     +
     +	ret = run_hooks(hook_name, hook_path, options);
    -+
     +cleanup:
     +	run_hooks_opt_clear(options);
     +
 3:  d71c90254ea !  3:  ff306debcb8 rebase: convert pre-rebase to use hook.h
    @@ builtin/rebase.c: 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);
    ++	strvec_push(&hook_opt.args, options.upstream_arg);
    ++	if (argc)
    ++		strvec_push(&hook_opt.args, argv[0]);
      	if (!ok_to_skip_pre_rebase &&
     -	    run_hook_le(NULL, "pre-rebase", options.upstream_arg,
     -			argc ? argv[0] : NULL, NULL))
 4:  ea3af2ccc4d =  4:  b1d529ca485 am: convert applypatch to use hook.h
 5:  fed0b52f88f !  5:  15d71fc210b hooks: convert 'post-checkout' hook to hook library
    @@ builtin/clone.c: static int checkout(int submodule_progress)
      
     -	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);
    ++	strvec_pushl(&hook_opt.args, oid_to_hex(null_oid()), oid_to_hex(&oid),
    ++		     "1", NULL);
     +	err |= run_hooks_oneshot("post-checkout", &hook_opt);
      
      	if (!err && (option_recurse_submodules.nr > 0)) {
 6:  53d8721a0e3 =  6:  08f27f0d6be merge: convert post-merge to use hook.h
 7:  d60827a2856 !  7:  107c14d740f git hook run: add an --ignore-missing flag
    @@ builtin/hook.c: static int run(int argc, const char **argv, const char *prefix)
      	const char *hook_path;
      	struct option run_options[] = {
     +		OPT_BOOL(0, "ignore-missing", &ignore_missing,
    -+			 N_("exit quietly with a zero exit code if the requested hook cannot be found")),
    ++			 N_("silently ignore missing requested <hook-name>")),
      		OPT_END(),
      	};
      	int ret;
 8:  d4976a0821f !  8:  1d30e2dbbe0 send-email: use 'git hook run' for 'sendemail-validate'
    @@ git-send-email.perl: sub validate_patch {
      				or die("chdir: $!");
      			local $ENV{"GIT_DIR"} = $repo->repo_path();
     -			$hook_error = system_or_msg([$validate_hook, $target]);
    -+			my @validate_hook = ("git", "hook", "run", "--ignore-missing", $hook_name, "--", $target);
    -+			$hook_error = system_or_msg(\@validate_hook, undef,
    -+						       "git hook run $hook_name -- <patch>");
    ++			my @cmd = ("git", "hook", "run", "--ignore-missing",
    ++				    $hook_name, "--");
    ++			my @cmd_msg = (@cmd, "<patch>");
    ++			my @cmd_run = (@cmd, $target);
    ++			$hook_error = system_or_msg(\@cmd_run, undef, "@cmd_msg");
      			chdir($cwd_save) or die("chdir: $!");
      		}
      		if ($hook_error) {
    @@ t/t9001-send-email.sh: test_expect_success $PREREQ "--validate respects relative
      	cat >expect <<-EOF &&
      	fatal: longline.patch: rejected by sendemail-validate hook
     -	fatal: command '"'"'my-hooks/sendemail-validate'"'"' died with exit code 1
    -+	fatal: command '"'"'git hook run sendemail-validate -- <patch>'"'"' died with exit code 1
    ++	fatal: command '"'"'git hook run --ignore-missing sendemail-validate -- <patch>'"'"' died with exit code 1
      	warning: no patches were sent
      	EOF
      	test_cmp expect actual
    @@ t/t9001-send-email.sh: test_expect_success $PREREQ "--validate respects absolute
      	cat >expect <<-EOF &&
      	fatal: longline.patch: rejected by sendemail-validate hook
     -	fatal: command '"'"'$hooks_path/sendemail-validate'"'"' died with exit code 1
    -+	fatal: command '"'"'git hook run sendemail-validate -- <patch>'"'"' died with exit code 1
    ++	fatal: command '"'"'git hook run --ignore-missing sendemail-validate -- <patch>'"'"' died with exit code 1
      	warning: no patches were sent
      	EOF
      	test_cmp expect actual
 9:  99f3dcd1945 =  9:  69cc447a1e1 git-p4: use 'git hook' to run hooks
10:  509761454e6 = 10:  1c22b2992cf commit: convert {pre-commit,prepare-commit-msg} hook to hook.h
11:  e2c94d95427 ! 11:  e762fce32af read-cache: convert post-index-change to use hook.h
    @@ Commit message
         Move the post-index-change hook away from run-command.h to and over to
         the new hook.h library.
     
    -    This removes the last direct user of run_hook_ve(), so we can make the
    -    function static now. It'll be removed entirely soon.
    +    This removes the last direct user of "run_hook_ve()" outside of
    +    run-command.c ("run_hook_le()" still uses it). So we can make the
    +    function static now. A subsequent commit will remove this code
    +    entirely when "run_hook_le()" itself goes away.
     
         Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
    @@ read-cache.c: static int do_write_locked_index(struct index_state *istate, struc
      	int was_full = !istate->sparse_index;
     +	struct run_hooks_opt hook_opt = RUN_HOOKS_OPT_INIT;
      
    - 	ret = convert_to_sparse(istate);
    + 	ret = convert_to_sparse(istate, 0);
      
     @@ read-cache.c: static int do_write_locked_index(struct index_state *istate, struct lock_file *l
      	else
12:  fa7d0d24ea2 = 12:  d63b91196ae receive-pack: convert push-to-checkout hook to hook.h
13:  428bb5a6792 = 13:  fe8996dda3e run-command: remove old run_hook_{le,ve}() hook API
-- 
2.33.0.1567.g7b23ce7ed9e
Ævar Arnfjörð Bjarmason· Oct 12, 2021, 13:30 UTC · re: Ævar Arnfjörð Bjarmason · lore

[PATCH 01/13] hook: add 'run' subcommand

From: Emily Shaffer <emilyshaffer@google.com>

In order to enable hooks to be run as an external process, by a standalone Git command, or by tools which wrap Git, provide an external means to run all configured hook commands for a given hook event.

Most of our hooks require more complex functionality than this, but let's start with the bare minimum required to support our simplest hooks.

In terms of implementation the usage_with_options() and "goto usage" pattern here mirrors that of builtin/{commit-graph,multi-pack-index}.c.

Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 .gitignore                 |   1 +
 Documentation/git-hook.txt |  38 +++++++++++
 Documentation/githooks.txt |   4 ++
 Makefile                   |   1 +
 builtin.h                  |   1 +
 builtin/hook.c             |  90 ++++++++++++++++++++++++++
 command-list.txt           |   1 +
 git.c                      |   1 +
 hook.c                     | 101 +++++++++++++++++++++++++++++
 hook.h                     |  39 +++++++++++
 t/t1800-hook.sh            | 129 +++++++++++++++++++++++++++++++++++++
 11 files changed, 406 insertions(+)
 create mode 100644 Documentation/git-hook.txt
 create mode 100644 builtin/hook.c
 create mode 100755 t/t1800-hook.sh
Show changes to 11 files +406 −0

.gitignore, Documentation/git-hook.txt, Documentation/githooks.txt, Makefile, builtin.h, builtin/hook.c, command-list.txt, git.c, hook.c, hook.h, t/t1800-hook.sh

diff --git a/.gitignore b/.gitignore
index 6be9de41ae8..66189ca3cdc 100644
--- a/.gitignore
+++ b/.gitignore
@@ -77,6 +77,7 @@
 /git-grep
 /git-hash-object
 /git-help
+/git-hook
 /git-http-backend
 /git-http-fetch
 /git-http-push
diff --git a/Documentation/git-hook.txt b/Documentation/git-hook.txt
new file mode 100644
index 00000000000..660d6a992a0
--- /dev/null
+++ b/Documentation/git-hook.txt
@@ -0,0 +1,38 @@
+git-hook(1)
+===========
+
+NAME
+----
+git-hook - run git hooks
+
+SYNOPSIS
+--------
+[verse]
+'git hook' run <hook-name> [-- <hook-args>]
+
+DESCRIPTION
+-----------
+
+This command is an interface to git hooks (see linkgit:githooks[5]).
+Currently it only provides a convenience wrapper for running hooks for
+use by git itself. In the future it might gain other functionality.
+
+SUBCOMMANDS
+-----------
+
+run::
+	Run the `<hook-name>` hook. See linkgit:githooks[5] for
+	the hook names we support.
++
+Any positional arguments to the hook should be passed after an
+optional `--` (or `--end-of-options`, see linkgit:gitcli[7]). The
+arguments (if any) differ by hook name, see linkgit:githooks[5] for
+what those are.
+
+SEE ALSO
+--------
+linkgit:githooks[5]
+
+GIT
+---
+Part of the linkgit:git[1] suite
diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
index b51959ff941..a16e62bc8c8 100644
--- a/Documentation/githooks.txt
+++ b/Documentation/githooks.txt
@@ -698,6 +698,10 @@ 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.
 
+SEE ALSO
+--------
+linkgit:git-hook[1]
+
 GIT
 ---
 Part of the linkgit:git[1] suite
diff --git a/Makefile b/Makefile
index 877492386ee..12b481fdabe 100644
--- a/Makefile
+++ b/Makefile
@@ -1108,6 +1108,7 @@ BUILTIN_OBJS += builtin/get-tar-commit-id.o
 BUILTIN_OBJS += builtin/grep.o
 BUILTIN_OBJS += builtin/hash-object.o
 BUILTIN_OBJS += builtin/help.o
+BUILTIN_OBJS += builtin/hook.o
 BUILTIN_OBJS += builtin/index-pack.o
 BUILTIN_OBJS += builtin/init-db.o
 BUILTIN_OBJS += builtin/interpret-trailers.o
diff --git a/builtin.h b/builtin.h
index 8a58743ed63..83379f3832c 100644
--- a/builtin.h
+++ b/builtin.h
@@ -164,6 +164,7 @@ int cmd_get_tar_commit_id(int argc, const char **argv, const char *prefix);
 int cmd_grep(int argc, const char **argv, const char *prefix);
 int cmd_hash_object(int argc, const char **argv, const char *prefix);
 int cmd_help(int argc, const char **argv, const char *prefix);
+int cmd_hook(int argc, const char **argv, const char *prefix);
 int cmd_index_pack(int argc, const char **argv, const char *prefix);
 int cmd_init_db(int argc, const char **argv, const char *prefix);
 int cmd_interpret_trailers(int argc, const char **argv, const char *prefix);
diff --git a/builtin/hook.c b/builtin/hook.c
new file mode 100644
index 00000000000..012a2973b38
--- /dev/null
+++ b/builtin/hook.c
@@ -0,0 +1,90 @@
+#include "cache.h"
+#include "builtin.h"
+#include "config.h"
+#include "hook.h"
+#include "parse-options.h"
+#include "strbuf.h"
+#include "strvec.h"
+
+#define BUILTIN_HOOK_RUN_USAGE \
+	N_("git hook run <hook-name> [-- <hook-args>]")
+
+static const char * const builtin_hook_usage[] = {
+	BUILTIN_HOOK_RUN_USAGE,
+	NULL
+};
+
+static const char * const builtin_hook_run_usage[] = {
+	BUILTIN_HOOK_RUN_USAGE,
+	NULL
+};
+
+static int run(int argc, const char **argv, const char *prefix)
+{
+	int i;
+	struct run_hooks_opt opt = RUN_HOOKS_OPT_INIT;
+	const char *hook_name;
+	const char *hook_path;
+	struct option run_options[] = {
+		OPT_END(),
+	};
+	int ret;
+
+	argc = parse_options(argc, argv, prefix, run_options,
+			     builtin_hook_run_usage,
+			     PARSE_OPT_KEEP_DASHDASH);
+
+	if (!argc)
+		goto usage;
+
+	/*
+	 * Having a -- for "run" when providing <hook-args> is
+	 * mandatory.
+	 */
+	if (argc > 1 && strcmp(argv[1], "--") &&
+	    strcmp(argv[1], "--end-of-options"))
+		goto usage;
+
+	/* Add our arguments, start after -- */
+	for (i = 2 ; i < argc; i++)
+		strvec_push(&opt.args, argv[i]);
+
+	/* Need to take into account core.hooksPath */
+	git_config(git_default_config, NULL);
+
+	/*
+	 * We are not using a plain run_hooks() because we'd like to
+	 * detect missing hooks. Let's find it ourselves and call
+	 * run_hooks() instead.
+	 */
+	hook_name = argv[0];
+	hook_path = find_hook(hook_name);
+	if (!hook_path) {
+		error("cannot find a hook named %s", hook_name);
+		return 1;
+	}
+
+	ret = run_hooks(hook_name, hook_path, &opt);
+	run_hooks_opt_clear(&opt);
+	return ret;
+usage:
+	usage_with_options(builtin_hook_run_usage, run_options);
+}
+
+int cmd_hook(int argc, const char **argv, const char *prefix)
+{
+	struct option builtin_hook_options[] = {
+		OPT_END(),
+	};
+
+	argc = parse_options(argc, argv, NULL, builtin_hook_options,
+			     builtin_hook_usage, PARSE_OPT_STOP_AT_NON_OPTION);
+	if (!argc)
+		goto usage;
+
+	if (!strcmp(argv[0], "run"))
+		return run(argc, argv, prefix);
+
+usage:
+	usage_with_options(builtin_hook_usage, builtin_hook_options);
+}
diff --git a/command-list.txt b/command-list.txt
index a289f09ed6f..9ccd8e5aebe 100644
--- a/command-list.txt
+++ b/command-list.txt
@@ -103,6 +103,7 @@ git-grep                                mainporcelain           info
 git-gui                                 mainporcelain
 git-hash-object                         plumbingmanipulators
 git-help                                ancillaryinterrogators          complete
+git-hook                                mainporcelain
 git-http-backend                        synchingrepositories
 git-http-fetch                          synchelpers
 git-http-push                           synchelpers
diff --git a/git.c b/git.c
index 60c2784be45..e5891e02021 100644
--- a/git.c
+++ b/git.c
@@ -538,6 +538,7 @@ static struct cmd_struct commands[] = {
 	{ "grep", cmd_grep, RUN_SETUP_GENTLY },
 	{ "hash-object", cmd_hash_object },
 	{ "help", cmd_help },
+	{ "hook", cmd_hook, RUN_SETUP },
 	{ "index-pack", cmd_index_pack, RUN_SETUP_GENTLY | NO_PARSEOPT },
 	{ "init", cmd_init_db },
 	{ "init-db", cmd_init_db },
diff --git a/hook.c b/hook.c
index 55e1145a4b7..240270db64e 100644
--- a/hook.c
+++ b/hook.c
@@ -1,6 +1,7 @@
 #include "cache.h"
 #include "hook.h"
 #include "run-command.h"
+#include "config.h"
 
 const char *find_hook(const char *name)
 {
@@ -40,3 +41,103 @@ int hook_exists(const char *name)
 {
 	return !!find_hook(name);
 }
+
+void run_hooks_opt_clear(struct run_hooks_opt *o)
+{
+	strvec_clear(&o->env);
+	strvec_clear(&o->args);
+}
+
+static int pick_next_hook(struct child_process *cp,
+			  struct strbuf *out,
+			  void *pp_cb,
+			  void **pp_task_cb)
+{
+	struct hook_cb_data *hook_cb = pp_cb;
+	struct hook *run_me = hook_cb->run_me;
+
+	if (!run_me)
+		return 0;
+
+	cp->no_stdin = 1;
+	cp->env = hook_cb->options->env.v;
+	cp->stdout_to_stderr = 1;
+	cp->trace2_hook_name = hook_cb->hook_name;
+
+	/* add command */
+	strvec_push(&cp->args, run_me->hook_path);
+
+	/*
+	 * add passed-in argv, without expanding - let the user get back
+	 * exactly what they put in
+	 */
+	strvec_pushv(&cp->args, hook_cb->options->args.v);
+
+	/* Provide context for errors if necessary */
+	*pp_task_cb = run_me;
+
+	/*
+	 * This pick_next_hook() will be called again, we're only
+	 * running one hook, so indicate that no more work will be
+	 * done.
+	 */
+	hook_cb->run_me = NULL;
+
+	return 1;
+}
+
+static int notify_start_failure(struct strbuf *out,
+				void *pp_cb,
+				void *pp_task_cp)
+{
+	struct hook_cb_data *hook_cb = pp_cb;
+	struct hook *attempted = pp_task_cp;
+
+	hook_cb->rc |= 1;
+
+	strbuf_addf(out, _("Couldn't start hook '%s'\n"),
+		    attempted->hook_path);
+
+	return 1;
+}
+
+static int notify_hook_finished(int result,
+				struct strbuf *out,
+				void *pp_cb,
+				void *pp_task_cb)
+{
+	struct hook_cb_data *hook_cb = pp_cb;
+
+	hook_cb->rc |= result;
+
+	return 0;
+}
+
+int run_hooks(const char *hook_name, const char *hook_path,
+	      struct run_hooks_opt *options)
+{
+	struct hook my_hook = {
+		.hook_path = hook_path,
+	};
+	struct hook_cb_data cb_data = {
+		.rc = 0,
+		.hook_name = hook_name,
+		.options = options,
+	};
+	int jobs = 1;
+
+	if (!options)
+		BUG("a struct run_hooks_opt must be provided to run_hooks");
+
+	cb_data.run_me = &my_hook;
+
+	run_processes_parallel_tr2(jobs,
+				   pick_next_hook,
+				   notify_start_failure,
+				   notify_hook_finished,
+				   &cb_data,
+				   "hook",
+				   hook_name);
+
+	return cb_data.rc;
+}
diff --git a/hook.h b/hook.h
index 6aa36fc7ff9..111c5cf9296 100644
--- a/hook.h
+++ b/hook.h
@@ -1,5 +1,33 @@
 #ifndef HOOK_H
 #define HOOK_H
+#include "strvec.h"
+
+struct hook {
+	/* The path to the hook */
+	const char *hook_path;
+};
+
+struct run_hooks_opt
+{
+	/* Environment vars to be set for each hook */
+	struct strvec env;
+
+	/* Args to be passed to each hook */
+	struct strvec args;
+};
+
+#define RUN_HOOKS_OPT_INIT { \
+	.env = STRVEC_INIT, \
+	.args = STRVEC_INIT, \
+}
+
+struct hook_cb_data {
+	/* rc reflects the cumulative failure state */
+	int rc;
+	const char *hook_name;
+	struct hook *run_me;
+	struct run_hooks_opt *options;
+};
 
 /*
  * Returns the path to the hook file, or NULL if the hook is missing
@@ -13,4 +41,15 @@ const char *find_hook(const char *name);
  */
 int hook_exists(const char *hookname);
 
+/**
+ * Clear data from an initialized "struct run_hooks-opt".
+ */
+void run_hooks_opt_clear(struct run_hooks_opt *o);
+
+/**
+ * Takes an already resolved hook found via find_hook() and runs
+ * it. Does not call run_hooks_opt_clear() for you.
+ */
+int run_hooks(const char *hookname, const char *hook_path,
+	      struct run_hooks_opt *options);
 #endif
diff --git a/t/t1800-hook.sh b/t/t1800-hook.sh
new file mode 100755
index 00000000000..3aea1b105f0
--- /dev/null
+++ b/t/t1800-hook.sh
@@ -0,0 +1,129 @@
+#!/bin/sh
+
+test_description='git-hook command'
+
+TEST_PASSES_SANITIZE_LEAK=true
+. ./test-lib.sh
+
+test_expect_success 'git hook usage' '
+	test_expect_code 129 git hook &&
+	test_expect_code 129 git hook run &&
+	test_expect_code 129 git hook run -h &&
+	test_expect_code 129 git hook run --unknown 2>err &&
+	grep "unknown option" err
+'
+
+test_expect_success 'git hook run: nonexistent hook' '
+	cat >stderr.expect <<-\EOF &&
+	error: cannot find a hook named test-hook
+	EOF
+	test_expect_code 1 git hook run test-hook 2>stderr.actual &&
+	test_cmp stderr.expect stderr.actual
+'
+
+test_expect_success 'git hook run: basic' '
+	write_script .git/hooks/test-hook <<-EOF &&
+	echo Test hook
+	EOF
+
+	cat >expect <<-\EOF &&
+	Test hook
+	EOF
+	git hook run test-hook 2>actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'git hook run: stdout and stderr both write to our stderr' '
+	write_script .git/hooks/test-hook <<-EOF &&
+	echo >&1 Will end up on stderr
+	echo >&2 Will end up on stderr
+	EOF
+
+	cat >stderr.expect <<-\EOF &&
+	Will end up on stderr
+	Will end up on stderr
+	EOF
+	git hook run test-hook >stdout.actual 2>stderr.actual &&
+	test_cmp stderr.expect stderr.actual &&
+	test_must_be_empty stdout.actual
+'
+
+test_expect_success 'git hook run: exit codes are passed along' '
+	write_script .git/hooks/test-hook <<-EOF &&
+	exit 1
+	EOF
+
+	test_expect_code 1 git hook run test-hook &&
+
+	write_script .git/hooks/test-hook <<-EOF &&
+	exit 2
+	EOF
+
+	test_expect_code 2 git hook run test-hook &&
+
+	write_script .git/hooks/test-hook <<-EOF &&
+	exit 128
+	EOF
+
+	test_expect_code 128 git hook run test-hook &&
+
+	write_script .git/hooks/test-hook <<-EOF &&
+	exit 129
+	EOF
+
+	test_expect_code 129 git hook run test-hook
+'
+
+test_expect_success 'git hook run arg u ments without -- is not allowed' '
+	test_expect_code 129 git hook run test-hook arg u ments
+'
+
+test_expect_success 'git hook run -- pass arguments' '
+	write_script .git/hooks/test-hook <<-\EOF &&
+	echo $1
+	echo $2
+	EOF
+
+	cat >expect <<-EOF &&
+	arg
+	u ments
+	EOF
+
+	git hook run test-hook -- arg "u ments" 2>actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'git hook run -- out-of-repo runs excluded' '
+	write_script .git/hooks/test-hook <<-EOF &&
+	echo Test hook
+	EOF
+
+	nongit test_must_fail git hook run test-hook
+'
+
+test_expect_success 'git -c core.hooksPath=<PATH> hook run' '
+	mkdir my-hooks &&
+	write_script my-hooks/test-hook <<-\EOF &&
+	echo Hook ran $1 >>actual
+	EOF
+
+	cat >expect <<-\EOF &&
+	Test hook
+	Hook ran one
+	Hook ran two
+	Hook ran three
+	Hook ran four
+	EOF
+
+	# Test various ways of specifying the path. See also
+	# t1350-config-hooks-path.sh
+	>actual &&
+	git hook run test-hook -- ignored 2>>actual &&
+	git -c core.hooksPath=my-hooks hook run test-hook -- one 2>>actual &&
+	git -c core.hooksPath=my-hooks/ hook run test-hook -- two 2>>actual &&
+	git -c core.hooksPath="$PWD/my-hooks" hook run test-hook -- three 2>>actual &&
+	git -c core.hooksPath="$PWD/my-hooks/" hook run test-hook -- four 2>>actual &&
+	test_cmp expect actual
+'
+
+test_done
-- 
2.33.0.1567.g7b23ce7ed9e
René Scharfe· Oct 12, 2021, 17:35 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [PATCH 01/13] hook: add 'run' subcommand

Am 12.10.21 um 15:30 schrieb Ævar Arnfjörð Bjarmason:
Show 69 quoted lines
> From: Emily Shaffer <emilyshaffer@google.com>
>
> In order to enable hooks to be run as an external process, by a
> standalone Git command, or by tools which wrap Git, provide an external
> means to run all configured hook commands for a given hook event.
>
> Most of our hooks require more complex functionality than this, but
> let's start with the bare minimum required to support our simplest
> hooks.
>
> In terms of implementation the usage_with_options() and "goto usage"
> pattern here mirrors that of
> builtin/{commit-graph,multi-pack-index}.c.
>
> Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  .gitignore                 |   1 +
>  Documentation/git-hook.txt |  38 +++++++++++
>  Documentation/githooks.txt |   4 ++
>  Makefile                   |   1 +
>  builtin.h                  |   1 +
>  builtin/hook.c             |  90 ++++++++++++++++++++++++++
>  command-list.txt           |   1 +
>  git.c                      |   1 +
>  hook.c                     | 101 +++++++++++++++++++++++++++++
>  hook.h                     |  39 +++++++++++
>  t/t1800-hook.sh            | 129 +++++++++++++++++++++++++++++++++++++
>  11 files changed, 406 insertions(+)
>  create mode 100644 Documentation/git-hook.txt
>  create mode 100644 builtin/hook.c
>  create mode 100755 t/t1800-hook.sh
>
> diff --git a/.gitignore b/.gitignore
> index 6be9de41ae8..66189ca3cdc 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -77,6 +77,7 @@
>  /git-grep
>  /git-hash-object
>  /git-help
> +/git-hook
>  /git-http-backend
>  /git-http-fetch
>  /git-http-push
> diff --git a/Documentation/git-hook.txt b/Documentation/git-hook.txt
> new file mode 100644
> index 00000000000..660d6a992a0
> --- /dev/null
> +++ b/Documentation/git-hook.txt
> @@ -0,0 +1,38 @@
> +git-hook(1)
> +===========
> +
> +NAME
> +----
> +git-hook - run git hooks
> +
> +SYNOPSIS
> +--------
> +[verse]
> +'git hook' run <hook-name> [-- <hook-args>]
> +
> +DESCRIPTION
> +-----------
> +
> +This command is an interface to git hooks (see linkgit:githooks[5]).
> +Currently it only provides a convenience wrapper for running hooks for
> +use by git itself. In the future it might gain other functionality.

As a man page reader I'm only interested in the present features of the command up here. Breaking changes could be mentioned in a HISTORY section, and missing critical features in a BUGS section at the bottom.

I assume the last sentence applies to almost all programs, and could be safely removed.

Show 10 quoted lines
> +
> +SUBCOMMANDS
> +-----------
> +
> +run::
> +	Run the `<hook-name>` hook. See linkgit:githooks[5] for
> +	the hook names we support.
> ++
> +Any positional arguments to the hook should be passed after an
> +optional `--` (or `--end-of-options`, see linkgit:gitcli[7]). The
Dash dash (or EoO) is optional.
Show 101 quoted lines
> +arguments (if any) differ by hook name, see linkgit:githooks[5] for
> +what those are.
> +
> +SEE ALSO
> +--------
> +linkgit:githooks[5]
> +
> +GIT
> +---
> +Part of the linkgit:git[1] suite
> diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
> index b51959ff941..a16e62bc8c8 100644
> --- a/Documentation/githooks.txt
> +++ b/Documentation/githooks.txt
> @@ -698,6 +698,10 @@ 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.
>
> +SEE ALSO
> +--------
> +linkgit:git-hook[1]
> +
>  GIT
>  ---
>  Part of the linkgit:git[1] suite
> diff --git a/Makefile b/Makefile
> index 877492386ee..12b481fdabe 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1108,6 +1108,7 @@ BUILTIN_OBJS += builtin/get-tar-commit-id.o
>  BUILTIN_OBJS += builtin/grep.o
>  BUILTIN_OBJS += builtin/hash-object.o
>  BUILTIN_OBJS += builtin/help.o
> +BUILTIN_OBJS += builtin/hook.o
>  BUILTIN_OBJS += builtin/index-pack.o
>  BUILTIN_OBJS += builtin/init-db.o
>  BUILTIN_OBJS += builtin/interpret-trailers.o
> diff --git a/builtin.h b/builtin.h
> index 8a58743ed63..83379f3832c 100644
> --- a/builtin.h
> +++ b/builtin.h
> @@ -164,6 +164,7 @@ int cmd_get_tar_commit_id(int argc, const char **argv, const char *prefix);
>  int cmd_grep(int argc, const char **argv, const char *prefix);
>  int cmd_hash_object(int argc, const char **argv, const char *prefix);
>  int cmd_help(int argc, const char **argv, const char *prefix);
> +int cmd_hook(int argc, const char **argv, const char *prefix);
>  int cmd_index_pack(int argc, const char **argv, const char *prefix);
>  int cmd_init_db(int argc, const char **argv, const char *prefix);
>  int cmd_interpret_trailers(int argc, const char **argv, const char *prefix);
> diff --git a/builtin/hook.c b/builtin/hook.c
> new file mode 100644
> index 00000000000..012a2973b38
> --- /dev/null
> +++ b/builtin/hook.c
> @@ -0,0 +1,90 @@
> +#include "cache.h"
> +#include "builtin.h"
> +#include "config.h"
> +#include "hook.h"
> +#include "parse-options.h"
> +#include "strbuf.h"
> +#include "strvec.h"
> +
> +#define BUILTIN_HOOK_RUN_USAGE \
> +	N_("git hook run <hook-name> [-- <hook-args>]")
> +
> +static const char * const builtin_hook_usage[] = {
> +	BUILTIN_HOOK_RUN_USAGE,
> +	NULL
> +};
> +
> +static const char * const builtin_hook_run_usage[] = {
> +	BUILTIN_HOOK_RUN_USAGE,
> +	NULL
> +};
> +
> +static int run(int argc, const char **argv, const char *prefix)
> +{
> +	int i;
> +	struct run_hooks_opt opt = RUN_HOOKS_OPT_INIT;
> +	const char *hook_name;
> +	const char *hook_path;
> +	struct option run_options[] = {
> +		OPT_END(),
> +	};
> +	int ret;
> +
> +	argc = parse_options(argc, argv, prefix, run_options,
> +			     builtin_hook_run_usage,
> +			     PARSE_OPT_KEEP_DASHDASH);
> +
> +	if (!argc)
> +		goto usage;
> +
> +	/*
> +	 * Having a -- for "run" when providing <hook-args> is
> +	 * mandatory.
> +	 */
> +	if (argc > 1 && strcmp(argv[1], "--") &&
> +	    strcmp(argv[1], "--end-of-options"))
> +		goto usage;

Dash dash (or EoO) is mandatory unless parse_options() left no arguments, contrary to what the documentation says above. Update the doc above?

Show 12 quoted lines
> +
> +	/* Add our arguments, start after -- */
> +	for (i = 2 ; i < argc; i++)
> +		strvec_push(&opt.args, argv[i]);
> +
> +	/* Need to take into account core.hooksPath */
> +	git_config(git_default_config, NULL);
> +
> +	/*
> +	 * We are not using a plain run_hooks() because we'd like to
> +	 * detect missing hooks. Let's find it ourselves and call
> +	 * run_hooks() instead.

So run_hooks(), which is introduced later in this patch, can find a hook as well, but would do so silently?

> +	 */
> +	hook_name = argv[0];
> +	hook_path = find_hook(hook_name);

This is the only find_hook() call introduced by this patch. Does run_hooks() really posses an unused hook-finding capability?

Show 98 quoted lines
> +	if (!hook_path) {
> +		error("cannot find a hook named %s", hook_name);
> +		return 1;
> +	}
> +
> +	ret = run_hooks(hook_name, hook_path, &opt);
> +	run_hooks_opt_clear(&opt);
> +	return ret;
> +usage:
> +	usage_with_options(builtin_hook_run_usage, run_options);
> +}
> +
> +int cmd_hook(int argc, const char **argv, const char *prefix)
> +{
> +	struct option builtin_hook_options[] = {
> +		OPT_END(),
> +	};
> +
> +	argc = parse_options(argc, argv, NULL, builtin_hook_options,
> +			     builtin_hook_usage, PARSE_OPT_STOP_AT_NON_OPTION);
> +	if (!argc)
> +		goto usage;
> +
> +	if (!strcmp(argv[0], "run"))
> +		return run(argc, argv, prefix);
> +
> +usage:
> +	usage_with_options(builtin_hook_usage, builtin_hook_options);
> +}
> diff --git a/command-list.txt b/command-list.txt
> index a289f09ed6f..9ccd8e5aebe 100644
> --- a/command-list.txt
> +++ b/command-list.txt
> @@ -103,6 +103,7 @@ git-grep                                mainporcelain           info
>  git-gui                                 mainporcelain
>  git-hash-object                         plumbingmanipulators
>  git-help                                ancillaryinterrogators          complete
> +git-hook                                mainporcelain
>  git-http-backend                        synchingrepositories
>  git-http-fetch                          synchelpers
>  git-http-push                           synchelpers
> diff --git a/git.c b/git.c
> index 60c2784be45..e5891e02021 100644
> --- a/git.c
> +++ b/git.c
> @@ -538,6 +538,7 @@ static struct cmd_struct commands[] = {
>  	{ "grep", cmd_grep, RUN_SETUP_GENTLY },
>  	{ "hash-object", cmd_hash_object },
>  	{ "help", cmd_help },
> +	{ "hook", cmd_hook, RUN_SETUP },
>  	{ "index-pack", cmd_index_pack, RUN_SETUP_GENTLY | NO_PARSEOPT },
>  	{ "init", cmd_init_db },
>  	{ "init-db", cmd_init_db },
> diff --git a/hook.c b/hook.c
> index 55e1145a4b7..240270db64e 100644
> --- a/hook.c
> +++ b/hook.c
> @@ -1,6 +1,7 @@
>  #include "cache.h"
>  #include "hook.h"
>  #include "run-command.h"
> +#include "config.h"
>
>  const char *find_hook(const char *name)
>  {
> @@ -40,3 +41,103 @@ int hook_exists(const char *name)
>  {
>  	return !!find_hook(name);
>  }
> +
> +void run_hooks_opt_clear(struct run_hooks_opt *o)
> +{
> +	strvec_clear(&o->env);
> +	strvec_clear(&o->args);
> +}
> +
> +static int pick_next_hook(struct child_process *cp,
> +			  struct strbuf *out,
> +			  void *pp_cb,
> +			  void **pp_task_cb)
> +{
> +	struct hook_cb_data *hook_cb = pp_cb;
> +	struct hook *run_me = hook_cb->run_me;
> +
> +	if (!run_me)
> +		return 0;
> +
> +	cp->no_stdin = 1;
> +	cp->env = hook_cb->options->env.v;
> +	cp->stdout_to_stderr = 1;
> +	cp->trace2_hook_name = hook_cb->hook_name;
> +
> +	/* add command */
> +	strvec_push(&cp->args, run_me->hook_path);
> +
> +	/*
> +	 * add passed-in argv, without expanding - let the user get back
> +	 * exactly what they put in

What kind of expanding is it doing without? I was expecting the arguments to passed on verbatim before reading this comment, so it leaves me wondering what I'm missing.

Show 12 quoted lines
> +	 */
> +	strvec_pushv(&cp->args, hook_cb->options->args.v);
> +
> +	/* Provide context for errors if necessary */
> +	*pp_task_cb = run_me;
> +
> +	/*
> +	 * This pick_next_hook() will be called again, we're only
> +	 * running one hook, so indicate that no more work will be
> +	 * done.
> +	 */
> +	hook_cb->run_me = NULL;
A single hook is picked.
Show 56 quoted lines
> +
> +	return 1;
> +}
> +
> +static int notify_start_failure(struct strbuf *out,
> +				void *pp_cb,
> +				void *pp_task_cp)
> +{
> +	struct hook_cb_data *hook_cb = pp_cb;
> +	struct hook *attempted = pp_task_cp;
> +
> +	hook_cb->rc |= 1;
> +
> +	strbuf_addf(out, _("Couldn't start hook '%s'\n"),
> +		    attempted->hook_path);
> +
> +	return 1;
> +}
> +
> +static int notify_hook_finished(int result,
> +				struct strbuf *out,
> +				void *pp_cb,
> +				void *pp_task_cb)
> +{
> +	struct hook_cb_data *hook_cb = pp_cb;
> +
> +	hook_cb->rc |= result;
> +
> +	return 0;
> +}
> +
> +int run_hooks(const char *hook_name, const char *hook_path,
> +	      struct run_hooks_opt *options)
> +{
> +	struct hook my_hook = {
> +		.hook_path = hook_path,
> +	};
> +	struct hook_cb_data cb_data = {
> +		.rc = 0,
> +		.hook_name = hook_name,
> +		.options = options,
> +	};
> +	int jobs = 1;
> +
> +	if (!options)
> +		BUG("a struct run_hooks_opt must be provided to run_hooks");
> +
> +	cb_data.run_me = &my_hook;
> +
> +	run_processes_parallel_tr2(jobs,
> +				   pick_next_hook,
> +				   notify_start_failure,
> +				   notify_hook_finished,
> +				   &cb_data,
> +				   "hook",
> +				   hook_name);

A single process (jobs == 1) is used to run a single hook. Why use run_processes_parallel_tr2() for that instead of run_command()? The latter would require a lot less code to achieve the same outcome, no?

Show 16 quoted lines
> +
> +	return cb_data.rc;
> +}
> diff --git a/hook.h b/hook.h
> index 6aa36fc7ff9..111c5cf9296 100644
> --- a/hook.h
> +++ b/hook.h
> @@ -1,5 +1,33 @@
>  #ifndef HOOK_H
>  #define HOOK_H
> +#include "strvec.h"
> +
> +struct hook {
> +	/* The path to the hook */
> +	const char *hook_path;
> +};

None of the patches in this series extend this structure. Why not use a bare string directly?

Show 31 quoted lines
> +
> +struct run_hooks_opt
> +{
> +	/* Environment vars to be set for each hook */
> +	struct strvec env;
> +
> +	/* Args to be passed to each hook */
> +	struct strvec args;
> +};
> +
> +#define RUN_HOOKS_OPT_INIT { \
> +	.env = STRVEC_INIT, \
> +	.args = STRVEC_INIT, \
> +}
> +
> +struct hook_cb_data {
> +	/* rc reflects the cumulative failure state */
> +	int rc;
> +	const char *hook_name;
> +	struct hook *run_me;
> +	struct run_hooks_opt *options;
> +};
>
>  /*
>   * Returns the path to the hook file, or NULL if the hook is missing
> @@ -13,4 +41,15 @@ const char *find_hook(const char *name);
>   */
>  int hook_exists(const char *hookname);
>
> +/**
> + * Clear data from an initialized "struct run_hooks-opt".
                                                      ^
y/-/_/
Show 9 quoted lines
> + */
> +void run_hooks_opt_clear(struct run_hooks_opt *o);
> +
> +/**
> + * Takes an already resolved hook found via find_hook() and runs
> + * it. Does not call run_hooks_opt_clear() for you.
> + */
> +int run_hooks(const char *hookname, const char *hook_path,
> +	      struct run_hooks_opt *options);
If it runs one hook, why is it called run_hooks(), plural?
Show 137 quoted lines
>  #endif
> diff --git a/t/t1800-hook.sh b/t/t1800-hook.sh
> new file mode 100755
> index 00000000000..3aea1b105f0
> --- /dev/null
> +++ b/t/t1800-hook.sh
> @@ -0,0 +1,129 @@
> +#!/bin/sh
> +
> +test_description='git-hook command'
> +
> +TEST_PASSES_SANITIZE_LEAK=true
> +. ./test-lib.sh
> +
> +test_expect_success 'git hook usage' '
> +	test_expect_code 129 git hook &&
> +	test_expect_code 129 git hook run &&
> +	test_expect_code 129 git hook run -h &&
> +	test_expect_code 129 git hook run --unknown 2>err &&
> +	grep "unknown option" err
> +'
> +
> +test_expect_success 'git hook run: nonexistent hook' '
> +	cat >stderr.expect <<-\EOF &&
> +	error: cannot find a hook named test-hook
> +	EOF
> +	test_expect_code 1 git hook run test-hook 2>stderr.actual &&
> +	test_cmp stderr.expect stderr.actual
> +'
> +
> +test_expect_success 'git hook run: basic' '
> +	write_script .git/hooks/test-hook <<-EOF &&
> +	echo Test hook
> +	EOF
> +
> +	cat >expect <<-\EOF &&
> +	Test hook
> +	EOF
> +	git hook run test-hook 2>actual &&
> +	test_cmp expect actual
> +'
> +
> +test_expect_success 'git hook run: stdout and stderr both write to our stderr' '
> +	write_script .git/hooks/test-hook <<-EOF &&
> +	echo >&1 Will end up on stderr
> +	echo >&2 Will end up on stderr
> +	EOF
> +
> +	cat >stderr.expect <<-\EOF &&
> +	Will end up on stderr
> +	Will end up on stderr
> +	EOF
> +	git hook run test-hook >stdout.actual 2>stderr.actual &&
> +	test_cmp stderr.expect stderr.actual &&
> +	test_must_be_empty stdout.actual
> +'
> +
> +test_expect_success 'git hook run: exit codes are passed along' '
> +	write_script .git/hooks/test-hook <<-EOF &&
> +	exit 1
> +	EOF
> +
> +	test_expect_code 1 git hook run test-hook &&
> +
> +	write_script .git/hooks/test-hook <<-EOF &&
> +	exit 2
> +	EOF
> +
> +	test_expect_code 2 git hook run test-hook &&
> +
> +	write_script .git/hooks/test-hook <<-EOF &&
> +	exit 128
> +	EOF
> +
> +	test_expect_code 128 git hook run test-hook &&
> +
> +	write_script .git/hooks/test-hook <<-EOF &&
> +	exit 129
> +	EOF
> +
> +	test_expect_code 129 git hook run test-hook
> +'
> +
> +test_expect_success 'git hook run arg u ments without -- is not allowed' '
> +	test_expect_code 129 git hook run test-hook arg u ments
> +'
> +
> +test_expect_success 'git hook run -- pass arguments' '
> +	write_script .git/hooks/test-hook <<-\EOF &&
> +	echo $1
> +	echo $2
> +	EOF
> +
> +	cat >expect <<-EOF &&
> +	arg
> +	u ments
> +	EOF
> +
> +	git hook run test-hook -- arg "u ments" 2>actual &&
> +	test_cmp expect actual
> +'
> +
> +test_expect_success 'git hook run -- out-of-repo runs excluded' '
> +	write_script .git/hooks/test-hook <<-EOF &&
> +	echo Test hook
> +	EOF
> +
> +	nongit test_must_fail git hook run test-hook
> +'
> +
> +test_expect_success 'git -c core.hooksPath=<PATH> hook run' '
> +	mkdir my-hooks &&
> +	write_script my-hooks/test-hook <<-\EOF &&
> +	echo Hook ran $1 >>actual
> +	EOF
> +
> +	cat >expect <<-\EOF &&
> +	Test hook
> +	Hook ran one
> +	Hook ran two
> +	Hook ran three
> +	Hook ran four
> +	EOF
> +
> +	# Test various ways of specifying the path. See also
> +	# t1350-config-hooks-path.sh
> +	>actual &&
> +	git hook run test-hook -- ignored 2>>actual &&
> +	git -c core.hooksPath=my-hooks hook run test-hook -- one 2>>actual &&
> +	git -c core.hooksPath=my-hooks/ hook run test-hook -- two 2>>actual &&
> +	git -c core.hooksPath="$PWD/my-hooks" hook run test-hook -- three 2>>actual &&
> +	git -c core.hooksPath="$PWD/my-hooks/" hook run test-hook -- four 2>>actual &&
> +	test_cmp expect actual
> +'
> +
> +test_done
>
Ævar Arnfjörð Bjarmason· Oct 13, 2021, 12:58 UTC · re: René Scharfe · lore

Re: [PATCH 01/13] hook: add 'run' subcommand

On Tue, Oct 12 2021, René Scharfe wrote:
Show 77 quoted lines
> Am 12.10.21 um 15:30 schrieb Ævar Arnfjörð Bjarmason:
>> From: Emily Shaffer <emilyshaffer@google.com>
>>
>> In order to enable hooks to be run as an external process, by a
>> standalone Git command, or by tools which wrap Git, provide an external
>> means to run all configured hook commands for a given hook event.
>>
>> Most of our hooks require more complex functionality than this, but
>> let's start with the bare minimum required to support our simplest
>> hooks.
>>
>> In terms of implementation the usage_with_options() and "goto usage"
>> pattern here mirrors that of
>> builtin/{commit-graph,multi-pack-index}.c.
>>
>> Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
>> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>> ---
>>  .gitignore                 |   1 +
>>  Documentation/git-hook.txt |  38 +++++++++++
>>  Documentation/githooks.txt |   4 ++
>>  Makefile                   |   1 +
>>  builtin.h                  |   1 +
>>  builtin/hook.c             |  90 ++++++++++++++++++++++++++
>>  command-list.txt           |   1 +
>>  git.c                      |   1 +
>>  hook.c                     | 101 +++++++++++++++++++++++++++++
>>  hook.h                     |  39 +++++++++++
>>  t/t1800-hook.sh            | 129 +++++++++++++++++++++++++++++++++++++
>>  11 files changed, 406 insertions(+)
>>  create mode 100644 Documentation/git-hook.txt
>>  create mode 100644 builtin/hook.c
>>  create mode 100755 t/t1800-hook.sh
>>
>> diff --git a/.gitignore b/.gitignore
>> index 6be9de41ae8..66189ca3cdc 100644
>> --- a/.gitignore
>> +++ b/.gitignore
>> @@ -77,6 +77,7 @@
>>  /git-grep
>>  /git-hash-object
>>  /git-help
>> +/git-hook
>>  /git-http-backend
>>  /git-http-fetch
>>  /git-http-push
>> diff --git a/Documentation/git-hook.txt b/Documentation/git-hook.txt
>> new file mode 100644
>> index 00000000000..660d6a992a0
>> --- /dev/null
>> +++ b/Documentation/git-hook.txt
>> @@ -0,0 +1,38 @@
>> +git-hook(1)
>> +===========
>> +
>> +NAME
>> +----
>> +git-hook - run git hooks
>> +
>> +SYNOPSIS
>> +--------
>> +[verse]
>> +'git hook' run <hook-name> [-- <hook-args>]
>> +
>> +DESCRIPTION
>> +-----------
>> +
>> +This command is an interface to git hooks (see linkgit:githooks[5]).
>> +Currently it only provides a convenience wrapper for running hooks for
>> +use by git itself. In the future it might gain other functionality.
>
> As a man page reader I'm only interested in the present features of the
> command up here.  Breaking changes could be mentioned in a HISTORY
> section, and missing critical features in a BUGS section at the bottom.
>
> I assume the last sentence applies to almost all programs, and could be
> safely removed.
Willdo.
Show 12 quoted lines
>> +
>> +SUBCOMMANDS
>> +-----------
>> +
>> +run::
>> +	Run the `<hook-name>` hook. See linkgit:githooks[5] for
>> +	the hook names we support.
>> ++
>> +Any positional arguments to the hook should be passed after an
>> +optional `--` (or `--end-of-options`, see linkgit:gitcli[7]). The
>
> Dash dash (or EoO) is optional.
This just needs to be rephrased, the -- is mandatory, i.e. it's either:
    git hook run [args] hook-name
Or:
    git hook run [args] hook-name -- [hook-args]
But not:
    git hook run [args] hook-name [hook-args]
Show 104 quoted lines
>> +arguments (if any) differ by hook name, see linkgit:githooks[5] for
>> +what those are.
>> +
>> +SEE ALSO
>> +--------
>> +linkgit:githooks[5]
>> +
>> +GIT
>> +---
>> +Part of the linkgit:git[1] suite
>> diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
>> index b51959ff941..a16e62bc8c8 100644
>> --- a/Documentation/githooks.txt
>> +++ b/Documentation/githooks.txt
>> @@ -698,6 +698,10 @@ 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.
>>
>> +SEE ALSO
>> +--------
>> +linkgit:git-hook[1]
>> +
>>  GIT
>>  ---
>>  Part of the linkgit:git[1] suite
>> diff --git a/Makefile b/Makefile
>> index 877492386ee..12b481fdabe 100644
>> --- a/Makefile
>> +++ b/Makefile
>> @@ -1108,6 +1108,7 @@ BUILTIN_OBJS += builtin/get-tar-commit-id.o
>>  BUILTIN_OBJS += builtin/grep.o
>>  BUILTIN_OBJS += builtin/hash-object.o
>>  BUILTIN_OBJS += builtin/help.o
>> +BUILTIN_OBJS += builtin/hook.o
>>  BUILTIN_OBJS += builtin/index-pack.o
>>  BUILTIN_OBJS += builtin/init-db.o
>>  BUILTIN_OBJS += builtin/interpret-trailers.o
>> diff --git a/builtin.h b/builtin.h
>> index 8a58743ed63..83379f3832c 100644
>> --- a/builtin.h
>> +++ b/builtin.h
>> @@ -164,6 +164,7 @@ int cmd_get_tar_commit_id(int argc, const char **argv, const char *prefix);
>>  int cmd_grep(int argc, const char **argv, const char *prefix);
>>  int cmd_hash_object(int argc, const char **argv, const char *prefix);
>>  int cmd_help(int argc, const char **argv, const char *prefix);
>> +int cmd_hook(int argc, const char **argv, const char *prefix);
>>  int cmd_index_pack(int argc, const char **argv, const char *prefix);
>>  int cmd_init_db(int argc, const char **argv, const char *prefix);
>>  int cmd_interpret_trailers(int argc, const char **argv, const char *prefix);
>> diff --git a/builtin/hook.c b/builtin/hook.c
>> new file mode 100644
>> index 00000000000..012a2973b38
>> --- /dev/null
>> +++ b/builtin/hook.c
>> @@ -0,0 +1,90 @@
>> +#include "cache.h"
>> +#include "builtin.h"
>> +#include "config.h"
>> +#include "hook.h"
>> +#include "parse-options.h"
>> +#include "strbuf.h"
>> +#include "strvec.h"
>> +
>> +#define BUILTIN_HOOK_RUN_USAGE \
>> +	N_("git hook run <hook-name> [-- <hook-args>]")
>> +
>> +static const char * const builtin_hook_usage[] = {
>> +	BUILTIN_HOOK_RUN_USAGE,
>> +	NULL
>> +};
>> +
>> +static const char * const builtin_hook_run_usage[] = {
>> +	BUILTIN_HOOK_RUN_USAGE,
>> +	NULL
>> +};
>> +
>> +static int run(int argc, const char **argv, const char *prefix)
>> +{
>> +	int i;
>> +	struct run_hooks_opt opt = RUN_HOOKS_OPT_INIT;
>> +	const char *hook_name;
>> +	const char *hook_path;
>> +	struct option run_options[] = {
>> +		OPT_END(),
>> +	};
>> +	int ret;
>> +
>> +	argc = parse_options(argc, argv, prefix, run_options,
>> +			     builtin_hook_run_usage,
>> +			     PARSE_OPT_KEEP_DASHDASH);
>> +
>> +	if (!argc)
>> +		goto usage;
>> +
>> +	/*
>> +	 * Having a -- for "run" when providing <hook-args> is
>> +	 * mandatory.
>> +	 */
>> +	if (argc > 1 && strcmp(argv[1], "--") &&
>> +	    strcmp(argv[1], "--end-of-options"))
>> +		goto usage;
> Dash dash (or EoO) is mandatory unless parse_options() left no
> arguments, contrary to what the documentation says above.  Update
> the doc above?
*nod*
Show 15 quoted lines
>> +
>> +	/* Add our arguments, start after -- */
>> +	for (i = 2 ; i < argc; i++)
>> +		strvec_push(&opt.args, argv[i]);
>> +
>> +	/* Need to take into account core.hooksPath */
>> +	git_config(git_default_config, NULL);
>> +
>> +	/*
>> +	 * We are not using a plain run_hooks() because we'd like to
>> +	 * detect missing hooks. Let's find it ourselves and call
>> +	 * run_hooks() instead.
>
> So run_hooks(), which is introduced later in this patch, can find a
> hook as well, but would do so silently?

Will clarify, I think Emily and I went back and forth on this comment a bit, and at some point I removed it entirely I think...

Show 6 quoted lines
>> +	 */
>> +	hook_name = argv[0];
>> +	hook_path = find_hook(hook_name);
>
> This is the only find_hook() call introduced by this patch.
> Does run_hooks() really posses an unused hook-finding capability?
Will address...
Show 102 quoted lines
>> +	if (!hook_path) {
>> +		error("cannot find a hook named %s", hook_name);
>> +		return 1;
>> +	}
>> +
>> +	ret = run_hooks(hook_name, hook_path, &opt);
>> +	run_hooks_opt_clear(&opt);
>> +	return ret;
>> +usage:
>> +	usage_with_options(builtin_hook_run_usage, run_options);
>> +}
>> +
>> +int cmd_hook(int argc, const char **argv, const char *prefix)
>> +{
>> +	struct option builtin_hook_options[] = {
>> +		OPT_END(),
>> +	};
>> +
>> +	argc = parse_options(argc, argv, NULL, builtin_hook_options,
>> +			     builtin_hook_usage, PARSE_OPT_STOP_AT_NON_OPTION);
>> +	if (!argc)
>> +		goto usage;
>> +
>> +	if (!strcmp(argv[0], "run"))
>> +		return run(argc, argv, prefix);
>> +
>> +usage:
>> +	usage_with_options(builtin_hook_usage, builtin_hook_options);
>> +}
>> diff --git a/command-list.txt b/command-list.txt
>> index a289f09ed6f..9ccd8e5aebe 100644
>> --- a/command-list.txt
>> +++ b/command-list.txt
>> @@ -103,6 +103,7 @@ git-grep                                mainporcelain           info
>>  git-gui                                 mainporcelain
>>  git-hash-object                         plumbingmanipulators
>>  git-help                                ancillaryinterrogators          complete
>> +git-hook                                mainporcelain
>>  git-http-backend                        synchingrepositories
>>  git-http-fetch                          synchelpers
>>  git-http-push                           synchelpers
>> diff --git a/git.c b/git.c
>> index 60c2784be45..e5891e02021 100644
>> --- a/git.c
>> +++ b/git.c
>> @@ -538,6 +538,7 @@ static struct cmd_struct commands[] = {
>>  	{ "grep", cmd_grep, RUN_SETUP_GENTLY },
>>  	{ "hash-object", cmd_hash_object },
>>  	{ "help", cmd_help },
>> +	{ "hook", cmd_hook, RUN_SETUP },
>>  	{ "index-pack", cmd_index_pack, RUN_SETUP_GENTLY | NO_PARSEOPT },
>>  	{ "init", cmd_init_db },
>>  	{ "init-db", cmd_init_db },
>> diff --git a/hook.c b/hook.c
>> index 55e1145a4b7..240270db64e 100644
>> --- a/hook.c
>> +++ b/hook.c
>> @@ -1,6 +1,7 @@
>>  #include "cache.h"
>>  #include "hook.h"
>>  #include "run-command.h"
>> +#include "config.h"
>>
>>  const char *find_hook(const char *name)
>>  {
>> @@ -40,3 +41,103 @@ int hook_exists(const char *name)
>>  {
>>  	return !!find_hook(name);
>>  }
>> +
>> +void run_hooks_opt_clear(struct run_hooks_opt *o)
>> +{
>> +	strvec_clear(&o->env);
>> +	strvec_clear(&o->args);
>> +}
>> +
>> +static int pick_next_hook(struct child_process *cp,
>> +			  struct strbuf *out,
>> +			  void *pp_cb,
>> +			  void **pp_task_cb)
>> +{
>> +	struct hook_cb_data *hook_cb = pp_cb;
>> +	struct hook *run_me = hook_cb->run_me;
>> +
>> +	if (!run_me)
>> +		return 0;
>> +
>> +	cp->no_stdin = 1;
>> +	cp->env = hook_cb->options->env.v;
>> +	cp->stdout_to_stderr = 1;
>> +	cp->trace2_hook_name = hook_cb->hook_name;
>> +
>> +	/* add command */
>> +	strvec_push(&cp->args, run_me->hook_path);
>> +
>> +	/*
>> +	 * add passed-in argv, without expanding - let the user get back
>> +	 * exactly what they put in
>
> What kind of expanding is it doing without?  I was expecting the
> arguments to passed on verbatim before reading this comment, so it
> leaves me wondering what I'm missing.
I think that's one of Emily's comments, will find out...
Show 14 quoted lines
>> +	 */
>> +	strvec_pushv(&cp->args, hook_cb->options->args.v);
>> +
>> +	/* Provide context for errors if necessary */
>> +	*pp_task_cb = run_me;
>> +
>> +	/*
>> +	 * This pick_next_hook() will be called again, we're only
>> +	 * running one hook, so indicate that no more work will be
>> +	 * done.
>> +	 */
>> +	hook_cb->run_me = NULL;
>
> A single hook is picked.
Right...
Show 60 quoted lines
>> +
>> +	return 1;
>> +}
>> +
>> +static int notify_start_failure(struct strbuf *out,
>> +				void *pp_cb,
>> +				void *pp_task_cp)
>> +{
>> +	struct hook_cb_data *hook_cb = pp_cb;
>> +	struct hook *attempted = pp_task_cp;
>> +
>> +	hook_cb->rc |= 1;
>> +
>> +	strbuf_addf(out, _("Couldn't start hook '%s'\n"),
>> +		    attempted->hook_path);
>> +
>> +	return 1;
>> +}
>> +
>> +static int notify_hook_finished(int result,
>> +				struct strbuf *out,
>> +				void *pp_cb,
>> +				void *pp_task_cb)
>> +{
>> +	struct hook_cb_data *hook_cb = pp_cb;
>> +
>> +	hook_cb->rc |= result;
>> +
>> +	return 0;
>> +}
>> +
>> +int run_hooks(const char *hook_name, const char *hook_path,
>> +	      struct run_hooks_opt *options)
>> +{
>> +	struct hook my_hook = {
>> +		.hook_path = hook_path,
>> +	};
>> +	struct hook_cb_data cb_data = {
>> +		.rc = 0,
>> +		.hook_name = hook_name,
>> +		.options = options,
>> +	};
>> +	int jobs = 1;
>> +
>> +	if (!options)
>> +		BUG("a struct run_hooks_opt must be provided to run_hooks");
>> +
>> +	cb_data.run_me = &my_hook;
>> +
>> +	run_processes_parallel_tr2(jobs,
>> +				   pick_next_hook,
>> +				   notify_start_failure,
>> +				   notify_hook_finished,
>> +				   &cb_data,
>> +				   "hook",
>> +				   hook_name);
>
> A single process (jobs == 1) is used to run a single hook.  Why use
> run_processes_parallel_tr2() for that instead of run_command()?  The
> latter would require a lot less code to achieve the same outcome, no?
...also for this...
Show 19 quoted lines
>> +
>> +	return cb_data.rc;
>> +}
>> diff --git a/hook.h b/hook.h
>> index 6aa36fc7ff9..111c5cf9296 100644
>> --- a/hook.h
>> +++ b/hook.h
>> @@ -1,5 +1,33 @@
>>  #ifndef HOOK_H
>>  #define HOOK_H
>> +#include "strvec.h"
>> +
>> +struct hook {
>> +	/* The path to the hook */
>> +	const char *hook_path;
>> +};
>
> None of the patches in this series extend this structure.  Why not
> use a bare string directly?

...the reason for all of this is that it's a multi-part series where we're eventually headed towards multi-hook parallel support.

An earlier version of this started with run_command() et al, then later migrated to the parallel API, since the parallel API can do a single run with a jobs=1 I found that easier than the curn of switching back and forth.

Ditto some other scaffolding like this one-member struct, which will be expanded later on.

Will address this by clarifying the plans in commit messages.
Show 33 quoted lines
>> +
>> +struct run_hooks_opt
>> +{
>> +	/* Environment vars to be set for each hook */
>> +	struct strvec env;
>> +
>> +	/* Args to be passed to each hook */
>> +	struct strvec args;
>> +};
>> +
>> +#define RUN_HOOKS_OPT_INIT { \
>> +	.env = STRVEC_INIT, \
>> +	.args = STRVEC_INIT, \
>> +}
>> +
>> +struct hook_cb_data {
>> +	/* rc reflects the cumulative failure state */
>> +	int rc;
>> +	const char *hook_name;
>> +	struct hook *run_me;
>> +	struct run_hooks_opt *options;
>> +};
>>
>>  /*
>>   * Returns the path to the hook file, or NULL if the hook is missing
>> @@ -13,4 +41,15 @@ const char *find_hook(const char *name);
>>   */
>>  int hook_exists(const char *hookname);
>>
>> +/**
>> + * Clear data from an initialized "struct run_hooks-opt".
>                                                       ^
> y/-/_/
*nod*
Show 11 quoted lines
>> + */
>> +void run_hooks_opt_clear(struct run_hooks_opt *o);
>> +
>> +/**
>> + * Takes an already resolved hook found via find_hook() and runs
>> + * it. Does not call run_hooks_opt_clear() for you.
>> + */
>> +int run_hooks(const char *hookname, const char *hook_path,
>> +	      struct run_hooks_opt *options);
>
> If it runs one hook, why is it called run_hooks(), plural?

Ditto above, will clarify, eventually it will run N, and then having the churn of changing all callers/re-indenting argument lists...

Thanks a lot for your time & the review. Will wait on a re-roll for more comments...

Bagas Sanjaya· Oct 13, 2021, 00:52 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [PATCH 01/13] hook: add 'run' subcommand

On 12/10/21 20.30, Ævar Arnfjörð Bjarmason wrote:
> +run::
> +	Run the `<hook-name>` hook. See linkgit:githooks[5] for
> +	the hook names we support.
> ++
Drop the first person, so it become `list of supported hooks`.
Show 5 quoted lines
> +Any positional arguments to the hook should be passed after an
> +optional `--` (or `--end-of-options`, see linkgit:gitcli[7]). The
> +arguments (if any) differ by hook name, see linkgit:githooks[5] for
> +what those are.
> +
s/what those are/supported list of them/ (dunno?)
-- 
An old man doll... just what I always wanted! - Clara
Ævar Arnfjörð Bjarmason· Oct 13, 2021, 13:04 UTC · re: Bagas Sanjaya · lore

Re: [PATCH 01/13] hook: add 'run' subcommand

On Wed, Oct 13 2021, Bagas Sanjaya wrote:
Show 15 quoted lines
> On 12/10/21 20.30, Ævar Arnfjörð Bjarmason wrote:
>> +run::
>> +	Run the `<hook-name>` hook. See linkgit:githooks[5] for
>> +	the hook names we support.
>> ++
>
> Drop the first person, so it become `list of supported hooks`.
>
>> +Any positional arguments to the hook should be passed after an
>> +optional `--` (or `--end-of-options`, see linkgit:gitcli[7]). The
>> +arguments (if any) differ by hook name, see linkgit:githooks[5] for
>> +what those are.
>> +
>
> s/what those are/supported list of them/ (dunno?)
Thanks, will address both.
Ævar Arnfjörð Bjarmason· Oct 12, 2021, 13:30 UTC · re: Ævar Arnfjörð Bjarmason · lore

[PATCH 02/13] gc: use hook library for pre-auto-gc hook

From: Emily Shaffer <emilyshaffer@google.com>

Move the pre-auto-gc hook away from run-command.h to and over to the new hook.h library.

To do this introduce a simple run_hooks_oneshot() wrapper, we'll be using it extensively for these simple cases of wanting to run a single hook under a given name, and having it free the memory we allocate for us.

Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/gc.c |  3 ++-
 hook.c       | 20 ++++++++++++++++++++
 hook.h       | 13 +++++++++++++
 3 files changed, 35 insertions(+), 1 deletion(-)
Show changes to 3 files +35 −1

builtin/gc.c, hook.c, hook.h

diff --git a/builtin/gc.c b/builtin/gc.c
index 6b3de3dd514..95939e77f53 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"
 
@@ -394,7 +395,7 @@ static int need_to_gc(void)
 	else
 		return 0;
 
-	if (run_hook_le(NULL, "pre-auto-gc", NULL))
+	if (run_hooks_oneshot("pre-auto-gc", NULL))
 		return 0;
 	return 1;
 }
diff --git a/hook.c b/hook.c
index 240270db64e..bfdc79e2f1a 100644
--- a/hook.c
+++ b/hook.c
@@ -141,3 +141,23 @@ int run_hooks(const char *hook_name, const char *hook_path,
 
 	return cb_data.rc;
 }
+
+int run_hooks_oneshot(const char *hook_name, struct run_hooks_opt *options)
+{
+	const char *hook_path;
+	struct run_hooks_opt hook_opt_scratch = RUN_HOOKS_OPT_INIT;
+	int ret = 0;
+
+	if (!options)
+		options = &hook_opt_scratch;
+
+	hook_path = find_hook(hook_name);
+	if (!hook_path)
+		goto cleanup;
+
+	ret = run_hooks(hook_name, hook_path, options);
+cleanup:
+	run_hooks_opt_clear(options);
+
+	return ret;
+}
diff --git a/hook.h b/hook.h
index 111c5cf9296..a2b8d4fc6bd 100644
--- a/hook.h
+++ b/hook.h
@@ -49,7 +49,20 @@ void run_hooks_opt_clear(struct run_hooks_opt *o);
 /**
  * Takes an already resolved hook found via find_hook() and runs
  * it. Does not call run_hooks_opt_clear() for you.
+ *
+ * See run_hooks_oneshot() for the simpler one-shot API.
  */
 int run_hooks(const char *hookname, const char *hook_path,
 	      struct run_hooks_opt *options);
+
+/**
+ * Calls find_hook() on your "hook_name" and runs the hooks (if any)
+ * with run_hooks().
+ *
+ * If "options" is provided calls run_hooks_opt_clear() on it for
+ * you. If "options" is NULL the default options from
+ * RUN_HOOKS_OPT_INIT will be used.
+ */
+int run_hooks_oneshot(const char *hook_name, struct run_hooks_opt *options);
+
 #endif
-- 
2.33.0.1567.g7b23ce7ed9e
Ævar Arnfjörð Bjarmason· Oct 12, 2021, 13:30 UTC · re: Ævar Arnfjörð Bjarmason · lore

[PATCH 03/13] rebase: convert pre-rebase to use hook.h

From: Emily Shaffer <emilyshaffer@google.com>

Move the pre-rebase hook away from run-command.h to and over to the new hook.h library.

Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/rebase.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)
Show changes to builtin/rebase.c +6 −2
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 8c6393f6d78..9a44939d662 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"
 
@@ -1305,6 +1306,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;
 	struct option builtin_rebase_options[] = {
 		OPT_STRING(0, "onto", &options.onto_name,
 			   N_("revision"),
@@ -2021,9 +2023,11 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 	}
 
 	/* If a hook exists, give it a chance to interrupt*/
+	strvec_push(&hook_opt.args, options.upstream_arg);
+	if (argc)
+		strvec_push(&hook_opt.args, argv[0]);
 	if (!ok_to_skip_pre_rebase &&
-	    run_hook_le(NULL, "pre-rebase", options.upstream_arg,
-			argc ? argv[0] : NULL, NULL))
+	    run_hooks_oneshot("pre-rebase", &hook_opt))
 		die(_("The pre-rebase hook refused to rebase."));
 
 	if (options.flags & REBASE_DIFFSTAT) {
-- 
2.33.0.1567.g7b23ce7ed9e
Ævar Arnfjörð Bjarmason· Oct 12, 2021, 13:30 UTC · re: Ævar Arnfjörð Bjarmason · lore

[PATCH 04/13] am: convert applypatch to use hook.h

From: Emily Shaffer <emilyshaffer@google.com>

Teach pre-applypatch, post-applypatch, and applypatch-msg to use the hook.h library instead of the run-command.h library.

Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/am.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)
Show changes to builtin/am.c +5 −3
diff --git a/builtin/am.c b/builtin/am.c
index 3527945a467..2cfe6451b6f 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -446,9 +446,11 @@ 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;
 
 	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_oneshot("applypatch-msg", &opt);
 
 	if (!ret) {
 		FREE_AND_NULL(state->msg);
@@ -1609,7 +1611,7 @@ static void do_commit(const struct am_state *state)
 	const char *reflog_msg, *author, *committer = NULL;
 	struct strbuf sb = STRBUF_INIT;
 
-	if (run_hook_le(NULL, "pre-applypatch", NULL))
+	if (run_hooks_oneshot("pre-applypatch", NULL))
 		exit(1);
 
 	if (write_cache_as_tree(&tree, 0, NULL))
@@ -1661,7 +1663,7 @@ static void do_commit(const struct am_state *state)
 		fclose(fp);
 	}
 
-	run_hook_le(NULL, "post-applypatch", NULL);
+	run_hooks_oneshot("post-applypatch", NULL);
 
 	strbuf_release(&sb);
 }
-- 
2.33.0.1567.g7b23ce7ed9e
Ævar Arnfjörð Bjarmason· Oct 12, 2021, 13:30 UTC · re: Ævar Arnfjörð Bjarmason · lore

[PATCH 05/13] hooks: convert 'post-checkout' hook to hook library

From: Emily Shaffer <emilyshaffer@google.com>

Move the running of the 'post-checkout' hook away from run-command.h to the new hook.h library. For "worktree" this requires a change to it to run the hooks from a given directory.

We could strictly speaking skip the "absolute_path" flag and just check if "dir" is specified, but let's split them up for clarity, as well as for any future user who'd like to set "dir" but not implicitly change the argument to an absolute path.

Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/checkout.c | 14 +++++++++-----
 builtin/clone.c    |  7 +++++--
 builtin/worktree.c | 28 ++++++++++++----------------
 hook.c             |  9 +++++++++
 hook.h             |  9 +++++++++
 read-cache.c       |  1 +
 reset.c            | 14 ++++++++++----
 7 files changed, 55 insertions(+), 27 deletions(-)
Show changes to 7 files +55 −27

builtin/checkout.c, builtin/clone.c, builtin/worktree.c, hook.c, hook.h, read-cache.c, reset.c

diff --git a/builtin/checkout.c b/builtin/checkout.c
index 8c69dcdf72a..555119cc322 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"
@@ -106,13 +107,16 @@ 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;
+
 	/* "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);
+	return run_hooks_oneshot("post-checkout", &opt);
 }
 
 static int update_some(const struct object_id *oid, struct strbuf *base,
diff --git a/builtin/clone.c b/builtin/clone.c
index c9d4ca26640..4b356770739 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:
@@ -659,6 +660,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;
 
 	if (option_no_checkout)
 		return 0;
@@ -704,8 +706,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_oneshot("post-checkout", &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 d22ece93e1a..330867c19bf 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -382,22 +382,18 @@ 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;
+
+		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;
+		opt.absolute_path = 1;
+
+		ret = run_hooks_oneshot("post-checkout", &opt);
 	}
 
 	strvec_clear(&child_env);
diff --git a/hook.c b/hook.c
index bfdc79e2f1a..186c249bbec 100644
--- a/hook.c
+++ b/hook.c
@@ -63,6 +63,7 @@ static int pick_next_hook(struct child_process *cp,
 	cp->env = hook_cb->options->env.v;
 	cp->stdout_to_stderr = 1;
 	cp->trace2_hook_name = hook_cb->hook_name;
+	cp->dir = hook_cb->options->dir;
 
 	/* add command */
 	strvec_push(&cp->args, run_me->hook_path);
@@ -116,6 +117,7 @@ static int notify_hook_finished(int result,
 int run_hooks(const char *hook_name, const char *hook_path,
 	      struct run_hooks_opt *options)
 {
+	struct strbuf abs_path = STRBUF_INIT;
 	struct hook my_hook = {
 		.hook_path = hook_path,
 	};
@@ -129,6 +131,10 @@ int run_hooks(const char *hook_name, const char *hook_path,
 	if (!options)
 		BUG("a struct run_hooks_opt must be provided to run_hooks");
 
+	if (options->absolute_path) {
+		strbuf_add_absolute_path(&abs_path, hook_path);
+		my_hook.hook_path = abs_path.buf;
+	}
 	cb_data.run_me = &my_hook;
 
 	run_processes_parallel_tr2(jobs,
@@ -139,6 +145,9 @@ int run_hooks(const char *hook_name, const char *hook_path,
 				   "hook",
 				   hook_name);
 
+	if (options->absolute_path)
+		strbuf_release(&abs_path);
+
 	return cb_data.rc;
 }
 
diff --git a/hook.h b/hook.h
index a2b8d4fc6bd..7a858a6beed 100644
--- a/hook.h
+++ b/hook.h
@@ -14,6 +14,15 @@ struct run_hooks_opt
 
 	/* Args to be passed to each hook */
 	struct strvec args;
+
+	/*
+	 * Resolve and run the "absolute_path(hook)" instead of
+	 * "hook". Used for "git worktree" hooks
+	 */
+	int absolute_path;
+
+	/* Path to initial working directory for subprocess */
+	const char *dir;
 };
 
 #define RUN_HOOKS_OPT_INIT { \
diff --git a/read-cache.c b/read-cache.c
index a78b88a41bf..9773118d078 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -28,6 +28,7 @@
 #include "sparse-index.h"
 #include "csum-file.h"
 #include "promisor-remote.h"
+#include "hook.h"
 
 /* Mask for the name length in ce_flags in the on-disk index */
 
diff --git a/reset.c b/reset.c
index 79310ae071b..1237ced8a58 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,
@@ -125,10 +126,15 @@ 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;
+		strvec_pushl(&opt.args,
+			     oid_to_hex(orig ? orig : null_oid()),
+			     oid_to_hex(oid),
+			     "1",
+			     NULL);
+		run_hooks_oneshot("post-checkout", &opt);
+	}
 
 leave_reset_head:
 	strbuf_release(&msg);
-- 
2.33.0.1567.g7b23ce7ed9e
Ævar Arnfjörð Bjarmason· Oct 12, 2021, 13:30 UTC · re: Ævar Arnfjörð Bjarmason · lore

[PATCH 06/13] merge: convert post-merge to use hook.h

From: Emily Shaffer <emilyshaffer@google.com>

Teach post-merge to use the hook.h library instead of the run-command.h library to run hooks.

Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/merge.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
Show changes to builtin/merge.c +3 −1
diff --git a/builtin/merge.c b/builtin/merge.c
index 956b6259f21..167d13bd737 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -448,6 +448,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;
 	const struct object_id *head = &head_commit->object.oid;
 
 	if (!msg)
@@ -488,7 +489,8 @@ 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_oneshot("post-merge", &opt);
 
 	apply_autostash(git_path_merge_autostash(the_repository));
 	strbuf_release(&reflog_message);
-- 
2.33.0.1567.g7b23ce7ed9e
Ævar Arnfjörð Bjarmason· Oct 12, 2021, 13:30 UTC · re: Ævar Arnfjörð Bjarmason · lore

[PATCH 09/13] git-p4: use 'git hook' to run hooks

From: Emily Shaffer <emilyshaffer@google.com>

Instead of duplicating the behavior of run-command.h:run_hook_le() in Python, we can directly call 'git hook run'. We emulate the existence check with the --ignore-missing flag.

As this is the last hook execution in git.git to not go through "git hook run" or the hook.[ch] library we can now be absolutely sure that our assertion in hook.c that only hooks known by the generated (from githooks(5)) hook-list.h are permitted.

Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 git-p4.py | 72 ++++++-------------------------------------------------
 1 file changed, 7 insertions(+), 65 deletions(-)
Show changes to git-p4.py +7 −65
diff --git a/git-p4.py b/git-p4.py
index 2b4500226aa..1f24cbf0bca 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -207,71 +207,13 @@ def decode_path(path):
         return 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):
-        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', '--ignore-missing', cmd]
+    if param:
+        args.append("--")
+        for p in param:
+            args.append(p)
+    return subprocess.call(args) == 0
 
 def write_pipe(c, stdin):
     if verbose:
-- 
2.33.0.1567.g7b23ce7ed9e
Ævar Arnfjörð Bjarmason· Oct 12, 2021, 13:30 UTC · re: Ævar Arnfjörð Bjarmason · lore

[PATCH 07/13] git hook run: add an --ignore-missing flag

For certain one-shot hooks we'd like to optimistically run them, and not complain if they don't exist. This will be used by send-email in a subsequent commit.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Documentation/git-hook.txt | 10 +++++++++-
 builtin/hook.c             | 10 ++++++++--
 t/t1800-hook.sh            |  5 +++++
 3 files changed, 22 insertions(+), 3 deletions(-)
Show changes to 3 files +22 −3

Documentation/git-hook.txt, builtin/hook.c, t/t1800-hook.sh

diff --git a/Documentation/git-hook.txt b/Documentation/git-hook.txt
index 660d6a992a0..097fb9de63b 100644
--- a/Documentation/git-hook.txt
+++ b/Documentation/git-hook.txt
@@ -8,7 +8,7 @@ git-hook - run git hooks
 SYNOPSIS
 --------
 [verse]
-'git hook' run <hook-name> [-- <hook-args>]
+'git hook' run [--ignore-missing] <hook-name> [-- <hook-args>]
 
 DESCRIPTION
 -----------
@@ -29,6 +29,14 @@ optional `--` (or `--end-of-options`, see linkgit:gitcli[7]). The
 arguments (if any) differ by hook name, see linkgit:githooks[5] for
 what those are.
 
+OPTIONS
+-------
+
+--ignore-missing::
+	Ignore any missing hook by quietly returning zero. Used for
+	tools that want to do a blind one-shot run of a hook that may
+	or may not be present.
+
 SEE ALSO
 --------
 linkgit:githooks[5]
diff --git a/builtin/hook.c b/builtin/hook.c
index 012a2973b38..e6b01a7f5c6 100644
--- a/builtin/hook.c
+++ b/builtin/hook.c
@@ -7,7 +7,7 @@
 #include "strvec.h"
 
 #define BUILTIN_HOOK_RUN_USAGE \
-	N_("git hook run <hook-name> [-- <hook-args>]")
+	N_("git hook run [--ignore-missing] <hook-name> [-- <hook-args>]")
 
 static const char * const builtin_hook_usage[] = {
 	BUILTIN_HOOK_RUN_USAGE,
@@ -23,9 +23,12 @@ static int run(int argc, const char **argv, const char *prefix)
 {
 	int i;
 	struct run_hooks_opt opt = RUN_HOOKS_OPT_INIT;
+	int ignore_missing = 0;
 	const char *hook_name;
 	const char *hook_path;
 	struct option run_options[] = {
+		OPT_BOOL(0, "ignore-missing", &ignore_missing,
+			 N_("silently ignore missing requested <hook-name>")),
 		OPT_END(),
 	};
 	int ret;
@@ -55,9 +58,12 @@ static int run(int argc, const char **argv, const char *prefix)
 	/*
 	 * We are not using a plain run_hooks() because we'd like to
 	 * detect missing hooks. Let's find it ourselves and call
-	 * run_hooks() instead.
+	 * run_hooks() instead...
 	 */
 	hook_name = argv[0];
+	if (ignore_missing)
+		/* ... act like a plain run_hooks() under --ignore-missing */
+		return run_hooks_oneshot(hook_name, &opt);
 	hook_path = find_hook(hook_name);
 	if (!hook_path) {
 		error("cannot find a hook named %s", hook_name);
diff --git a/t/t1800-hook.sh b/t/t1800-hook.sh
index 3aea1b105f0..29718aa9913 100755
--- a/t/t1800-hook.sh
+++ b/t/t1800-hook.sh
@@ -21,6 +21,11 @@ test_expect_success 'git hook run: nonexistent hook' '
 	test_cmp stderr.expect stderr.actual
 '
 
+test_expect_success 'git hook run: nonexistent hook with --ignore-missing' '
+	git hook run --ignore-missing does-not-exist 2>stderr.actual &&
+	test_must_be_empty stderr.actual
+'
+
 test_expect_success 'git hook run: basic' '
 	write_script .git/hooks/test-hook <<-EOF &&
 	echo Test hook
-- 
2.33.0.1567.g7b23ce7ed9e
Ævar Arnfjörð Bjarmason· Oct 12, 2021, 13:30 UTC · re: Ævar Arnfjörð Bjarmason · lore

[PATCH 10/13] commit: convert {pre-commit,prepare-commit-msg} hook to hook.h

From: Emily Shaffer <emilyshaffer@google.com>

Move these hooks hook away from run-command.h to and over to the new hook.h library.

Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 commit.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)
Show changes to commit.c +8 −7
diff --git a/commit.c b/commit.c
index 551de4903c1..4e7cbd7585d 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 **);
 
@@ -1700,22 +1701,22 @@ size_t ignore_non_trailer(const char *buf, size_t len)
 int run_commit_hook(int editor_is_used, const char *index_file,
 		    const char *name, ...)
 {
-	struct strvec hook_env = STRVEC_INIT;
+	struct run_hooks_opt opt = RUN_HOOKS_OPT_INIT;
 	va_list args;
-	int ret;
+	const char *arg;
 
-	strvec_pushf(&hook_env, "GIT_INDEX_FILE=%s", index_file);
+	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);
 
-	return ret;
+	return run_hooks_oneshot(name, &opt);
 }
-- 
2.33.0.1567.g7b23ce7ed9e
Ævar Arnfjörð Bjarmason· Oct 12, 2021, 13:30 UTC · re: Ævar Arnfjörð Bjarmason · lore

[PATCH 12/13] receive-pack: convert push-to-checkout hook to hook.h

From: Emily Shaffer <emilyshaffer@google.com>

Move the push-to-checkout hook away from run-command.h to and over to the new hook.h library.

This removes the last direct user of run_hook_le(), so we could remove that function now, but let's leave that to a follow-up cleanup commit.

Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/receive-pack.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)
Show changes to builtin/receive-pack.c +5 −2
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 25cc0c907e1..22e7a431728 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -1435,9 +1435,12 @@ 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;
+
 	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_oneshot(push_to_checkout_hook, &opt))
 		return "push-to-checkout hook declined";
 	else
 		return NULL;
-- 
2.33.0.1567.g7b23ce7ed9e
Ævar Arnfjörð Bjarmason· Oct 12, 2021, 13:30 UTC · re: Ævar Arnfjörð Bjarmason · lore

[PATCH 11/13] read-cache: convert post-index-change to use hook.h

From: Emily Shaffer <emilyshaffer@google.com>

Move the post-index-change hook away from run-command.h to and over to the new hook.h library.

This removes the last direct user of "run_hook_ve()" outside of run-command.c ("run_hook_le()" still uses it). So we can make the function static now. A subsequent commit will remove this code entirely when "run_hook_le()" itself goes away.

Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 read-cache.c  | 10 +++++++---
 run-command.c |  2 +-
 run-command.h |  1 -
 3 files changed, 8 insertions(+), 5 deletions(-)
Show changes to 3 files +8 −5

read-cache.c, run-command.c, run-command.h

diff --git a/read-cache.c b/read-cache.c
index 9773118d078..84cc3d88196 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -3087,6 +3087,7 @@ static int do_write_locked_index(struct index_state *istate, struct lock_file *l
 {
 	int ret;
 	int was_full = !istate->sparse_index;
+	struct run_hooks_opt hook_opt = RUN_HOOKS_OPT_INIT;
 
 	ret = convert_to_sparse(istate, 0);
 
@@ -3115,9 +3116,12 @@ 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_oneshot("post-index-change", &hook_opt);
+
 	istate->updated_workdir = 0;
 	istate->updated_skipworktree = 0;
 
diff --git a/run-command.c b/run-command.c
index e4862b8f46d..7a05aa73e3d 100644
--- a/run-command.c
+++ b/run-command.c
@@ -1323,7 +1323,7 @@ int async_with_fork(void)
 #endif
 }
 
-int run_hook_ve(const char *const *env, const char *name, va_list args)
+static int run_hook_ve(const char *const *env, const char *name, va_list args)
 {
 	struct child_process hook = CHILD_PROCESS_INIT;
 	const char *p;
diff --git a/run-command.h b/run-command.h
index 5e544acf4bc..f06b39c50ee 100644
--- a/run-command.h
+++ b/run-command.h
@@ -239,7 +239,6 @@ int run_command(struct child_process *);
  */
 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.33.0.1567.g7b23ce7ed9e
Ævar Arnfjörð Bjarmason· Oct 12, 2021, 13:30 UTC · re: Ævar Arnfjörð Bjarmason · lore

[PATCH 13/13] run-command: remove old run_hook_{le,ve}() hook API

From: Emily Shaffer <emilyshaffer@google.com>

The new hook.h library has replaced all run-command.h hook-related functionality. So let's delete this dead code.

Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 run-command.c | 32 --------------------------------
 run-command.h | 16 ----------------
 2 files changed, 48 deletions(-)
Show changes to 2 files +0 −48

run-command.c, run-command.h

diff --git a/run-command.c b/run-command.c
index 7a05aa73e3d..e39ce17d009 100644
--- a/run-command.c
+++ b/run-command.c
@@ -1323,38 +1323,6 @@ int async_with_fork(void)
 #endif
 }
 
-static 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 f06b39c50ee..6d6c50fe0a9 100644
--- a/run-command.h
+++ b/run-command.h
@@ -224,22 +224,6 @@ int finish_command_in_signal(struct child_process *);
  */
 int run_command(struct child_process *);
 
-/**
- * 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, ...);
-
 /*
  * Trigger an auto-gc
  */
-- 
2.33.0.1567.g7b23ce7ed9e
Ævar Arnfjörð Bjarmason· Oct 12, 2021, 13:30 UTC · re: Ævar Arnfjörð Bjarmason · lore

[PATCH 08/13] send-email: use 'git hook run' for 'sendemail-validate'

From: Emily Shaffer <emilyshaffer@google.com>

Change the "sendmail-validate" hook to be run via the "git hook run" wrapper instead of via a direct invocation.

This is the smallest possibly change to get "send-email" using "git hook run". We still check the hook itself with "-x", and set a "GIT_DIR" variable, both of which are asserted by our tests. We'll need to get rid of this special behavior if we start running N hooks, but for now let's be as close to bug-for-bug compatible as possible.

Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 git-send-email.perl   | 22 ++++++++++++++--------
 t/t9001-send-email.sh |  4 ++--
 2 files changed, 16 insertions(+), 10 deletions(-)
Show changes to 2 files +16 −10

git-send-email.perl, t/t9001-send-email.sh

diff --git a/git-send-email.perl b/git-send-email.perl
index 5262d88ee32..4c20c0bbb79 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -195,13 +195,13 @@ sub format_2822_time {
 my $editor;
 
 sub system_or_msg {
-	my ($args, $msg) = @_;
+	my ($args, $msg, $cmd_name) = @_;
 	system(@$args);
 	my $signalled = $? & 127;
 	my $exit_code = $? >> 8;
 	return unless $signalled or $exit_code;
 
-	my @sprintf_args = ($args->[0], $exit_code);
+	my @sprintf_args = ($cmd_name ? $cmd_name : $args->[0], $exit_code);
 	if (defined $msg) {
 		# Quiet the 'redundant' warning category, except we
 		# need to support down to Perl 5.8, so we can't do a
@@ -2039,10 +2039,10 @@ sub validate_patch {
 	my ($fn, $xfer_encoding) = @_;
 
 	if ($repo) {
+		my $hook_name = 'sendemail-validate';
 		my $hooks_path = $repo->command_oneline('rev-parse', '--git-path', 'hooks');
 		require File::Spec;
-		my $validate_hook = File::Spec->catfile($hooks_path,
-					    'sendemail-validate');
+		my $validate_hook = File::Spec->catfile($hooks_path, $hook_name);
 		my $hook_error;
 		if (-x $validate_hook) {
 			require Cwd;
@@ -2052,13 +2052,19 @@ sub validate_patch {
 			chdir($repo->wc_path() or $repo->repo_path())
 				or die("chdir: $!");
 			local $ENV{"GIT_DIR"} = $repo->repo_path();
-			$hook_error = system_or_msg([$validate_hook, $target]);
+			my @cmd = ("git", "hook", "run", "--ignore-missing",
+				    $hook_name, "--");
+			my @cmd_msg = (@cmd, "<patch>");
+			my @cmd_run = (@cmd, $target);
+			$hook_error = system_or_msg(\@cmd_run, undef, "@cmd_msg");
 			chdir($cwd_save) or die("chdir: $!");
 		}
 		if ($hook_error) {
-			die sprintf(__("fatal: %s: rejected by sendemail-validate hook\n" .
-				       "%s\n" .
-				       "warning: no patches were sent\n"), $fn, $hook_error);
+			$hook_error = sprintf(__("fatal: %s: rejected by %s hook\n" .
+						 $hook_error . "\n" .
+						 "warning: no patches were sent\n"),
+					      $fn, $hook_name);
+			die $hook_error;
 		}
 	}
 
diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
index aa0c20499ba..84d0f40d76a 100755
--- a/t/t9001-send-email.sh
+++ b/t/t9001-send-email.sh
@@ -539,7 +539,7 @@ test_expect_success $PREREQ "--validate respects relative core.hooksPath path" '
 	test_path_is_file my-hooks.ran &&
 	cat >expect <<-EOF &&
 	fatal: longline.patch: rejected by sendemail-validate hook
-	fatal: command '"'"'my-hooks/sendemail-validate'"'"' died with exit code 1
+	fatal: command '"'"'git hook run --ignore-missing sendemail-validate -- <patch>'"'"' died with exit code 1
 	warning: no patches were sent
 	EOF
 	test_cmp expect actual
@@ -558,7 +558,7 @@ test_expect_success $PREREQ "--validate respects absolute core.hooksPath path" '
 	test_path_is_file my-hooks.ran &&
 	cat >expect <<-EOF &&
 	fatal: longline.patch: rejected by sendemail-validate hook
-	fatal: command '"'"'$hooks_path/sendemail-validate'"'"' died with exit code 1
+	fatal: command '"'"'git hook run --ignore-missing sendemail-validate -- <patch>'"'"' died with exit code 1
 	warning: no patches were sent
 	EOF
 	test_cmp expect actual
-- 
2.33.0.1567.g7b23ce7ed9e

← back to recent threads