{"thread":{"id":"57929","subject":"[PATCH] hook: provide GIT_HOOK for all hooks","startedAt":"2022-05-27T20:52:58Z","lastAt":"2022-05-31T01:31:46Z","messageCount":7,"participants":["John Cai via GitGitGadget","Junio C Hamano","John Cai","Ævar Arnfjörð Bjarmason"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"456322","messageId":"pull.1271.git.git.1653684771998.gitgitgadget@gmail.com","threadId":"57929","inReplyTo":null,"subject":"[PATCH] hook: provide GIT_HOOK for all hooks","fromName":"John Cai via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-05-27T20:52:51Z","receivedAt":"2022-05-27T20:52:58Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"From: John Cai <johncai86@gmail.com>\n\nIn order to allow users to use one executable for multiple hooks,\nprovide a GIT_HOOK variable that is set to the hook event that triggered\nit.\n\nSigned-off-by: John Cai <johncai86@gmail.com>\n---\n    hook: Provide GIT_HOOK for all hooks\n    \n    In order to allow users to use one executable for multiple hooks,\n    provide a GIT_HOOK variable that is set to the hook event that triggered\n    it.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1271%2Fjohn-cai%2Fjc%2Fset-git-hooks-env-var-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1271/john-cai/jc/set-git-hooks-env-var-v1\nPull-Request: https://github.com/git/git/pull/1271\n\n Documentation/githooks.txt |  4 ++++\n hook.c                     |  2 ++\n t/t1800-hook.sh            | 12 ++++++++++++\n 3 files changed, 18 insertions(+)\n\ndiff --git a/Documentation/githooks.txt b/Documentation/githooks.txt\nindex a16e62bc8c8..b27ed8d11b6 100644\n--- a/Documentation/githooks.txt\n+++ b/Documentation/githooks.txt\n@@ -31,6 +31,10 @@ Hooks can get their arguments via the environment, command-line\n arguments, and stdin. See the documentation for each hook below for\n details.\n \n+The `$GIT_HOOK` environment variable is passed to all hooks and holds the\n+triggering hook event, eg: `pre-commit`, `update`, etc. This allows one\n+executable to be used for multiple hooks.\n+\n `git init` may copy hooks to the new repository, depending on its\n configuration. See the \"TEMPLATE DIRECTORY\" section in\n linkgit:git-init[1] for details. When the rest of this document refers\ndiff --git a/hook.c b/hook.c\nindex 1d51be3b77a..966f2114db4 100644\n--- a/hook.c\n+++ b/hook.c\n@@ -144,6 +144,8 @@ int run_hooks_opt(const char *hook_name, struct run_hooks_opt *options)\n \t\tcb_data.hook_path = abs_path.buf;\n \t}\n \n+\tstrvec_pushf(&cb_data.options->env,\"GIT_HOOK=%s\", hook_name);\n+\n \trun_processes_parallel_tr2(jobs,\n \t\t\t\t   pick_next_hook,\n \t\t\t\t   notify_start_failure,\ndiff --git a/t/t1800-hook.sh b/t/t1800-hook.sh\nindex 26ed5e11bc8..a22c1a82a5e 100755\n--- a/t/t1800-hook.sh\n+++ b/t/t1800-hook.sh\n@@ -38,6 +38,18 @@ test_expect_success 'git hook run: basic' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'git hook run: $GIT_HOOK' '\n+\ttest_hook test-hook <<-EOF &&\n+\tprintenv GIT_HOOK\n+\tEOF\n+\n+\tcat >expect <<-\\EOF &&\n+\ttest-hook\n+\tEOF\n+\tgit hook run test-hook 2>actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'git hook run: stdout and stderr both write to our stderr' '\n \ttest_hook test-hook <<-EOF &&\n \techo >&1 Will end up on stderr\n\nbase-commit: 8ddf593a250e07d388059f7e3f471078e1d2ed5c\n-- \ngitgitgadget\n"},{"id":"456329","messageId":"xmqqzgj21xm4.fsf@gitster.g","threadId":"57929","inReplyTo":"pull.1271.git.git.1653684771998.gitgitgadget@gmail.com","subject":"Re: [PATCH] hook: provide GIT_HOOK for all hooks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-05-27T21:20:19Z","receivedAt":"2022-05-27T21:20:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"John Cai via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> From: John Cai <johncai86@gmail.com>\n>\n> In order to allow users to use one executable for multiple hooks,\n> provide a GIT_HOOK variable that is set to the hook event that triggered\n> it.\n\nI agree it would be handy to give hooks to play multiple roles by\ndispatching on its name, just like our \"git\" potty can dispatch\nbuilt-ins when called \"git-foo\".\n\nI do not think GIT_HOOK is a good name for the environment variable\nthat is used for that purpose, though.  It is easily mistaken as if\nend users can set GIT_HOOK environment themselves to point at a\nprogram and cause \"git\" to run it whenever it may want to run any\nhook, for example.  IOW, the name is overly broad.\n\nHow about calling it with a name with \"HOOK\" and \"NAME\" in it?\n\n> diff --git a/t/t1800-hook.sh b/t/t1800-hook.sh\n> index 26ed5e11bc8..a22c1a82a5e 100755\n> --- a/t/t1800-hook.sh\n> +++ b/t/t1800-hook.sh\n> @@ -38,6 +38,18 @@ test_expect_success 'git hook run: basic' '\n>  \ttest_cmp expect actual\n>  '\n>  \n> +test_expect_success 'git hook run: $GIT_HOOK' '\n> +\ttest_hook test-hook <<-EOF &&\n> +\tprintenv GIT_HOOK\n> +\tEOF\n\nThis will introduce the first hit from \"git grep printenv\".\n\nIt is not even in POSIX.  Do we absolutely need to?\n\nPerhaps\n\n    echo \"$GIT_HOOK\"\n\nis sufficient, or if you want to distinguish an unset and set to\nempty string:\n\n    if test \"${GIT_HOOK+set}\" = \"set\"\n    then\n        echo \"GIT_HOOK is set to '$GIT_HOOK'\"\n    else\n        echo \"GIT_HOOK is unset\"\n\texit 1\n    fi\n\nmay be another way.\n\n> +\tcat >expect <<-\\EOF &&\n> +\ttest-hook\n> +\tEOF\n\nFor one-liner, \n\n\techo test-hook >expect &&\n\nshould be a more compact and equally understandable way to write this.\n\n> +\tgit hook run test-hook 2>actual &&\n> +\ttest_cmp expect actual\n> +'\n"},{"id":"456342","messageId":"E0D2A9A0-FB95-47ED-B672-93AAFE798CD9@gmail.com","threadId":"57929","inReplyTo":"xmqqzgj21xm4.fsf@gitster.g","subject":"Re: [PATCH] hook: provide GIT_HOOK for all hooks","fromName":"John Cai","fromEmail":"johncai86@gmail.com","sentAt":"2022-05-28T04:26:40Z","receivedAt":"2022-05-28T04:27:03Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"Hi Junio\n\nOn 27 May 2022, at 17:20, Junio C Hamano wrote:\n\n> \"John Cai via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n>\n>> From: John Cai <johncai86@gmail.com>\n>>\n>> In order to allow users to use one executable for multiple hooks,\n>> provide a GIT_HOOK variable that is set to the hook event that triggered\n>> it.\n>\n> I agree it would be handy to give hooks to play multiple roles by\n> dispatching on its name, just like our \"git\" potty can dispatch\n> built-ins when called \"git-foo\".\n>\n> I do not think GIT_HOOK is a good name for the environment variable\n> that is used for that purpose, though.  It is easily mistaken as if\n> end users can set GIT_HOOK environment themselves to point at a\n> program and cause \"git\" to run it whenever it may want to run any\n> hook, for example.  IOW, the name is overly broad.\n\nYes, I see what you mean. It would be good to pick a more specific variable.\n\n>\n> How about calling it with a name with \"HOOK\" and \"NAME\" in it?\n\nFor lack of imagination, would GIT_HOOK_NAME still be too broad?\n\n>\n>> diff --git a/t/t1800-hook.sh b/t/t1800-hook.sh\n>> index 26ed5e11bc8..a22c1a82a5e 100755\n>> --- a/t/t1800-hook.sh\n>> +++ b/t/t1800-hook.sh\n>> @@ -38,6 +38,18 @@ test_expect_success 'git hook run: basic' '\n>>  \ttest_cmp expect actual\n>>  '\n>>\n>> +test_expect_success 'git hook run: $GIT_HOOK' '\n>> +\ttest_hook test-hook <<-EOF &&\n>> +\tprintenv GIT_HOOK\n>> +\tEOF\n>\n> This will introduce the first hit from \"git grep printenv\".\n>\n> It is not even in POSIX.  Do we absolutely need to?\n\ncertainly not, I'll change this.\n\n>\n> Perhaps\n>\n>     echo \"$GIT_HOOK\"\n>\n> is sufficient, or if you want to distinguish an unset and set to\n> empty string:\n>\n>     if test \"${GIT_HOOK+set}\" = \"set\"\n>     then\n>         echo \"GIT_HOOK is set to '$GIT_HOOK'\"\n>     else\n>         echo \"GIT_HOOK is unset\"\n> \texit 1\n>     fi\n>\n> may be another way.\n>\n>> +\tcat >expect <<-\\EOF &&\n>> +\ttest-hook\n>> +\tEOF\n>\n> For one-liner,\n>\n> \techo test-hook >expect &&\n>\n> should be a more compact and equally understandable way to write this.\n\ngood point!\n\n>\n>> +\tgit hook run test-hook 2>actual &&\n>> +\ttest_cmp expect actual\n>> +'\n"},{"id":"456344","messageId":"220528.86bkvh3b3q.gmgdl@evledraar.gmail.com","threadId":"57929","inReplyTo":"pull.1271.git.git.1653684771998.gitgitgadget@gmail.com","subject":"Re: [PATCH] hook: provide GIT_HOOK for all hooks","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-05-28T15:53:35Z","receivedAt":"2022-05-28T15:55:58Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, May 27 2022, John Cai via GitGitGadget wrote:\n\n> From: John Cai <johncai86@gmail.com>\n>\n> In order to allow users to use one executable for multiple hooks,\n> provide a GIT_HOOK variable that is set to the hook event that triggered\n> it.\n\nYou can use one executable for multiple hooks already, I've written such\ndispatchers that just look at the argv of the process.\n\nWhat we will need something like this for is for the config-based hooks,\nand I think it makes sense to have a facility that's portable across\nboth methods of hook invocations.\n\nI really don't mind this change, and I think it's a good one to\nmake.\n\nBut the commit message & documentation here really should be updated to\nreflect that this is currently superfluous to inspecting argv in the\nhook process, and that we're providing this anyway for XYZ reason.\n"},{"id":"456345","messageId":"4B161C12-4D12-4403-9C92-1A969A859E8F@gmail.com","threadId":"57929","inReplyTo":"220528.86bkvh3b3q.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH] hook: provide GIT_HOOK for all hooks","fromName":"John Cai","fromEmail":"johncai86@gmail.com","sentAt":"2022-05-28T16:42:55Z","receivedAt":"2022-05-28T16:43:02Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"Hi Ævar\n\nOn 28 May 2022, at 11:53, Ævar Arnfjörð Bjarmason wrote:\n\n> On Fri, May 27 2022, John Cai via GitGitGadget wrote:\n>\n>> From: John Cai <johncai86@gmail.com>\n>>\n>> In order to allow users to use one executable for multiple hooks,\n>> provide a GIT_HOOK variable that is set to the hook event that triggered\n>> it.\n>\n> You can use one executable for multiple hooks already, I've written such\n> dispatchers that just look at the argv of the process.\n>\n> What we will need something like this for is for the config-based hooks,\n> and I think it makes sense to have a facility that's portable across\n> both methods of hook invocations.\n\nAh yes, thanks for pointing this out. I will re-roll the commit message as we\nas clarity the documentation.\n\n>\n> I really don't mind this change, and I think it's a good one to\n> make.\n>\n> But the commit message & documentation here really should be updated to\n> reflect that this is currently superfluous to inspecting argv in the\n> hook process, and that we're providing this anyway for XYZ reason.\n"},{"id":"456346","messageId":"xmqqr14dzi2o.fsf@gitster.g","threadId":"57929","inReplyTo":"220528.86bkvh3b3q.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH] hook: provide GIT_HOOK for all hooks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-05-28T17:24:15Z","receivedAt":"2022-05-28T17:24:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:\n\n> But the commit message & documentation here really should be updated to\n> reflect that this is currently superfluous to inspecting argv in the\n> hook process, and that we're providing this anyway for XYZ reason.\n\nOr this probably is better added as part of the series that actually\nadds the mechanism to trigger hooks defined in the configuration\nfile.  \n\nThen \"we do not need it now, but we will in the future because we\nwill do XYZ\" does not have to be said, which is a huge plus.\n\nThanks.\n"},{"id":"456380","messageId":"FB3E687E-C381-4C9E-A1B4-13CBE3A48B7A@gmail.com","threadId":"57929","inReplyTo":"xmqqr14dzi2o.fsf@gitster.g","subject":"Re: [PATCH] hook: provide GIT_HOOK for all hooks","fromName":"John Cai","fromEmail":"johncai86@gmail.com","sentAt":"2022-05-31T01:31:37Z","receivedAt":"2022-05-31T01:31:46Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"\n\nOn 28 May 2022, at 13:24, Junio C Hamano wrote:\n\n> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:\n>\n>> But the commit message & documentation here really should be updated to\n>> reflect that this is currently superfluous to inspecting argv in the\n>> hook process, and that we're providing this anyway for XYZ reason.\n>\n> Or this probably is better added as part of the series that actually\n> adds the mechanism to trigger hooks defined in the configuration\n> file.\n\nI don't mind including this as part of Ævar's config hook series. On the other\nhand this patch could allow the config hooks series to be smaller and more\neasily reviewed.\n\nI'm okay either way--maybe Ævar can speak to what his preference is.\n\n>\n> Then \"we do not need it now, but we will in the future because we\n> will do XYZ\" does not have to be said, which is a huge plus.\n>\n> Thanks.\n\nthanks\nJohn\n"}]}