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

From: Junio C Hamano <gitster@pobox.com>
Date: May 27, 2022, 21:20 UTC
Message-ID: <xmqqzgj21xm4.fsf@gitster.g>
In-Reply-To: <pull.1271.git.git.1653684771998.gitgitgadget@gmail.com>

"John Cai via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 5 quoted lines

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

Show 12 quoted lines

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