git: add --no-hooks global option

From: Derrick Stolee <[email protected]>

Git has several hooks which are executed during certain events as long as those hooks are enabled in the hooks directory (possibly moved from .git/hooks via the core.hooksPath config option). These are configured by the user, or perhaps by tooling the user has agreed to, and are not required to operate a Git repository.

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.

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

Signed-off-by Derrick Stolee <[email protected]>

---
git: add --no-hooks global option

This is hopefully a helpful feature to more than just the experts I've been hearing from.

Quoted 9 lines · Derrick Stolee via GitGitGadget

From: Derrick Stolee <[email protected]>

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.

Quoted 6 lines

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.

This is hopefully a helpful feature to more than just the experts I've been hearing from.

I think this is functionality that Jenkins wants because they've configured core.hooksPath to /dev/null, allegedly for security reasons. Of course, enabling this feature will also break Git LFS, but in a less noticeable and detectable way (currently, Git LFS will fail on attempting to install hooks with that setting of core.hooksPath, which is at least noticeable).

I do think that in general certain types of hooks, such as pre-commit hooks, should absolutely be optional. There are lots of reasons to commit WIP data that doesn't meet whatever standard and we shouldn't impede expert users from expert workflows, even if there are many fewer reasons to do things like bypass pushing Git LFS objects (which are important for integrity).

So I can see the utility of this feature but I can also see how it can break lots of things when handled poorly. Of course, we also have git reset --hard and there's lots of hand-wringing on Stack Overflow about having deleted important data, so we have some precedent for expert features that could break things badly.

I don't otherwise have a strong opinion either way, although I'd lean slightly in favour of this series. I'd of course welcome other people's thoughts here.

From: Derrick Stolee <[email protected]>

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.

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

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.

Signed-off-by Derrick Stolee <[email protected]>

---
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(-)

Documentation/git.adoc +12 -1

index 743b7b00e4d..a34c8cfbe78 100644
@@ -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.

environment.h +6 -0

index 45e690f203f..22ddf201144 100644
@@ -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

git.c +5 -1

index 77c43595223..d7ebcf60947 100644
@@ -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);

hook.c +7 -0

index b3de1048bf4..b209553d7a8 100644
@@ -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");

t/t1350-config-hooks-path.sh +34 -0

index 45a04929170..4c6a0eafe4e 100755
@@ -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

From: Derrick Stolee <[email protected]>

Git has several hooks which are executed during certain events as long as those hooks are enabled in the hooks directory (possibly moved from .git/hooks via the core.hooksPath config option). These are configured by the user, or perhaps by tooling the user has agreed to, and are not required to operate a Git repository.

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.

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

Signed-off-by Derrick Stolee <[email protected]>

---
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(-)

Documentation/git.adoc +12 -1

index 743b7b00e4d..a34c8cfbe78 100644
@@ -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.

environment.h +6 -0

index 45e690f203f..22ddf201144 100644
@@ -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

git.c +5 -1

index 77c43595223..d7ebcf60947 100644
@@ -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);

hook.c +7 -0

index b3de1048bf4..b209553d7a8 100644
@@ -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");

t/t1350-config-hooks-path.sh +34 -0

index 45a04929170..4c6a0eafe4e 100755
@@ -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 --
gitgitgadget

Hi Stolee

Quoted 7 lines · Derrick Stolee via GitGitGadget

From: Derrick Stolee <[email protected]>

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.

Quoted 3 lines

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?

Quoted 10 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.

Quoted 3 lines

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.

I thought "git -c core.hooksPath=/dev/null" was a fairly standard way of disabling hooks

Hi!

Quoted 1 line

I thought "git -c core.hooksPath=/dev/null" was a fairly standard way of disabling hooks

Given that, wouldn't it be a case to turn this into a documentation patch?

I just searched here I found that we even have a test for it (introduced in c8f6478), but I couldn't find that as a recommendation in our docs.

Quoted 14 lines · Phillip Wood

Quoted 12 lines · Derrick Stolee via GitGitGadget

From: Derrick Stolee <[email protected]>

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.

Quoted 2 lines

That certainly makes the implementation much more viable. However I'm not really convinced this is a good idea.

I don't read a strong reason in your message that this is a _bad_ idea either. As in, there's nothing that hints that this will cause significant harm to users other than providing a new footgun (and we have plenty of those for folks willing to look, including the _existence_ of hooks).

Quoted 5 lines

I don't read a strong reason in your message that this is a _bad_ idea either. As in, there's nothing that hints that this will cause significant harm to users other than providing a new footgun (and we have plenty of those for folks willing to look, including the _existence_ of hooks).

It is certainly not a terrible idea given that it is possible to disable hooks already but I'm not clear what the motivation is. I don't find the example of a skipping a pre-commit hook persuasive as we already provide a convenient way for users to skip that hook. Elsewhere in this thread you mention the "pre-command" and "post-command" hooks but they are not part of git - if a fork is running its own hooks and that is causing problems for users I'm not sure we want to change the upstream project to address that. If there was a clearer motivation it would be easier to understand the benefits of this change.

Quoted 6 lines · Phillip Wood

Quoted 5 lines

I don't read a strong reason in your message that this is a _bad_ idea either. As in, there's nothing that hints that this will cause significant harm to users other than providing a new footgun (and we have plenty of those for folks willing to look, including the _existence_ of hooks).

Quoted 10 lines

It is certainly not a terrible idea given that it is possible to disable hooks already but I'm not clear what the motivation is. I don't find the example of a skipping a pre-commit hook persuasive as we already provide a convenient way for users to skip that hook. Elsewhere in this thread you mention the "pre-command" and "post-command" hooks but they are not part of git - if a fork is running its own hooks and that is causing problems for users I'm not sure we want to change the upstream project to address that. If there was a clearer motivation it would be easier to understand the benefits of this change.

Thanks for pushing back. The default for any new changes is not to apply unless there is a compelling reason why it is a good idea, saying that this is not a bad thing does not serve as an effective justification.

If we want to give scripters a more stable foundation to build on, the answer should not be to pile more and more "no hooks, no configurations, just a vanilla mode of operation" options to end-user facing porcelain commands, but to clean up the internal implementation of such porcelain commands to refactor into stable plumbing commands that scripters can rely on.

Hi!

Review

Quoted 2 lines

(quoted diff) +2 -0

+You can also disable all hooks entirely by setting `core.hooksPath`
+to `/dev/null`.

Personally I think it would be better to focus on the non-expert user,

Quoted 6 lines · Lucas Seiki Oshiro

Hi!

Review

Quoted 2 lines

(quoted diff) +2 -0

+You can also disable all hooks entirely by setting `core.hooksPath`
+to `/dev/null`.

Personally I think it would be better to focus on the non-expert user,

I absolutely want this to be targeted for expert users, so users self- select themselves into the risk of what happens when disabling hooks. This is a "there be dragons here" kind of warning, implying that you better know what you're doing if you are messing with hook paths.

I thought the text in the patch looked good to me. I appreciate you graciously pivoting approaches and documenting this, both for the benefit of users and as an approach to help make sure we don't break this functionality.

This is a "there be dragons here" kind of warning, implying that you better know what you're doing if you are messing with hook paths.