Am 12.10.21 um 15:30 schrieb Ævar Arnfjörð Bjarmason:
> From: Emily Shaffer <emilyshaffer@xxxxxxxxxx>
>
> 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@xxxxxxxxxx>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx>
> ---
> .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.
> +
> +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.
> +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?
> +
> + /* 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?
> + 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.
> + */
> + 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.
> +
> + 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?
> +
> + 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?
> +
> +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/-/_/
> + */
> +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?
> #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
>
