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

On Fri, Mar 12, 2021 at 09:54:28AM +0100, Ævar Arnfjörð Bjarmason wrote:

On Thu, Mar 11 2021, Emily Shaffer wrote:

quoted

 'git hook' list <hook-name>
+'git hook' run [(-e|--env)=<var>...] [(-a|--arg)=<arg>...] <hook-name>

[...]

quoted

+	switch (cfg)
+	{
+		case HOOKDIR_ERROR:

Overly indented case statements again.

Thanks. Probably need to fix my autoformatter or something.

quoted

+			fprintf(stderr, _("Skipping legacy hook at '%s'\n"),
+				path);
+			/* FALLTHROUGH */
+		case HOOKDIR_NO:
+			return 0;
+		case HOOKDIR_WARN:
+			fprintf(stderr, _("Running legacy hook at '%s'\n"),
+				path);
+			return 1;
+		case HOOKDIR_INTERACTIVE:
+			do {
+				/*
+				 * TRANSLATORS: Make sure to include [Y] and [n]
+				 * in your translation. Only English input is
+				 * accepted. Default option is "yes".
+				 */
+				fprintf(stderr, _("Run '%s'? [Yn] "), path);

Nit: [Y/n]

ACK

quoted

+				} else if (starts_with(prompt.buf, "y")) {

So also "Y", "yes" and "yellow"...

That's also how add-patch.c:prompt_yesno(),
builtin/bisect--helper.c:decide_next(), and
git-add--interactive.perl:prompt_yesno (assuming I'm grokking the perl
correctly) work. builtin/clean.c:ask_each_cmd checks that the user's
reply matches a substring anchored to the beginning (e.g. "y", "ye",
"yes").  git-diftool--helper.sh:launch_merge_tool just checks for "not
'n'". And git-send-email.perl:ask just checks for "'y' or not".

So I think there's a little flexibility :) But I like builtin/clean.c's
approach most out of these, so I'll switch.

quoted

[...]
 	git hook list pre-commit >actual &&
 	# the hookdir annotation is translated
-	test_i18ncmp expected actual
+	test_i18ncmp expected actual &&
+
+	test_write_lines n | git hook run pre-commit 2>actual &&
+	! grep "Legacy Hook" actual &&
+
+	test_write_lines y | git hook run pre-commit 2>actual &&
+	grep "Legacy Hook" actual
+'
+
+test_expect_success 'inline hook definitions execute oneliners' '
+	test_config hook.pre-commit.command "echo \"Hello World\"" &&
+
+	echo "Hello World" >expected &&
+
+	# hooks are run with stdout_to_stderr = 1
+	git hook run pre-commit 2>actual &&
+	test_cmp expected actual
+'
+
+test_expect_success 'inline hook definitions resolve paths' '
+	write_script sample-hook.sh <<-EOF &&
+	echo \"Sample Hook\"
+	EOF
+
+	test_when_finished "rm sample-hook.sh" &&
+
+	test_config hook.pre-commit.command "\"$(pwd)/sample-hook.sh\"" &&
+
+	echo \"Sample Hook\" >expected &&
+
+	# hooks are run with stdout_to_stderr = 1
+	git hook run pre-commit 2>actual &&
+	test_cmp expected actual
+'
+
+test_expect_success 'hookdir hook included in git hook run' '
+	setup_hookdir &&
+
+	echo \"Legacy Hook\" >expected &&
+
+	# hooks are run with stdout_to_stderr = 1
+	git hook run pre-commit 2>actual &&
+	test_cmp expected actual
+'
+
+test_expect_success 'out-of-repo runs excluded' '
+	setup_hooks &&
+
+	nongit test_must_fail git hook run pre-commit
 '
 
 test_expect_success 'hook.runHookDir is tolerant to unknown values' '

No tests for --env or --arg?

Yikes, I guess not. Thanks, will add.

 - Emily