hook: Provide GIT_HOOK for all hooks

[john-cai]

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.

CC: Emily Shaffer emilyshaffer@google.com
cc: Ævar Arnfjörð Bjarmason avarab@gmail.com

[gitgitgadget-git]

There are issues in commit dd3f440:
hook: Provide GIT_HOOK for all hooks
Prefixed commit message must be in lower case
Commit not signed off

[gitgitgadget-git]

There are issues in commit 20b0fed:
hook: Provide GIT_HOOK for all hooks
Prefixed commit message must be in lower case
Commit not signed off

[john-cai]

/preview

[gitgitgadget-git]

Preview email sent as pull.1271.git.git.1653683544318.gitgitgadget@gmail.com

[john-cai]

/preview

[gitgitgadget-git]

Preview email sent as pull.1271.git.git.1653684451756.gitgitgadget@gmail.com

[john-cai]

/submit

[gitgitgadget-git]

Submitted as pull.1271.git.git.1653684771998.gitgitgadget@gmail.com

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git/ pr-git-1271/john-cai/jc/set-git-hooks-env-var-v1

To fetch this version to local tag pr-git-1271/john-cai/jc/set-git-hooks-env-var-v1:

git fetch --no-tags https://github.com/gitgitgadget/git/ tag pr-git-1271/john-cai/jc/set-git-hooks-env-var-v1

[gitgitgadget-git]

On the Git mailing list, Junio C Hamano wrote (reply to this):

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

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

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

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.

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

[gitgitgadget-git]

On the Git mailing list, John Cai wrote (reply to this):

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

[gitgitgadget-git]

On the Git mailing list, Ævar Arnfjörð Bjarmason wrote (reply to this):

On Fri, May 27 2022, John Cai via GitGitGadget wrote:

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

You can use one executable for multiple hooks already, I've written such
dispatchers that just look at the argv of the process.

What we will need something like this for is for the config-based hooks,
and I think it makes sense to have a facility that's portable across
both methods of hook invocations.

I really don't mind this change, and I think it's a good one to
make.

But the commit message & documentation here really should be updated to
reflect that this is currently superfluous to inspecting argv in the
hook process, and that we're providing this anyway for XYZ reason.

[gitgitgadget-git]

User Ævar Arnfjörð Bjarmason <avarab@gmail.com> has been added to the cc: list.

[gitgitgadget-git]

On the Git mailing list, John Cai wrote (reply to this):

Hi Ævar

On 28 May 2022, at 11:53, Ævar Arnfjörð Bjarmason wrote:

> On Fri, May 27 2022, John Cai via GitGitGadget wrote:
>
>> 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.
>
> You can use one executable for multiple hooks already, I've written such
> dispatchers that just look at the argv of the process.
>
> What we will need something like this for is for the config-based hooks,
> and I think it makes sense to have a facility that's portable across
> both methods of hook invocations.

Ah yes, thanks for pointing this out. I will re-roll the commit message as we
as clarity the documentation.

>
> I really don't mind this change, and I think it's a good one to
> make.
>
> But the commit message & documentation here really should be updated to
> reflect that this is currently superfluous to inspecting argv in the
> hook process, and that we're providing this anyway for XYZ reason.

[gitgitgadget-git]

On the Git mailing list, Junio C Hamano wrote (reply to this):

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> But the commit message & documentation here really should be updated to
> reflect that this is currently superfluous to inspecting argv in the
> hook process, and that we're providing this anyway for XYZ reason.

Or this probably is better added as part of the series that actually
adds the mechanism to trigger hooks defined in the configuration
file.  

Then "we do not need it now, but we will in the future because we
will do XYZ" does not have to be said, which is a huge plus.

Thanks.

[gitgitgadget-git]

On the Git mailing list, John Cai wrote (reply to this):

On 28 May 2022, at 13:24, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>> But the commit message & documentation here really should be updated to
>> reflect that this is currently superfluous to inspecting argv in the
>> hook process, and that we're providing this anyway for XYZ reason.
>
> Or this probably is better added as part of the series that actually
> adds the mechanism to trigger hooks defined in the configuration
> file.

I don't mind including this as part of Ævar's config hook series. On the other
hand this patch could allow the config hooks series to be smaller and more
easily reviewed.

I'm okay either way--maybe Ævar can speak to what his preference is.

>
> Then "we do not need it now, but we will in the future because we
> will do XYZ" does not have to be said, which is a huge plus.
>
> Thanks.

thanks
John