[PATCH v2] Run global hooks from the directory at hooks.dir

[PATCH v2] Run global hooks from the directory at hooks.dir

[Brian Collins]

Run global hooks in the directory specified by the config variable
hooks.dir before every attempt at running a local hook. If the
global hook fails, the local hook will not run. If the global hook is
absent, the local hook runs normally. This is useful because it means
you can have scripts that run as hooks for multiple repositories, for
example coding style enforcement for an entire organization, or
system-wide commit analytics.

Signed-off-by: Brian Collins <bricollins@gmail.com>
---

The possibility of adding this feature was previously discussed here:
http://marc.info/?l=git&m=127808782807807&w=2

Fixed some bad whitespace in contrib/completion/git-completion.bash

Cheers,
Brian

 Documentation/config.txt               |    5 ++
 cache.h                                |    2 +
 config.c                               |   12 ++++
 contrib/completion/git-completion.bash |    1 +
 run-command.c                          |   51 +++++++++++++-----
 t/t7510-global-hooks.sh                |   88 ++++++++++++++++++++++++++++++++
 6 files changed, 144 insertions(+), 15 deletions(-)
 create mode 100755 t/t7510-global-hooks.sh

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 538ebb5..3f18208 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1199,6 +1199,11 @@ help.autocorrect::
 	value is 0 - the command will be just shown but not executed.
 	This is the default.
 
+hooks.dir::
+	Directory containing executable hooks. Usage is identical to
+	.git/hooks except they are triggered for any repository used
+	while this configuration is set.
+
 http.proxy::
 	Override the HTTP proxy, normally configured using the 'http_proxy'
 	environment variable (see linkgit:curl[1]).  This can be overridden
diff --git a/cache.h b/cache.h
index 33decd9..c75a444 100644
--- a/cache.h
+++ b/cache.h
@@ -1018,6 +1018,8 @@ extern const char *git_commit_encoding;
 extern const char *git_log_output_encoding;
 extern const char *git_mailmap_file;
 
+extern const char *git_hooks_dir;
+
 /* IO helper functions */
 extern void maybe_flush_or_die(FILE *, const char *);
 extern int copy_fd(int ifd, int ofd);
diff --git a/config.c b/config.c
index 4b0a820..1a9d5ee 100644
--- a/config.c
+++ b/config.c
@@ -745,6 +745,15 @@ static int git_default_mailmap_config(const char *var, const char *value)
 	return 0;
 }
 
+static int git_default_hooks_config(const char *var, const char *value)
+{
+	if (!strcmp(var, "hooks.dir"))
+		return git_config_pathname(&git_hooks_dir, var, value);
+
+	/* Add other config variables here and to Documentation/config.txt. */
+	return 0;
+}
+
 int git_default_config(const char *var, const char *value, void *dummy)
 {
 	if (!prefixcmp(var, "core."))
@@ -768,6 +777,9 @@ int git_default_config(const char *var, const char *value, void *dummy)
 	if (!prefixcmp(var, "advice."))
 		return git_default_advice_config(var, value);
 
+	if (!prefixcmp(var, "hooks."))
+		return git_default_hooks_config(var, value);
+
 	if (!strcmp(var, "pager.color") || !strcmp(var, "color.pager")) {
 		pager_use_color = git_config_bool(var,value);
 		return 0;
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 168669b..b266f83 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1913,6 +1913,7 @@ _git_config ()
 		help.autocorrect
 		help.browser
 		help.format
+		hooks.dir
 		http.lowSpeedLimit
 		http.lowSpeedTime
 		http.maxRequests
diff --git a/run-command.c b/run-command.c
index 2a1041e..8e33b80 100644
--- a/run-command.c
+++ b/run-command.c
@@ -2,6 +2,8 @@
 #include "run-command.h"
 #include "exec_cmd.h"
 
+const char *git_hooks_dir;
+
 static inline void close_pair(int fd[2])
 {
 	close(fd[0]);
@@ -603,26 +605,16 @@ int finish_async(struct async *async)
 #endif
 }
 
-int run_hook(const char *index_file, const char *name, ...)
+int run_hook_file(const char *path, const char *index_file, const char **argv)
 {
 	struct child_process hook;
-	const char **argv = NULL, *env[2];
 	char index[PATH_MAX];
-	va_list args;
-	int ret;
-	size_t i = 0, alloc = 0;
+	const char *env[2];
 
-	if (access(git_path("hooks/%s", name), X_OK) < 0)
+	if (access(path, X_OK) < 0)
 		return 0;
 
-	va_start(args, name);
-	ALLOC_GROW(argv, i + 1, alloc);
-	argv[i++] = git_path("hooks/%s", name);
-	while (argv[i-1]) {
-		ALLOC_GROW(argv, i + 1, alloc);
-		argv[i++] = va_arg(args, const char *);
-	}
-	va_end(args);
+	argv[0] = path;
 
 	memset(&hook, 0, sizeof(hook));
 	hook.argv = argv;
@@ -635,7 +627,36 @@ int run_hook(const char *index_file, const char *name, ...)
 		hook.env = env;
 	}
 
-	ret = run_command(&hook);
+	return run_command(&hook);
+}
+
+int run_hook(const char *index_file, const char *name, ...)
+{
+	const char **argv = NULL;
+	va_list args;
+	int ret;
+	size_t i = 1, alloc = 0;
+	const char *local_path = git_path("hooks/%s", name);
+	const char *global_path = NULL;
+	if (git_hooks_dir != NULL)
+		global_path = mkpath("%s/%s", git_hooks_dir, name);
+
+	if ((global_path == NULL || access(global_path, X_OK) > 0) &&
+			access(local_path, X_OK) > 0)
+		return 0;
+
+	va_start(args, name);
+	do {
+		ALLOC_GROW(argv, i + 1, alloc);
+		argv[i++] = va_arg(args, const char *);
+	} while (argv[i-1]);
+	va_end(args);
+
+	if (global_path != NULL)
+		ret = run_hook_file(global_path, index_file, argv) ||
+			run_hook_file(local_path, index_file, argv);
+	else
+		ret = run_hook_file(local_path, index_file, argv);
 	free(argv);
 	return ret;
 }
diff --git a/t/t7510-global-hooks.sh b/t/t7510-global-hooks.sh
new file mode 100755
index 0000000..d2481ea
--- /dev/null
+++ b/t/t7510-global-hooks.sh
@@ -0,0 +1,88 @@
+#!/bin/sh
+
+test_description='global hooks'
+
+. ./test-lib.sh
+
+test_expect_success 'with no hooks' '
+	echo "foo" > file &&
+	git add file &&
+	git commit -m "first"
+'
+
+GIT_DIR="$(git rev-parse --git-dir)"
+
+# now install global hook that always succeeds
+GLOBALHOOKDIR="global-hooks"
+GLOBALHOOK="$GLOBALHOOKDIR/pre-commit"
+mkdir -p "$GLOBALHOOKDIR"
+cat > $GLOBALHOOK <<EOF
+#!/bin/sh
+touch $GIT_DIR/global-hook-fired
+exit 0
+EOF
+
+chmod +x "$GLOBALHOOK"
+
+# and a local hook that always succeeds
+HOOKDIR="$GIT_DIR/hooks"
+HOOK="$HOOKDIR/pre-commit"
+mkdir -p "$HOOKDIR"
+cat > $HOOK <<EOF
+#!/bin/sh
+touch $GIT_DIR/local-hook-fired
+exit 0
+EOF
+
+chmod +x "$HOOK"
+
+
+test_expect_success 'configure global hooks directory' '
+	git config hooks.dir ${GLOBALHOOKDIR}
+'
+
+test_expect_success 'with succeeding hook' '
+	echo "more" >> file &&
+	git add file &&
+	git commit -m "more" &&
+	test -e ${GIT_DIR}/global-hook-fired &&
+	test -e ${GIT_DIR}/local-hook-fired
+'
+
+rm $GIT_DIR/global-hook-fired
+rm $GIT_DIR/local-hook-fired
+
+# now a local hook that fails, both hooks should fire
+cat > $HOOK <<EOF
+#!/bin/sh
+touch $GIT_DIR/local-hook-fired
+exit 1
+EOF
+
+test_expect_success 'with failing global hook' '
+	echo "another" >> file &&
+	git add file &&
+	test_must_fail git commit -m "another" &&
+	test -e ${GIT_DIR}/global-hook-fired &&
+	test -e ${GIT_DIR}/local-hook-fired
+'
+
+rm $GIT_DIR/global-hook-fired
+rm $GIT_DIR/local-hook-fired
+
+# now a global hook that fails, the local hook shouldn't fire
+cat > "$GLOBALHOOK" <<EOF
+#!/bin/sh
+touch $GIT_DIR/global-hook-fired
+exit 1
+EOF
+
+test_expect_success 'with failing global hook' '
+	echo "another" >> file &&
+	git add file &&
+	test_must_fail git commit -m "another" &&
+	test -e ${GIT_DIR}/global-hook-fired &&
+	test_must_fail test -e ${GIT_DIR}/local-hook-fired
+'
+
+test_done
-- 
1.7.3.2.162.g4671

Re: [PATCH v2] Run global hooks from the directory at hooks.dir

[Johannes Sixt]

Am 11/8/2010 13:32, schrieb Brian Collins:
> Run global hooks in the directory specified by the config variable
> hooks.dir before every attempt at running a local hook. If the
> global hook fails, the local hook will not run. If the global hook is
> absent, the local hook runs normally. This is useful because it means
> you can have scripts that run as hooks for multiple repositories, for
> example coding style enforcement for an entire organization, or
> system-wide commit analytics.
> 
> Signed-off-by: Brian Collins <bricollins@gmail.com>
> ---
> 
> The possibility of adding this feature was previously discussed here:
> http://marc.info/?l=git&m=127808782807807&w=2

I'm not in favor of this change, as a number of alternatives were
suggested in the thread you cite (Gmane:
http://thread.gmane.org/gmane.comp.version-control.git/150141).

A few hints, just in case you deploy this code:

> +	if ((global_path == NULL || access(global_path, X_OK) > 0) &&
> +			access(local_path, X_OK) > 0)
> +		return 0;

access(2) returns zero on success, negative on failure.

> +	if (global_path != NULL)
> +		ret = run_hook_file(global_path, index_file, argv) ||
> +			run_hook_file(local_path, index_file, argv);

What is the value of ret if the hook in global_path fails? Is it the exit
code, or is it 1? What should it be?

-- Hannes

Re: [PATCH v2] Run global hooks from the directory at hooks.dir

[Brian Collins]

> I'm not in favor of this change, as a number of alternatives were
> suggested in the thread you cite (Gmane:
> http://thread.gmane.org/gmane.comp.version-control.git/150141).

The alternatives suggested need to be applied individually to each
repository. This can be problematic because you may want repository-specific
scripts as well. Then when you update your global hooks you have to deal
with merging with your repository-specific scripts. Using templates is not
ideal because it is system-wide and they still need to be renamed for every
clone or init.

> A few hints, just in case you deploy this code:

Thanks, I have fixed those issues.

Re: [PATCH v2] Run global hooks from the directory at hooks.dir

[Jonathan Nieder]

Brian Collins wrote:

>                                                     Using templates is not
> ideal because it is system-wide and they still need to be renamed for every
> clone or init.

Why do they have to be renamed?  And can we do better?

Re: [PATCH v2] Run global hooks from the directory at hooks.dir

[Junio C Hamano]

Brian Collins <bricollins@gmail.com> writes:

> Run global hooks in the directory specified by the config variable
> hooks.dir before every attempt at running a local hook. If the
> global hook fails, the local hook will not run. If the global hook is
> absent, the local hook runs normally.

It is left unspecified what happens when the global hook exists and it
succeeds.  Watch out for hooks that read from their standard input.

In any case, the above is totally backwards from the usual practice and
expectation of local things overriding the global default.  If you want a
site-wide policy suggestion, default templates would be a more acceptable
way (and the implementation of hooks you install to developer repositories
can choose to look at $GIT_DIR/hooks/local-foo-hook).

By the way, with a distributed scm, anything-wide policy enforcement at
the level of developer's individual working repositories is a lost cause.
You are giving freedom to do anything on their own copy of the history to
the developers; the project-wide policy is to be enforced at the perimeter
of your authoritative repository of the project.

Re: [PATCH v2] Run global hooks from the directory at hooks.dir

[Brian Collins]

> It is left unspecified what happens when the global hook exists and it
> succeeds.  Watch out for hooks that read from their standard input.

The local hook will execute. They are chained.

> In any case, the above is totally backwards from the usual practice and
> expectation of local things overriding the global default.  If you want a
> site-wide policy suggestion, default templates would be a more acceptable
> way (and the implementation of hooks you install to developer repositories
> can choose to look at $GIT_DIR/hooks/local-foo-hook).

The global hook doesn't override the local hook, it supplements it. In any
case, a developer can set hooks.dir to be something else in a specific
repository, and it will override the global default.

> By the way, with a distributed scm, anything-wide policy enforcement at
> the level of developer's individual working repositories is a lost cause.
> You are giving freedom to do anything on their own copy of the history to
> the developers; the project-wide policy is to be enforced at the perimeter
> of your authoritative repository of the project.

The developer is in total control, it is a user configuration option.
It is the same
as setting colours for diffs. Just a preference that makes the developer's life
easier by adding some way of automating certain tasks. Say you want to
post to IRC every time you commit, this could do that. Or if you want to set up
a shell script to jump to your most recently used repository. There are lots of
possible applications. I admit "site-wide coding style enforcement" was a bad
example, and I certainly don't plan to use it for that.