Re: [PATCH] hook: provide GIT_HOOK for all hooks

[John Cai <johncai86@gmail.com>]

Hi Junio

On 27 May 2022, at 17:20, Junio C Hamano wrote:

> "John Cai via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>> From: John Cai <johncai86@gmail.com>
>>
>> In order to allow users to use one executable for multiple hooks,
>> provide a GIT_HOOK variable that is set to the hook event that triggered
>> it.
>
> I agree it would be handy to give hooks to play multiple roles by
> dispatching on its name, just like our "git" potty can dispatch
> built-ins when called "git-foo".
>
> I do not think GIT_HOOK is a good name for the environment variable
> that is used for that purpose, though.  It is easily mistaken as if
> end users can set GIT_HOOK environment themselves to point at a
> program and cause "git" to run it whenever it may want to run any
> hook, for example.  IOW, the name is overly broad.

Yes, I see what you mean. It would be good to pick a more specific variable.

>
> How about calling it with a name with "HOOK" and "NAME" in it?

For lack of imagination, would GIT_HOOK_NAME still be too broad?

>
>> diff --git a/t/t1800-hook.sh b/t/t1800-hook.sh
>> index 26ed5e11bc8..a22c1a82a5e 100755
>> --- a/t/t1800-hook.sh
>> +++ b/t/t1800-hook.sh
>> @@ -38,6 +38,18 @@ test_expect_success 'git hook run: basic' '
>>  	test_cmp expect actual
>>  '
>>
>> +test_expect_success 'git hook run: $GIT_HOOK' '
>> +	test_hook test-hook <<-EOF &&
>> +	printenv GIT_HOOK
>> +	EOF
>
> This will introduce the first hit from "git grep printenv".
>
> It is not even in POSIX.  Do we absolutely need to?

certainly not, I'll change this.

>
> Perhaps
>
>     echo "$GIT_HOOK"
>
> is sufficient, or if you want to distinguish an unset and set to
> empty string:
>
>     if test "${GIT_HOOK+set}" = "set"
>     then
>         echo "GIT_HOOK is set to '$GIT_HOOK'"
>     else
>         echo "GIT_HOOK is unset"
> 	exit 1
>     fi
>
> may be another way.
>
>> +	cat >expect <<-\EOF &&
>> +	test-hook
>> +	EOF
>
> For one-liner,
>
> 	echo test-hook >expect &&
>
> should be a more compact and equally understandable way to write this.

good point!

>
>> +	git hook run test-hook 2>actual &&
>> +	test_cmp expect actual
>> +'