On Fri, Dec 11, 2020 at 10:15:26AM +0000, Phillip Wood wrote:
>
> Hi Emily
>
> On 05/12/2020 01:45, Emily Shaffer wrote:
> > 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.
> >
> > For now, the hook commands will run in config order, in series. As
> > alternate ordering or parallelism is supported in the future, we should
> > add knobs to use those to the command line as well.
> >
> > As with the legacy hook implementation, all stdout generated by hook
> > commands is redirected to stderr. Piping from stdin is not yet
> > supported.
> >
> > Legacy hooks (those present in $GITDIR/hooks) are run at the end of the
> > execution list. For now, there is no way to disable them.
> >
> > Users may wish to provide hook commands like 'git config
> > hook.pre-commit.command "~/linter.sh --pre-commit"'. To enable this, the
> > contents of the 'hook.*.command' and 'hookcmd.*.command' strings are
> > first split by space or quotes into an argv_array, then expanded with
> > 'expand_user_path()'.
>
> I'm a bit confused by this last paragraph, the docs below say we pass the
> string to the shell and that's what the implementation seems to do. If we're
> running a lot of hooks then maybe it would be worth using split_cmdline()
> and expand_user_path() rather than invoking the shell for each hook we run.
Yeah, I think you are right that the commit message is stale. I had some
trouble getting things to work correctly with split_cmdline() and
expand_user_path(), so I'd prefer to run with shell.
>
> I'm afraid I've only had time to skip the patch, there are a couple of minor
> comments below.
No problem. Thanks for having a look.
> > +static int should_include_hookdir(const char *path, enum hookdir_opt cfg)
> > +{
> > + struct strbuf prompt = STRBUF_INIT;
> > + /*
> > + * If the path doesn't exist, don't bother adding the empty hook and
> > + * don't bother checking the config or prompting the user.
> > + */
> > + if (!path)
> > + return 0;
> > +
> > + switch (cfg)
> > + {
> > + case hookdir_no:
>
> Style nit: we normally use uppercase for constants and enums.
OK. Thanks - will fix where it's introduced and update subsequent
patches.
>
> > + return 0;
> > + case hookdir_unknown:
> > + fprintf(stderr,
> > + _("Unrecognized value for 'hook.runHookDir'. "
> > + "Is there a typo? "));
>
> What happens at the moment if core.hooksPath does not exist?
When core.hooksPath does not exist then $GIT_DIR/hooks/ is used instead.
My setup currently doesn't have $GIT_DIR/hooks/ and runs happily. That
bit of logic (core.hooksPath or $GIT_DIR/hooks) is done in
run-command.h:find_hook() so I don't worry about it manually here.
However, your comment caused me to investigate what happens when
core.hooksPath DOES exist - and I found a bug. Because the 'git hook'
builtin doesn't call the default configuration callback, I miss
core.hooksPath hooks during 'git hook list' - but not during hooks
invoked during regular Git process runs. Very confusing :) So thanks for
the hint.
- Emily
