Subject: Re: [PATCH 08/17] hook: add 'run' subcommand

From: Emily Shaffer <[email-masked]>
Date: Tue, 15 Dec 2020 13:41:09 -0800

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