| Message ID | 20210812004258.74318-1-emilyshaffer@google.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | config-based hooks restarted | expand |
Emily Shaffer <emilyshaffer@google.com> writes: > This is the config-based hooks topic rebased onto v4 of Ævar's > branch[1]. I have [1] in my tree, but these patches do not seem to apply cleanly; I see a failure in "git hooks list" step. Should I perhaps try merging [1] to one of the v2.33-rc and then queue these on top?
Junio C Hamano <gitster@pobox.com> writes: > Emily Shaffer <emilyshaffer@google.com> writes: > >> This is the config-based hooks topic rebased onto v4 of Ævar's >> branch[1]. > > I have [1] in my tree, but these patches do not seem to apply > cleanly; I see a failure in "git hooks list" step. > > Should I perhaps try merging [1] to one of the v2.33-rc and then > queue these on top? Ah, I figured it out. Your 3/6 seems to be stale wrt 4787177b (hook: support passing stdin to hooks, 2021-08-03) that came from https://lore.kernel.org/git/patch-v4-22.36-639e59e9ed0-20210803T191505Z-avarab@gmail.com/ That patch did this to the early part of builtin/hook.c: diff --git a/builtin/hook.c b/builtin/hook.c index f33db9953c..27dce6a2f0 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 [--ignore-missing] <hook-name> [-- <hook-args>]") + N_("git hook run [--ignore-missing] [--to-stdin=<path>] <hook-name> [-- <hook-args>]") static const char * const builtin_hook_usage[] = { BUILTIN_HOOK_RUN_USAGE, but your [v2 3/6] here expects something entirely different. diff --git a/builtin/hook.c b/builtin/hook.c index 12c9126032..c36b05376c 100644 --- a/builtin/hook.c +++ b/builtin/hook.c @@ -8,8 +8,11 @@ #define BUILTIN_HOOK_RUN_USAGE \ N_("git hook run [--to-stdin=<path>] <hook-name> [-- <hook-args>]") +#define BUILTIN_HOOK_LIST_USAGE \ + N_("git hook list <hook-name>") I've wiggled the patch in, as there wasn't any other funny inconsistency like this one, but please double check the result after I push it out perhaps tomorrow morning. Thanks.
On Wed, Aug 11, 2021 at 10:02:53PM -0700, Junio C Hamano wrote: > > Junio C Hamano <gitster@pobox.com> writes: > > > Emily Shaffer <emilyshaffer@google.com> writes: > > > >> This is the config-based hooks topic rebased onto v4 of Ævar's > >> branch[1]. > > > > I have [1] in my tree, but these patches do not seem to apply > > cleanly; I see a failure in "git hooks list" step. > > > > Should I perhaps try merging [1] to one of the v2.33-rc and then > > queue these on top? > > Ah, I figured it out. Your 3/6 seems to be stale wrt 4787177b > (hook: support passing stdin to hooks, 2021-08-03) that came from > https://lore.kernel.org/git/patch-v4-22.36-639e59e9ed0-20210803T191505Z-avarab@gmail.com/ Ah, I am sorry! > That patch did this to the early part of builtin/hook.c: > > diff --git a/builtin/hook.c b/builtin/hook.c > index f33db9953c..27dce6a2f0 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 [--ignore-missing] <hook-name> [-- <hook-args>]") > + N_("git hook run [--ignore-missing] [--to-stdin=<path>] <hook-name> [-- <hook-args>]") > > static const char * const builtin_hook_usage[] = { > BUILTIN_HOOK_RUN_USAGE, > > > but your [v2 3/6] here expects something entirely different. > > diff --git a/builtin/hook.c b/builtin/hook.c > index 12c9126032..c36b05376c 100644 > --- a/builtin/hook.c > +++ b/builtin/hook.c > @@ -8,8 +8,11 @@ > > #define BUILTIN_HOOK_RUN_USAGE \ > N_("git hook run [--to-stdin=<path>] <hook-name> [-- <hook-args>]") > +#define BUILTIN_HOOK_LIST_USAGE \ > + N_("git hook list <hook-name>") > > I've wiggled the patch in, as there wasn't any other funny > inconsistency like this one, but please double check the result > after I push it out perhaps tomorrow morning. I'll take a look and update local accordingly. Sorry for the trouble. - Emily
This is the config-based hooks topic rebased onto v4 of Ævar's branch[1]. There is a happy CI build of it on GitHub[2]. The topic overall adds the ability to set up hooks by modifying the config, in addition to placing specially named hooks into the hookdir. This enables users to specify multiple hooks for a given event, and so this topic also fleshes out the use of the run_processes_parallel() API which is now introduced in Ævar's reordering of prior patches. Patches 1-4 make some minor changes to prepare Ævar's series to handle more than one hook at a time. With the exception of patch 4, there should be no behavior change for existing hooks. Patch 2 is opinionated about which hooks should and shouldn't be allowed to run in parallel; if you care about a specific hook, please take a look there. Patch 5 is the motivating feature - it begins to parse the config looking for hooks. Patch 6 takes advantage of the decoupling of hooks and GITDIR to allow out-of-repo hook runs, which would only run hooks specified in the system or global config. This mainly targets 'sendemail-validate', but 'git-sendemail.perl' still explicitly disallows out-of-repo hook execution on that hook for now. (Maybe that change should be added in this series? Or maybe patch 6 belongs with that kind of change?) Since v1: The largest change is that the config schema is different, following the discussion from [3] and on. 'hookcmd' has gone away entirely, and all config-specified hooks need a user-provided name associated with them. I have also dropped the 'skip' config from this series (also discussed in [3]) as users can reset their 'hook.myhook.command' to 'true' or some other noop in a later config to effectively disable any hook. The 'skip' feature may be added later or may not, depending on if users ask us for it. The 'from_hookdir' field has been removed entirely. When storing hooks by name instead of by command, it makes sense enough to store the one from the hookdir in a special way - NULL name - because it does not have a name to specify. This way we can use that field as an indicator that the hook came from the hookdir, and we also do not need to reserve a name to use for hookdir hooks (as opposed to setting hookdir hooks to have name = "FROM_HOOKDIR" or something). The tests have been moved into t1800-hook.sh and simplified. The 'struct run_hooks_opt' initializer has been moved to macros (as opposed to functions), and .jobs=0 indicates that we should look up the configured job count or nproc at hook run time. Ævar asked me to change these macros to RUN_HOOKS_OPT_INIT and RUN_HOOKS_OPT_INIT_SYNC or something to simplify the merge, but I did not do that - I removed all instances of RUN_HOOKS_OPT_INIT and now everybody uses RUN_HOOKS_OPT_INIT_SYNC or RUN_HOOKS_OPT_INIT_ASYNC. I think it's important for contributors to be able to tell at a glance whether the hook event expects resource contention that would prevent parallelism. I also think it makes the review easier (if longer) for folks who are reviewing trying to decide whether the parallelism is appropriate for each hook event. I do think that run_hooks_oneshot() also is a step backwards in this regard, but I didn't get a chance to say so on the commit that introduces it yet. Thanks in advance, all. - Emily 1: https://lore.kernel.org/git/cover-v4-00.36-00000000000-20210803T191505Z-avarab%40gmail.com 2: https://github.com/nasamuffin/git/actions/runs/1122126800 (which points to an earlier successul run before I messed with the commit messages) 3: https://lore.kernel.org/git/87fswey5wd.fsf%40evledraar.gmail.com Emily Shaffer (6): hook: run a list of hooks instead hook: allow parallel hook execution hook: introduce "git hook list" hook: allow running non-native hooks hook: include hooks from the config hook: allow out-of-repo 'git hook' invocations Documentation/config/hook.txt | 9 ++ Documentation/git-hook.txt | 48 +++++- builtin/am.c | 4 +- builtin/checkout.c | 2 +- builtin/clone.c | 2 +- builtin/hook.c | 68 +++++++- builtin/merge.c | 2 +- builtin/rebase.c | 2 +- builtin/receive-pack.c | 9 +- builtin/worktree.c | 2 +- commit.c | 2 +- git.c | 2 +- hook.c | 289 +++++++++++++++++++++++++++++----- hook.h | 61 +++++-- read-cache.c | 2 +- refs.c | 2 +- reset.c | 3 +- sequencer.c | 4 +- t/t1800-hook.sh | 161 ++++++++++++++++++- transport.c | 2 +- 20 files changed, 596 insertions(+), 80 deletions(-) create mode 100644 Documentation/config/hook.txt Range-diff against v1: 1: c4b95fb08a < -: ---------- hook: treat hookdir hook specially -: ---------- > 1: 5177e8ba2c hook: run a list of hooks instead -: ---------- > 2: eda439cd57 hook: allow parallel hook execution -: ---------- > 3: cdfe3b6e16 hook: introduce "git hook list" 2: e6a56ac674 = 4: eb4e03e22b hook: allow running non-native hooks 3: 32ad49ea9b ! 5: 2c8e874158 hook: include hooks from the config @@ Commit message hooks to run for a given event. Multiple commands can be specified for a given hook by providing - multiple "hook.<hookname>.command = <path-to-hook>" lines. Hooks will be - run in config order. + multiple "hook.<friendly-name>.command = <path-to-hook>" and + "hook.<friendly-name>.event = <hook-event>" lines. Hooks will be run in + config order of the "hook.<name>.event" lines. For example: $ git config --list | grep ^hook - hook.pre-commit.command=~/bar.sh + hook.bar.command=~/bar.sh + hook.bar.event=pre-commit $ git hook run # Runs ~/bar.sh @@ hook.c: const char *find_hook_gently(const char *name) - struct hook *to_add = xmalloc(sizeof(*to_add)); - to_add->hook_path = hook_path; - to_add->feed_pipe_cb_data = NULL; -- to_add->from_hookdir = 1; - list_add_tail(&to_add->list, hook_head); - } + /* Add the hook from the hookdir. The placeholder makes it easier to @@ hook.c: static int pick_next_hook(struct child_process *cp, + cp->use_shell = !!run_me->name; + /* add command */ -- if (run_me->from_hookdir && hook_cb->options->absolute_path) +- if (hook_cb->options->absolute_path) - strvec_push(&cp->args, absolute_path(run_me->hook_path)); - else - strvec_push(&cp->args, run_me->hook_path); @@ hook.h: int hook_exists(const char *hookname); struct list_head list; - /* The path to the hook */ - const char *hook_path; -- -- unsigned from_hookdir : 1; + /* + * The friendly name of the hook. NULL indicates the hook is from the + * hookdir. 4: 91e54185b3 = 6: 3216e51b6b hook: allow out-of-repo 'git hook' invocations