hook: provide GIT_HOOK for all hooks

PATCH In review John Cai via GitGitGadget 4 participants 7 messages May 27, 2022 – May 31, 2022 Documentation

lore

Share & tools

lore

John Cai via GitGitGadget

author

2022-05-27 20:52 UTC #0 lore raw

PATCH 1/1 hook: provide GIT_HOOK for all hooks

From: John Cai <[email protected]>

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.

Signed-off-by John Cai <[email protected]>

---
hook: Provide GIT_HOOK for all hooks

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.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1271%2Fjohn-cai%2Fjc%2Fset-git-hooks-env-var-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1271/john-cai/jc/set-git-hooks-env-var-v1
Pull-Request: https://github.com/git/git/pull/1271

 Documentation/githooks.txt |  4 ++++
 hook.c                     |  2 ++
 t/t1800-hook.sh            | 12 ++++++++++++
 3 files changed, 18 insertions(+)

Documentation/githooks.txt +4 -0

index a16e62bc8c8..b27ed8d11b6 100644
@@ -31,6 +31,10 @@ Hooks can get their arguments via the environment, command-line
 arguments, and stdin. See the documentation for each hook below for
 details.
 
+The `$GIT_HOOK` environment variable is passed to all hooks and holds the
+triggering hook event, eg: `pre-commit`, `update`, etc. This allows one
+executable to be used for multiple hooks.
+
 `git init` may copy hooks to the new repository, depending on its
 configuration. See the "TEMPLATE DIRECTORY" section in
 linkgit:git-init[1] for details. When the rest of this document refers

hook.c +2 -0

index 1d51be3b77a..966f2114db4 100644
@@ -144,6 +144,8 @@ int run_hooks_opt(const char *hook_name, struct run_hooks_opt *options)
 		cb_data.hook_path = abs_path.buf;
 	}
 
+	strvec_pushf(&cb_data.options->env,"GIT_HOOK=%s", hook_name);
+
 	run_processes_parallel_tr2(jobs,
 				   pick_next_hook,
 				   notify_start_failure,

t/t1800-hook.sh +12 -0

index 26ed5e11bc8..a22c1a82a5e 100755
@@ -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
+
+	cat >expect <<-\EOF &&
+	test-hook
+	EOF
+	git hook run test-hook 2>actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'git hook run: stdout and stderr both write to our stderr' '
 	test_hook test-hook <<-EOF &&
 	echo >&1 Will end up on stderr

base-commit: 8ddf593a250e07d388059f7e3f471078e1d2ed5c

Signature · 1 line

gitgitgadget

Junio C Hamano

2022-05-27 21:20 UTC #1 lore raw

Quoted 5 lines · John Cai via GitGitGadget

From: John Cai <[email protected]>
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?

t/t1800-hook.sh test_expect_success 'git hook run: basic' ' · line 44

Quoted 12 lines

t/t1800-hook.sh +4 -0

index 26ed5e11bc8..a22c1a82a5e 100755
@@ -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.

t/t1800-hook.sh test_expect_success 'git hook run: basic' ' · line 48

Quoted 3 lines

t/t1800-hook.sh +3 -0

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

Quoted 3 lines

t/t1800-hook.sh +3 -0

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

John Cai

2022-05-28 04:27 UTC #2 lore raw

Hi Junio

Quoted 12 lines · Junio C Hamano

Quoted 5 lines · John Cai via GitGitGadget
From: John Cai <[email protected]>
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".

Quoted 5 lines

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.

Quoted 2 lines

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?

Quoted 9 lines

t/t1800-hook.sh +0 -0

index 26ed5e11bc8..a22c1a82a5e 100755
@@ -38,6 +38,18 @@ test_expect_success 'git hook run: basic' '
 	test_cmp expect actual
 '

Quoted 8 lines

t/t1800-hook.sh test_expect_success 'git hook run: basic' ' · line 44
Quoted 4 lines
t/t1800-hook.sh +4 -0
+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.

Quoted 24 lines

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.
t/t1800-hook.sh test_expect_success 'git hook run: basic' ' · line 48
Quoted 3 lines
t/t1800-hook.sh +3 -0
+	cat >expect <<-\EOF &&
+	test-hook
+	EOF
For one-liner,

Quoted 3 lines

	echo test-hook >expect &&
should be a more compact and equally understandable way to write this.

good point!

Quoted 4 lines

Quoted 4 lines
t/t1800-hook.sh +3 -0
+	git hook run test-hook 2>actual &&
+	test_cmp expect actual
+'

Ævar Arnfjörð Bjarmason

2022-05-28 15:55 UTC #3 lore raw

In reply to John Cai via GitGitGadget

Quoted 5 lines · John Cai via GitGitGadget

From: John Cai <[email protected]>
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.

John Cai

2022-05-28 16:43 UTC #4 lore raw

Hi Ævar

Quoted 8 lines · Ævar Arnfjörð Bjarmason

Quoted 5 lines · John Cai via GitGitGadget
From: John Cai <[email protected]>
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.

Quoted 6 lines

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.

Quoted 7 lines

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.

Junio C Hamano

2022-05-28 17:24 UTC #5 lore raw

In reply to Ævar Arnfjörð Bjarmason

Quoted 3 lines · Ævar Arnfjörð Bjarmason

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.

John Cai

2022-05-31 01:31 UTC #6 lore raw

Quoted 9 lines · Junio C Hamano

Quoted 3 lines · Ævar Arnfjörð Bjarmason
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.

Quoted 5 lines

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