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

From: Emily Shaffer <emilyshaffer@google.com>
Date: Mar 24, 2021, 21:29 UTC
Message-ID: <YFuvUlMOaJ0qAxzk@google.com>
In-Reply-To: <87a6r8hhvf.fsf@evledraar.gmail.com>

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

Show 14 quoted lines

> 
> 
> On Thu, Mar 11 2021, Emily Shaffer wrote:
> 
> >  'git hook' list <hook-name>
> > +'git hook' run [(-e|--env)=<var>...] [(-a|--arg)=<arg>...] <hook-name>
> 
> [...]
> 
> > +	switch (cfg)
> > +	{
> > +		case HOOKDIR_ERROR:
> 
> Overly indented case statements again.

Thanks. Probably need to fix my autoformatter or something.

Show 20 quoted lines

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

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

Show 59 quoted lines

> 
> > [...]
> >  	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