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

On Fri, Dec 11, 2020 at 10:15:26AM +0000, Phillip Wood wrote:

Hi Emily

On 05/12/2020 01:45, Emily Shaffer wrote:

quoted

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.

quoted

+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.

quoted

+			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