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

From: Emily Shaffer <emilyshaffer@google.com>
Date: Jan 7, 2022, 21:53 UTC
Message-ID: <Ydi2XEvZywEyImfC@google.com>
In-Reply-To: <patch-v6-01.17-ba6fd47482e-20211222T035755Z-avarab@gmail.com>

On Wed, Dec 22, 2021 at 04:59:27AM +0100, Ævar Arnfjörð Bjarmason wrote:

Show 11 quoted lines

> diff --git a/hook.h b/hook.h
> +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;
> +
> +	/* Emit an error if the hook is missing */
> +	unsigned int error_if_missing:1;

I wonder if it's premature to include error_if_missing, if we will always set it to 1 until way down in patch 11.

But I do not care all that much - at this point, I'll be honest, I'd just like to see the series merged.

Show 9 quoted lines

> diff --git a/t/t1800-hook.sh b/t/t1800-hook.sh
> new file mode 100755
> +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
> +'

It's a little unclear to me what "nonexistent" means - does it mean that it's a hook unknown to Git (e.g. not in hook-list.h)? Does it mean that the hook path doesn't exist? Does it mean the hook path is not executable?

I had to poke in the code a little bit but it looks like this whole known_hook() distinction has disappeared, so maybe I only find it confusing because I reviewed previous iterations.

It might be nice to at least leave a comment on this test case and point out that it relies on nobody having created a test-hook script in an earlier test. Or to clean up that path just in case, at the beginning of this one. I could see someone well-meaning adding another test before this for something simple and breaking this test.

No serious concerns here though - just a couple thoughts.

 - Emily