Re: [PATCH] git: add --no-hooks global option
- From
- Phillip Wood <phillip.wood123@gmail.com>
- Date
- Apr 4, 2025, 14:15 UTC
- Message-ID
- <2c7994bc-2be0-43a5-9627-0d530746b3ab@gmail.com>
- In-Reply-To
- <pull.1899.git.1743719888430.gitgitgadget@gmail.com>
Hi Stolee
On 03/04/2025 23:38, Derrick Stolee via GitGitGadget wrote:
Show 7 quoted lines
> From: Derrick Stolee <stolee@gmail.com> > > In some situations, these hooks have poor performance and expert users > may want to skip the hooks as they don't seem to affect the current > situation. One example is a pre-commit hook that checks for certain > structures in the local changes, but expert users are likely to have > done the right thing in advance.
Next they'll be saying that they never make a mistake when writing a one line patch! More seriously I agree there are times when one may want to bypass the pre-commit hook but we already have "git commit --no-verify" to do that. In general hooks that are so slow that the user wants to bypass them are self-defeating and I'd argue that the solution is to fix the performance of the hook rather than make it easier to skip it. One solution for speeding up pre-commit hooks is to process files in parallel. Unfortunately git does not provide support for that but there are hook frameworks that do.
> I have come across users who have disabled hooks themselves either by > deleting hooks (supported, safe) or setting 'core.hooksPath' to some > bogus path (seems unsafe).
I thought "git -c core.hooksPath=/dev/null" was a fairly standard way of disabling hooks on a one-off basis - what makes it unsafe?
Show 13 quoted lines
> The supported process is painful to swap > between the hook-enabled scenario and the hook-disabled scenario. > > To that end, add a new --no-hooks global option to allow users to > disable hooks quickly. This option is modeled similarly to the > --no-advice option in b79deeb554 (advice: add --no-advice global option, > 2024-05-03). This uses a GIT_HOOKS environment variable to communicate > to subprocesses as well as making this a backwards-compatible way for > tools to signal that they want to disable hooks. > > The critical piece is that all hooks pass through run_hooks_opt() where > a static int will evaluate the environment variable and store that the > variable is initialized for faster repeated runs.
That certainly makes the implementation much more viable. However I'm not really convinced this is a good idea.
Best Wishes
Phillip
Show 164 quoted lines
> Signed-off-by: Derrick Stolee <stolee@gmail.com>
> ---
> git: add --no-hooks global option
>
> This is hopefully a helpful feature to more than just the experts I've
> been hearing from.
>
> Thanks,
>
> * Stolee
>
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1899%2Fderrickstolee%2Fno-hooks-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1899/derrickstolee/no-hooks-v1
> Pull-Request: https://github.com/gitgitgadget/git/pull/1899
>
> Documentation/git.adoc | 13 ++++++++++++-
> environment.h | 6 ++++++
> git.c | 6 +++++-
> hook.c | 7 +++++++
> t/t1350-config-hooks-path.sh | 34 ++++++++++++++++++++++++++++++++++
> 5 files changed, 64 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/git.adoc b/Documentation/git.adoc
> index 743b7b00e4d..a34c8cfbe78 100644
> --- a/Documentation/git.adoc
> +++ b/Documentation/git.adoc
> @@ -14,7 +14,7 @@ SYNOPSIS
> [-p | --paginate | -P | --no-pager] [--no-replace-objects] [--no-lazy-fetch]
> [--no-optional-locks] [--no-advice] [--bare] [--git-dir=<path>]
> [--work-tree=<path>] [--namespace=<name>] [--config-env=<name>=<envvar>]
> - <command> [<args>]
> + [--no-hooks] <command> [<args>]
>
> DESCRIPTION
> -----------
> @@ -230,6 +230,12 @@ If you just want to run git as if it was started in `<path>` then use
> linkgit:gitattributes[5]. This is equivalent to setting the
> `GIT_ATTR_SOURCE` environment variable.
>
> +--no-hooks::
> + Skip running local Git hooks, even if configured locally. Hooks
> + are an opt-in feature, so be sure that you know the impact of
> + ignoring hooks when running with this option. This is equivalent
> + to setting `GIT_HOOKS=0` environment variable.
> +
> GIT COMMANDS
> ------------
>
> @@ -771,6 +777,11 @@ for further details.
> not set, Git will choose buffered or record-oriented flushing
> based on whether stdout appears to be redirected to a file or not.
>
> +`GIT_HOOKS`::
> + If this Boolean environment variable is set to false, then commands
> + will ignore any configured hooks as if the `--no-hooks` option was
> + provided.
> +
> `GIT_TRACE`::
> Enables general trace messages, e.g. alias expansion, built-in
> command execution and external command execution.
> diff --git a/environment.h b/environment.h
> index 45e690f203f..22ddf201144 100644
> --- a/environment.h
> +++ b/environment.h
> @@ -50,6 +50,12 @@
> */
> #define GIT_ADVICE_ENVIRONMENT "GIT_ADVICE"
>
> +/*
> + * Environment variable used to propagate the --no-hooks global option to
> + * the hooks layer and to any child processes.
> + */
> +#define GIT_HOOKS "GIT_HOOKS"
> +
> /*
> * Environment variable used in handshaking the wire protocol.
> * Contains a colon ':' separated list of keys with optional values
> diff --git a/git.c b/git.c
> index 77c43595223..d7ebcf60947 100644
> --- a/git.c
> +++ b/git.c
> @@ -41,7 +41,7 @@ const char git_usage_string[] =
> " [-p | --paginate | -P | --no-pager] [--no-replace-objects] [--no-lazy-fetch]\n"
> " [--no-optional-locks] [--no-advice] [--bare] [--git-dir=<path>]\n"
> " [--work-tree=<path>] [--namespace=<name>] [--config-env=<name>=<envvar>]\n"
> - " <command> [<args>]");
> + " [--no-hooks] <command> [<args>]");
>
> const char git_more_info_string[] =
> N_("'git help -a' and 'git help -g' list available subcommands and some\n"
> @@ -349,6 +349,10 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)
> setenv(GIT_ADVICE_ENVIRONMENT, "0", 1);
> if (envchanged)
> *envchanged = 1;
> + } else if (!strcmp(cmd, "--no-hooks")) {
> + setenv(GIT_HOOKS, "0", 1);
> + if (envchanged)
> + *envchanged = 1;
> } else {
> fprintf(stderr, _("unknown option: %s\n"), cmd);
> usage(git_usage_string);
> diff --git a/hook.c b/hook.c
> index b3de1048bf4..b209553d7a8 100644
> --- a/hook.c
> +++ b/hook.c
> @@ -144,6 +144,13 @@ int run_hooks_opt(struct repository *r, const char *hook_name,
>
> .data = &cb_data,
> };
> + static int do_run_hooks = -1;
> +
> + if (do_run_hooks < 0)
> + do_run_hooks = git_env_bool(GIT_HOOKS, 1);
> +
> + if (!do_run_hooks)
> + goto cleanup;
>
> if (!options)
> BUG("a struct run_hooks_opt must be provided to run_hooks");
> diff --git a/t/t1350-config-hooks-path.sh b/t/t1350-config-hooks-path.sh
> index 45a04929170..4c6a0eafe4e 100755
> --- a/t/t1350-config-hooks-path.sh
> +++ b/t/t1350-config-hooks-path.sh
> @@ -48,4 +48,38 @@ test_expect_success 'core.hooksPath=/dev/null' '
> { test /dev/null = "$value" || test nul = "$value"; }
> '
>
> +test_expect_success '--no-hooks' '
> + rm -f actual &&
> + test_might_fail git config --unset core.hooksPath &&
> +
> + write_script .git/hooks/pre-commit <<-\EOF &&
> + echo HOOK >>actual
> + EOF
> +
> + echo HOOK >expect &&
> +
> + git commit --allow-empty -m "A" &&
> + test_cmp expect actual &&
> +
> + git --no-hooks commit --allow-empty -m "B" &&
> + test_cmp expect actual
> +'
> +
> +test_expect_success 'GIT_HOOKS' '
> + rm -f actual &&
> + test_might_fail git config --unset core.hooksPath &&
> +
> + write_script .git/hooks/pre-commit <<-\EOF &&
> + echo HOOK >>actual
> + EOF
> +
> + echo HOOK >expect &&
> +
> + GIT_HOOKS=1 git commit --allow-empty -m "A" &&
> + test_cmp expect actual &&
> +
> + GIT_HOOKS=0 git commit --allow-empty -m "B" &&
> + test_cmp expect actual
> +'
> +
> test_done
>
> base-commit: 5b97a56fa0e7d580dc8865b73107407c9b3f0eff