Repository navigation
Two hook configuration changes - #2272
Open
thomasbachem wants to merge 2 commits into
Open
thomasbachem wants to merge 2 commits into
thomasbachem wants to merge 2 commits into
Conversation
Author
|
/preview |
|
Preview email sent as pull.2272.git.1792513805.gitgitgadget@gmail.com |
Author
|
/preview |
|
Preview email sent as pull.2272.git.1792516893.gitgitgadget@gmail.com |
Author
|
/submit |
|
Submitted as pull.2272.git.1792520353.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
![gitgitgadget[bot]](https://avatars.githubusercontent.com/in/12836?s=60&v=4){kind=small}
| @@ -82,7 +82,9 @@ static int list(int argc, const char **argv, const char *prefix, | |||
|
|
|||
There was a problem hiding this comment.
Junio C Hamano wrote on the Git mailing list (how to reply to this email):
"Thomas Bachem via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Thomas Bachem <mail@thomasbachem.com>
>
> The documentation added by dcfb5af67e (hook: add hook.<event>.enabled
> switch, 2026-04-10) says that with hook.<event>.enabled set to false
> "no hooks fire for that event", but only hooks defined in the
> configuration are skipped.
It is good to see that the original author and those who helped
were CC'ed.
I have a suspicion that the current behavior may be very much
deliberate, and the documentation was written too loosely. In
other words, the author may have been so excited about their shiny
new toy, the configuration-defined hook mechanism, that they wrote
as if that were the only kind of hook that existed in the world,
even though that is not true.
Let's see what others want to happen.
Thanks.There was a problem hiding this comment.
Adrian Ratiu wrote on the Git mailing list (how to reply to this email):
On October 20, 2026 6:46:43 PM UTC, Junio C Hamano <gitster@pobox.com> wrote:
>"Thomas Bachem via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>> From: Thomas Bachem <mail@thomasbachem.com>
>>
>> The documentation added by dcfb5af67e (hook: add hook.<event>.enabled
>> switch, 2026-04-10) says that with hook.<event>.enabled set to false
>> "no hooks fire for that event", but only hooks defined in the
>> configuration are skipped.
>
>It is good to see that the original author and those who helped
>were CC'ed.
>
>I have a suspicion that the current behavior may be very much
>deliberate, and the documentation was written too loosely. In
>other words, the author may have been so excited about their shiny
>new toy, the configuration-defined hook mechanism, that they wrote
>as if that were the only kind of hook that existed in the world,
>even though that is not true.
The suspicion is correct: I did not intend to disable the hookdir hooks
via configs when I wrote that initially.
My reasoning was something like "configs affect just the config-defined
hooks, the traditional hookdir hooks behave the same as before".
So currently the doc is indeed just too loose.
However, having said that, I don't have a strong opinion and am ok
either way, so if others think it's a good idea, we could accept this
patch instead of fixing the doc. :)There was a problem hiding this comment.
Junio C Hamano wrote on the Git mailing list (how to reply to this email):
Adrian Ratiu <adrian.ratiu@collabora.com> writes:
> The suspicion is correct: I did not intend to disable the hookdir hooks
> via configs when I wrote that initially.
>
> My reasoning was something like "configs affect just the config-defined
> hooks, the traditional hookdir hooks behave the same as before".
>
> So currently the doc is indeed just too loose.
Thanks for confirming my suspicion. After re-reading the document,
I am not sure if it was overly loose (see below).
> However, having said that, I don't have a strong opinion and am ok
> either way, so if others think it's a good idea, we could accept this
> patch instead of fixing the doc. :)
I am undecided if we should treat it as a backward incompatible
change or if we should just label this change as a "bugfix".
The additional documentation in question said
hook.<event>.enabled::
Switch to enable or disable all hooks for the `<event>` hook event.
When set to `false`, no hooks fire for that event, regardless of any
per-hook `hook.<friendly-name>.enabled` settings. Defaults to `true`.
See linkgit:git-hook[1].
which talks about "hook.<friendly-name>.enabled" setting, hinting
that this is also about the hooks that can be controlled by that
other setting, which was added by 1ecce722cd (hook: allow disabling
config hooks, 2026-02-19), which added "diable" specifically for
"config hooks", and the documentation from that addition says:
hook.<friendly-name>.enabled::
Whether the hook `hook.<name>` is enabled. Defaults to `true`.
Set to `false` to disable the hook without removing its
configuration. This is particularly useful when a hook is defined
in a system or global config file and needs to be disabled for a
specific repository. See linkgit:git-hook[1].
mentioning "removing its configuration", and "defined in ... config
file", to me, it is very clear that this configuration variable is
only about the hooks defined in the configuration system.
Which tempts me to say that the change being discussed is a backward
incompatible breakage, rather than a bugfix.
But casual end-users would of course do not know how the feature
evolved, and may not read the documentation of the feature they use
carefully enough. So I dunno. Perhaps changing the behaviour at
the same time updating the documentation to make it clear these
cover all kinds of hooks, not just the configuration-defined ones,
may be sufficient.
Thanks.
There was a problem hiding this comment.
Adrian Ratiu wrote on the Git mailing list (how to reply to this email):
On Wed, 21 Oct 2026, Junio C Hamano <gitster@pobox.com> wrote:
> Adrian Ratiu <adrian.ratiu@collabora.com> writes:
>> However, having said that, I don't have a strong opinion and am ok
>> either way, so if others think it's a good idea, we could accept this
>> patch instead of fixing the doc. :)
>
> I am undecided if we should treat it as a backward incompatible
> change or if we should just label this change as a "bugfix".
>
> The additional documentation in question said
>
> hook.<event>.enabled::
> Switch to enable or disable all hooks for the `<event>` hook event.
> When set to `false`, no hooks fire for that event, regardless of any
> per-hook `hook.<friendly-name>.enabled` settings. Defaults to `true`.
> See linkgit:git-hook[1].
>
> which talks about "hook.<friendly-name>.enabled" setting, hinting
> that this is also about the hooks that can be controlled by that
> other setting, which was added by 1ecce722cd (hook: allow disabling
> config hooks, 2026-02-19), which added "diable" specifically for
> "config hooks", and the documentation from that addition says:
>
> hook.<friendly-name>.enabled::
> Whether the hook `hook.<name>` is enabled. Defaults to `true`.
> Set to `false` to disable the hook without removing its
> configuration. This is particularly useful when a hook is defined
> in a system or global config file and needs to be disabled for a
> specific repository. See linkgit:git-hook[1].
>
> mentioning "removing its configuration", and "defined in ... config
> file", to me, it is very clear that this configuration variable is
> only about the hooks defined in the configuration system.
>
> Which tempts me to say that the change being discussed is a backward
> incompatible breakage, rather than a bugfix.
>
> But casual end-users would of course do not know how the feature
> evolved, and may not read the documentation of the feature they use
> carefully enough. So I dunno. Perhaps changing the behaviour at
> the same time updating the documentation to make it clear these
> cover all kinds of hooks, not just the configuration-defined ones,
> may be sufficient.
Agreed, let's keep it simple.
@Thomas, can you please send a v2 to update the documentation to make it
clear that disabling the hook event disables both the config-defined
hooks and the hookdir hooks for that event?
Just a short mention like you specified in hook.h, to avoid any
ambiguity.
Many thanks,
Adrian
hook.<event>.enabled, added by dcfb5af (hook: add hook.<event>.enabled switch, 2026-04-10), skips only the hooks defined in the configuration when set to false. With hook.pre-commit.enabled=false, "git commit" still runs .git/hooks/pre-commit, and "git hook list" shows that hook without the "event-disabled" mark. That was the intent, but the documentation says "no hooks fire for that event", which reads as covering the hookdir hook too. No other setting turns off the hookdir hook of a single event either: core.hooksPath=/dev/null turns off the hookdir hooks of all events. Make the switch cover the hookdir hook as well, and say so in the documentation. This changes behavior for anyone who set the switch and relied on the hookdir hook still running. The test for hook.<event>.enabled=false looks only at the standard output, but "git hook run" sends hook output to the standard error, so the test passes even when a disabled hook runs. Add a hookdir hook to it and check the standard error as well. Assisted-by: Claude Fable 5.1 Signed-off-by: Thomas Bachem <mail@thomasbachem.com>
hook.<name>.enabled and hook.<name>.parallel are booleans, and git-config(1) documents a boolean without a value as true. But hook_config_lookup_all() rejects every hook.<name>.<key> that has no value, so "git -c hook.lint.parallel hook run pre-commit" fails with "missing value for 'hook.lint.parallel'", and so does a bare "parallel" under [hook "lint"]. The same happens with .enabled, for a hook as well as for an event. Keep rejecting a missing value for the other keys. For .enabled and .parallel, pass it to git_parse_maybe_bool(), which takes it as true. Assisted-by: Claude Fable 5.1 Signed-off-by: Thomas Bachem <mail@thomasbachem.com>
thomasbachem
force-pushed
the
hook-event-enabled-hookdir
branch
from
October 23, 2026 10:54
3db167c to
53a5a17
Compare
thomasbachem
changed the title
Author
|
/preview |
|
Preview email sent as pull.2272.v2.git.1792752927.gitgitgadget@gmail.com |
Author
|
/submit |
|
Submitted as pull.2272.v2.git.1792761542.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
Junio C Hamano wrote on the Git mailing list (how to reply to this email): "Thomas Bachem via GitGitGadget" <gitgitgadget@gmail.com> writes:
> Two small changes to hook configuration. With the first,
> hook.pre-commit.enabled=false also stops the pre-commit hook in the hooks
> directory. With the second, hook.*.enabled and hook.*.parallel can be given
> without a value, which means true, as for any other boolean.
>
> Changes since v1:
>
> * The documentation of hook.<event>.enabled now says that it covers
> configured hooks and the hook in the hooks directory alike. The subject
> and message of patch 1 present this as a change in behavior rather than a
> fix (Adrian, Junio).
>
> * The test that hook.<event>.enabled=false leaves the hooks of other events
> alone now has a hookdir hook too.
>
> Thomas Bachem (2):
> hook: make hook.<event>.enabled cover the hookdir hook
> hook: accept .enabled and .parallel given without a value
Thanks, the range-diff looks good. Will queue.
|
|
This patch series was integrated into seen via git@96e13f0. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two small changes to hook configuration. With the first, hook.pre-commit.enabled=false also stops the pre-commit hook in the hooks directory. With the second, hook.*.enabled and hook.*.parallel can be given without a value, which means true, as for any other boolean.
Changes since v1:
The documentation of hook.<event>.enabled now says that it covers configured hooks and the hook in the hooks directory alike. The subject and message of patch 1 present this as a change in behavior rather than a fix (Adrian, Junio).
The test that hook.<event>.enabled=false leaves the hooks of other events alone now has a hookdir hook too.
Cc: Adrian Ratiu adrian.ratiu@collabora.com
Cc: Patrick Steinhardt ps@pks.im
Cc: Emily Shaffer nasamuffin@google.com
Cc: Junio C Hamano gitster@pobox.com