[PATCH 0/3] Add a generated list of hooks in hook-list.h

[PATCH 0/3] Add a generated list of hooks in hook-list.h

[Ævar Arnfjörð Bjarmason]

This goes on top of my just-submitted trivial Makefile fixes[1], and
adds a list of hooks in hook-list.h, similar to the existing
config-list.h.

We can then error when a C API provides us with an unknown hook, so
non-type-checked things in the codebase like find_hook("proc-receive")
now effectively have a type check of sorts (well, we'd catch a typo in
our tests).

These changes are currently at the end of the
ab/config-based-hooks-base series[2], I'm carving them out to make
that topic even easier to digest. We can this without any of the "git
hook run" stuff.

We also had a big but inaccurate list of hooks in builtin/help.c, as
part of submitting a bugreport. That's now using githooks(5) as a
source of truth.

This also builds on Windows with cmake, unlike the outstanding [2]
`restart of "config-based-hooks"` series, at least that part of the CI
now passes, and the change to contrib/buildsystems/CMakeLists.txt
looks trivially correct to me.

1. https://lore.kernel.org/git/cover-0.3-0000000000-20210617T095827Z-avarab@gmail.com/
2. https://lore.kernel.org/git/cover-00.30-00000000000-20210614T101920Z-avarab@gmail.com/

Emily Shaffer (1):
  hook.c: add a hook_exists() wrapper and use it in bugreport.c

Ævar Arnfjörð Bjarmason (2):
  hook.[ch]: move find_hook() to this new library
  hook-list.h: add a generated list of hooks, like config-list.h

 .gitignore                          |  1 +
 Makefile                            | 12 +++++-
 builtin/am.c                        |  1 +
 builtin/bugreport.c                 | 46 +++++-----------------
 builtin/commit.c                    |  1 +
 builtin/merge.c                     |  1 +
 builtin/receive-pack.c              |  1 +
 builtin/worktree.c                  |  1 +
 contrib/buildsystems/CMakeLists.txt |  7 ++++
 generate-hooklist.sh                | 24 ++++++++++++
 hook.c                              | 61 +++++++++++++++++++++++++++++
 hook.h                              | 16 ++++++++
 refs.c                              |  1 +
 run-command.c                       | 35 +----------------
 run-command.h                       |  7 ----
 sequencer.c                         |  1 +
 transport.c                         |  1 +
 17 files changed, 138 insertions(+), 79 deletions(-)
 create mode 100755 generate-hooklist.sh
 create mode 100644 hook.c
 create mode 100644 hook.h

-- 
2.32.0.576.g59759b6ca7d

[PATCH 1/3] hook.[ch]: move find_hook() to this new library

[Ævar Arnfjörð Bjarmason]

Move the find_hook() function from run-command.c to a new hook.c
library. This change establishes a stub library that's pretty
pointless right now, but will see much wider use with Emily Shaffer's
upcoming "configuration-based hooks" series.

Eventually all the hook related code will live in hook.[ch]. Let's
start that process by moving the simple find_hook() function over
as-is.

Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Makefile               |  1 +
 builtin/am.c           |  1 +
 builtin/bugreport.c    |  2 +-
 builtin/commit.c       |  1 +
 builtin/merge.c        |  1 +
 builtin/receive-pack.c |  1 +
 builtin/worktree.c     |  1 +
 hook.c                 | 37 +++++++++++++++++++++++++++++++++++++
 hook.h                 | 11 +++++++++++
 refs.c                 |  1 +
 run-command.c          | 35 +----------------------------------
 run-command.h          |  7 -------
 sequencer.c            |  1 +
 transport.c            |  1 +
 14 files changed, 59 insertions(+), 42 deletions(-)
 create mode 100644 hook.c
 create mode 100644 hook.h

diff --git a/Makefile b/Makefile
index 29a152cd4f..d155b7be21 100644
--- a/Makefile
+++ b/Makefile
@@ -903,6 +903,7 @@ LIB_OBJS += hash-lookup.o
 LIB_OBJS += hashmap.o
 LIB_OBJS += help.o
 LIB_OBJS += hex.o
+LIB_OBJS += hook.o
 LIB_OBJS += ident.o
 LIB_OBJS += json-writer.o
 LIB_OBJS += kwset.o
diff --git a/builtin/am.c b/builtin/am.c
index 0b2d886c81..1c8a548903 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -11,6 +11,7 @@
 #include "parse-options.h"
 #include "dir.h"
 #include "run-command.h"
+#include "hook.h"
 #include "quote.h"
 #include "tempfile.h"
 #include "lockfile.h"
diff --git a/builtin/bugreport.c b/builtin/bugreport.c
index 9915a5841d..596f079a7f 100644
--- a/builtin/bugreport.c
+++ b/builtin/bugreport.c
@@ -3,7 +3,7 @@
 #include "strbuf.h"
 #include "help.h"
 #include "compat/compiler.h"
-#include "run-command.h"
+#include "hook.h"
 
 
 static void get_system_info(struct strbuf *sys_info)
diff --git a/builtin/commit.c b/builtin/commit.c
index 190d215d43..f1aafd67d4 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -19,6 +19,7 @@
 #include "revision.h"
 #include "wt-status.h"
 #include "run-command.h"
+#include "hook.h"
 #include "refs.h"
 #include "log-tree.h"
 #include "strbuf.h"
diff --git a/builtin/merge.c b/builtin/merge.c
index a8a843b1f5..be98d66b0a 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -13,6 +13,7 @@
 #include "builtin.h"
 #include "lockfile.h"
 #include "run-command.h"
+#include "hook.h"
 #include "diff.h"
 #include "diff-merges.h"
 #include "refs.h"
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index a34742513a..1e0e04c62f 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -7,6 +7,7 @@
 #include "pkt-line.h"
 #include "sideband.h"
 #include "run-command.h"
+#include "hook.h"
 #include "exec-cmd.h"
 #include "commit.h"
 #include "object.h"
diff --git a/builtin/worktree.c b/builtin/worktree.c
index 976bf8ed06..b1350640fe 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -8,6 +8,7 @@
 #include "branch.h"
 #include "refs.h"
 #include "run-command.h"
+#include "hook.h"
 #include "sigchain.h"
 #include "submodule.h"
 #include "utf8.h"
diff --git a/hook.c b/hook.c
new file mode 100644
index 0000000000..c4dbef1d0e
--- /dev/null
+++ b/hook.c
@@ -0,0 +1,37 @@
+#include "cache.h"
+#include "hook.h"
+#include "run-command.h"
+
+const char *find_hook(const char *name)
+{
+	static struct strbuf path = STRBUF_INIT;
+
+	strbuf_reset(&path);
+	strbuf_git_path(&path, "hooks/%s", name);
+	if (access(path.buf, X_OK) < 0) {
+		int err = errno;
+
+#ifdef STRIP_EXTENSION
+		strbuf_addstr(&path, STRIP_EXTENSION);
+		if (access(path.buf, X_OK) >= 0)
+			return path.buf;
+		if (errno == EACCES)
+			err = errno;
+#endif
+
+		if (err == EACCES && advice_ignored_hook) {
+			static struct string_list advise_given = STRING_LIST_INIT_DUP;
+
+			if (!string_list_lookup(&advise_given, name)) {
+				string_list_insert(&advise_given, name);
+				advise(_("The '%s' hook was ignored because "
+					 "it's not set as executable.\n"
+					 "You can disable this warning with "
+					 "`git config advice.ignoredHook false`."),
+				       path.buf);
+			}
+		}
+		return NULL;
+	}
+	return path.buf;
+}
diff --git a/hook.h b/hook.h
new file mode 100644
index 0000000000..68624f1605
--- /dev/null
+++ b/hook.h
@@ -0,0 +1,11 @@
+#ifndef HOOK_H
+#define HOOK_H
+
+/*
+ * Returns the path to the hook file, or NULL if the hook is missing
+ * or disabled. Note that this points to static storage that will be
+ * overwritten by further calls to find_hook and run_hook_*.
+ */
+const char *find_hook(const char *name);
+
+#endif
diff --git a/refs.c b/refs.c
index 8c9490235e..59be29cf08 100644
--- a/refs.c
+++ b/refs.c
@@ -10,6 +10,7 @@
 #include "refs.h"
 #include "refs/refs-internal.h"
 #include "run-command.h"
+#include "hook.h"
 #include "object-store.h"
 #include "object.h"
 #include "tag.h"
diff --git a/run-command.c b/run-command.c
index be6bc128cd..82fdf29656 100644
--- a/run-command.c
+++ b/run-command.c
@@ -8,6 +8,7 @@
 #include "string-list.h"
 #include "quote.h"
 #include "config.h"
+#include "hook.h"
 
 void child_process_init(struct child_process *child)
 {
@@ -1320,40 +1321,6 @@ int async_with_fork(void)
 #endif
 }
 
-const char *find_hook(const char *name)
-{
-	static struct strbuf path = STRBUF_INIT;
-
-	strbuf_reset(&path);
-	strbuf_git_path(&path, "hooks/%s", name);
-	if (access(path.buf, X_OK) < 0) {
-		int err = errno;
-
-#ifdef STRIP_EXTENSION
-		strbuf_addstr(&path, STRIP_EXTENSION);
-		if (access(path.buf, X_OK) >= 0)
-			return path.buf;
-		if (errno == EACCES)
-			err = errno;
-#endif
-
-		if (err == EACCES && advice_ignored_hook) {
-			static struct string_list advise_given = STRING_LIST_INIT_DUP;
-
-			if (!string_list_lookup(&advise_given, name)) {
-				string_list_insert(&advise_given, name);
-				advise(_("The '%s' hook was ignored because "
-					 "it's not set as executable.\n"
-					 "You can disable this warning with "
-					 "`git config advice.ignoredHook false`."),
-				       path.buf);
-			}
-		}
-		return NULL;
-	}
-	return path.buf;
-}
-
 int run_hook_ve(const char *const *env, const char *name, va_list args)
 {
 	struct child_process hook = CHILD_PROCESS_INIT;
diff --git a/run-command.h b/run-command.h
index d08414a92e..b58531a7eb 100644
--- a/run-command.h
+++ b/run-command.h
@@ -201,13 +201,6 @@ int finish_command_in_signal(struct child_process *);
  */
 int run_command(struct child_process *);
 
-/*
- * Returns the path to the hook file, or NULL if the hook is missing
- * or disabled. Note that this points to static storage that will be
- * overwritten by further calls to find_hook and run_hook_*.
- */
-const char *find_hook(const char *name);
-
 /**
  * Run a hook.
  * The first argument is a pathname to an index file, or NULL
diff --git a/sequencer.c b/sequencer.c
index 0bec01cf38..3de479f90e 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -8,6 +8,7 @@
 #include "sequencer.h"
 #include "tag.h"
 #include "run-command.h"
+#include "hook.h"
 #include "exec-cmd.h"
 #include "utf8.h"
 #include "cache-tree.h"
diff --git a/transport.c b/transport.c
index 50f5830eb6..2ed270171f 100644
--- a/transport.c
+++ b/transport.c
@@ -2,6 +2,7 @@
 #include "config.h"
 #include "transport.h"
 #include "run-command.h"
+#include "hook.h"
 #include "pkt-line.h"
 #include "fetch-pack.h"
 #include "remote.h"
-- 
2.32.0.576.g59759b6ca7d

[PATCH 2/3] hook.c: add a hook_exists() wrapper and use it in bugreport.c

[Ævar Arnfjörð Bjarmason]

From: Emily Shaffer <emilyshaffer@google.com>

Add a boolean version of the find_hook() function for those callers
who are only interested in checking whether the hook exists, not what
the path to it is.

Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/bugreport.c | 2 +-
 hook.c              | 5 +++++
 hook.h              | 5 +++++
 3 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/builtin/bugreport.c b/builtin/bugreport.c
index 596f079a7f..941c8d5e27 100644
--- a/builtin/bugreport.c
+++ b/builtin/bugreport.c
@@ -82,7 +82,7 @@ static void get_populated_hooks(struct strbuf *hook_info, int nongit)
 	}
 
 	for (i = 0; i < ARRAY_SIZE(hook); i++)
-		if (find_hook(hook[i]))
+		if (hook_exists(hook[i]))
 			strbuf_addf(hook_info, "%s\n", hook[i]);
 }
 
diff --git a/hook.c b/hook.c
index c4dbef1d0e..97cd799a32 100644
--- a/hook.c
+++ b/hook.c
@@ -35,3 +35,8 @@ const char *find_hook(const char *name)
 	}
 	return path.buf;
 }
+
+int hook_exists(const char *name)
+{
+	return !!find_hook(name);
+}
diff --git a/hook.h b/hook.h
index 68624f1605..4c547ac15e 100644
--- a/hook.h
+++ b/hook.h
@@ -8,4 +8,9 @@
  */
 const char *find_hook(const char *name);
 
+/*
+ * A boolean version of find_hook()
+ */
+int hook_exists(const char *hookname);
+
 #endif
-- 
2.32.0.576.g59759b6ca7d

[PATCH 3/3] hook-list.h: add a generated list of hooks, like config-list.h

[Ævar Arnfjörð Bjarmason]

Make githooks(5) the source of truth for what hooks git supports, and
die hooks we don't know about in find_hook(). This ensures that the
documentation and the C code's idea about existing hooks doesn't
diverge.

We still have Perl and Python code running its own hooks, but that'll
be addressed by Emily Shaffer's upcoming "git hook run" command.

This resolves a long-standing TODO item in bugreport.c of there being
no centralized listing of hooks, and fixes a bug with the bugreport
listing only knowing about 1/4 of the p4 hooks. It didn't know about
the recent "reference-transaction" hook either.

I have not been able to directly test the CMake change being made
here. Since 4c2c38e800 (ci: modification of main.yml to use cmake for
vs-build job, 2020-06-26) some of the Windows CI has a hard dependency
on CMake, this change works there, and is to my eyes an obviously
correct use of a pattern established in previous CMake changes,
namely:

 - 061c2240b1 (Introduce CMake support for configuring Git,
    2020-06-12)
 - 709df95b78 (help: move list_config_help to builtin/help,
    2020-04-16)
 - 976aaedca0 (msvc: add a Makefile target to pre-generate the Visual
   Studio solution, 2019-07-29)

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 .gitignore                          |  1 +
 Makefile                            | 11 +++++++-
 builtin/bugreport.c                 | 44 ++++++-----------------------
 contrib/buildsystems/CMakeLists.txt |  7 +++++
 generate-hooklist.sh                | 24 ++++++++++++++++
 hook.c                              | 19 +++++++++++++
 6 files changed, 69 insertions(+), 37 deletions(-)
 create mode 100755 generate-hooklist.sh

diff --git a/.gitignore b/.gitignore
index 311841f9be..6be9de41ae 100644
--- a/.gitignore
+++ b/.gitignore
@@ -190,6 +190,7 @@
 /gitweb/static/gitweb.min.*
 /config-list.h
 /command-list.h
+/hook-list.h
 *.tar.gz
 *.dsc
 *.deb
diff --git a/Makefile b/Makefile
index d155b7be21..9bd31e1ac5 100644
--- a/Makefile
+++ b/Makefile
@@ -817,6 +817,8 @@ XDIFF_LIB = xdiff/lib.a
 
 GENERATED_H += command-list.h
 GENERATED_H += config-list.h
+GENERATED_H += hook-list.h
+
 .PHONY: generated-hdrs
 generated-hdrs: $(GENERATED_H)
 
@@ -2208,7 +2210,9 @@ git$X: git.o GIT-LDFLAGS $(BUILTIN_OBJS) $(GITLIBS)
 
 help.sp help.s help.o: command-list.h
 
-builtin/help.sp builtin/help.s builtin/help.o: config-list.h GIT-PREFIX
+hook.sp hook.s hook.o: hook-list.h
+
+builtin/help.sp builtin/help.s builtin/help.o: config-list.h hook-list.h GIT-PREFIX
 builtin/help.sp builtin/help.s builtin/help.o: EXTRA_CPPFLAGS = \
 	'-DGIT_HTML_PATH="$(htmldir_relative_SQ)"' \
 	'-DGIT_MAN_PATH="$(mandir_relative_SQ)"' \
@@ -2241,6 +2245,11 @@ command-list.h: $(wildcard Documentation/git*.txt)
 		$(patsubst %,--exclude-program %,$(EXCLUDED_PROGRAMS)) \
 		command-list.txt >$@+ && mv $@+ $@
 
+hook-list.h: generate-hooklist.sh
+hook-list.h: Documentation/githooks.txt
+	$(QUIET_GEN)$(SHELL_PATH) ./generate-hooklist.sh \
+		>$@+ && mv $@+ $@
+
 SCRIPT_DEFINES = $(SHELL_PATH_SQ):$(DIFF_SQ):$(GIT_VERSION):\
 	$(localedir_SQ):$(NO_CURL):$(USE_GETTEXT_SCHEME):$(SANE_TOOL_PATH_SQ):\
 	$(gitwebdir_SQ):$(PERL_PATH_SQ):$(SANE_TEXT_GREP):$(PAGER_ENV):\
diff --git a/builtin/bugreport.c b/builtin/bugreport.c
index 941c8d5e27..a7a1fcb8a7 100644
--- a/builtin/bugreport.c
+++ b/builtin/bugreport.c
@@ -4,6 +4,7 @@
 #include "help.h"
 #include "compat/compiler.h"
 #include "hook.h"
+#include "hook-list.h"
 
 
 static void get_system_info(struct strbuf *sys_info)
@@ -41,39 +42,7 @@ static void get_system_info(struct strbuf *sys_info)
 
 static void get_populated_hooks(struct strbuf *hook_info, int nongit)
 {
-	/*
-	 * NEEDSWORK: Doesn't look like there is a list of all possible hooks;
-	 * so below is a transcription of `git help hooks`. Later, this should
-	 * be replaced with some programmatically generated list (generated from
-	 * doc or else taken from some library which tells us about all the
-	 * hooks)
-	 */
-	static const char *hook[] = {
-		"applypatch-msg",
-		"pre-applypatch",
-		"post-applypatch",
-		"pre-commit",
-		"pre-merge-commit",
-		"prepare-commit-msg",
-		"commit-msg",
-		"post-commit",
-		"pre-rebase",
-		"post-checkout",
-		"post-merge",
-		"pre-push",
-		"pre-receive",
-		"update",
-		"post-receive",
-		"post-update",
-		"push-to-checkout",
-		"pre-auto-gc",
-		"post-rewrite",
-		"sendemail-validate",
-		"fsmonitor-watchman",
-		"p4-pre-submit",
-		"post-index-change",
-	};
-	int i;
+	const char **p;
 
 	if (nongit) {
 		strbuf_addstr(hook_info,
@@ -81,9 +50,12 @@ static void get_populated_hooks(struct strbuf *hook_info, int nongit)
 		return;
 	}
 
-	for (i = 0; i < ARRAY_SIZE(hook); i++)
-		if (hook_exists(hook[i]))
-			strbuf_addf(hook_info, "%s\n", hook[i]);
+	for (p = hook_name_list; *p; p++) {
+		const char *hook = *p;
+
+		if (hook_exists(hook))
+			strbuf_addf(hook_info, "%s\n", hook);
+	}
 }
 
 static const char * const bugreport_usage[] = {
diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt
index a87841340e..c216940945 100644
--- a/contrib/buildsystems/CMakeLists.txt
+++ b/contrib/buildsystems/CMakeLists.txt
@@ -602,6 +602,13 @@ if(NOT EXISTS ${CMAKE_BINARY_DIR}/config-list.h)
 			OUTPUT_FILE ${CMAKE_BINARY_DIR}/config-list.h)
 endif()
 
+if(NOT EXISTS ${CMAKE_BINARY_DIR}/hook-list.h)
+	message("Generating hook-list.h")
+	execute_process(COMMAND ${SH_EXE} ${CMAKE_SOURCE_DIR}/generate-hooklist.sh
+			WORKING_DIRECTORY ${CMAKE_SOURCE_DIR}
+			OUTPUT_FILE ${CMAKE_BINARY_DIR}/hook-list.h)
+endif()
+
 include_directories(${CMAKE_BINARY_DIR})
 
 #build
diff --git a/generate-hooklist.sh b/generate-hooklist.sh
new file mode 100755
index 0000000000..5a3f7f849c
--- /dev/null
+++ b/generate-hooklist.sh
@@ -0,0 +1,24 @@
+#!/bin/sh
+
+echo "/* Automatically generated by generate-hooklist.sh */"
+
+print_hook_list () {
+	cat <<EOF
+static const char *hook_name_list[] = {
+EOF
+	perl -ne '
+		chomp;
+		@l[$.] = $_;
+		push @h => $l[$. - 1] if /^~~~+$/s;
+		END {
+			print qq[\t"$_",\n] for sort @h;
+		}
+	' <Documentation/githooks.txt
+	cat <<EOF
+	NULL,
+};
+EOF
+}
+
+echo
+print_hook_list
diff --git a/hook.c b/hook.c
index 97cd799a32..1f1db1ec9b 100644
--- a/hook.c
+++ b/hook.c
@@ -1,11 +1,30 @@
 #include "cache.h"
 #include "hook.h"
 #include "run-command.h"
+#include "hook-list.h"
+
+static int known_hook(const char *name)
+{
+	const char **p;
+	size_t len = strlen(name);
+	for (p = hook_name_list; *p; p++) {
+		const char *hook = *p;
+
+		if (!strncmp(name, hook, len) && hook[len] == '\0')
+			return 1;
+	}
+
+	return 0;
+}
 
 const char *find_hook(const char *name)
 {
 	static struct strbuf path = STRBUF_INIT;
 
+	if (!known_hook(name))
+		die(_("the hook '%s' is not known to git, should be in hook-list.h via githooks(5)"),
+		    name);
+
 	strbuf_reset(&path);
 	strbuf_git_path(&path, "hooks/%s", name);
 	if (access(path.buf, X_OK) < 0) {
-- 
2.32.0.576.g59759b6ca7d

Re: [PATCH 3/3] hook-list.h: add a generated list of hooks, like config-list.h

[SZEDER Gábor]

On Thu, Jun 17, 2021 at 12:09:36PM +0200, Ævar Arnfjörð Bjarmason wrote:
> diff --git a/generate-hooklist.sh b/generate-hooklist.sh
> new file mode 100755
> index 0000000000..5a3f7f849c
> --- /dev/null
> +++ b/generate-hooklist.sh
> @@ -0,0 +1,24 @@
> +#!/bin/sh
> +
> +echo "/* Automatically generated by generate-hooklist.sh */"
> +
> +print_hook_list () {
> +	cat <<EOF
> +static const char *hook_name_list[] = {
> +EOF
> +	perl -ne '

Why Perl?

At the moment I can run 'make' and get a functioning git even when
Perl is not installed.  With this patch that wouldn't work anymore.

Both 'generate-cmdlist.sh' and 'generate-configlist.sh' can process
the documentation into a header file with a long list in it without
resorting to Perl; I'm sure that hooks could be processed without Perl
as well.

> +		chomp;
> +		@l[$.] = $_;
> +		push @h => $l[$. - 1] if /^~~~+$/s;
> +		END {
> +			print qq[\t"$_",\n] for sort @h;
> +		}
> +	' <Documentation/githooks.txt
> +	cat <<EOF
> +	NULL,
> +};
> +EOF
> +}
> +
> +echo
> +print_hook_list

Re: [PATCH 3/3] hook-list.h: add a generated list of hooks, like config-list.h

[Eric Sunshine]

On Fri, Jun 18, 2021 at 1:06 PM SZEDER Gábor <szeder.dev@gmail.com> wrote:
> On Thu, Jun 17, 2021 at 12:09:36PM +0200, Ævar Arnfjörð Bjarmason wrote:
> > diff --git a/generate-hooklist.sh b/generate-hooklist.sh
> > @@ -0,0 +1,24 @@
> > +#!/bin/sh
> > +
> > +echo "/* Automatically generated by generate-hooklist.sh */"
> > +
> > +print_hook_list () {
> > +     cat <<EOF
> > +static const char *hook_name_list[] = {
> > +EOF
> > +     perl -ne '
>
> Why Perl?
>
> At the moment I can run 'make' and get a functioning git even when
> Perl is not installed.  With this patch that wouldn't work anymore.
>
> Both 'generate-cmdlist.sh' and 'generate-configlist.sh' can process
> the documentation into a header file with a long list in it without
> resorting to Perl; I'm sure that hooks could be processed without Perl
> as well.

That's a good point and not an idle question. It wasn't all that long
ago that I rewrote `generate-cmdlist` in Perl and got a complaint that
the Git project was no longer buildable on FreeBSD[1], with the result
that I ended up re-implementing it (again) in shell[2].

[1]: https://lore.kernel.org/git/loom.20150814T171757-901@post.gmane.org/
[2]: https://lore.kernel.org/git/1440365469-9928-1-git-send-email-sunshine@sunshineco.com/

Re: [PATCH 3/3] hook-list.h: add a generated list of hooks, like config-list.h

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> That's a good point and not an idle question. It wasn't all that long
> ago that I rewrote `generate-cmdlist` in Perl and got a complaint that
> the Git project was no longer buildable on FreeBSD[1], with the result
> that I ended up re-implementing it (again) in shell[2].
>
> [1]: https://lore.kernel.org/git/loom.20150814T171757-901@post.gmane.org/
> [2]: https://lore.kernel.org/git/1440365469-9928-1-git-send-email-sunshine@sunshineco.com/

If it works without Perl, I would not complain too much, but NO_PERL
is purely about the execution environment (i.e. do not build and/or
install what requires Perl at runtime), so the old complaint [1]
misses the point, I would have to say.  We do expect a working Perl
in test enviroment to run test suite, for example.

Re: [PATCH 3/3] hook-list.h: add a generated list of hooks, like config-list.h

[Ævar Arnfjörð Bjarmason]

On Fri, Jun 18 2021, Eric Sunshine wrote:

> On Fri, Jun 18, 2021 at 1:06 PM SZEDER Gábor <szeder.dev@gmail.com> wrote:
>> On Thu, Jun 17, 2021 at 12:09:36PM +0200, Ævar Arnfjörð Bjarmason wrote:
>> > diff --git a/generate-hooklist.sh b/generate-hooklist.sh
>> > @@ -0,0 +1,24 @@
>> > +#!/bin/sh
>> > +
>> > +echo "/* Automatically generated by generate-hooklist.sh */"
>> > +
>> > +print_hook_list () {
>> > +     cat <<EOF
>> > +static const char *hook_name_list[] = {
>> > +EOF
>> > +     perl -ne '
>>
>> Why Perl?
>>
>> At the moment I can run 'make' and get a functioning git even when
>> Perl is not installed.  With this patch that wouldn't work anymore.
>>
>> Both 'generate-cmdlist.sh' and 'generate-configlist.sh' can process
>> the documentation into a header file with a long list in it without
>> resorting to Perl; I'm sure that hooks could be processed without Perl
>> as well.
>
> That's a good point and not an idle question. It wasn't all that long
> ago that I rewrote `generate-cmdlist` in Perl and got a complaint that
> the Git project was no longer buildable on FreeBSD[1], with the result
> that I ended up re-implementing it (again) in shell[2].
>
> [1]: https://lore.kernel.org/git/loom.20150814T171757-901@post.gmane.org/
> [2]: https://lore.kernel.org/git/1440365469-9928-1-git-send-email-sunshine@sunshineco.com/

I'm sympathetic to changing this from Perl, as can be seen from the "v1"
(as part of another series) I intially used an unportable grep flag for
it. I'll test and probably convert it to René's few-liner.

The [1] link is not it not being buildable on FreeBSD though, it's
someone understandably not understanding what NO_PERL=Y means, because
we really should call it PERL_ONLY_FOR_BUILDING_AND_TESTING or
something.

Aside from this series it would be useful to clarify that point, what do
we really think it means? I was under the impression that it meant that,
and I really shouldn't be giving it a second thought if we introduced a
perl dependency during the build or testing.

Re: [PATCH 3/3] hook-list.h: add a generated list of hooks, like config-list.h

[René Scharfe]

Am 18.06.21 um 19:05 schrieb SZEDER Gábor:
> On Thu, Jun 17, 2021 at 12:09:36PM +0200, Ævar Arnfjörð Bjarmason wrote:
>> diff --git a/generate-hooklist.sh b/generate-hooklist.sh
>> new file mode 100755
>> index 0000000000..5a3f7f849c
>> --- /dev/null
>> +++ b/generate-hooklist.sh
>> @@ -0,0 +1,24 @@
>> +#!/bin/sh
>> +
>> +echo "/* Automatically generated by generate-hooklist.sh */"
>> +
>> +print_hook_list () {
>> +	cat <<EOF
>> +static const char *hook_name_list[] = {
>> +EOF
>> +	perl -ne '
>
> Why Perl?
>
> At the moment I can run 'make' and get a functioning git even when
> Perl is not installed.  With this patch that wouldn't work anymore.
>
> Both 'generate-cmdlist.sh' and 'generate-configlist.sh' can process
> the documentation into a header file with a long list in it without
> resorting to Perl; I'm sure that hooks could be processed without Perl
> as well.
>
>> +		chomp;
>> +		@l[$.] = $_;
>> +		push @h => $l[$. - 1] if /^~~~+$/s;
>> +		END {
>> +			print qq[\t"$_",\n] for sort @h;
>> +		}
>> +	' <Documentation/githooks.txt

How about something like this?

	sed -n '/^~~~~*$/ {x; p;}; x' Documentation/githooks.txt |
	sort |
	sed 's/^.*$/	"&",/'

sed is already used by generate-configlist.sh.

>> +	cat <<EOF
>> +	NULL,
>> +};
>> +EOF
>> +}
>> +
>> +echo
>> +print_hook_list

Re: [PATCH 3/3] hook-list.h: add a generated list of hooks, like config-list.h

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 1577 bytes --]

Hi René,

On Sun, 20 Jun 2021, René Scharfe wrote:

> Am 18.06.21 um 19:05 schrieb SZEDER Gábor:
> > On Thu, Jun 17, 2021 at 12:09:36PM +0200, Ævar Arnfjörð Bjarmason wrote:
> >> diff --git a/generate-hooklist.sh b/generate-hooklist.sh
> >> new file mode 100755
> >> index 0000000000..5a3f7f849c
> >> --- /dev/null
> >> +++ b/generate-hooklist.sh
> >> @@ -0,0 +1,24 @@
> >> +#!/bin/sh
> >> +
> >> +echo "/* Automatically generated by generate-hooklist.sh */"
> >> +
> >> +print_hook_list () {
> >> +	cat <<EOF
> >> +static const char *hook_name_list[] = {
> >> +EOF
> >> +	perl -ne '
> >
> > Why Perl?
> >
> > At the moment I can run 'make' and get a functioning git even when
> > Perl is not installed.  With this patch that wouldn't work anymore.
> >
> > Both 'generate-cmdlist.sh' and 'generate-configlist.sh' can process
> > the documentation into a header file with a long list in it without
> > resorting to Perl; I'm sure that hooks could be processed without Perl
> > as well.
> >
> >> +		chomp;
> >> +		@l[$.] = $_;
> >> +		push @h => $l[$. - 1] if /^~~~+$/s;
> >> +		END {
> >> +			print qq[\t"$_",\n] for sort @h;
> >> +		}
> >> +	' <Documentation/githooks.txt
>
> How about something like this?
>
> 	sed -n '/^~~~~*$/ {x; p;}; x' Documentation/githooks.txt |
> 	sort |
> 	sed 's/^.*$/	"&",/'
>
> sed is already used by generate-configlist.sh.

I do like me a good sed script.

Thanks,
Dscho

>
> >> +	cat <<EOF
> >> +	NULL,
> >> +};
> >> +EOF
> >> +}
> >> +
> >> +echo
> >> +print_hook_list
>
>

Re: [PATCH 3/3] hook-list.h: add a generated list of hooks, like config-list.h

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> On Sun, 20 Jun 2021, René Scharfe wrote:
>
>> How about something like this?
>>
>> 	sed -n '/^~~~~*$/ {x; p;}; x' Documentation/githooks.txt |
>> 	sort |
>> 	sed 's/^.*$/	"&",/'
>>
>> sed is already used by generate-configlist.sh.
>
> I do like me a good sed script.
>
> Thanks,
> Dscho

Yup.  

This is buried below the 27-patch series, so the whole thing cannot
advance until this gets sorted out.

Thanks.

Re: [PATCH 3/3] hook-list.h: add a generated list of hooks, like config-list.h

[René Scharfe]

Am 29.06.21 um 02:32 schrieb Junio C Hamano:
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
>> On Sun, 20 Jun 2021, René Scharfe wrote:
>>
>>> How about something like this?
>>>
>>> 	sed -n '/^~~~~*$/ {x; p;}; x' Documentation/githooks.txt |
>>> 	sort |
>>> 	sed 's/^.*$/	"&",/'
>>>
>>> sed is already used by generate-configlist.sh.
>>
>> I do like me a good sed script.
>>
>> Thanks,
>> Dscho
>
> Yup.
>
> This is buried below the 27-patch series, so the whole thing cannot
> advance until this gets sorted out.

If it helps: You can add the patch below as number 28 or squash
it in.

--- >8 ---
Subject: [PATCH] generate-hooklist.sh: use sed instead of Perl

Allow hooklist.h to be built without Perl by finding, sorting and
formatting hook names using two short sed(1) scripts and sort(1).

Signed-off-by: René Scharfe <l.s.r@web.de>
---
 generate-hooklist.sh | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/generate-hooklist.sh b/generate-hooklist.sh
index 5a3f7f849c..6f3de0a2ec 100755
--- a/generate-hooklist.sh
+++ b/generate-hooklist.sh
@@ -6,14 +6,9 @@ print_hook_list () {
 	cat <<EOF
 static const char *hook_name_list[] = {
 EOF
-	perl -ne '
-		chomp;
-		@l[$.] = $_;
-		push @h => $l[$. - 1] if /^~~~+$/s;
-		END {
-			print qq[\t"$_",\n] for sort @h;
-		}
-	' <Documentation/githooks.txt
+	sed -n '/^~~~~*$/ {x; p;}; x' Documentation/githooks.txt |
+	sort |
+	sed 's/^.*$/	"&",/'
 	cat <<EOF
 	NULL,
 };
--
2.32.0

[PATCH v2 0/3] Add a generated list of hooks in hook-list.h

[Ævar Arnfjörð Bjarmason]

A re-roll of this v1[1] to stop hardcoding the hook list, and instead generate it.

The v1 used a Perl script to generate the hook-list.h, it's now a
shellscsript that uses "sed" instead.

The sed script is an adaptation of René's in [2]. I fixed a regression
in it vis-a-vis the Perl version, we need to set LC_ALL so we don't
sort the list at build-time with the locale of the user performing the
build.

I also turned the two sed commands into one, with a trick that's
perhaps too clever, but which has worked in my cross-platform testing
so far.

1. http://lore.kernel.org/git/cover-0.3-0000000000-20210617T100239Z-avarab@gmail.com
2. https://lore.kernel.org/git/648321ed-bda9-d7fc-73e1-7ccf48addf9c@web.de/

Emily Shaffer (1):
  hook.c: add a hook_exists() wrapper and use it in bugreport.c

Ævar Arnfjörð Bjarmason (2):
  hook.[ch]: move find_hook() to this new library
  hook-list.h: add a generated list of hooks, like config-list.h

 .gitignore                          |  1 +
 Makefile                            | 11 +++++-
 builtin/am.c                        |  1 +
 builtin/bugreport.c                 | 46 +++++-----------------
 builtin/commit.c                    |  1 +
 builtin/merge.c                     |  1 +
 builtin/receive-pack.c              |  1 +
 builtin/worktree.c                  |  1 +
 contrib/buildsystems/CMakeLists.txt |  7 ++++
 generate-hooklist.sh                | 18 +++++++++
 hook.c                              | 61 +++++++++++++++++++++++++++++
 hook.h                              | 16 ++++++++
 refs.c                              |  1 +
 run-command.c                       | 35 +----------------
 run-command.h                       |  7 ----
 sequencer.c                         |  1 +
 transport.c                         |  1 +
 17 files changed, 131 insertions(+), 79 deletions(-)
 create mode 100755 generate-hooklist.sh
 create mode 100644 hook.c
 create mode 100644 hook.h

Range-diff against v1:
-:  ---------- > 1:  58c37e4f06 hook.[ch]: move find_hook() to this new library
-:  ---------- > 2:  0cf7e078ef hook.c: add a hook_exists() wrapper and use it in bugreport.c
1:  f343fc7ae6 ! 3:  ba7f01f4f6 hook-list.h: add a generated list of hooks, like config-list.h
    @@ Commit message
          - 976aaedca0 (msvc: add a Makefile target to pre-generate the Visual
            Studio solution, 2019-07-29)
     
    +    The LC_ALL=C is needed because at least in my locale the dash ("-") is
    +    ignored for the purposes of sorting, which results in a different
    +    order. I'm not aware of anything in git that has a hard dependency on
    +    the order, but e.g. the bugreport output would end up using whatever
    +    locale was in effect when git was compiled.
    +
         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
    +    Signed-off-by: René Scharfe <l.s.r@web.de>
     
      ## .gitignore ##
     @@
    @@ Makefile: command-list.h: $(wildcard Documentation/git*.txt)
      		$(patsubst %,--exclude-program %,$(EXCLUDED_PROGRAMS)) \
      		command-list.txt >$@+ && mv $@+ $@
      
    -+hook-list.h: generate-hooklist.sh
    -+hook-list.h: Documentation/githooks.txt
    ++hook-list.h: generate-hooklist.sh Documentation/githooks.txt
     +	$(QUIET_GEN)$(SHELL_PATH) ./generate-hooklist.sh \
     +		>$@+ && mv $@+ $@
     +
    @@ contrib/buildsystems/CMakeLists.txt: if(NOT EXISTS ${CMAKE_BINARY_DIR}/config-li
      ## generate-hooklist.sh (new) ##
     @@
     +#!/bin/sh
    ++#
    ++# Usage: ./generate-hooklist.sh >hook-list.h
     +
    -+echo "/* Automatically generated by generate-hooklist.sh */"
    ++cat <<EOF
    ++/* Automatically generated by generate-hooklist.sh */
     +
    -+print_hook_list () {
    -+	cat <<EOF
     +static const char *hook_name_list[] = {
     +EOF
    -+	perl -ne '
    -+		chomp;
    -+		@l[$.] = $_;
    -+		push @h => $l[$. - 1] if /^~~~+$/s;
    -+		END {
    -+			print qq[\t"$_",\n] for sort @h;
    -+		}
    -+	' <Documentation/githooks.txt
    -+	cat <<EOF
    ++
    ++sed -n -e '/^~~~~*$/ {x; s/^.*$/	"&",/; p;}; x' \
    ++	<Documentation/githooks.txt |
    ++	LC_ALL=C sort
    ++
    ++cat <<EOF
     +	NULL,
     +};
     +EOF
    -+}
    -+
    -+echo
    -+print_hook_list
     
      ## hook.c ##
     @@
-- 
2.32.0.615.g90fb4d7369

[PATCH v2 1/3] hook.[ch]: move find_hook() to this new library

[Ævar Arnfjörð Bjarmason]

Move the find_hook() function from run-command.c to a new hook.c
library. This change establishes a stub library that's pretty
pointless right now, but will see much wider use with Emily Shaffer's
upcoming "configuration-based hooks" series.

Eventually all the hook related code will live in hook.[ch]. Let's
start that process by moving the simple find_hook() function over
as-is.

Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Makefile               |  1 +
 builtin/am.c           |  1 +
 builtin/bugreport.c    |  2 +-
 builtin/commit.c       |  1 +
 builtin/merge.c        |  1 +
 builtin/receive-pack.c |  1 +
 builtin/worktree.c     |  1 +
 hook.c                 | 37 +++++++++++++++++++++++++++++++++++++
 hook.h                 | 11 +++++++++++
 refs.c                 |  1 +
 run-command.c          | 35 +----------------------------------
 run-command.h          |  7 -------
 sequencer.c            |  1 +
 transport.c            |  1 +
 14 files changed, 59 insertions(+), 42 deletions(-)
 create mode 100644 hook.c
 create mode 100644 hook.h

diff --git a/Makefile b/Makefile
index 29a152cd4f..d155b7be21 100644
--- a/Makefile
+++ b/Makefile
@@ -903,6 +903,7 @@ LIB_OBJS += hash-lookup.o
 LIB_OBJS += hashmap.o
 LIB_OBJS += help.o
 LIB_OBJS += hex.o
+LIB_OBJS += hook.o
 LIB_OBJS += ident.o
 LIB_OBJS += json-writer.o
 LIB_OBJS += kwset.o
diff --git a/builtin/am.c b/builtin/am.c
index 0b2d886c81..1c8a548903 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -11,6 +11,7 @@
 #include "parse-options.h"
 #include "dir.h"
 #include "run-command.h"
+#include "hook.h"
 #include "quote.h"
 #include "tempfile.h"
 #include "lockfile.h"
diff --git a/builtin/bugreport.c b/builtin/bugreport.c
index 9915a5841d..596f079a7f 100644
--- a/builtin/bugreport.c
+++ b/builtin/bugreport.c
@@ -3,7 +3,7 @@
 #include "strbuf.h"
 #include "help.h"
 #include "compat/compiler.h"
-#include "run-command.h"
+#include "hook.h"
 
 
 static void get_system_info(struct strbuf *sys_info)
diff --git a/builtin/commit.c b/builtin/commit.c
index 190d215d43..f1aafd67d4 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -19,6 +19,7 @@
 #include "revision.h"
 #include "wt-status.h"
 #include "run-command.h"
+#include "hook.h"
 #include "refs.h"
 #include "log-tree.h"
 #include "strbuf.h"
diff --git a/builtin/merge.c b/builtin/merge.c
index a8a843b1f5..be98d66b0a 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -13,6 +13,7 @@
 #include "builtin.h"
 #include "lockfile.h"
 #include "run-command.h"
+#include "hook.h"
 #include "diff.h"
 #include "diff-merges.h"
 #include "refs.h"
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index a34742513a..1e0e04c62f 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -7,6 +7,7 @@
 #include "pkt-line.h"
 #include "sideband.h"
 #include "run-command.h"
+#include "hook.h"
 #include "exec-cmd.h"
 #include "commit.h"
 #include "object.h"
diff --git a/builtin/worktree.c b/builtin/worktree.c
index 976bf8ed06..b1350640fe 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -8,6 +8,7 @@
 #include "branch.h"
 #include "refs.h"
 #include "run-command.h"
+#include "hook.h"
 #include "sigchain.h"
 #include "submodule.h"
 #include "utf8.h"
diff --git a/hook.c b/hook.c
new file mode 100644
index 0000000000..c4dbef1d0e
--- /dev/null
+++ b/hook.c
@@ -0,0 +1,37 @@
+#include "cache.h"
+#include "hook.h"
+#include "run-command.h"
+
+const char *find_hook(const char *name)
+{
+	static struct strbuf path = STRBUF_INIT;
+
+	strbuf_reset(&path);
+	strbuf_git_path(&path, "hooks/%s", name);
+	if (access(path.buf, X_OK) < 0) {
+		int err = errno;
+
+#ifdef STRIP_EXTENSION
+		strbuf_addstr(&path, STRIP_EXTENSION);
+		if (access(path.buf, X_OK) >= 0)
+			return path.buf;
+		if (errno == EACCES)
+			err = errno;
+#endif
+
+		if (err == EACCES && advice_ignored_hook) {
+			static struct string_list advise_given = STRING_LIST_INIT_DUP;
+
+			if (!string_list_lookup(&advise_given, name)) {
+				string_list_insert(&advise_given, name);
+				advise(_("The '%s' hook was ignored because "
+					 "it's not set as executable.\n"
+					 "You can disable this warning with "
+					 "`git config advice.ignoredHook false`."),
+				       path.buf);
+			}
+		}
+		return NULL;
+	}
+	return path.buf;
+}
diff --git a/hook.h b/hook.h
new file mode 100644
index 0000000000..68624f1605
--- /dev/null
+++ b/hook.h
@@ -0,0 +1,11 @@
+#ifndef HOOK_H
+#define HOOK_H
+
+/*
+ * Returns the path to the hook file, or NULL if the hook is missing
+ * or disabled. Note that this points to static storage that will be
+ * overwritten by further calls to find_hook and run_hook_*.
+ */
+const char *find_hook(const char *name);
+
+#endif
diff --git a/refs.c b/refs.c
index 8c9490235e..59be29cf08 100644
--- a/refs.c
+++ b/refs.c
@@ -10,6 +10,7 @@
 #include "refs.h"
 #include "refs/refs-internal.h"
 #include "run-command.h"
+#include "hook.h"
 #include "object-store.h"
 #include "object.h"
 #include "tag.h"
diff --git a/run-command.c b/run-command.c
index be6bc128cd..82fdf29656 100644
--- a/run-command.c
+++ b/run-command.c
@@ -8,6 +8,7 @@
 #include "string-list.h"
 #include "quote.h"
 #include "config.h"
+#include "hook.h"
 
 void child_process_init(struct child_process *child)
 {
@@ -1320,40 +1321,6 @@ int async_with_fork(void)
 #endif
 }
 
-const char *find_hook(const char *name)
-{
-	static struct strbuf path = STRBUF_INIT;
-
-	strbuf_reset(&path);
-	strbuf_git_path(&path, "hooks/%s", name);
-	if (access(path.buf, X_OK) < 0) {
-		int err = errno;
-
-#ifdef STRIP_EXTENSION
-		strbuf_addstr(&path, STRIP_EXTENSION);
-		if (access(path.buf, X_OK) >= 0)
-			return path.buf;
-		if (errno == EACCES)
-			err = errno;
-#endif
-
-		if (err == EACCES && advice_ignored_hook) {
-			static struct string_list advise_given = STRING_LIST_INIT_DUP;
-
-			if (!string_list_lookup(&advise_given, name)) {
-				string_list_insert(&advise_given, name);
-				advise(_("The '%s' hook was ignored because "
-					 "it's not set as executable.\n"
-					 "You can disable this warning with "
-					 "`git config advice.ignoredHook false`."),
-				       path.buf);
-			}
-		}
-		return NULL;
-	}
-	return path.buf;
-}
-
 int run_hook_ve(const char *const *env, const char *name, va_list args)
 {
 	struct child_process hook = CHILD_PROCESS_INIT;
diff --git a/run-command.h b/run-command.h
index d08414a92e..b58531a7eb 100644
--- a/run-command.h
+++ b/run-command.h
@@ -201,13 +201,6 @@ int finish_command_in_signal(struct child_process *);
  */
 int run_command(struct child_process *);
 
-/*
- * Returns the path to the hook file, or NULL if the hook is missing
- * or disabled. Note that this points to static storage that will be
- * overwritten by further calls to find_hook and run_hook_*.
- */
-const char *find_hook(const char *name);
-
 /**
  * Run a hook.
  * The first argument is a pathname to an index file, or NULL
diff --git a/sequencer.c b/sequencer.c
index 0bec01cf38..3de479f90e 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -8,6 +8,7 @@
 #include "sequencer.h"
 #include "tag.h"
 #include "run-command.h"
+#include "hook.h"
 #include "exec-cmd.h"
 #include "utf8.h"
 #include "cache-tree.h"
diff --git a/transport.c b/transport.c
index 50f5830eb6..2ed270171f 100644
--- a/transport.c
+++ b/transport.c
@@ -2,6 +2,7 @@
 #include "config.h"
 #include "transport.h"
 #include "run-command.h"
+#include "hook.h"
 #include "pkt-line.h"
 #include "fetch-pack.h"
 #include "remote.h"
-- 
2.32.0.615.g90fb4d7369

[PATCH v2 2/3] hook.c: add a hook_exists() wrapper and use it in bugreport.c

[Ævar Arnfjörð Bjarmason]

From: Emily Shaffer <emilyshaffer@google.com>

Add a boolean version of the find_hook() function for those callers
who are only interested in checking whether the hook exists, not what
the path to it is.

Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/bugreport.c | 2 +-
 hook.c              | 5 +++++
 hook.h              | 5 +++++
 3 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/builtin/bugreport.c b/builtin/bugreport.c
index 596f079a7f..941c8d5e27 100644
--- a/builtin/bugreport.c
+++ b/builtin/bugreport.c
@@ -82,7 +82,7 @@ static void get_populated_hooks(struct strbuf *hook_info, int nongit)
 	}
 
 	for (i = 0; i < ARRAY_SIZE(hook); i++)
-		if (find_hook(hook[i]))
+		if (hook_exists(hook[i]))
 			strbuf_addf(hook_info, "%s\n", hook[i]);
 }
 
diff --git a/hook.c b/hook.c
index c4dbef1d0e..97cd799a32 100644
--- a/hook.c
+++ b/hook.c
@@ -35,3 +35,8 @@ const char *find_hook(const char *name)
 	}
 	return path.buf;
 }
+
+int hook_exists(const char *name)
+{
+	return !!find_hook(name);
+}
diff --git a/hook.h b/hook.h
index 68624f1605..4c547ac15e 100644
--- a/hook.h
+++ b/hook.h
@@ -8,4 +8,9 @@
  */
 const char *find_hook(const char *name);
 
+/*
+ * A boolean version of find_hook()
+ */
+int hook_exists(const char *hookname);
+
 #endif
-- 
2.32.0.615.g90fb4d7369

[PATCH v2 3/3] hook-list.h: add a generated list of hooks, like config-list.h

[Ævar Arnfjörð Bjarmason]

Make githooks(5) the source of truth for what hooks git supports, and
die hooks we don't know about in find_hook(). This ensures that the
documentation and the C code's idea about existing hooks doesn't
diverge.

We still have Perl and Python code running its own hooks, but that'll
be addressed by Emily Shaffer's upcoming "git hook run" command.

This resolves a long-standing TODO item in bugreport.c of there being
no centralized listing of hooks, and fixes a bug with the bugreport
listing only knowing about 1/4 of the p4 hooks. It didn't know about
the recent "reference-transaction" hook either.

I have not been able to directly test the CMake change being made
here. Since 4c2c38e800 (ci: modification of main.yml to use cmake for
vs-build job, 2020-06-26) some of the Windows CI has a hard dependency
on CMake, this change works there, and is to my eyes an obviously
correct use of a pattern established in previous CMake changes,
namely:

 - 061c2240b1 (Introduce CMake support for configuring Git,
    2020-06-12)
 - 709df95b78 (help: move list_config_help to builtin/help,
    2020-04-16)
 - 976aaedca0 (msvc: add a Makefile target to pre-generate the Visual
   Studio solution, 2019-07-29)

The LC_ALL=C is needed because at least in my locale the dash ("-") is
ignored for the purposes of sorting, which results in a different
order. I'm not aware of anything in git that has a hard dependency on
the order, but e.g. the bugreport output would end up using whatever
locale was in effect when git was compiled.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: René Scharfe <l.s.r@web.de>
---
 .gitignore                          |  1 +
 Makefile                            | 10 ++++++-
 builtin/bugreport.c                 | 44 ++++++-----------------------
 contrib/buildsystems/CMakeLists.txt |  7 +++++
 generate-hooklist.sh                | 18 ++++++++++++
 hook.c                              | 19 +++++++++++++
 6 files changed, 62 insertions(+), 37 deletions(-)
 create mode 100755 generate-hooklist.sh

diff --git a/.gitignore b/.gitignore
index 311841f9be..6be9de41ae 100644
--- a/.gitignore
+++ b/.gitignore
@@ -190,6 +190,7 @@
 /gitweb/static/gitweb.min.*
 /config-list.h
 /command-list.h
+/hook-list.h
 *.tar.gz
 *.dsc
 *.deb
diff --git a/Makefile b/Makefile
index d155b7be21..9b811d3548 100644
--- a/Makefile
+++ b/Makefile
@@ -817,6 +817,8 @@ XDIFF_LIB = xdiff/lib.a
 
 GENERATED_H += command-list.h
 GENERATED_H += config-list.h
+GENERATED_H += hook-list.h
+
 .PHONY: generated-hdrs
 generated-hdrs: $(GENERATED_H)
 
@@ -2208,7 +2210,9 @@ git$X: git.o GIT-LDFLAGS $(BUILTIN_OBJS) $(GITLIBS)
 
 help.sp help.s help.o: command-list.h
 
-builtin/help.sp builtin/help.s builtin/help.o: config-list.h GIT-PREFIX
+hook.sp hook.s hook.o: hook-list.h
+
+builtin/help.sp builtin/help.s builtin/help.o: config-list.h hook-list.h GIT-PREFIX
 builtin/help.sp builtin/help.s builtin/help.o: EXTRA_CPPFLAGS = \
 	'-DGIT_HTML_PATH="$(htmldir_relative_SQ)"' \
 	'-DGIT_MAN_PATH="$(mandir_relative_SQ)"' \
@@ -2241,6 +2245,10 @@ command-list.h: $(wildcard Documentation/git*.txt)
 		$(patsubst %,--exclude-program %,$(EXCLUDED_PROGRAMS)) \
 		command-list.txt >$@+ && mv $@+ $@
 
+hook-list.h: generate-hooklist.sh Documentation/githooks.txt
+	$(QUIET_GEN)$(SHELL_PATH) ./generate-hooklist.sh \
+		>$@+ && mv $@+ $@
+
 SCRIPT_DEFINES = $(SHELL_PATH_SQ):$(DIFF_SQ):$(GIT_VERSION):\
 	$(localedir_SQ):$(NO_CURL):$(USE_GETTEXT_SCHEME):$(SANE_TOOL_PATH_SQ):\
 	$(gitwebdir_SQ):$(PERL_PATH_SQ):$(SANE_TEXT_GREP):$(PAGER_ENV):\
diff --git a/builtin/bugreport.c b/builtin/bugreport.c
index 941c8d5e27..a7a1fcb8a7 100644
--- a/builtin/bugreport.c
+++ b/builtin/bugreport.c
@@ -4,6 +4,7 @@
 #include "help.h"
 #include "compat/compiler.h"
 #include "hook.h"
+#include "hook-list.h"
 
 
 static void get_system_info(struct strbuf *sys_info)
@@ -41,39 +42,7 @@ static void get_system_info(struct strbuf *sys_info)
 
 static void get_populated_hooks(struct strbuf *hook_info, int nongit)
 {
-	/*
-	 * NEEDSWORK: Doesn't look like there is a list of all possible hooks;
-	 * so below is a transcription of `git help hooks`. Later, this should
-	 * be replaced with some programmatically generated list (generated from
-	 * doc or else taken from some library which tells us about all the
-	 * hooks)
-	 */
-	static const char *hook[] = {
-		"applypatch-msg",
-		"pre-applypatch",
-		"post-applypatch",
-		"pre-commit",
-		"pre-merge-commit",
-		"prepare-commit-msg",
-		"commit-msg",
-		"post-commit",
-		"pre-rebase",
-		"post-checkout",
-		"post-merge",
-		"pre-push",
-		"pre-receive",
-		"update",
-		"post-receive",
-		"post-update",
-		"push-to-checkout",
-		"pre-auto-gc",
-		"post-rewrite",
-		"sendemail-validate",
-		"fsmonitor-watchman",
-		"p4-pre-submit",
-		"post-index-change",
-	};
-	int i;
+	const char **p;
 
 	if (nongit) {
 		strbuf_addstr(hook_info,
@@ -81,9 +50,12 @@ static void get_populated_hooks(struct strbuf *hook_info, int nongit)
 		return;
 	}
 
-	for (i = 0; i < ARRAY_SIZE(hook); i++)
-		if (hook_exists(hook[i]))
-			strbuf_addf(hook_info, "%s\n", hook[i]);
+	for (p = hook_name_list; *p; p++) {
+		const char *hook = *p;
+
+		if (hook_exists(hook))
+			strbuf_addf(hook_info, "%s\n", hook);
+	}
 }
 
 static const char * const bugreport_usage[] = {
diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt
index a87841340e..c216940945 100644
--- a/contrib/buildsystems/CMakeLists.txt
+++ b/contrib/buildsystems/CMakeLists.txt
@@ -602,6 +602,13 @@ if(NOT EXISTS ${CMAKE_BINARY_DIR}/config-list.h)
 			OUTPUT_FILE ${CMAKE_BINARY_DIR}/config-list.h)
 endif()
 
+if(NOT EXISTS ${CMAKE_BINARY_DIR}/hook-list.h)
+	message("Generating hook-list.h")
+	execute_process(COMMAND ${SH_EXE} ${CMAKE_SOURCE_DIR}/generate-hooklist.sh
+			WORKING_DIRECTORY ${CMAKE_SOURCE_DIR}
+			OUTPUT_FILE ${CMAKE_BINARY_DIR}/hook-list.h)
+endif()
+
 include_directories(${CMAKE_BINARY_DIR})
 
 #build
diff --git a/generate-hooklist.sh b/generate-hooklist.sh
new file mode 100755
index 0000000000..6d4e56d1a3
--- /dev/null
+++ b/generate-hooklist.sh
@@ -0,0 +1,18 @@
+#!/bin/sh
+#
+# Usage: ./generate-hooklist.sh >hook-list.h
+
+cat <<EOF
+/* Automatically generated by generate-hooklist.sh */
+
+static const char *hook_name_list[] = {
+EOF
+
+sed -n -e '/^~~~~*$/ {x; s/^.*$/	"&",/; p;}; x' \
+	<Documentation/githooks.txt |
+	LC_ALL=C sort
+
+cat <<EOF
+	NULL,
+};
+EOF
diff --git a/hook.c b/hook.c
index 97cd799a32..1f1db1ec9b 100644
--- a/hook.c
+++ b/hook.c
@@ -1,11 +1,30 @@
 #include "cache.h"
 #include "hook.h"
 #include "run-command.h"
+#include "hook-list.h"
+
+static int known_hook(const char *name)
+{
+	const char **p;
+	size_t len = strlen(name);
+	for (p = hook_name_list; *p; p++) {
+		const char *hook = *p;
+
+		if (!strncmp(name, hook, len) && hook[len] == '\0')
+			return 1;
+	}
+
+	return 0;
+}
 
 const char *find_hook(const char *name)
 {
 	static struct strbuf path = STRBUF_INIT;
 
+	if (!known_hook(name))
+		die(_("the hook '%s' is not known to git, should be in hook-list.h via githooks(5)"),
+		    name);
+
 	strbuf_reset(&path);
 	strbuf_git_path(&path, "hooks/%s", name);
 	if (access(path.buf, X_OK) < 0) {
-- 
2.32.0.615.g90fb4d7369

Re: [PATCH v2 3/3] hook-list.h: add a generated list of hooks, like config-list.h

[René Scharfe]

Am 29.06.21 um 20:54 schrieb Ævar Arnfjörð Bjarmason:
> Make githooks(5) the source of truth for what hooks git supports, and
> die hooks we don't know about in find_hook(). This ensures that the
> documentation and the C code's idea about existing hooks doesn't
> diverge.
>
> We still have Perl and Python code running its own hooks, but that'll
> be addressed by Emily Shaffer's upcoming "git hook run" command.
>
> This resolves a long-standing TODO item in bugreport.c of there being
> no centralized listing of hooks, and fixes a bug with the bugreport
> listing only knowing about 1/4 of the p4 hooks. It didn't know about
> the recent "reference-transaction" hook either.
>
> I have not been able to directly test the CMake change being made
> here. Since 4c2c38e800 (ci: modification of main.yml to use cmake for
> vs-build job, 2020-06-26) some of the Windows CI has a hard dependency
> on CMake, this change works there, and is to my eyes an obviously
> correct use of a pattern established in previous CMake changes,
> namely:
>
>  - 061c2240b1 (Introduce CMake support for configuring Git,
>     2020-06-12)
>  - 709df95b78 (help: move list_config_help to builtin/help,
>     2020-04-16)
>  - 976aaedca0 (msvc: add a Makefile target to pre-generate the Visual
>    Studio solution, 2019-07-29)
>
> The LC_ALL=C is needed because at least in my locale the dash ("-") is
> ignored for the purposes of sorting, which results in a different
> order. I'm not aware of anything in git that has a hard dependency on
> the order, but e.g. the bugreport output would end up using whatever
> locale was in effect when git was compiled.
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> Signed-off-by: René Scharfe <l.s.r@web.de>

Please remove my sign-off line.  The code looks OK, but there are
basically only trivial traces of my suggestion left.  That's fine, but
it makes my sign-off unnecessary, and I cannot certify the origin of the
rest of the patch.  You could turn it into a Helped-by or
Contributions-by if you like.

> ---
>  .gitignore                          |  1 +
>  Makefile                            | 10 ++++++-
>  builtin/bugreport.c                 | 44 ++++++-----------------------
>  contrib/buildsystems/CMakeLists.txt |  7 +++++
>  generate-hooklist.sh                | 18 ++++++++++++
>  hook.c                              | 19 +++++++++++++
>  6 files changed, 62 insertions(+), 37 deletions(-)
>  create mode 100755 generate-hooklist.sh
>
> diff --git a/.gitignore b/.gitignore
> index 311841f9be..6be9de41ae 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -190,6 +190,7 @@
>  /gitweb/static/gitweb.min.*
>  /config-list.h
>  /command-list.h
> +/hook-list.h
>  *.tar.gz
>  *.dsc
>  *.deb
> diff --git a/Makefile b/Makefile
> index d155b7be21..9b811d3548 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -817,6 +817,8 @@ XDIFF_LIB = xdiff/lib.a
>
>  GENERATED_H += command-list.h
>  GENERATED_H += config-list.h
> +GENERATED_H += hook-list.h
> +
>  .PHONY: generated-hdrs
>  generated-hdrs: $(GENERATED_H)
>
> @@ -2208,7 +2210,9 @@ git$X: git.o GIT-LDFLAGS $(BUILTIN_OBJS) $(GITLIBS)
>
>  help.sp help.s help.o: command-list.h
>
> -builtin/help.sp builtin/help.s builtin/help.o: config-list.h GIT-PREFIX
> +hook.sp hook.s hook.o: hook-list.h
> +
> +builtin/help.sp builtin/help.s builtin/help.o: config-list.h hook-list.h GIT-PREFIX
>  builtin/help.sp builtin/help.s builtin/help.o: EXTRA_CPPFLAGS = \
>  	'-DGIT_HTML_PATH="$(htmldir_relative_SQ)"' \
>  	'-DGIT_MAN_PATH="$(mandir_relative_SQ)"' \
> @@ -2241,6 +2245,10 @@ command-list.h: $(wildcard Documentation/git*.txt)
>  		$(patsubst %,--exclude-program %,$(EXCLUDED_PROGRAMS)) \
>  		command-list.txt >$@+ && mv $@+ $@
>
> +hook-list.h: generate-hooklist.sh Documentation/githooks.txt
> +	$(QUIET_GEN)$(SHELL_PATH) ./generate-hooklist.sh \
> +		>$@+ && mv $@+ $@
> +
>  SCRIPT_DEFINES = $(SHELL_PATH_SQ):$(DIFF_SQ):$(GIT_VERSION):\
>  	$(localedir_SQ):$(NO_CURL):$(USE_GETTEXT_SCHEME):$(SANE_TOOL_PATH_SQ):\
>  	$(gitwebdir_SQ):$(PERL_PATH_SQ):$(SANE_TEXT_GREP):$(PAGER_ENV):\
> diff --git a/builtin/bugreport.c b/builtin/bugreport.c
> index 941c8d5e27..a7a1fcb8a7 100644
> --- a/builtin/bugreport.c
> +++ b/builtin/bugreport.c
> @@ -4,6 +4,7 @@
>  #include "help.h"
>  #include "compat/compiler.h"
>  #include "hook.h"
> +#include "hook-list.h"
>
>
>  static void get_system_info(struct strbuf *sys_info)
> @@ -41,39 +42,7 @@ static void get_system_info(struct strbuf *sys_info)
>
>  static void get_populated_hooks(struct strbuf *hook_info, int nongit)
>  {
> -	/*
> -	 * NEEDSWORK: Doesn't look like there is a list of all possible hooks;
> -	 * so below is a transcription of `git help hooks`. Later, this should
> -	 * be replaced with some programmatically generated list (generated from
> -	 * doc or else taken from some library which tells us about all the
> -	 * hooks)
> -	 */
> -	static const char *hook[] = {
> -		"applypatch-msg",
> -		"pre-applypatch",
> -		"post-applypatch",
> -		"pre-commit",
> -		"pre-merge-commit",
> -		"prepare-commit-msg",
> -		"commit-msg",
> -		"post-commit",
> -		"pre-rebase",
> -		"post-checkout",
> -		"post-merge",
> -		"pre-push",
> -		"pre-receive",
> -		"update",
> -		"post-receive",
> -		"post-update",
> -		"push-to-checkout",
> -		"pre-auto-gc",
> -		"post-rewrite",
> -		"sendemail-validate",
> -		"fsmonitor-watchman",
> -		"p4-pre-submit",
> -		"post-index-change",
> -	};
> -	int i;
> +	const char **p;
>
>  	if (nongit) {
>  		strbuf_addstr(hook_info,
> @@ -81,9 +50,12 @@ static void get_populated_hooks(struct strbuf *hook_info, int nongit)
>  		return;
>  	}
>
> -	for (i = 0; i < ARRAY_SIZE(hook); i++)
> -		if (hook_exists(hook[i]))
> -			strbuf_addf(hook_info, "%s\n", hook[i]);
> +	for (p = hook_name_list; *p; p++) {
> +		const char *hook = *p;
> +
> +		if (hook_exists(hook))
> +			strbuf_addf(hook_info, "%s\n", hook);
> +	}
>  }
>
>  static const char * const bugreport_usage[] = {
> diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt
> index a87841340e..c216940945 100644
> --- a/contrib/buildsystems/CMakeLists.txt
> +++ b/contrib/buildsystems/CMakeLists.txt
> @@ -602,6 +602,13 @@ if(NOT EXISTS ${CMAKE_BINARY_DIR}/config-list.h)
>  			OUTPUT_FILE ${CMAKE_BINARY_DIR}/config-list.h)
>  endif()
>
> +if(NOT EXISTS ${CMAKE_BINARY_DIR}/hook-list.h)
> +	message("Generating hook-list.h")
> +	execute_process(COMMAND ${SH_EXE} ${CMAKE_SOURCE_DIR}/generate-hooklist.sh
> +			WORKING_DIRECTORY ${CMAKE_SOURCE_DIR}
> +			OUTPUT_FILE ${CMAKE_BINARY_DIR}/hook-list.h)
> +endif()
> +
>  include_directories(${CMAKE_BINARY_DIR})
>
>  #build
> diff --git a/generate-hooklist.sh b/generate-hooklist.sh
> new file mode 100755
> index 0000000000..6d4e56d1a3
> --- /dev/null
> +++ b/generate-hooklist.sh
> @@ -0,0 +1,18 @@
> +#!/bin/sh
> +#
> +# Usage: ./generate-hooklist.sh >hook-list.h
> +
> +cat <<EOF
> +/* Automatically generated by generate-hooklist.sh */
> +
> +static const char *hook_name_list[] = {
> +EOF
> +
> +sed -n -e '/^~~~~*$/ {x; s/^.*$/	"&",/; p;}; x' \
> +	<Documentation/githooks.txt |
> +	LC_ALL=C sort
> +
> +cat <<EOF
> +	NULL,
> +};
> +EOF
> diff --git a/hook.c b/hook.c
> index 97cd799a32..1f1db1ec9b 100644
> --- a/hook.c
> +++ b/hook.c
> @@ -1,11 +1,30 @@
>  #include "cache.h"
>  #include "hook.h"
>  #include "run-command.h"
> +#include "hook-list.h"
> +
> +static int known_hook(const char *name)
> +{
> +	const char **p;
> +	size_t len = strlen(name);
> +	for (p = hook_name_list; *p; p++) {
> +		const char *hook = *p;
> +
> +		if (!strncmp(name, hook, len) && hook[len] == '\0')
> +			return 1;
> +	}
> +
> +	return 0;
> +}
>
>  const char *find_hook(const char *name)
>  {
>  	static struct strbuf path = STRBUF_INIT;
>
> +	if (!known_hook(name))
> +		die(_("the hook '%s' is not known to git, should be in hook-list.h via githooks(5)"),
> +		    name);
> +
>  	strbuf_reset(&path);
>  	strbuf_git_path(&path, "hooks/%s", name);
>  	if (access(path.buf, X_OK) < 0) {
>

Re: [PATCH v2 3/3] hook-list.h: add a generated list of hooks, like config-list.h

[Ævar Arnfjörð Bjarmason]

On Tue, Jun 29 2021, René Scharfe wrote:

> Am 29.06.21 um 20:54 schrieb Ævar Arnfjörð Bjarmason:
>> Make githooks(5) the source of truth for what hooks git supports, and
>> die hooks we don't know about in find_hook(). This ensures that the
>> documentation and the C code's idea about existing hooks doesn't
>> diverge.
>>
>> We still have Perl and Python code running its own hooks, but that'll
>> be addressed by Emily Shaffer's upcoming "git hook run" command.
>>
>> This resolves a long-standing TODO item in bugreport.c of there being
>> no centralized listing of hooks, and fixes a bug with the bugreport
>> listing only knowing about 1/4 of the p4 hooks. It didn't know about
>> the recent "reference-transaction" hook either.
>>
>> I have not been able to directly test the CMake change being made
>> here. Since 4c2c38e800 (ci: modification of main.yml to use cmake for
>> vs-build job, 2020-06-26) some of the Windows CI has a hard dependency
>> on CMake, this change works there, and is to my eyes an obviously
>> correct use of a pattern established in previous CMake changes,
>> namely:
>>
>>  - 061c2240b1 (Introduce CMake support for configuring Git,
>>     2020-06-12)
>>  - 709df95b78 (help: move list_config_help to builtin/help,
>>     2020-04-16)
>>  - 976aaedca0 (msvc: add a Makefile target to pre-generate the Visual
>>    Studio solution, 2019-07-29)
>>
>> The LC_ALL=C is needed because at least in my locale the dash ("-") is
>> ignored for the purposes of sorting, which results in a different
>> order. I'm not aware of anything in git that has a hard dependency on
>> the order, but e.g. the bugreport output would end up using whatever
>> locale was in effect when git was compiled.
>>
>> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>> Signed-off-by: René Scharfe <l.s.r@web.de>
>
> Please remove my sign-off line.  The code looks OK, but there are
> basically only trivial traces of my suggestion left.  That's fine, but
> it makes my sign-off unnecessary, and I cannot certify the origin of the
> rest of the patch.  You could turn it into a Helped-by or
> Contributions-by if you like.

Isn't that what the SOB is for though? I.e. a copyright audit trail, I
wouldn't have come up with that sed code myself, it's copied & adjusted
from your version.

I can re-roll with a Helped-by if you/Junio think that's appropriate, I
just thought this was /the/ use-case for SOB.

Re: [PATCH v2 3/3] hook-list.h: add a generated list of hooks, like config-list.h

[Emily Shaffer]

On Tue, Jun 29, 2021 at 08:54:02PM +0200, Ævar Arnfjörð Bjarmason wrote:
> 
> Make githooks(5) the source of truth for what hooks git supports, and
> die hooks we don't know about in find_hook(). This ensures that the
> documentation and the C code's idea about existing hooks doesn't
> diverge.
> 
> We still have Perl and Python code running its own hooks, but that'll
> be addressed by Emily Shaffer's upcoming "git hook run" command.
> 
> This resolves a long-standing TODO item in bugreport.c of there being
> no centralized listing of hooks, and fixes a bug with the bugreport
> listing only knowing about 1/4 of the p4 hooks. It didn't know about
> the recent "reference-transaction" hook either.
> 
> I have not been able to directly test the CMake change being made
> here. Since 4c2c38e800 (ci: modification of main.yml to use cmake for
> vs-build job, 2020-06-26) some of the Windows CI has a hard dependency
> on CMake, this change works there, and is to my eyes an obviously
> correct use of a pattern established in previous CMake changes,
> namely:
> 
>  - 061c2240b1 (Introduce CMake support for configuring Git,
>     2020-06-12)
>  - 709df95b78 (help: move list_config_help to builtin/help,
>     2020-04-16)
>  - 976aaedca0 (msvc: add a Makefile target to pre-generate the Visual
>    Studio solution, 2019-07-29)
> 
> The LC_ALL=C is needed because at least in my locale the dash ("-") is
> ignored for the purposes of sorting, which results in a different
> order. I'm not aware of anything in git that has a hard dependency on
> the order, but e.g. the bugreport output would end up using whatever
> locale was in effect when git was compiled.
> 
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> Signed-off-by: René Scharfe <l.s.r@web.de>
> ---
>  .gitignore                          |  1 +
>  Makefile                            | 10 ++++++-
>  builtin/bugreport.c                 | 44 ++++++-----------------------
>  contrib/buildsystems/CMakeLists.txt |  7 +++++
>  generate-hooklist.sh                | 18 ++++++++++++
>  hook.c                              | 19 +++++++++++++
>  6 files changed, 62 insertions(+), 37 deletions(-)
>  create mode 100755 generate-hooklist.sh
> 
> diff --git a/.gitignore b/.gitignore
> index 311841f9be..6be9de41ae 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -190,6 +190,7 @@
>  /gitweb/static/gitweb.min.*
>  /config-list.h
>  /command-list.h
> +/hook-list.h
>  *.tar.gz
>  *.dsc
>  *.deb
> diff --git a/Makefile b/Makefile
> index d155b7be21..9b811d3548 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -817,6 +817,8 @@ XDIFF_LIB = xdiff/lib.a
>  
>  GENERATED_H += command-list.h
>  GENERATED_H += config-list.h
> +GENERATED_H += hook-list.h
> +
>  .PHONY: generated-hdrs
>  generated-hdrs: $(GENERATED_H)
>  
> @@ -2208,7 +2210,9 @@ git$X: git.o GIT-LDFLAGS $(BUILTIN_OBJS) $(GITLIBS)
>  
>  help.sp help.s help.o: command-list.h
>  
> -builtin/help.sp builtin/help.s builtin/help.o: config-list.h GIT-PREFIX
> +hook.sp hook.s hook.o: hook-list.h
> +
> +builtin/help.sp builtin/help.s builtin/help.o: config-list.h hook-list.h GIT-PREFIX
>  builtin/help.sp builtin/help.s builtin/help.o: EXTRA_CPPFLAGS = \
>  	'-DGIT_HTML_PATH="$(htmldir_relative_SQ)"' \
>  	'-DGIT_MAN_PATH="$(mandir_relative_SQ)"' \
> @@ -2241,6 +2245,10 @@ command-list.h: $(wildcard Documentation/git*.txt)
>  		$(patsubst %,--exclude-program %,$(EXCLUDED_PROGRAMS)) \
>  		command-list.txt >$@+ && mv $@+ $@
>  
> +hook-list.h: generate-hooklist.sh Documentation/githooks.txt
> +	$(QUIET_GEN)$(SHELL_PATH) ./generate-hooklist.sh \
> +		>$@+ && mv $@+ $@
> +
>  SCRIPT_DEFINES = $(SHELL_PATH_SQ):$(DIFF_SQ):$(GIT_VERSION):\
>  	$(localedir_SQ):$(NO_CURL):$(USE_GETTEXT_SCHEME):$(SANE_TOOL_PATH_SQ):\
>  	$(gitwebdir_SQ):$(PERL_PATH_SQ):$(SANE_TEXT_GREP):$(PAGER_ENV):\
> diff --git a/builtin/bugreport.c b/builtin/bugreport.c
> index 941c8d5e27..a7a1fcb8a7 100644
> --- a/builtin/bugreport.c
> +++ b/builtin/bugreport.c
> @@ -4,6 +4,7 @@
>  #include "help.h"
>  #include "compat/compiler.h"
>  #include "hook.h"
> +#include "hook-list.h"
>  
>  
>  static void get_system_info(struct strbuf *sys_info)
> @@ -41,39 +42,7 @@ static void get_system_info(struct strbuf *sys_info)
>  
>  static void get_populated_hooks(struct strbuf *hook_info, int nongit)
>  {
> -	/*
> -	 * NEEDSWORK: Doesn't look like there is a list of all possible hooks;
> -	 * so below is a transcription of `git help hooks`. Later, this should
> -	 * be replaced with some programmatically generated list (generated from
> -	 * doc or else taken from some library which tells us about all the
> -	 * hooks)
> -	 */
> -	static const char *hook[] = {
> -		"applypatch-msg",
> -		"pre-applypatch",
> -		"post-applypatch",
> -		"pre-commit",
> -		"pre-merge-commit",
> -		"prepare-commit-msg",
> -		"commit-msg",
> -		"post-commit",
> -		"pre-rebase",
> -		"post-checkout",
> -		"post-merge",
> -		"pre-push",
> -		"pre-receive",
> -		"update",
> -		"post-receive",
> -		"post-update",
> -		"push-to-checkout",
> -		"pre-auto-gc",
> -		"post-rewrite",
> -		"sendemail-validate",
> -		"fsmonitor-watchman",
> -		"p4-pre-submit",
> -		"post-index-change",
> -	};
> -	int i;
> +	const char **p;
>  
>  	if (nongit) {
>  		strbuf_addstr(hook_info,
> @@ -81,9 +50,12 @@ static void get_populated_hooks(struct strbuf *hook_info, int nongit)
>  		return;
>  	}
>  
> -	for (i = 0; i < ARRAY_SIZE(hook); i++)
> -		if (hook_exists(hook[i]))
> -			strbuf_addf(hook_info, "%s\n", hook[i]);
> +	for (p = hook_name_list; *p; p++) {
> +		const char *hook = *p;
> +
> +		if (hook_exists(hook))
> +			strbuf_addf(hook_info, "%s\n", hook);
> +	}
>  }
>  
>  static const char * const bugreport_usage[] = {
> diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt
> index a87841340e..c216940945 100644
> --- a/contrib/buildsystems/CMakeLists.txt
> +++ b/contrib/buildsystems/CMakeLists.txt
> @@ -602,6 +602,13 @@ if(NOT EXISTS ${CMAKE_BINARY_DIR}/config-list.h)
>  			OUTPUT_FILE ${CMAKE_BINARY_DIR}/config-list.h)
>  endif()
>  
> +if(NOT EXISTS ${CMAKE_BINARY_DIR}/hook-list.h)
> +	message("Generating hook-list.h")
> +	execute_process(COMMAND ${SH_EXE} ${CMAKE_SOURCE_DIR}/generate-hooklist.sh
> +			WORKING_DIRECTORY ${CMAKE_SOURCE_DIR}
> +			OUTPUT_FILE ${CMAKE_BINARY_DIR}/hook-list.h)
> +endif()
> +
>  include_directories(${CMAKE_BINARY_DIR})
>  
>  #build
> diff --git a/generate-hooklist.sh b/generate-hooklist.sh
> new file mode 100755
> index 0000000000..6d4e56d1a3
> --- /dev/null
> +++ b/generate-hooklist.sh
> @@ -0,0 +1,18 @@
> +#!/bin/sh
> +#
> +# Usage: ./generate-hooklist.sh >hook-list.h
> +
> +cat <<EOF
> +/* Automatically generated by generate-hooklist.sh */
> +
> +static const char *hook_name_list[] = {
> +EOF
> +
> +sed -n -e '/^~~~~*$/ {x; s/^.*$/	"&",/; p;}; x' \
> +	<Documentation/githooks.txt |
> +	LC_ALL=C sort
> +
> +cat <<EOF
> +	NULL,
> +};
> +EOF
> diff --git a/hook.c b/hook.c
> index 97cd799a32..1f1db1ec9b 100644
> --- a/hook.c
> +++ b/hook.c
> @@ -1,11 +1,30 @@
>  #include "cache.h"
>  #include "hook.h"
>  #include "run-command.h"
> +#include "hook-list.h"
> +
> +static int known_hook(const char *name)
> +{
> +	const char **p;
> +	size_t len = strlen(name);
> +	for (p = hook_name_list; *p; p++) {
> +		const char *hook = *p;
> +
> +		if (!strncmp(name, hook, len) && hook[len] == '\0')
> +			return 1;
> +	}
> +
> +	return 0;
> +}
>  
>  const char *find_hook(const char *name)
>  {
>  	static struct strbuf path = STRBUF_INIT;
>  
> +	if (!known_hook(name))
> +		die(_("the hook '%s' is not known to git, should be in hook-list.h via githooks(5)"),
> +		    name);
> +

I'm not sure that it's necessary to require this, to be honest. I see a
use case for wrappers to want to store and run hooks in an idiomatic
way, and doing so by instructing their users to stick in
.git/hooks/wrapper-clone (for example) and then calling 'git hook run
wrapper-clone'. That's doubly compelling in a later config-based-hooks
world where 'git hook run' gets you free multihook features like
ordering and parallelism. I will likely want to remove this when
rebasing my config-based hooks work on top of your restart.

>  	strbuf_reset(&path);
>  	strbuf_git_path(&path, "hooks/%s", name);
>  	if (access(path.buf, X_OK) < 0) {
> -- 
> 2.32.0.615.g90fb4d7369
> 

Re: [PATCH v2 3/3] hook-list.h: add a generated list of hooks, like config-list.h

[Ævar Arnfjörð Bjarmason]

On Fri, Jul 09 2021, Emily Shaffer wrote:

> On Tue, Jun 29, 2021 at 08:54:02PM +0200, Ævar Arnfjörð Bjarmason wrote:
>>  const char *find_hook(const char *name)
>>  {
>>  	static struct strbuf path = STRBUF_INIT;
>>  
>> +	if (!known_hook(name))
>> +		die(_("the hook '%s' is not known to git, should be in hook-list.h via githooks(5)"),
>> +		    name);
>> +
>
> I'm not sure that it's necessary to require this, to be honest. I see a
> use case for wrappers to want to store and run hooks in an idiomatic
> way, and doing so by instructing their users to stick in
> .git/hooks/wrapper-clone (for example) and then calling 'git hook run
> wrapper-clone'. That's doubly compelling in a later config-based-hooks
> world where 'git hook run' gets you free multihook features like
> ordering and parallelism. I will likely want to remove this when
> rebasing my config-based hooks work on top of your restart.

Indeed, FWIW this was part of my general approach of narrowly supporting
existing git behavior only with 'git hook run', i.e. there's no general
"run this thing like a hook for me" now, so we're not losing anything by
not having it support that.

But yes, I can see how "run this script for me as if though it were a
hook" would be useful, will be trivial to support it & still somehow
assert typos/that hook-list.h / githooks.txt is a source of truth about
our known hooks.

Re: [PATCH v2 3/3] hook-list.h: add a generated list of hooks, like config-list.h

[Emily Shaffer]

On Sat, Jul 10, 2021 at 11:03:50AM +0200, Ævar Arnfjörð Bjarmason wrote:
> 
> 
> On Fri, Jul 09 2021, Emily Shaffer wrote:
> 
> > On Tue, Jun 29, 2021 at 08:54:02PM +0200, Ævar Arnfjörð Bjarmason wrote:
> >>  const char *find_hook(const char *name)
> >>  {
> >>  	static struct strbuf path = STRBUF_INIT;
> >>  
> >> +	if (!known_hook(name))
> >> +		die(_("the hook '%s' is not known to git, should be in hook-list.h via githooks(5)"),
> >> +		    name);
> >> +
> >
> > I'm not sure that it's necessary to require this, to be honest. I see a
> > use case for wrappers to want to store and run hooks in an idiomatic
> > way, and doing so by instructing their users to stick in
> > .git/hooks/wrapper-clone (for example) and then calling 'git hook run
> > wrapper-clone'. That's doubly compelling in a later config-based-hooks
> > world where 'git hook run' gets you free multihook features like
> > ordering and parallelism. I will likely want to remove this when
> > rebasing my config-based hooks work on top of your restart.
> 
> Indeed, FWIW this was part of my general approach of narrowly supporting
> existing git behavior only with 'git hook run', i.e. there's no general
> "run this thing like a hook for me" now, so we're not losing anything by
> not having it support that.
> 
> But yes, I can see how "run this script for me as if though it were a
> hook" would be useful, will be trivial to support it & still somehow
> assert typos/that hook-list.h / githooks.txt is a source of truth about
> our known hooks.

Hm, I see - you're using the BUG() to gently remind people that they
should go and update githooks.txt if they call find_hook("new-hook").
Ok, I'll add a flag or a wrapper or something in my own rebase - so 'git
hook run' doesn't check but internal calls do.

Thanks for explaining the intent better.
 - Emily