| Message ID | patch-01.13-a39c0748d3f-20211012T131934Z-avarab@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | hook.[ch]: new library to run hooks + simple hook conversion | expand |
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. > + > +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 >
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?)
On Tue, Oct 12 2021, René Scharfe wrote: > 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. >> + >> +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...
On Wed, Oct 13 2021, Bagas Sanjaya wrote: > 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.
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