On Tue, Oct 12 2021, René Scharfe wrote:
> 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.
Willdo.
>> +
>> +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]
>> +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*
>> +
>> + /* 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...
>> + */
>> + 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...
>> + 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...
>> + */
>> + 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...
>> +
>> + 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...
>> +
>> + 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.
>> +
>> +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*
>> + */
>> +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...
